| Message ID | 50FEB78A.6050709@arm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jan 22, 2013 at 5:00 PM, Ramana Radhakrishnan <ramrad01@arm.com> wrote: > Hi, > > I ran into PR driver/47785 when doing some testing with an option passed to > the testsuite and I chose to fix this by putting out COLLECT_AS_OPTIONS as > though these are options for the driver by prepending them with a "'-Wa", > and suffixing them with a "'" character and additionally providing spaces as > duly required. I've chosen a simple implementation. > > Tested in the past with x86_64-linux-gnu and arm-none-eabi cross tests (with > an additional -Wa option passed to the default flags in a site.exp) > > Thoughts ? > > Ok for trunk now or should I stage this for 4.9 ? I don't think this fix will work reliably - it's probably desirable anyway for other uses of -Wa,... but providing a symbol definition is something that needs to be understood by LTO at WPA time, otherwise we will get confusing / wrong symbol resolutions and eventually wrong code generated in the end. Thus with the patch you get some false sense of security I think (consider -fno-fat-lto-objects, you'd get x UNDEFed, and with -ffat-lto-objects you'd get a prevailing IRONLY def but the symbol wasn't in the LTO symbol table and we don't find a prevailing definition at WPA time ...) Thus, I think it at least has to wait ;) Richard. > regards, > Ramana > > > gcc/ > > <DATE> Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> > > PR driver/47785 > * gcc.c (set_collect_as_options): New. > (main): Call this. > * lto-wrapper.c (run_gcc): Handle COLLECT_AS_OPTIONS. > > > testsuite/ > > <DATE> Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> > > PR driver/47785 > * gcc.dg/pr47785.c: New test. > > >
On 01/23/13 15:36, Richard Biener wrote: > On Tue, Jan 22, 2013 at 5:00 PM, Ramana Radhakrishnan <ramrad01@arm.com> wrote: >> Hi, >> >> I ran into PR driver/47785 when doing some testing with an option passed to >> the testsuite and I chose to fix this by putting out COLLECT_AS_OPTIONS as >> though these are options for the driver by prepending them with a "'-Wa", >> and suffixing them with a "'" character and additionally providing spaces as >> duly required. I've chosen a simple implementation. >> >> Tested in the past with x86_64-linux-gnu and arm-none-eabi cross tests (with >> an additional -Wa option passed to the default flags in a site.exp) >> >> Thoughts ? >> >> Ok for trunk now or should I stage this for 4.9 ? > > I don't think this fix will work reliably - it's probably desirable > anyway for other uses > of -Wa,... but providing a symbol definition is something that needs > to be understood > by LTO at WPA time, otherwise we will get confusing / wrong symbol > resolutions and > eventually wrong code generated in the end. Thus with the patch you get some > false sense of security I think (consider -fno-fat-lto-objects, you'd > get x UNDEFed, > and with -ffat-lto-objects you'd get a prevailing IRONLY def but the > symbol wasn't > in the LTO symbol table and we don't find a prevailing definition at > WPA time ...) Can you define a symbol on the command line for the assembler ? I didn't know GAS or assemblers could do that and even if it did, I don't understand why ... The linker allows you a -Wl,--defsym=foo=<val> or whatever you want to do there, so even if the assembler were to have an undef reference to a symbol in an object file, it would get fixed up at link time by the linker. So if you are saying that we need to make LTO handle -Wl, --defsym=sym=<value> I'd envisage the need to handle potential confusion but even that's a separate patch and unrelated to this one ...... :) Intrigued :) cheers, Ramana
On Wed, Jan 23, 2013 at 4:50 PM, Ramana Radhakrishnan <ramrad01@arm.com> wrote: > On 01/23/13 15:36, Richard Biener wrote: >> >> On Tue, Jan 22, 2013 at 5:00 PM, Ramana Radhakrishnan <ramrad01@arm.com> >> wrote: >>> >>> Hi, >>> >>> I ran into PR driver/47785 when doing some testing with an option passed >>> to >>> the testsuite and I chose to fix this by putting out COLLECT_AS_OPTIONS >>> as >>> though these are options for the driver by prepending them with a "'-Wa", >>> and suffixing them with a "'" character and additionally providing spaces >>> as >>> duly required. I've chosen a simple implementation. >>> >>> Tested in the past with x86_64-linux-gnu and arm-none-eabi cross tests >>> (with >>> an additional -Wa option passed to the default flags in a site.exp) >>> >>> Thoughts ? >>> >>> Ok for trunk now or should I stage this for 4.9 ? >> >> >> I don't think this fix will work reliably - it's probably desirable >> anyway for other uses >> of -Wa,... but providing a symbol definition is something that needs >> to be understood >> by LTO at WPA time, otherwise we will get confusing / wrong symbol >> resolutions and >> eventually wrong code generated in the end. Thus with the patch you get >> some >> false sense of security I think (consider -fno-fat-lto-objects, you'd >> get x UNDEFed, >> and with -ffat-lto-objects you'd get a prevailing IRONLY def but the >> symbol wasn't >> in the LTO symbol table and we don't find a prevailing definition at >> WPA time ...) > > > Can you define a symbol on the command line for the assembler ? I didn't > know GAS or assemblers could do that and even if it did, I don't understand > why ... > > The linker allows you a -Wl,--defsym=foo=<val> or whatever you want to do > there, so even if the assembler were to have an undef reference to a symbol > in an object file, it would get fixed up at link time by the linker. > > So if you are saying that we need to make LTO handle -Wl, > --defsym=sym=<value> I'd envisage the need to handle potential confusion but > even that's a separate patch and unrelated to this one ...... :) > > Intrigued :) Well, if you look at the testcase you added with your patch you see -Wa,--defsym=x=42, so the answer is yes. Richard. > cheers, > Ramana > >
> Well, if you look at the testcase you added with your patch you see > -Wa,--defsym=x=42, so the answer is yes. Bah I must be blind. Ramana > > Richard. > >> cheers, >> Ramana >> >> >
diff --git a/gcc/gcc.c b/gcc/gcc.c index 13e93e5..df18317 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -221,6 +221,7 @@ static const char *eval_spec_function (const char *, const char *); static const char *handle_spec_function (const char *); static char *save_string (const char *, int); static void set_collect_gcc_options (void); +static void set_collect_as_options (void); static int do_spec_1 (const char *, int, const char *); static int do_spec_2 (const char *); static void do_option_spec (const char *, const char *); @@ -4047,6 +4048,30 @@ process_command (unsigned int decoded_options_count, infiles[n_infiles].name = 0; } +static void +set_collect_as_options (void) +{ + int ix; + char *opt; + int first_time = TRUE; + /* Build COLLECT_AS_OPTIONS to have all of the options specified to + the compiler. */ + obstack_grow (&collect_obstack, "COLLECT_AS_OPTIONS=", + sizeof ("COLLECT_AS_OPTIONS=") - 1); + + FOR_EACH_VEC_ELT (assembler_options, ix, opt) + { + if (!first_time) + obstack_grow (&collect_obstack, " ", 1); + obstack_grow (&collect_obstack, "\'-Wa,", 5); + obstack_grow (&collect_obstack, opt, strlen (opt)); + obstack_grow (&collect_obstack, "\'", 1); + first_time = FALSE; + } + obstack_grow (&collect_obstack, "\0", 1); + xputenv (XOBFINISH (&collect_obstack, char *)); +} + /* Store switches not filtered out by %<S in spec in COLLECT_GCC_OPTIONS and place that in the environment. */ @@ -6579,6 +6604,8 @@ main (int argc, char **argv) obstack_grow (&collect_obstack, argv[0], strlen (argv[0]) + 1); xputenv (XOBFINISH (&collect_obstack, char *)); + set_collect_as_options (); + /* Set up to remember the pathname of the lto wrapper. */ if (have_c) diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c index 24de743..97786ce 100644 --- a/gcc/lto-wrapper.c +++ b/gcc/lto-wrapper.c @@ -445,6 +445,7 @@ run_gcc (unsigned argc, char *argv[]) char *list_option_full = NULL; const char *linker_output = NULL; const char *collect_gcc, *collect_gcc_options; + const char *collect_as_options; int parallel = 0; int jobserver = 0; bool no_partition = false; @@ -454,6 +455,7 @@ run_gcc (unsigned argc, char *argv[]) unsigned int decoded_options_count; struct obstack argv_obstack; int new_head_argc; + char * collect_gcc_as_options; /* Get the driver and options. */ collect_gcc = getenv ("COLLECT_GCC"); @@ -462,7 +464,14 @@ run_gcc (unsigned argc, char *argv[]) collect_gcc_options = getenv ("COLLECT_GCC_OPTIONS"); if (!collect_gcc_options) fatal ("environment variable COLLECT_GCC_OPTIONS must be set"); - get_options_from_collect_gcc_options (collect_gcc, collect_gcc_options, + + collect_as_options = getenv ("COLLECT_AS_OPTIONS"); + collect_gcc_as_options = (char *)xmalloc (strlen (collect_gcc_options) + + strlen (collect_as_options) + 1); + + strcpy (collect_gcc_as_options, collect_gcc_options); + strcat (collect_gcc_as_options, collect_as_options); + get_options_from_collect_gcc_options (collect_gcc, collect_gcc_as_options, CL_LANG_ALL, &decoded_options, &decoded_options_count); --- /dev/null 2012-12-06 14:23:58.292304805 +0000 +++ ./gcc/testsuite/gcc.dg/pr47785.c 2012-12-13 14:20:31.000000000 +0000 @@ -0,0 +1,11 @@ +/* { dg-options "-flto -Wa,--defsym=x=42" {target lto} } */ +extern int x; +extern void abort (void); +int +main (void) +{ + if ((int)&x != 42) + abort (); + + return 0; +}
Hi, I ran into PR driver/47785 when doing some testing with an option passed to the testsuite and I chose to fix this by putting out COLLECT_AS_OPTIONS as though these are options for the driver by prepending them with a "'-Wa", and suffixing them with a "'" character and additionally providing spaces as duly required. I've chosen a simple implementation. Tested in the past with x86_64-linux-gnu and arm-none-eabi cross tests (with an additional -Wa option passed to the default flags in a site.exp) Thoughts ? Ok for trunk now or should I stage this for 4.9 ? regards, Ramana gcc/ <DATE> Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> PR driver/47785 * gcc.c (set_collect_as_options): New. (main): Call this. * lto-wrapper.c (run_gcc): Handle COLLECT_AS_OPTIONS. testsuite/ <DATE> Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> PR driver/47785 * gcc.dg/pr47785.c: New test.