Re: Do not lose track of resolution info due to tree merging

[Jan Hubicka <hubicka at ucw dot cz>]

Hi,
it turns out that I have hit bit of can of worms here, so I spent most of
yesterday looking into minimal fix.  There are multiple issues remaining - we
forget to output symbols that have only DECL_EXTERNAL flag set; we behave funny
to builtins (in some cases it makes sense in others it does not); we do not
handle well references from extern inlines and constructors.

While preparing previous patch I misread unify_scc and it registers in some
cases prevailed decl instead of prevailing resulting in memory corruption.
This eventually was triggered by assert in get_resolution.

There is other bug that the sanity check in read_cgraph_and_symbols triggers
because we do not output some symbols into the lto symbol table.  This is partly
intended even though these are hacks from early LTO days. To silence false
posities I moved all the (sloppy) logic deciding whether to output symbol
into symtab_node::output_to_lto_symbol_table_p and use consistent check
at stream out time and stream in.

We still may possibly get errors on duplicated decls but I hope we do not have
those in practice anymore. (Those are bugs too) So we may need to end up
disabling this sanity check for release but I hope it won't be needed
and I would like to see if now the mechanizm works reliably, so I have decided
to keep it for now.

Patch survives bootstrap/regtest and lto-bootstrap.  I am giving it bit extra
checking and plan to commit later today unless there are complains.
I will see if I can reproduce the other issues and open separate bugs for them.

Honza

	PR ipa/81360
	* cgraph.h (symtab_node::output_to_lto_symbol_table_p): Declare
	* symtab.c: Include builtins.h
	(symtab_node::output_to_lto_symbol_table_p): Move here
	from lto-streamer-out.c:output_symbol_p.
	* lto-streamer-out.c (write_symbol): Turn early exit to assert.
	(output_symbol_p): Move all logic to symtab.c
	(produce_symtab): Update.

	* lto.c (unify_scc): Register prevailing trees, not trees to be freed.
	(read_cgraph_and_symbols): Use
	symtab_node::output_to_lto_symbol_table_p.
	
Index: cgraph.h
===================================================================
--- cgraph.h	(revision 257412)
+++ cgraph.h	(working copy)
@@ -328,6 +328,9 @@ public:
      or abstract function kept for debug info purposes only.  */
   bool real_symbol_p (void);
 
+  /* Return true when the symbol needs to be output to the LTO symbol table.  */
+  bool output_to_lto_symbol_table_p (void);
+
   /* Determine if symbol declaration is needed.  That is, visible to something
      either outside this translation unit, something magic in the system
      configury. This function is used just during symbol creation.  */
Index: lto/lto.c
===================================================================
--- lto/lto.c	(revision 257442)
+++ lto/lto.c	(working copy)
@@ -1648,13 +1648,16 @@ unify_scc (struct data_in *data_in, unsi
 		{
 		  map2[i*2] = (tree)(uintptr_t)(from + i);
 		  map2[i*2+1] = scc->entries[i];
-		  lto_maybe_register_decl (data_in, scc->entries[i], from + i);
 		}
 	      qsort (map2, len, 2 * sizeof (tree), cmp_tree);
 	      qsort (map, len, 2 * sizeof (tree), cmp_tree);
 	      for (unsigned i = 0; i < len; ++i)
-		streamer_tree_cache_replace_tree (cache, map[2*i],
-						  (uintptr_t)map2[2*i]);
+		{
+		  lto_maybe_register_decl (data_in, map[2*i],
+					   (uintptr_t)map2[2*i]);
+		  streamer_tree_cache_replace_tree (cache, map[2*i],
+						    (uintptr_t)map2[2*i]);
+		}
 	    }
 
 	  /* Free the tree nodes from the read SCC.  */
@@ -2901,8 +2904,12 @@ read_cgraph_and_symbols (unsigned nfiles
 
 	res = snode->lto_file_data->resolution_map->get (snode->decl);
 	if (!res || *res == LDPR_UNKNOWN)
-	  fatal_error (input_location, "missing resolution data for %s",
-		       IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (snode->decl)));
+	  {
+	    if (snode->output_to_lto_symbol_table_p ())
+	      fatal_error (input_location, "missing resolution data for %s",
+		           IDENTIFIER_POINTER
+			     (DECL_ASSEMBLER_NAME (snode->decl)));
+	  }
 	else
           snode->resolution = *res;
       }
