[PATCH 1/2] Fix -fno-lto (PR lto/46905)

[PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

This fixes PR lto/46905.

It's sometimes convenient in large Makefiles to globally enable LTO
in CFLAGS, but disable it again for specific files. The simplest
way to do that is appending -fno-lto, but that didn't work.
Add explicit code to handle this case.

Passes bootstrap and full test on x86_64-linux. Ok?

gcc/

2010-12-15  Andi Kleen	<ak@linux.intel.com>

	PR lto/46905
	* collect2.c (main): Handle -fno-lto.
	* common.opt (fno-lto): Add.
	* opts.c (common_handle_option): Handle OPT_fno_lto.
---
 gcc/collect2.c |    2 ++
 gcc/common.opt |    4 ++++
 gcc/opts.c     |    4 ++++
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 89b21d5..77f794f 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -1211,6 +1211,8 @@ main (int argc, char **argv)
         else if ((! strncmp (argv[i], "-flto=", 6)
 		  || ! strcmp (argv[i], "-flto")) && ! use_plugin)
 	  lto_mode = LTO_MODE_WHOPR;
+	else if (!strncmp (argv[i], "-fno-lto", 8))
+	  lto_mode = LTO_MODE_NONE;
         else if (! strcmp (argv[i], "-plugin"))
 	  {
 	    use_plugin = true;
diff --git a/gcc/common.opt b/gcc/common.opt
index 32df6fc..e6cb5e4 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1302,6 +1302,10 @@ floop-optimize
 Common Ignore
 Does nothing.  Preserved for backward compatibility.
 
+fno-lto
+Common RejectNegative
+Disable link-time optimization
+
 flto
 Common
 Enable link-time optimization.
diff --git a/gcc/opts.c b/gcc/opts.c
index 2c8e767..fc6f272 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1673,6 +1673,10 @@ common_handle_option (struct gcc_options *opts,
       opts->x_flag_lto = "";
       break;
 
+    case OPT_fno_lto:
+      opts->x_flag_lto = NULL;
+      break;
+
     case OPT_w:
       dc->dc_inhibit_warnings = true;
       break;
-- 
1.7.1

[PATCH 2/2] Improve reporting of section conflict errors

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

When making large projects LTO clean section conflicts can be surprisingly
common. Currently they are hard to debug. This patch improves the error
reporting a bit by printing out what flags actually differ.

It's still not great -- better would be to print the other locations
that actually conflict too. But that would be more complicated.

Passes bootstrap and full test on x86_64-linux. Ok?

gcc/

2010-12-15  Andi Kleen <ak@linux.intel.com>

	* varasm.c (section_print_flags): Add.
	(get_section): Print more details on conflicting flags.
---
 gcc/varasm.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 62 insertions(+), 1 deletions(-)

diff --git a/gcc/varasm.c b/gcc/varasm.c
index ed44610..f676255 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -268,6 +268,59 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
   return sect;
 }
 
+/* Print section flags F into BUF */
+
+static char *
+section_print_flags (char *buf, unsigned f)
+{
+  static struct flag
+  {
+    const char *name;
+    unsigned flag;
+    unsigned mask;
+  } flags[] =
+      {
+	{ "code", SECTION_CODE, 0 },
+	{ "write", SECTION_WRITE, 0 },
+	{ "debug", SECTION_DEBUG, 0},
+	{ "linkonce", SECTION_LINKONCE, 0 },
+	{ "small", SECTION_SMALL, 0 },
+	{ "bss", SECTION_BSS, 0 },
+	{ "forget", SECTION_FORGET, 0 },
+	{ "merge", SECTION_MERGE, 0 },
+	{ "strings", SECTION_STRINGS, 0 },
+	{ "override", SECTION_OVERRIDE, 0 },
+	{ "tls", SECTION_TLS, 0 },
+	{ "common", SECTION_COMMON, 0 },
+	{ "unnamed", SECTION_UNNAMED, SECTION_STYLE_MASK },
+	{ "named", SECTION_NAMED, SECTION_STYLE_MASK },
+	{ "noswitch", SECTION_NOSWITCH, SECTION_STYLE_MASK },
+	{ NULL, 0, 0 }
+      };
+  struct flag *fl;
+  char *start = buf;
+
+  f &= ~SECTION_OVERRIDE;
+  buf[0] = 0;
+  for (fl = &flags[0]; fl->name; fl++)
+    {
+      unsigned mask = fl->mask;
+
+      if (mask == 0)
+        mask = 0xffffffff;
+      if ((f & mask) & fl->flag)
+        {
+	  strcpy (buf, fl->name);
+	  strcat (buf, " ");
+	  buf += strlen (buf);
+	  f &= ~fl->flag;
+        }
+    }
+  if (f)
+    sprintf (buf, "rest %x", f);
+  return start;
+}
+
 /* Return the named section structure associated with NAME.  Create
    a new section with the given fields if no such structure exists.  */
 
@@ -290,15 +343,23 @@ get_section (const char *name, unsigned int flags, tree decl)
     }
   else
     {
+      unsigned oflags;
+
       sect = *slot;
-      if ((sect->common.flags & ~SECTION_DECLARED) != flags
+      oflags = sect->common.flags & ~SECTION_DECLARED;
+      if (oflags != flags
 	  && ((sect->common.flags | flags) & SECTION_OVERRIDE) == 0)
 	{
+	  char buf[1024]; 
+
 	  /* Sanity check user variables for flag changes.  */
 	  if (decl == 0)
 	    decl = sect->named.decl;
 	  gcc_assert (decl);
 	  error ("%+D causes a section type conflict", decl);
+	  inform (DECL_SOURCE_LOCATION (decl), 
+		  "New section declaration differs in: %s", 
+		  section_print_flags (buf, oflags ^ flags));
 	}
     }
   return sect;
-- 
1.7.1

Re: [PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Richard Guenther]

On Thu, Dec 16, 2010 at 1:41 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> This fixes PR lto/46905.
>
> It's sometimes convenient in large Makefiles to globally enable LTO
> in CFLAGS, but disable it again for specific files. The simplest
> way to do that is appending -fno-lto, but that didn't work.
> Add explicit code to handle this case.
>
> Passes bootstrap and full test on x86_64-linux. Ok?

Do you really need the common.opt and opts.c hunks?

> gcc/
>
> 2010-12-15  Andi Kleen  <ak@linux.intel.com>
>
>        PR lto/46905
>        * collect2.c (main): Handle -fno-lto.
>        * common.opt (fno-lto): Add.
>        * opts.c (common_handle_option): Handle OPT_fno_lto.
> ---
>  gcc/collect2.c |    2 ++
>  gcc/common.opt |    4 ++++
>  gcc/opts.c     |    4 ++++
>  3 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index 89b21d5..77f794f 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -1211,6 +1211,8 @@ main (int argc, char **argv)
>         else if ((! strncmp (argv[i], "-flto=", 6)
>                  || ! strcmp (argv[i], "-flto")) && ! use_plugin)
>          lto_mode = LTO_MODE_WHOPR;
> +       else if (!strncmp (argv[i], "-fno-lto", 8))
> +         lto_mode = LTO_MODE_NONE;
>         else if (! strcmp (argv[i], "-plugin"))
>          {
>            use_plugin = true;
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 32df6fc..e6cb5e4 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1302,6 +1302,10 @@ floop-optimize
>  Common Ignore
>  Does nothing.  Preserved for backward compatibility.
>
> +fno-lto
> +Common RejectNegative
> +Disable link-time optimization
> +
>  flto
>  Common
>  Enable link-time optimization.
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 2c8e767..fc6f272 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -1673,6 +1673,10 @@ common_handle_option (struct gcc_options *opts,
>       opts->x_flag_lto = "";
>       break;
>
> +    case OPT_fno_lto:
> +      opts->x_flag_lto = NULL;
> +      break;
> +
>     case OPT_w:
>       dc->dc_inhibit_warnings = true;
>       break;
> --
> 1.7.1
>
>

Re: [PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Andi Kleen]

Richard Guenther <richard.guenther@gmail.com> writes:

> On Thu, Dec 16, 2010 at 1:41 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> From: Andi Kleen <ak@linux.intel.com>
>>
>> This fixes PR lto/46905.
>>
>> It's sometimes convenient in large Makefiles to globally enable LTO
>> in CFLAGS, but disable it again for specific files. The simplest
>> way to do that is appending -fno-lto, but that didn't work.
>> Add explicit code to handle this case.
>>
>> Passes bootstrap and full test on x86_64-linux. Ok?
>
> Do you really need the common.opt and opts.c hunks?

Yes. The previous state without them didn't work.

I also tried to do it without opts.c, but setting an 0 initialization
value for the -fno-lto entry, but that didn't work either.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 2/2] Improve reporting of section conflict errors

[Richard Guenther]

On Thu, Dec 16, 2010 at 1:41 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> When making large projects LTO clean section conflicts can be surprisingly
> common. Currently they are hard to debug. This patch improves the error
> reporting a bit by printing out what flags actually differ.
>
> It's still not great -- better would be to print the other locations
> that actually conflict too. But that would be more complicated.
>
> Passes bootstrap and full test on x86_64-linux. Ok?
>
> gcc/
>
> 2010-12-15  Andi Kleen <ak@linux.intel.com>
>
>        * varasm.c (section_print_flags): Add.
>        (get_section): Print more details on conflicting flags.
> ---
>  gcc/varasm.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 62 insertions(+), 1 deletions(-)
>
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index ed44610..f676255 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -268,6 +268,59 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
>   return sect;
>  }
>
> +/* Print section flags F into BUF */
> +
> +static char *
> +section_print_flags (char *buf, unsigned f)
> +{
> +  static struct flag
> +  {
> +    const char *name;
> +    unsigned flag;
> +    unsigned mask;
> +  } flags[] =
> +      {
> +       { "code", SECTION_CODE, 0 },
> +       { "write", SECTION_WRITE, 0 },
> +       { "debug", SECTION_DEBUG, 0},
> +       { "linkonce", SECTION_LINKONCE, 0 },
> +       { "small", SECTION_SMALL, 0 },
> +       { "bss", SECTION_BSS, 0 },
> +       { "forget", SECTION_FORGET, 0 },
> +       { "merge", SECTION_MERGE, 0 },
> +       { "strings", SECTION_STRINGS, 0 },
> +       { "override", SECTION_OVERRIDE, 0 },
> +       { "tls", SECTION_TLS, 0 },
> +       { "common", SECTION_COMMON, 0 },
> +       { "unnamed", SECTION_UNNAMED, SECTION_STYLE_MASK },
> +       { "named", SECTION_NAMED, SECTION_STYLE_MASK },
> +       { "noswitch", SECTION_NOSWITCH, SECTION_STYLE_MASK },
> +       { NULL, 0, 0 }
> +      };
> +  struct flag *fl;
> +  char *start = buf;
> +
> +  f &= ~SECTION_OVERRIDE;

Any reason to do this here and not in the caller?

Can you split out a helper function that does return a reference to
the name for a single section flag?  I suppose it might be useful
on its own.

> +  buf[0] = 0;
> +  for (fl = &flags[0]; fl->name; fl++)
> +    {
> +      unsigned mask = fl->mask;
> +
> +      if (mask == 0)
> +        mask = 0xffffffff;
> +      if ((f & mask) & fl->flag)
> +        {
> +         strcpy (buf, fl->name);
> +         strcat (buf, " ");
> +         buf += strlen (buf);
> +         f &= ~fl->flag;
> +        }
> +    }
> +  if (f)
> +    sprintf (buf, "rest %x", f);
> +  return start;
> +}
> +
>  /* Return the named section structure associated with NAME.  Create
>    a new section with the given fields if no such structure exists.  */
>
> @@ -290,15 +343,23 @@ get_section (const char *name, unsigned int flags, tree decl)
>     }
>   else
>     {
> +      unsigned oflags;
> +
>       sect = *slot;
> -      if ((sect->common.flags & ~SECTION_DECLARED) != flags
> +      oflags = sect->common.flags & ~SECTION_DECLARED;
> +      if (oflags != flags
>          && ((sect->common.flags | flags) & SECTION_OVERRIDE) == 0)
>        {
> +         char buf[1024];

Ick ... but well.  I suppose you could use asprintf in section_print_flags
(which is oddly named, better would be section_flags_string).  The function
shouldn't be timing critical.

I agree the patch is useful.

Richard.

> +
>          /* Sanity check user variables for flag changes.  */
>          if (decl == 0)
>            decl = sect->named.decl;
>          gcc_assert (decl);
>          error ("%+D causes a section type conflict", decl);
> +         inform (DECL_SOURCE_LOCATION (decl),
> +                 "New section declaration differs in: %s",
> +                 section_print_flags (buf, oflags ^ flags));
>        }
>     }
>   return sect;
> --
> 1.7.1
>
>

Re: [PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Richard Guenther]

On Thu, Dec 16, 2010 at 2:20 PM, Andi Kleen <andi@firstfloor.org> wrote:
> Richard Guenther <richard.guenther@gmail.com> writes:
>
>> On Thu, Dec 16, 2010 at 1:41 PM, Andi Kleen <andi@firstfloor.org> wrote:
>>> From: Andi Kleen <ak@linux.intel.com>
>>>
>>> This fixes PR lto/46905.
>>>
>>> It's sometimes convenient in large Makefiles to globally enable LTO
>>> in CFLAGS, but disable it again for specific files. The simplest
>>> way to do that is appending -fno-lto, but that didn't work.
>>> Add explicit code to handle this case.
>>>
>>> Passes bootstrap and full test on x86_64-linux. Ok?
>>
>> Do you really need the common.opt and opts.c hunks?
>
> Yes. The previous state without them didn't work.
>
> I also tried to do it without opts.c, but setting an 0 initialization
> value for the -fno-lto entry, but that didn't work either.

Huh, that's strange.  Joseph, do you have any idea why?  Is it because
of how flags get passed to collect2?

Richard.

> -Andi
>
> --
> ak@linux.intel.com -- Speaking for myself only.
>

Re: [PATCH 2/2] Improve reporting of section conflict errors

[Nathan Froyd]

On Thu, Dec 16, 2010 at 01:41:48PM +0100, Andi Kleen wrote:
> +static char *
> +section_print_flags (char *buf, unsigned f)
> +{
> +  static struct flag
> +  {
> +    const char *name;
> +    unsigned flag;
> +    unsigned mask;
> +  } flags[] =

static const?

-Nathan

Re: [PATCH 2/2] Improve reporting of section conflict errors

[Andi Kleen]

On Thu, Dec 16, 2010 at 02:25:05PM +0100, Richard Guenther wrote:
> > +  struct flag *fl;
> > +  char *start = buf;
> > +
> > +  f &= ~SECTION_OVERRIDE;
> 
> Any reason to do this here and not in the caller?

I can move it to the caller.

> 
> Can you split out a helper function that does return a reference to
> the name for a single section flag?  I suppose it might be useful
> on its own.

What do you mean with single? If you pass only a single flag you should
already know what it is? I think this is general enough for most 
debugging purposes.

> > @@ -290,15 +343,23 @@ get_section (const char *name, unsigned int flags, tree decl)
> >     }
> >   else
> >     {
> > +      unsigned oflags;
> > +
> >       sect = *slot;
> > -      if ((sect->common.flags & ~SECTION_DECLARED) != flags
> > +      oflags = sect->common.flags & ~SECTION_DECLARED;
> > +      if (oflags != flags
> >          && ((sect->common.flags | flags) & SECTION_OVERRIDE) == 0)
> >        {
> > +         char buf[1024];
> 
> Ick ... but well.  I suppose you could use asprintf in section_print_flags
> (which is oddly named, better would be section_flags_string).  The function
> shouldn't be timing critical.

I can rename it.

> 
> I agree the patch is useful.

Is that an approval?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 2/2] Improve reporting of section conflict errors

[Richard Guenther]

On Thu, Dec 16, 2010 at 2:31 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Thu, Dec 16, 2010 at 02:25:05PM +0100, Richard Guenther wrote:
>> > +  struct flag *fl;
>> > +  char *start = buf;
>> > +
>> > +  f &= ~SECTION_OVERRIDE;
>>
>> Any reason to do this here and not in the caller?
>
> I can move it to the caller.

Thanks.

>>
>> Can you split out a helper function that does return a reference to
>> the name for a single section flag?  I suppose it might be useful
>> on its own.
>
> What do you mean with single? If you pass only a single flag you should
> already know what it is? I think this is general enough for most
> debugging purposes.

Single as in expose the local array you have via a helper function.
But well, if you think that's not useful then leave it to whoever would
have a use for it.

>> > @@ -290,15 +343,23 @@ get_section (const char *name, unsigned int flags, tree decl)
>> >     }
>> >   else
>> >     {
>> > +      unsigned oflags;
>> > +
>> >       sect = *slot;
>> > -      if ((sect->common.flags & ~SECTION_DECLARED) != flags
>> > +      oflags = sect->common.flags & ~SECTION_DECLARED;
>> > +      if (oflags != flags
>> >          && ((sect->common.flags | flags) & SECTION_OVERRIDE) == 0)
>> >        {
>> > +         char buf[1024];
>>
>> Ick ... but well.  I suppose you could use asprintf in section_print_flags
>> (which is oddly named, better would be section_flags_string).  The function
>> shouldn't be timing critical.
>
> I can rename it.

Thanks.  Please also make the array const as suggested.

>>
>> I agree the patch is useful.
>
> Is that an approval?

I'd like to see an updated patch with the fixed size array removed.

Richard.

> -Andi
>
> --
> ak@linux.intel.com -- Speaking for myself only.
>

Re: [PATCH 2/2] Improve reporting of section conflict errors

[Andi Kleen]

> 
> >>
> >> I agree the patch is useful.
> >
> > Is that an approval?
> 
> I'd like to see an updated patch with the fixed size array removed.

I hadn't planned planned to do that change. Even with a 64bit flags
word 1024 is unlikely to be ever exceeded. 

-Andi

here's the current patch:

commit 74039621b54efce2a14808a2905367033763f419
Author: Andi Kleen <ak@linux.intel.com>
Date:   Thu Dec 16 10:38:25 2010 +0100

    Improve reporting of section conflict errors
    
    When making large projects LTO clean section conflicts can be surprisingly
    common. Currently they are hard to debug. This patch improves the error
    reporting a bit by printing out what flags actually differ.
    
    It's still not great -- better would be to print the other locations
    that actually conflict too. But that would be more complicated.
    
    Passes bootstrap and full test on x86_64-linux. Ok?
    
    gcc/
    
    2010-12-15  Andi Kleen <ak@linux.intel.com>
    
    	* varasm.c (section_print_flags): Add.
    	(get_section): Print more details on conflicting flags.

diff --git a/gcc/varasm.c b/gcc/varasm.c
index ed44610..55dcae3 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -268,6 +268,58 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
   return sect;
 }
 
+/* Print section flags F into BUF */
+
+static char *
+section_flags_string (char *buf, unsigned f)
+{
+  static const struct flag
+  {
+    const char *name;
+    unsigned flag;
+    unsigned mask;
+  } flags[] =
+      {
+	{ "code", SECTION_CODE, 0 },
+	{ "write", SECTION_WRITE, 0 },
+	{ "debug", SECTION_DEBUG, 0},
+	{ "linkonce", SECTION_LINKONCE, 0 },
+	{ "small", SECTION_SMALL, 0 },
+	{ "bss", SECTION_BSS, 0 },
+	{ "forget", SECTION_FORGET, 0 },
+	{ "merge", SECTION_MERGE, 0 },
+	{ "strings", SECTION_STRINGS, 0 },
+	{ "override", SECTION_OVERRIDE, 0 },
+	{ "tls", SECTION_TLS, 0 },
+	{ "common", SECTION_COMMON, 0 },
+	{ "unnamed", SECTION_UNNAMED, SECTION_STYLE_MASK },
+	{ "named", SECTION_NAMED, SECTION_STYLE_MASK },
+	{ "noswitch", SECTION_NOSWITCH, SECTION_STYLE_MASK },
+	{ NULL, 0, 0 }
+      };
+  const struct flag *fl;
+  char *start = buf;
+
+  buf[0] = 0;
+  for (fl = &flags[0]; fl->name; fl++)
+    {
+      unsigned mask = fl->mask;
+
+      if (mask == 0)
+        mask = 0xffffffff;
+      if ((f & mask) & fl->flag)
+        {
+	  strcpy (buf, fl->name);
+	  strcat (buf, " ");
+	  buf += strlen (buf);
+	  f &= ~fl->flag;
+        }
+    }
+  if (f)
+    sprintf (buf, "rest %x", f);
+  return start;
+}
+
 /* Return the named section structure associated with NAME.  Create
    a new section with the given fields if no such structure exists.  */
 
@@ -290,15 +342,24 @@ get_section (const char *name, unsigned int flags, tree decl)
     }
   else
     {
+      unsigned oflags;
+
       sect = *slot;
-      if ((sect->common.flags & ~SECTION_DECLARED) != flags
+      oflags = sect->common.flags & ~SECTION_DECLARED;
+      if (oflags != flags
 	  && ((sect->common.flags | flags) & SECTION_OVERRIDE) == 0)
 	{
+	  char buf[1024]; 
+
 	  /* Sanity check user variables for flag changes.  */
 	  if (decl == 0)
 	    decl = sect->named.decl;
 	  gcc_assert (decl);
 	  error ("%+D causes a section type conflict", decl);
+	  flags = (oflags ^ flags) & ~SECTION_OVERRIDE;
+	  inform (DECL_SOURCE_LOCATION (decl), 
+		  "New section declaration differs in: %s", 
+		  section_flags_string (buf, flags));
 	}
     }
   return sect;



-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Jan Hubicka]

> On Thu, Dec 16, 2010 at 2:20 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > Richard Guenther <richard.guenther@gmail.com> writes:
> >
> >> On Thu, Dec 16, 2010 at 1:41 PM, Andi Kleen <andi@firstfloor.org> wrote:
> >>> From: Andi Kleen <ak@linux.intel.com>
> >>>
> >>> This fixes PR lto/46905.
> >>>
> >>> It's sometimes convenient in large Makefiles to globally enable LTO
> >>> in CFLAGS, but disable it again for specific files. The simplest
> >>> way to do that is appending -fno-lto, but that didn't work.
> >>> Add explicit code to handle this case.
> >>>
> >>> Passes bootstrap and full test on x86_64-linux. Ok?
> >>
> >> Do you really need the common.opt and opts.c hunks?
> >
> > Yes. The previous state without them didn't work.
> >
> > I also tried to do it without opts.c, but setting an 0 initialization
> > value for the -fno-lto entry, but that didn't work either.
> 
> Huh, that's strange.  Joseph, do you have any idea why?  Is it because
> of how flags get passed to collect2?
I think it is because -flto is not switch but accept string argument?

Honza
> 
> Richard.
> 
> > -Andi
> >
> > --
> > ak@linux.intel.com -- Speaking for myself only.
> >

Re: [PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Joseph S. Myers]

On Thu, 16 Dec 2010, Richard Guenther wrote:

> > I also tried to do it without opts.c, but setting an 0 initialization
> > value for the -fno-lto entry, but that didn't work either.
> 
> Huh, that's strange.  Joseph, do you have any idea why?  Is it because
> of how flags get passed to collect2?

Having both flto and fno-lto in common.opt seems dodgy.  Try making the 
code in opts.c that handles OPT_flto check "value" (value == 0 meaning 
-fno-lto) instead.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 2/2] Improve reporting of section conflict errors

[Joseph S. Myers]

On Thu, 16 Dec 2010, Andi Kleen wrote:

> +/* Print section flags F into BUF */

End comment with ".  */".

> +	{ "code", SECTION_CODE, 0 },
[...]

> +  if (f)
> +    sprintf (buf, "rest %x", f);

Are these strings meant to be English language or literal programming 
language text?  If English, there should be appropriate arrangements for 
translation.

> +	  char buf[1024]; 
> +

I agree a variable size buffer would be better - but the minimum is 
runtime checks to avoid overflow in the case of long translations etc. (so 
don't use sprintf etc. without knowing there is enough space left at 
runtime).

It would be nice to eliminate direct use of sprintf from GCC - even if you 
do know there is enough space, using snprintf is better discipline.  (And 
asprintf/vasprintf - in favour of xasprintf/xvasprintf - if someone could 
just approve <http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01448.html and 
<http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01449.html>.)

> +	  inform (DECL_SOURCE_LOCATION (decl), 
> +		  "New section declaration differs in: %s", 
> +		  section_flags_string (buf, flags));

Diagnostics start with lowercase letters.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH 2/2] Improve reporting of section conflict errors

[Andi Kleen]

Given the excessive requirements imposed by the reviewers 
to solve this simple problem I retract the patch, since
it blows my time budget.

I guess users who need to debug sections problems can 
apply it manually.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Andi Kleen]

On Thu, Dec 16, 2010 at 03:39:33PM +0000, Joseph S. Myers wrote:
> On Thu, 16 Dec 2010, Richard Guenther wrote:
> 
> > > I also tried to do it without opts.c, but setting an 0 initialization
> > > value for the -fno-lto entry, but that didn't work either.
> > 
> > Huh, that's strange.  Joseph, do you have any idea why?  Is it because
> > of how flags get passed to collect2?
> 
> Having both flto and fno-lto in common.opt seems dodgy.  Try making the 
> code in opts.c that handles OPT_flto check "value" (value == 0 meaning 
> -fno-lto) instead.

That worked. Here's an updated version. Ok now?

From: Andi Kleen <ak@linux.intel.com>
Date: Wed, 15 Dec 2010 20:10:22 +0100
Subject: [PATCH] Fix -fno-lto (PR lto/46905)

This fixes PR lto/46905.

It's sometimes convenient in large Makefiles to globally enable LTO
in CFLAGS, but disable it again for specific files. The simplest
way to do that is appending -fno-lto, but that didn't work.
Add explicit code to handle this case.

gcc/

2010-12-15  Andi Kleen	<ak@linux.intel.com>

	PR lto/46905
	* collect2.c (main): Handle -fno-lto.
	* common.opt (fno-lto): Add.
	* opts.c (common_handle_option): Handle OPT_fno_lto.

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 89b21d5..77f794f 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -1211,6 +1211,8 @@ main (int argc, char **argv)
         else if ((! strncmp (argv[i], "-flto=", 6)
 		  || ! strcmp (argv[i], "-flto")) && ! use_plugin)
 	  lto_mode = LTO_MODE_WHOPR;
+	else if (!strncmp (argv[i], "-fno-lto", 8))
+	  lto_mode = LTO_MODE_NONE;
         else if (! strcmp (argv[i], "-plugin"))
 	  {
 	    use_plugin = true;
diff --git a/gcc/opts.c b/gcc/opts.c
index 2c8e767..cc4181e 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1670,7 +1670,7 @@ common_handle_option (struct gcc_options *opts,
       break;
 
     case OPT_flto:
-      opts->x_flag_lto = "";
+      opts->x_flag_lto = value ? "" : NULL;
       break;
 
     case OPT_w:

Re: [PATCH 2/2] Improve reporting of section conflict errors

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]

On Thu, Dec 16, 2010 at 7:46 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Thu, 16 Dec 2010, Andi Kleen wrote:
>
>> +/* Print section flags F into BUF */
>
> End comment with ".  */".
>
>> +     { "code", SECTION_CODE, 0 },
> [...]
>
>> +  if (f)
>> +    sprintf (buf, "rest %x", f);
>
> Are these strings meant to be English language or literal programming
> language text?  If English, there should be appropriate arrangements for
> translation.
>
>> +       char buf[1024];
>> +
>
> I agree a variable size buffer would be better - but the minimum is
> runtime checks to avoid overflow in the case of long translations etc. (so
> don't use sprintf etc. without knowing there is enough space left at
> runtime).
>
> It would be nice to eliminate direct use of sprintf from GCC - even if you
> do know there is enough space, using snprintf is better discipline.  (And
> asprintf/vasprintf - in favour of xasprintf/xvasprintf - if someone could
> just approve <http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01448.html and
> <http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01449.html>.)
>
>> +       inform (DECL_SOURCE_LOCATION (decl),
>> +               "New section declaration differs in: %s",
>> +               section_flags_string (buf, flags));
>
> Diagnostics start with lowercase letters.
>

Is this patch OK for trunk?

Thanks.

-- 
H.J.
---
2010-12-16  Andi Kleen	<ak@linux.intel.com>
	    H.J. Lu  <hongjiu.lu@intel.com>

	* varasm.c (section_flags_string): New.
	(get_section): Print more details on conflicting section flags.

[-- Attachment #2: gcc-section-flag-1.patch --]
[-- Type: text/plain, Size: 2371 bytes --]

2010-12-16  Andi Kleen	<ak@linux.intel.com>
	    H.J. Lu  <hongjiu.lu@intel.com>

	* varasm.c (section_flags_string): New.
	(get_section): Print more details on conflicting section flags.

diff --git a/gcc/varasm.c b/gcc/varasm.c
index ed44610..d91497c 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -268,6 +268,47 @@ get_noswitch_section (unsigned int flags, noswitch_section_callback callback)
   return sect;
 }
 
+/* Return a string for section flags F.  */
+
+static char *
+section_flags_string (unsigned int f)
+{
+  static const struct section_flag
+    {
+      const char *name;
+      unsigned int flag;
+    }
+  section_flags[] =
+    {
+	{ "code", SECTION_CODE },
+	{ "write", SECTION_WRITE },
+	{ "debug", SECTION_DEBUG },
+	{ "linkonce", SECTION_LINKONCE },
+	{ "small", SECTION_SMALL },
+	{ "bss", SECTION_BSS },
+	{ "forget", SECTION_FORGET },
+	{ "merge", SECTION_MERGE },
+	{ "strings", SECTION_STRINGS },
+	{ "override", SECTION_OVERRIDE },
+	{ "tls", SECTION_TLS },
+	{ "common", SECTION_COMMON },
+	{ "unnamed", SECTION_UNNAMED},
+	{ "named", SECTION_NAMED },
+	{ "noswitch", SECTION_NOSWITCH }
+    };
+  unsigned int i;
+  char *flag = NULL;
+  for (i = 0; i < ARRAY_SIZE (section_flags); i++)
+    if ((f & section_flags[i].flag) != 0)
+      {
+	if (flag)
+	  flag = concat (flag, " ", section_flags[i].name, NULL);
+	else
+	  flag = concat (section_flags[i].name, NULL);
+      }
+  return flag;
+}
+
 /* Return the named section structure associated with NAME.  Create
    a new section with the given fields if no such structure exists.  */
 
@@ -290,15 +331,24 @@ get_section (const char *name, unsigned int flags, tree decl)
     }
   else
     {
+      unsigned int oflags;
+
       sect = *slot;
-      if ((sect->common.flags & ~SECTION_DECLARED) != flags
+      oflags = sect->common.flags & ~SECTION_DECLARED;
+      if (oflags != flags
 	  && ((sect->common.flags | flags) & SECTION_OVERRIDE) == 0)
 	{
+	  char *str;
 	  /* Sanity check user variables for flag changes.  */
 	  if (decl == 0)
 	    decl = sect->named.decl;
 	  gcc_assert (decl);
 	  error ("%+D causes a section type conflict", decl);
+	  flags = (oflags ^ flags) & ~SECTION_OVERRIDE;
+	  str = section_flags_string (flags);
+	  inform (DECL_SOURCE_LOCATION (decl), 
+		  "new section declaration differs in: %s", str);
+	  free (str);
 	}
     }
   return sect;

Re: [PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Richard Guenther]

On Thu, Dec 16, 2010 at 5:56 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Thu, Dec 16, 2010 at 03:39:33PM +0000, Joseph S. Myers wrote:
>> On Thu, 16 Dec 2010, Richard Guenther wrote:
>>
>> > > I also tried to do it without opts.c, but setting an 0 initialization
>> > > value for the -fno-lto entry, but that didn't work either.
>> >
>> > Huh, that's strange.  Joseph, do you have any idea why?  Is it because
>> > of how flags get passed to collect2?
>>
>> Having both flto and fno-lto in common.opt seems dodgy.  Try making the
>> code in opts.c that handles OPT_flto check "value" (value == 0 meaning
>> -fno-lto) instead.
>
> That worked. Here's an updated version. Ok now?

Ok.

Can you check if you also need to update the linker plugin to not
claim object files if -fno-lto is in effect?

Btw, what happens with -flto -fno-lto -flto?

Thanks,
Richard.

> From: Andi Kleen <ak@linux.intel.com>
> Date: Wed, 15 Dec 2010 20:10:22 +0100
> Subject: [PATCH] Fix -fno-lto (PR lto/46905)
>
> This fixes PR lto/46905.
>
> It's sometimes convenient in large Makefiles to globally enable LTO
> in CFLAGS, but disable it again for specific files. The simplest
> way to do that is appending -fno-lto, but that didn't work.
> Add explicit code to handle this case.
>
> gcc/
>
> 2010-12-15  Andi Kleen  <ak@linux.intel.com>
>
>        PR lto/46905
>        * collect2.c (main): Handle -fno-lto.
>        * common.opt (fno-lto): Add.
>        * opts.c (common_handle_option): Handle OPT_fno_lto.
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index 89b21d5..77f794f 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -1211,6 +1211,8 @@ main (int argc, char **argv)
>         else if ((! strncmp (argv[i], "-flto=", 6)
>                  || ! strcmp (argv[i], "-flto")) && ! use_plugin)
>          lto_mode = LTO_MODE_WHOPR;
> +       else if (!strncmp (argv[i], "-fno-lto", 8))
> +         lto_mode = LTO_MODE_NONE;
>         else if (! strcmp (argv[i], "-plugin"))
>          {
>            use_plugin = true;
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 2c8e767..cc4181e 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -1670,7 +1670,7 @@ common_handle_option (struct gcc_options *opts,
>       break;
>
>     case OPT_flto:
> -      opts->x_flag_lto = "";
> +      opts->x_flag_lto = value ? "" : NULL;
>       break;
>
>     case OPT_w:
>

Re: [PATCH 1/2] Fix -fno-lto (PR lto/46905)

[Andi Kleen]

On Sat, Dec 18, 2010 at 09:24:39PM +0100, Richard Guenther wrote:
> On Thu, Dec 16, 2010 at 5:56 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > On Thu, Dec 16, 2010 at 03:39:33PM +0000, Joseph S. Myers wrote:
> >> On Thu, 16 Dec 2010, Richard Guenther wrote:
> >>
> >> > > I also tried to do it without opts.c, but setting an 0 initialization
> >> > > value for the -fno-lto entry, but that didn't work either.
> >> >
> >> > Huh, that's strange.  Joseph, do you have any idea why?  Is it because
> >> > of how flags get passed to collect2?
> >>
> >> Having both flto and fno-lto in common.opt seems dodgy.  Try making the
> >> code in opts.c that handles OPT_flto check "value" (value == 0 meaning
> >> -fno-lto) instead.
> >
> > That worked. Here's an updated version. Ok now?
> 
> Ok.

Committed.

> 
> Can you check if you also need to update the linker plugin to not
> claim object files if -fno-lto is in effect?

When -fuse-linker-plugin is specified the plugin will be unconditionally
called and always claim independent of any other options.

> 
> Btw, what happens with -flto -fno-lto -flto?

Does LTO on a simple test.

-Andi