Fix pr61848, linux kernel miscompile

Alan Modra amodra@gmail.com
Mon Sep 15 04:02:00 GMT 2014


This patch cures the linux kernel boot failure when compiled using
trunk gcc.  (Andrew, apologies for hijacking your bugzilla, I started
work on this before finding the bugzilla..)

At its heart, the problem is caused by merge_decls merging from the
old decl to the new decl, then copying back to the old decl and
discarding the new.  When Jan moved some fields to the symtab,
"copying back to the old decl" was lost for those fields.  Really,
it would be best if merge_decls was rewritten to merge everything to
the kept decl, but here I'm just doing that for fields accessed via
decl_with_vis.symtab_node.

I also make a few other fixes
1)  Trunk does not honour the last section attribute.
	extern char foo;
	char foo __attribute__ ((__section__(".machine")));
	char foo __attribute__ ((__section__(".mymachine")));
    results in a section of ".machine" for foo, rather than
    ".mymachine", the result for previous compilers back to 2.95 and
    possibly beyond.
1b) The comment about issuing "an error if the sections conflict"
    being done "later in decl_attributes" is seriously out of date.
    decl_attributes is called earlier on a fresh decl, so no error is
    issued (which I think makes the code in handle_section_attribute
    issuing this error, dead).  It's been that way since at least 2.95.
2)  TLS model attributes have never been merged as far as I can tell,
    except for #pragma omp threadprivate variables.  I think
	extern int __thread x;
	int __thread x __attribute__ ((__tls_model__("local-exec")));
    ought to result in a local-exec "x" rather than a default model
    "x", but see the "isn't quite correct" comment below.  Fixing that
    will, I think, require changing enum tls_model to make
    TLS_MODEL_REAL different to TLS_MODEL_GLOBAL_DYNAMIC (which will
    also fix the mismatch between enum tls_model and tls_model_names[]).
3)  The patch hunks below in cp/decl.c that just s/olddecl/newdecl/
    are to fix "if (TREE_CODE (newdecl) == FUNCTION_DECL) ...
    else switch (TREE_CODE (olddecl))" which looks horrible to me.
    It's really just cosmetic since we know
    TREE_CODE (newdecl) == TREE_CODE (olddecl) at this point.

Bootstrapped and regression tested x86_64-linux.  OK to apply?

gcc/c/
	PR middle-end/61848
	* c-decl.c (merge_decls): Don't merge section name or tls model
	to newdecl symtab node, instead merge to olddecl.  Override
	existing olddecl section name.  Set tls_model for all thread-local
	vars, not just OMP thread-private ones.  Remove incorrect comment.
gcc/cp/
	PR middle-end/61848
	* decl.c (merge_decls): Don't merge section name, comdat group or
	tls model to newdecl symtab node, instead merge to olddecl.
	Override existing olddecl section name.  Set tls_model for all
	thread-local vars, not just OMP thread-private ones.  Remove
	incorrect comment.

Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 215233)
+++ gcc/c/c-decl.c	(working copy)
@@ -2292,22 +2292,10 @@ merge_decls (tree newdecl, tree olddecl, tree newt
 
   /* Merge the threadprivate attribute.  */
   if (TREE_CODE (olddecl) == VAR_DECL && C_DECL_THREADPRIVATE_P (olddecl))
-    {
-      set_decl_tls_model (newdecl, DECL_TLS_MODEL (olddecl));
-      C_DECL_THREADPRIVATE_P (newdecl) = 1;
-    }
+    C_DECL_THREADPRIVATE_P (newdecl) = 1;
 
   if (CODE_CONTAINS_STRUCT (TREE_CODE (olddecl), TS_DECL_WITH_VIS))
     {
-      /* Merge the section attribute.
-	 We want to issue an error if the sections conflict but that
-	 must be done later in decl_attributes since we are called
-	 before attributes are assigned.  */
-      if ((DECL_EXTERNAL (olddecl) || TREE_PUBLIC (olddecl) || TREE_STATIC (olddecl))
-	  && DECL_SECTION_NAME (newdecl) == NULL
-	  && DECL_SECTION_NAME (olddecl))
-	set_decl_section_name (newdecl, DECL_SECTION_NAME (olddecl));
-
       /* Copy the assembler name.
 	 Currently, it can only be defined in the prototype.  */
       COPY_DECL_ASSEMBLER_NAME (olddecl, newdecl);
@@ -2517,6 +2505,20 @@ merge_decls (tree newdecl, tree olddecl, tree newt
 		  (char *) newdecl + sizeof (struct tree_decl_common),
 		  tree_code_size (TREE_CODE (olddecl)) - sizeof (struct tree_decl_common));
 	  olddecl->decl_with_vis.symtab_node = snode;
+
+	  if ((DECL_EXTERNAL (olddecl)
+	       || TREE_PUBLIC (olddecl)
+	       || TREE_STATIC (olddecl))
+	      && DECL_SECTION_NAME (newdecl) != NULL)
+	    set_decl_section_name (olddecl, DECL_SECTION_NAME (newdecl));
+
+	  /* This isn't quite correct for something like
+		int __thread x attribute ((tls_model ("local-exec")));
+		extern int __thread x;
+	     as we'll lose the "local-exec" model.  */
+	  if (TREE_CODE (olddecl) == VAR_DECL
+	      && DECL_THREAD_LOCAL_P (newdecl))
+	    set_decl_tls_model (olddecl, DECL_TLS_MODEL (newdecl));
 	  break;
 	}
 
Index: gcc/cp/decl.c
===================================================================
--- gcc/cp/decl.c	(revision 215233)
+++ gcc/cp/decl.c	(working copy)
@@ -1970,7 +1970,6 @@ duplicate_decls (tree newdecl, tree olddecl, bool
 	      if (!DECL_LANG_SPECIFIC (newdecl))
 		retrofit_lang_decl (newdecl);
 
-	      set_decl_tls_model (newdecl, DECL_TLS_MODEL (olddecl));
 	      CP_DECL_THREADPRIVATE_P (newdecl) = 1;
 	    }
 	}
