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

[Richard Guenther <richard.guenther@gmail.com>]

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
>
>