Patchwork Do not redirect ld stdout/stderr in collect2 with -debug

login
register
mail settings
Submitter Richard Guenther
Date June 10, 2013, 12:01 p.m.
Message ID <alpine.LNX.2.00.1306101358420.26078@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/250227/
State New
Headers show

Comments

Richard Guenther - June 10, 2013, 12:01 p.m.
This fixes one very annoying thing collect2 does when trying to
debug LTO WPA issues.  Even with -v you need to wait until all
LTRANS stages completed to see the lto1 -fwpa invocation which
is because collect2 buffers and replays stdout/stderr of ld
(to avoid duplicating that in some cases).  But I really want
to see the output immediately but there is no way to do that.
The easiest is to disable the buffering with -debug (that is,
-Wl,-debug to the -flto driver command line).

Tested with/without -debug.

Ok for trunk?

Thanks,
Richard.

2013-06-10  Richard Biener  <rguenther@suse.de>

	* collect2.c (main): Do not redirect ld stdout/stderr when
	debugging.
Joseph S. Myers - June 10, 2013, 2:44 p.m.
On Mon, 10 Jun 2013, Richard Biener wrote:

> This fixes one very annoying thing collect2 does when trying to
> debug LTO WPA issues.  Even with -v you need to wait until all
> LTRANS stages completed to see the lto1 -fwpa invocation which
> is because collect2 buffers and replays stdout/stderr of ld
> (to avoid duplicating that in some cases).  But I really want
> to see the output immediately but there is no way to do that.
> The easiest is to disable the buffering with -debug (that is,
> -Wl,-debug to the -flto driver command line).
> 
> Tested with/without -debug.
> 
> Ok for trunk?

OK.
Thomas Schwinge - Feb. 21, 2015, 4:22 p.m.
Hi Richard!

On Mon, 10 Jun 2013 14:01:53 +0200 (CEST), Richard Biener <rguenther@suse.de> wrote:
> This fixes one very annoying thing collect2 does when trying to
> debug LTO WPA issues.  Even with -v you need to wait until all
> LTRANS stages completed to see the lto1 -fwpa invocation which
> is because collect2 buffers and replays stdout/stderr of ld
> (to avoid duplicating that in some cases).  But I really want
> to see the output immediately but there is no way to do that.
> The easiest is to disable the buffering with -debug (that is,
> -Wl,-debug to the -flto driver command line).
> 
> Tested with/without -debug.
> 
> Ok for trunk?

> 	* collect2.c (main): Do not redirect ld stdout/stderr when
> 	debugging.

> --- gcc/collect2.c	(revision 199732)
> +++ gcc/collect2.c	(working copy)
> @@ -1189,8 +1189,11 @@ main (int argc, char **argv)
>  #ifdef COLLECT_EXPORT_LIST
>    export_file = make_temp_file (".x");
>  #endif
> -  ldout = make_temp_file (".ld");
> -  lderrout = make_temp_file (".le");
> +  if (!debug)
> +    {
> +      ldout = make_temp_file (".ld");
> +      lderrout = make_temp_file (".le");
> +    }
>    *c_ptr++ = c_file_name;
>    *c_ptr++ = "-x";
>    *c_ptr++ = "c";

This change (r199936) is problematic, given the usage of ldout and
lderrout in gcc/tlink.c:do_tlink.  If, for -debug, they're not
initialized, they'll be NULL, which in do_tlink, the tlink_execute call
will handle fine (as I understand it), but after that, for example,
dump_ld_file will attempt to fopen (NULL), which will cause a SIGSEGV on
GNU Hurd at least.  (Correct me if I'm wrong -- I have not yet read the
relevant standards in detail -- but from what I remember, that's
appropriate: NULL is not a string naming a file.)

I found this when running binutils' gold testsuite:

    $ gcc-4.9 -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fmerge-constants -g -O2 -fno-use-linker-plugin -o incremental_test -Bgcctestdir/ -Wl,--incremental-full incremental_test_1.o incremental_test_2.o -v -Wl,-debug
    [...]
    gcc-4.9: internal compiler error: Segmentation fault (program collect2)
    [...]