@@ -2033,15 +2032,6 @@ duplicate_decls (tree newdecl, tree olddecl, bool
 	    }
 	}
 
-      /* Merge the section attribute.
-	 We want to issue an error if the sections conflict but that must be
-	 done later in decl_attributes since we are called before attributes
-	 are assigned.  */
-      if ((DECL_EXTERNAL (olddecl) || TREE_PUBLIC (olddecl) || TREE_STATIC (olddecl))
-	  && DECL_SECTION_NAME (newdecl) == NULL
-	  && DECL_SECTION_NAME (olddecl) != NULL)
-	set_decl_section_name (newdecl, DECL_SECTION_NAME (olddecl));
-
       if (TREE_CODE (newdecl) == FUNCTION_DECL)
 	{
 	  DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (newdecl)
@@ -2086,19 +2076,6 @@ duplicate_decls (tree newdecl, tree olddecl, bool
   /* Merge the storage class information.  */
   merge_weak (newdecl, olddecl);
 
-  if ((TREE_CODE (olddecl) == FUNCTION_DECL || TREE_CODE (olddecl) == VAR_DECL)
-      && (DECL_EXTERNAL (olddecl) || TREE_PUBLIC (olddecl) || TREE_STATIC (olddecl))
-      && DECL_ONE_ONLY (olddecl))
-    {
-      struct symtab_node *symbol;
-      if (TREE_CODE (olddecl) == FUNCTION_DECL)
-	symbol = cgraph_node::get_create (newdecl);
-      else
-	symbol = varpool_node::get_create (newdecl);
-      symbol->set_comdat_group (symtab_node::get
-	(olddecl)->get_comdat_group ());
-    }
-
   DECL_DEFER_OUTPUT (newdecl) |= DECL_DEFER_OUTPUT (olddecl);
   TREE_PUBLIC (newdecl) = TREE_PUBLIC (olddecl);
   TREE_STATIC (olddecl) = TREE_STATIC (newdecl) |= TREE_STATIC (olddecl);
@@ -2452,12 +2429,12 @@ duplicate_decls (tree newdecl, tree olddecl, bool
     }
   else
     {
-      size_t size = tree_code_size (TREE_CODE (olddecl));
+      size_t size = tree_code_size (TREE_CODE (newdecl));
 
       memcpy ((char *) olddecl + sizeof (struct tree_common),
 	      (char *) newdecl + sizeof (struct tree_common),
 	      sizeof (struct tree_decl_common) - sizeof (struct tree_common));
-      switch (TREE_CODE (olddecl))
+      switch (TREE_CODE (newdecl))
 	{
 	case LABEL_DECL:
 	case VAR_DECL:
@@ -2469,7 +2446,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool
 	  {
             struct symtab_node *snode = NULL;
 
-            if (TREE_CODE (olddecl) == VAR_DECL
+            if (TREE_CODE (newdecl) == VAR_DECL
 		&& (TREE_STATIC (olddecl) || TREE_PUBLIC (olddecl) || DECL_EXTERNAL (olddecl)))
 	      snode = symtab_node::get (olddecl);
 	    memcpy ((char *) olddecl + sizeof (struct tree_decl_common),
@@ -2476,7 +2453,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool
 		    (char *) newdecl + sizeof (struct tree_decl_common),
 		    size - sizeof (struct tree_decl_common)
 		    + TREE_CODE_LENGTH (TREE_CODE (newdecl)) * sizeof (char *));
-            if (TREE_CODE (olddecl) == VAR_DECL)
+            if (TREE_CODE (newdecl) == VAR_DECL)
 	      olddecl->decl_with_vis.symtab_node = snode;
 	  }
 	  break;
@@ -2488,6 +2465,38 @@ duplicate_decls (tree newdecl, tree olddecl, bool
 	  break;
 	}
     }
+
+  if (TREE_CODE (newdecl) == FUNCTION_DECL
+      || TREE_CODE (newdecl) == VAR_DECL)
+    {
+      if (DECL_EXTERNAL (olddecl)
+	  || TREE_PUBLIC (olddecl)
+	  || TREE_STATIC (olddecl))
+	{
+	  /* Merge the section attribute.
+	     We want to issue an error if the sections conflict but that must be
+	     done later in decl_attributes since we are called before attributes
+	     are assigned.  */
+	  if (DECL_SECTION_NAME (newdecl) != NULL)
+	    set_decl_section_name (olddecl, DECL_SECTION_NAME (newdecl));
+
+	  if (DECL_ONE_ONLY (newdecl))
+	    {
+	      struct symtab_node *oldsym, *newsym;
+	      if (TREE_CODE (olddecl) == FUNCTION_DECL)
+		oldsym = cgraph_node::get_create (olddecl);
+	      else
+		oldsym = varpool_node::get_create (olddecl);
+	      newsym = symtab_node::get (newdecl);
+	      oldsym->set_comdat_group (newsym->get_comdat_group ());
+	    }
+	}
+
+      if (TREE_CODE (newdecl) == VAR_DECL
+	  && DECL_THREAD_LOCAL_P (newdecl))
+	set_decl_tls_model (olddecl, DECL_TLS_MODEL (newdecl));
+    }
+
   DECL_UID (olddecl) = olddecl_uid;
   if (olddecl_friend)
     DECL_FRIEND_P (olddecl) = 1;

-- 
Alan Modra
Australia Development Lab, IBM



More information about the Gcc-patches mailing list