[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Do not redirect ld stdout/stderr in collect2 with -debug
From: |
Thomas Schwinge |
Subject: |
Re: [PATCH] Do not redirect ld stdout/stderr in collect2 with -debug |
Date: |
Sat, 21 Feb 2015 19:19:10 +0100 |
User-agent: |
Notmuch/0.9-101-g81dad07 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
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
#include <sys/cdefs.h>
#undef __nonnull
#define __nonnull(params)
#include <dlfcn.h>
#include <stdio.h>
#include <unistd.h>
FILE *fopen64(const char *path, const char *mode)
{
if (path == NULL)
return NULL;
static FILE *(*fopen64_)(const char*, const char*) = NULL;
if (__atomic_load_n (&fopen64_, __ATOMIC_SEQ_CST) == NULL)
{
void *fopen64__ = dlsym(RTLD_NEXT, __FUNCTION__);
__atomic_store_n (&fopen64_, fopen64__, __ATOMIC_SEQ_CST);
}
return __atomic_load_n (&fopen64_, __ATOMIC_SEQ_CST)(path, mode);
}
int unlink(const char *path)
{
if (path == NULL)
return 0;
static int (*unlink_)(const char*) = NULL;
if (__atomic_load_n (&unlink_, __ATOMIC_SEQ_CST) == NULL)
{
void *unlink__ = dlsym(RTLD_NEXT, __FUNCTION__);
__atomic_store_n (&unlink_, unlink__, __ATOMIC_SEQ_CST);
}
return __atomic_load_n (&unlink_, __ATOMIC_SEQ_CST)(path);
}
/* gcc -shared -o fopen,unlink-NULL-wrapper.so fopen,unlink-NULL-wrapper.c
-Wall -D_GNU_SOURCE -O2 -fPIC -ldl */
signature.asc
Description: PGP signature