In do_tlink, the last four uses of ldout and lderrout should be guarded
by NULL and empty string checks (as done in gcc/collect2.c:tool_cleanup),
but I'm not sure what the correct fix is in the »if (ret)« block:

    void
    do_tlink (char **ld_argv, char **object_lst ATTRIBUTE_UNUSED)
    {
      int ret = tlink_execute ("ld", ld_argv, ldout, lderrout,
                               HAVE_GNU_LD && at_file_supplied);
    
      tlink_init ();
    
      if (ret)
        {
          int i = 0;
    
          /* Until collect does a better job of figuring out which are object
             files, assume that everything on the command line could be.  */
          if (read_repo_files (ld_argv))
            while (ret && i++ < MAX_ITERATIONS)
              {
                if (tlink_verbose >= 3)
                  {
                    dump_ld_file (ldout, stdout);
                    dump_ld_file (lderrout, stderr);
                  }
                demangle_new_symbols ();
                if (! scan_linker_output (ldout)
                    && ! scan_linker_output (lderrout))
                  break;
                if (! recompile_files ())
                  break;
                if (tlink_verbose)
                  fprintf (stderr, _("collect: relinking\n"));
                ret = tlink_execute ("ld", ld_argv, ldout, lderrout,
                                     HAVE_GNU_LD && at_file_supplied);
              }
        }
    
      dump_ld_file (ldout, stdout);
      unlink (ldout);
      dump_ld_file (lderrout, stderr);
      unlink (lderrout);
    [...]


Grüße,
 Thomas
Thomas Schwinge - Feb. 21, 2015, 6:19 p.m.
Hi!

On Sat, 21 Feb 2015 17:22:40 +0100, I wrote:
> On Mon, 10 Jun 2013 14:01:53 +0200 (CEST), Richard Biener <rguenther@suse.de> wrote:
> > This fixes one very annoying thing collect2 does when trying to
> > debug LTO WPA issues.  Even with -v you need to wait until all
> > LTRANS stages completed to see the lto1 -fwpa invocation which
> > is because collect2 buffers and replays stdout/stderr of ld
> > (to avoid duplicating that in some cases).  But I really want
> > to see the output immediately but there is no way to do that.
> > The easiest is to disable the buffering with -debug (that is,
> > -Wl,-debug to the -flto driver command line).
> > 
> > Tested with/without -debug.
> > 
> > Ok for trunk?
> 
> > 	* collect2.c (main): Do not redirect ld stdout/stderr when
> > 	debugging.
> 
> > --- gcc/collect2.c	(revision 199732)
> > +++ gcc/collect2.c	(working copy)
> > @@ -1189,8 +1189,11 @@ main (int argc, char **argv)
> >  #ifdef COLLECT_EXPORT_LIST
> >    export_file = make_temp_file (".x");
> >  #endif
> > -  ldout = make_temp_file (".ld");
> > -  lderrout = make_temp_file (".le");
> > +  if (!debug)
> > +    {
> > +      ldout = make_temp_file (".ld");
> > +      lderrout = make_temp_file (".le");
> > +    }
> >    *c_ptr++ = c_file_name;
> >    *c_ptr++ = "-x";
> >    *c_ptr++ = "c";
> 
> This change (r199936) is problematic, given the usage of ldout and
> lderrout in gcc/tlink.c:do_tlink.  If, for -debug, they're not
> initialized, they'll be NULL, which in do_tlink, the tlink_execute call
> will handle fine (as I understand it), but after that, for example,
> dump_ld_file will attempt to fopen (NULL), which will cause a SIGSEGV on
> GNU Hurd at least.  (Correct me if I'm wrong -- I have not yet read the
> relevant standards in detail -- but from what I remember, that's
> appropriate: NULL is not a string naming a file.)
> 
> I found this when running binutils' gold testsuite:
> 
>     $ gcc-4.9 -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fmerge-constants -g -O2 -fno-use-linker-plugin -o incremental_test -Bgcctestdir/ -Wl,--incremental-full incremental_test_1.o incremental_test_2.o -v -Wl,-debug
>     [...]
>     gcc-4.9: internal compiler error: Segmentation fault (program collect2)
>     [...]
> 
> In do_tlink, the last four uses of ldout and lderrout should be guarded
> by NULL and empty string checks (as done in gcc/collect2.c:tool_cleanup),
> but I'm not sure what the correct fix is in the »if (ret)« block:
> 
>     void
>     do_tlink (char **ld_argv, char **object_lst ATTRIBUTE_UNUSED)
>     {
>       int ret = tlink_execute ("ld", ld_argv, ldout, lderrout,
>                                HAVE_GNU_LD && at_file_supplied);
>     
>       tlink_init ();
>     
>       if (ret)
>         {
>           int i = 0;
>     
>           /* Until collect does a better job of figuring out which are object
>              files, assume that everything on the command line could be.  */
>           if (read_repo_files (ld_argv))
>             while (ret && i++ < MAX_ITERATIONS)
>               {
>                 if (tlink_verbose >= 3)
>                   {
>                     dump_ld_file (ldout, stdout);
>                     dump_ld_file (lderrout, stderr);
>                   }
>                 demangle_new_symbols ();
>                 if (! scan_linker_output (ldout)
>                     && ! scan_linker_output (lderrout))
>                   break;
>                 if (! recompile_files ())
>                   break;
>                 if (tlink_verbose)
>                   fprintf (stderr, _("collect: relinking\n"));
>                 ret = tlink_execute ("ld", ld_argv, ldout, lderrout,
>                                      HAVE_GNU_LD && at_file_supplied);
>               }
>         }
>     
>       dump_ld_file (ldout, stdout);
>       unlink (ldout);
>       dump_ld_file (lderrout, stderr);
>       unlink (lderrout);
>     [...]

By the way, to make progress without having to rebuild Debian's gcc-4.9
package, I had the idea of creating a LD_PRELOAD wrapper to wrap the
fopen (NULL) and unlink (NULL) calls, but this turned out to be an
interesting exercise in its own right: I relatively quickly realized that
I actually need to wrap fopen64 instead of fopen, but it took me longer
to realize why the unlink wrapper just didn't work.  GCC has been happily
optimizing away my path != NULL check, because of:

/usr/include/unistd.h:

    extern int unlink (const char *__name) __THROW __nonnull ((1));

/usr/include/i386-gnu/sys/cdefs.h:

    /* The nonull function attribute allows to mark pointer parameters which
       must not be NULL.  */
    #if __GNUC_PREREQ (3,3)
    # define __nonnull(params) __attribute__ ((__nonnull__ params))
    #else
    # define __nonnull(params)
    #endif

Certainly this is another indication that unlink (NULL) really isn't
meant to be done.  ;-)

Got this resolved by defusing the __nonnull macro.  (See attached.  Not
sure if I'm using the atomic builtins correctly.)


Grüße,
 Thomas
Richard Guenther - Feb. 23, 2015, 9:01 a.m.
On Sat, 21 Feb 2015, Thomas Schwinge wrote:

> Hi!
> 
> On Sat, 21 Feb 2015 17:22:40 +0100, I wrote:
> > On Mon, 10 Jun 2013 14:01:53 +0200 (CEST), Richard Biener <rguenther@suse.de> wrote:
> > > This fixes one very annoying thing collect2 does when trying to
> > > debug LTO WPA issues.  Even with -v you need to wait until all
> > > LTRANS stages completed to see the lto1 -fwpa invocation which
> > > is because collect2 buffers and replays stdout/stderr of ld
> > > (to avoid duplicating that in some cases).  But I really want
> > > to see the output immediately but there is no way to do that.
> > > The easiest is to disable the buffering with -debug (that is,
> > > -Wl,-debug to the -flto driver command line).
> > > 
> > > Tested with/without -debug.
> > > 
> > > Ok for trunk?
> > 
> > > 	* collect2.c (main): Do not redirect ld stdout/stderr when
> > > 	debugging.
> > 
> > > --- gcc/collect2.c	(revision 199732)
> > > +++ gcc/collect2.c	(working copy)
> > > @@ -1189,8 +1189,11 @@ main (int argc, char **argv)
> > >  #ifdef COLLECT_EXPORT_LIST
> > >    export_file = make_temp_file (".x");
> > >  #endif
> > > -  ldout = make_temp_file (".ld");
> > > -  lderrout = make_temp_file (".le");
> > > +  if (!debug)
> > > +    {
> > > +      ldout = make_temp_file (".ld");
> > > +      lderrout = make_temp_file (".le");
> > > +    }
> > >    *c_ptr++ = c_file_name;
> > >    *c_ptr++ = "-x";
> > >    *c_ptr++ = "c";
> > 
> > This change (r199936) is problematic, given the usage of ldout and
> > lderrout in gcc/tlink.c:do_tlink.  If, for -debug, they're not
> > initialized, they'll be NULL, which in do_tlink, the tlink_execute call
> > will handle fine (as I understand it), but after that, for example,
> > dump_ld_file will attempt to fopen (NULL), which will cause a SIGSEGV on
> > GNU Hurd at least.  (Correct me if I'm wrong -- I have not yet read the
> > relevant standards in detail -- but from what I remember, that's
> > appropriate: NULL is not a string naming a file.)
> > 
> > I found this when running binutils' gold testsuite:
> > 
> >     $ gcc-4.9 -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fmerge-constants -g -O2 -fno-use-linker-plugin -o incremental_test -Bgcctestdir/ -Wl,--incremental-full incremental_test_1.o incremental_test_2.o -v -Wl,-debug
> >     [...]
> >     gcc-4.9: internal compiler error: Segmentation fault (program collect2)
> >     [...]
> > 
> > In do_tlink, the last four uses of ldout and lderrout should be guarded
> > by NULL and empty string checks (as done in gcc/collect2.c:tool_cleanup),
> > but I'm not sure what the correct fix is in the »if (ret)« block:
> > 
> >     void
> >     do_tlink (char **ld_argv, char **object_lst ATTRIBUTE_UNUSED)
> >     {
> >       int ret = tlink_execute ("ld", ld_argv, ldout, lderrout,
> >                                HAVE_GNU_LD && at_file_supplied);
> >     
> >       tlink_init ();
> >     
> >       if (ret)
> >         {
> >           int i = 0;
> >     
> >           /* Until collect does a better job of figuring out which are object
> >              files, assume that everything on the command line could be.  */
> >           if (read_repo_files (ld_argv))
> >             while (ret && i++ < MAX_ITERATIONS)
> >               {
> >                 if (tlink_verbose >= 3)
> >                   {
> >                     dump_ld_file (ldout, stdout);
> >                     dump_ld_file (lderrout, stderr);
> >                   }
> >                 demangle_new_symbols ();
> >                 if (! scan_linker_output (ldout)
> >                     && ! scan_linker_output (lderrout))
> >                   break;
> >                 if (! recompile_files ())
> >                   break;
> >                 if (tlink_verbose)
> >                   fprintf (stderr, _("collect: relinking\n"));
> >                 ret = tlink_execute ("ld", ld_argv, ldout, lderrout,
> >                                      HAVE_GNU_LD && at_file_supplied);
> >               }
> >         }
> >     
> >       dump_ld_file (ldout, stdout);
> >       unlink (ldout);
> >       dump_ld_file (lderrout, stderr);
> >       unlink (lderrout);
> >     [...]
> 
> By the way, to make progress without having to rebuild Debian's gcc-4.9
> package, I had the idea of creating a LD_PRELOAD wrapper to wrap the
> fopen (NULL) and unlink (NULL) calls, but this turned out to be an
> interesting exercise in its own right: I relatively quickly realized that
> I actually need to wrap fopen64 instead of fopen, but it took me longer
> to realize why the unlink wrapper just didn't work.  GCC has been happily
> optimizing away my path != NULL check, because of:
> 
> /usr/include/unistd.h:
> 
>     extern int unlink (const char *__name) __THROW __nonnull ((1));
> 
> /usr/include/i386-gnu/sys/cdefs.h:
> 
>     /* The nonull function attribute allows to mark pointer parameters which
>        must not be NULL.  */
>     #if __GNUC_PREREQ (3,3)
>     # define __nonnull(params) __attribute__ ((__nonnull__ params))
>     #else
>     # define __nonnull(params)
>     #endif
> 
> Certainly this is another indication that unlink (NULL) really isn't
> meant to be done.  ;-)
> 
> Got this resolved by defusing the __nonnull macro.  (See attached.  Not
> sure if I'm using the atomic builtins correctly.)

Well, the issue should be as simple as to guard the dump_ld_file
calls properly, like collect2.c tool_cleanup does.

Richard.

Patch

Index: gcc/collect2.c
===================================================================
--- gcc/collect2.c	(revision 199732)
+++ gcc/collect2.c	(working copy)
@@ -1189,8 +1189,11 @@  main (int argc, char **argv)
 #ifdef COLLECT_EXPORT_LIST
   export_file = make_temp_file (".x");
 #endif
-  ldout = make_temp_file (".ld");
-  lderrout = make_temp_file (".le");
+  if (!debug)
+    {
+      ldout = make_temp_file (".ld");
+      lderrout = make_temp_file (".le");
+    }
   *c_ptr++ = c_file_name;
   *c_ptr++ = "-x";
   *c_ptr++ = "c";