| Message ID | alpine.LNX.2.00.1306101358420.26078@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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.
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";