Patchwork [RFC] Fix for PR driver/47785

login
register
mail settings
Submitter Ramana Radhakrishnan
Date Jan. 22, 2013, 4 p.m.
Message ID <50FEB78A.6050709@arm.com>
Download mbox | patch
Permalink /patch/214579/
State New
Headers show

Comments

Ramana Radhakrishnan - Jan. 22, 2013, 4 p.m.
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.
Richard Guenther - Jan. 23, 2013, 3:36 p.m.
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.
>
>
>
Ramana Radhakrishnan - Jan. 23, 2013, 3:50 p.m.
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
Richard Guenther - Jan. 23, 2013, 3:52 p.m.
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
>
>
Ramana Radhakrishnan - Jan. 23, 2013, 3:53 p.m.
> 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
>>
>>
>

Patch

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;
+}