diff mbox

Fix materialization wrt multiple decls with same assembler name

Message ID 20100820115413.GD9926@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka Aug. 20, 2010, 11:54 a.m. UTC 
Hi,
LTO sreaming in is reading in all function that do have corresponding sections
(based on assebmler name).

This breaks when we have two decls with same assembler name (that is the case
of bultins). In this case we would attempt to read same body to two different
declarations that naturally breaks in many interesting ways and thus we test
DECL_IS_BUILTIN to avoid materialization.

This test is just symptomatic, since it won't work with user aliases and such.
I am also running into problem with DECL_IS_BUILTIN returning true for all
compiler born functions since it test locator and UNKNOWN_LOCATION is considred
to be builtin.

This patch changes lto_materialize_function to work out from callgraph if
function body is needed instead of checking presence of the section.  This is
the case when cgraph node is analyzed or when it have such clone.

Bootstrapped/regtested x86_64, testing with mozilla build now, OK?

Honza

	* lto/lto.c (has_analyzed_clone_p): New function
	(lto_materialize_function): Use callgraph to determine if
	body is needed.
	(materialize_cgraph): Remove DECL_IS_BUILTIN check.

Comments

Diego Novillo Aug. 20, 2010, 12:32 p.m. UTC | #1 
On 10-08-20 07:54 , Jan Hubicka wrote:

> 	* lto/lto.c (has_analyzed_clone_p): New function
> 	(lto_materialize_function): Use callgraph to determine if
> 	body is needed.
> 	(materialize_cgraph): Remove DECL_IS_BUILTIN check.

OK with

> Index: lto/lto.c
> ===================================================================
> --- lto/lto.c	(revision 163374)
> +++ lto/lto.c	(working copy)
> @@ -117,6 +117,34 @@ lto_materialize_constructors_and_inits (
>   			 data, len);
>   }
>
> +/* Return true when NODE has clone that is analyzed (i.e. we need

s/has clone/has a clone/


Diego.
H.J. Lu Aug. 20, 2010, 2:36 p.m. UTC | #2 
On Fri, Aug 20, 2010 at 4:54 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> LTO sreaming in is reading in all function that do have corresponding sections
> (based on assebmler name).
>
> This breaks when we have two decls with same assembler name (that is the case
> of bultins). In this case we would attempt to read same body to two different
> declarations that naturally breaks in many interesting ways and thus we test
> DECL_IS_BUILTIN to avoid materialization.
>
> This test is just symptomatic, since it won't work with user aliases and such.
> I am also running into problem with DECL_IS_BUILTIN returning true for all
> compiler born functions since it test locator and UNKNOWN_LOCATION is considred
> to be builtin.
>
> This patch changes lto_materialize_function to work out from callgraph if
> function body is needed instead of checking presence of the section.  This is
> the case when cgraph node is analyzed or when it have such clone.
>
> Bootstrapped/regtested x86_64, testing with mozilla build now, OK?
>
> Honza
>
>        * lto/lto.c (has_analyzed_clone_p): New function
>        (lto_materialize_function): Use callgraph to determine if
>        body is needed.
>        (materialize_cgraph): Remove DECL_IS_BUILTIN check.

Your checkin is different from you posted and caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45357
diff mbox

Patch

Index: lto/lto.c
===================================================================
--- lto/lto.c	(revision 163374)
+++ lto/lto.c	(working copy)
@@ -117,6 +117,34 @@  lto_materialize_constructors_and_inits (
 			 data, len);
 }
 
+/* Return true when NODE has clone that is analyzed (i.e. we need
+   to load its body even if the node itself is not needed).  */
+
+static bool
+has_analyzed_clone_p (struct cgraph_node *node)
+{
+  struct cgraph_node *orig = node;
+  node = node->clones;
+  if (node)
+    while (node != orig)
+      {
+	if (node->analyzed)
+	  return true;
+	if (node->clones)
+	  node = node->clones;
+	else if (node->next_sibling_clone)
+	  node = node->next_sibling_clone;
+	else
+	  {
+	    while (node != orig && !node->next_sibling_clone)
+	      node = node->clone_of;
+	    if (node != orig)
+	      node = node->next_sibling_clone;
+	  }
+      }
+  return false;
+}
+
 /* Read the function body for the function associated with NODE.  */
 
 static void
@@ -127,28 +155,30 @@  lto_materialize_function (struct cgraph_
   const char *data, *name;
   size_t len;
 
-  /* Ignore clone nodes.  Read the body only from the original one.
-     We may find clone nodes during LTRANS after WPA has made inlining
-     decisions.  */
-  if (node->clone_of)
-    return;
-
   decl = node->decl;
-  file_data = node->local.lto_file_data;
-  name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); 
-
-  /* We may have renamed the declaration, e.g., a static function.  */
-  name = lto_get_decl_name_mapping (file_data, name);
-
-  data = lto_get_section_data (file_data, LTO_section_function_body,
-			       name, &len);
-  if (data)
+  /* Read in functions with body (analyzed nodes)
+     and also functions that are needed to produce virtual clones.  */
+  if (node->analyzed || has_analyzed_clone_p (node))
     {
-      gcc_assert (!DECL_IS_BUILTIN (decl));
-
       /* This function has a definition.  */
       TREE_STATIC (decl) = 1;
 
+      /* Clones don't need to be read.  */
+      if (node->clone_of)
+	return;
+      file_data = node->local.lto_file_data;
+      name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)); 
+
+      /* We may have renamed the declaration, e.g., a static function.  */
+      name = lto_get_decl_name_mapping (file_data, name);
+
+      data = lto_get_section_data (file_data, LTO_section_function_body,
+				   name, &len);
+      if (!data)
+	fatal_error ("%s: section %s is missing",
+		     file_data->file_name,
+		     name);
+
       gcc_assert (DECL_STRUCT_FUNCTION (decl) == NULL);
 
       /* Load the function body only if not operating in WPA mode.  In
@@ -1862,17 +1892,7 @@  materialize_cgraph (void)
 
   for (node = cgraph_nodes; node; node = node->next)
     {
-      /* Some cgraph nodes get created on the fly, and they don't need
-	 to be materialized.  For instance, nodes for nested functions
-	 where the parent function was not streamed out or builtin
-	 functions.  Additionally, builtin functions should not be
-	 materialized and may, in fact, cause confusion because there
-	 may be a regular function in the file whose assembler name
-	 matches that of the function.
-	 See gcc.c-torture/execute/20030125-1.c and
-	 gcc.c-torture/execute/921215-1.c.  */
-      if (node->local.lto_file_data
-          && !DECL_IS_BUILTIN (node->decl))
+      if (node->local.lto_file_data)
 	{
 	  lto_materialize_function (node);
 	  lto_stats.num_input_cgraph_nodes++;