diff mbox

[2/2] Improve reporting of section conflict errors

Message ID 20101216140106.GB20612@basil.fritz.box
State New
Headers show

Commit Message

Andi Kleen Dec. 16, 2010, 2:01 p.m. UTC 
> 
> >>
> >> 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.

Comments

Joseph Myers Dec. 16, 2010, 3:46 p.m. UTC | #1 
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.
Andi Kleen Dec. 16, 2010, 3:53 p.m. UTC | #2 
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
diff mbox

Patch

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;