| Message ID | 1292503308-11258-2-git-send-email-andi@firstfloor.org |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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
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. >
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;
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(-)