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

login
register
mail settings
Submitter Andi Kleen
Date Dec. 16, 2010, 12:41 p.m.
Message ID <1292503308-11258-2-git-send-email-andi@firstfloor.org>
Download mbox | patch
Permalink /patch/75752/
State New
Headers show

Comments

Andi Kleen - Dec. 16, 2010, 12:41 p.m.
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(-)
Richard Guenther - Dec. 16, 2010, 1:25 p.m.
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
>
>
Nathan Froyd - Dec. 16, 2010, 1:28 p.m.
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
Andi Kleen - Dec. 16, 2010, 1:31 p.m.
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
Richard Guenther - Dec. 16, 2010, 1:45 p.m.
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.
>

Patch

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;