Index: lto-streamer-out.c
===================================================================
--- lto-streamer-out.c	(revision 257412)
+++ lto-streamer-out.c	(working copy)
@@ -2598,13 +2598,10 @@ write_symbol (struct streamer_tree_cache
   const char *comdat;
   unsigned char c;
 
-  /* None of the following kinds of symbols are needed in the
-     symbol table.  */
-  if (!TREE_PUBLIC (t)
-      || is_builtin_fn (t)
-      || DECL_ABSTRACT_P (t)
-      || (VAR_P (t) && DECL_HARD_REGISTER (t)))
-    return;
+  gcc_checking_assert (TREE_PUBLIC (t)
+		       && !is_builtin_fn (t)
+		       && !DECL_ABSTRACT_P (t)
+		       && (!VAR_P (t) || !DECL_HARD_REGISTER (t)));
 
   gcc_assert (VAR_OR_FUNCTION_DECL_P (t));
 
@@ -2692,45 +2689,6 @@ write_symbol (struct streamer_tree_cache
   lto_write_data (&slot_num, 4);
 }
 
-/* Return true if NODE should appear in the plugin symbol table.  */
-
-bool
-output_symbol_p (symtab_node *node)
-{
-  struct cgraph_node *cnode;
-  if (!node->real_symbol_p ())
-    return false;
-  /* We keep external functions in symtab for sake of inlining
-     and devirtualization.  We do not want to see them in symbol table as
-     references unless they are really used.  */
-  cnode = dyn_cast <cgraph_node *> (node);
-  if (cnode && (!node->definition || DECL_EXTERNAL (cnode->decl))
-      && cnode->callers)
-    return true;
-
- /* Ignore all references from external vars initializers - they are not really
-    part of the compilation unit until they are used by folding.  Some symbols,
-    like references to external construction vtables can not be referred to at all.
-    We decide this at can_refer_decl_in_current_unit_p.  */
- if (!node->definition || DECL_EXTERNAL (node->decl))
-    {
-      int i;
-      struct ipa_ref *ref;
-      for (i = 0; node->iterate_referring (i, ref); i++)
-	{
-	  if (ref->use == IPA_REF_ALIAS)
-	    continue;
-          if (is_a <cgraph_node *> (ref->referring))
-	    return true;
-	  if (!DECL_EXTERNAL (ref->referring->decl))
-	    return true;
-	}
-      return false;
-    }
-  return true;
-}
-
-
 /* Write an IL symbol table to OB.
    SET and VSET are cgraph/varpool node sets we are outputting.  */
 
@@ -2755,7 +2713,7 @@ produce_symtab (struct output_block *ob)
     {
       symtab_node *node = lsei_node (lsei);
 
-      if (!output_symbol_p (node) || DECL_EXTERNAL (node->decl))
+      if (DECL_EXTERNAL (node->decl) || !node->output_to_lto_symbol_table_p ())
 	continue;
       write_symbol (cache, node->decl, &seen, false);
     }
@@ -2764,7 +2722,7 @@ produce_symtab (struct output_block *ob)
     {
       symtab_node *node = lsei_node (lsei);
 
-      if (!output_symbol_p (node) || !DECL_EXTERNAL (node->decl))
+      if (!DECL_EXTERNAL (node->decl) || !node->output_to_lto_symbol_table_p ())
 	continue;
       write_symbol (cache, node->decl, &seen, false);
     }
Index: symtab.c
===================================================================
--- symtab.c	(revision 257412)
+++ symtab.c	(working copy)
@@ -37,6 +37,7 @@ along with GCC; see the file COPYING3.
 #include "calls.h"
 #include "stringpool.h"
 #include "attribs.h"
+#include "builtins.h"
 
 static const char *ipa_ref_use_name[] = {"read","write","addr","alias","chkp"};
 
@@ -2292,3 +2298,58 @@ symtab_node::binds_to_current_def_p (sym
 
   return false;
 }
+
+/* Return true if symbol should be output to the symbol table.  */
+
+bool
+symtab_node::output_to_lto_symbol_table_p (void)
+{
+  /* Only externally visible symbols matter.  */
+  if (!TREE_PUBLIC (decl))
+    return false;
+  if (!real_symbol_p ())
+    return false;
+  /* FIXME: variables probably should not be considered as real symbols at
+     first place.  */
+  if (VAR_P (decl) && DECL_HARD_REGISTER (decl))
+    return false;
+  /* FIXME: Builtins corresponding to real functions probably should have
+     symbol table entries.  */
+  if (is_builtin_fn (decl))
+    return false;
+
+  /* We have real symbol that should be in symbol table.  However try to trim
+     down the refernces to libraries bit more because linker will otherwise
+     bring unnecesary object files into the final link.
+     FIXME: The following checks can easily be confused i.e. by self recursive
+     function or self-referring variable.  */
+
+  /* We keep external functions in symtab for sake of inlining
+     and devirtualization.  We do not want to see them in symbol table as
+     references unless they are really used.  */
+  cgraph_node *cnode = dyn_cast <cgraph_node *> (this);
+  if (cnode && (!definition || DECL_EXTERNAL (decl))
+      && cnode->callers)
+    return true;
+
+ /* Ignore all references from external vars initializers - they are not really
+    part of the compilation unit until they are used by folding.  Some symbols,
+    like references to external construction vtables can not be referred to at
+    all.  We decide this at can_refer_decl_in_current_unit_p.  */
+ if (!definition || DECL_EXTERNAL (decl))
+    {
+      int i;
+      struct ipa_ref *ref;
+      for (i = 0; iterate_referring (i, ref); i++)
+	{
+	  if (ref->use == IPA_REF_ALIAS)
+	    continue;
+          if (is_a <cgraph_node *> (ref->referring))
+	    return true;
+	  if (!DECL_EXTERNAL (ref->referring->decl))
+	    return true;
+	}
+      return false;
+    }
+  return true;
+}