Rewrite lto symtab streaming

Rewrite lto symtab streaming

[Jan Hubicka]

Hi,
this patch fixes another problem seen in Mozilla build.  Here we optimize out
comdat function, but its reference stays in function body in debug info.  THis
makes produce_symtab to output it into symbol table section that makes gold
to prevail all other real copies by this non-existent function.

This patch rewrites symbol table streaming to walk callgraph, varpool and
alias pairs and output only what is really there.

Bootstrapped/regtested x86_64-linux, tested on mozilla build, OK?

Honza

	* lto-streamer.c (write_symbol_vec): Rename to ...
	(write_symbol) ... this one; write only symbol given and when
	present in cache. Sanity check that what is defined is present
	in cgraph/varpool with body/finalized decl.
	(write_symbols_of_kind): Remove.
	(produce_symtab): Take outputblock and sets; use cgraph/varpool/alias
	pairs to produce symtab.
	(produce_asm_for_decls): Update call of produce_symtab; don't do so
	when doing WPA streaming.
Index: lto-streamer-out.c
===================================================================
--- lto-streamer-out.c	(revision 161847)
+++ lto-streamer-out.c	(working copy)
@@ -2262,155 +2262,158 @@ lto_out_decl_state_written_size (struct
 }
 
 
-/* Helper function of write_symbols_of_kind.  CACHE is the streamer
-   cache with all the pickled nodes.  STREAM is the stream where to
-   write the table.  V is a vector with the DECLs that should be on
-   the table.  SEEN is a bitmap of symbols written so far.  */
+/* Write symbol T into STREAM in CACHE. SEEN specify symbols we wrote so far.
+   */
 
 static void
-write_symbol_vec (struct lto_streamer_cache_d *cache,
-		  struct lto_output_stream *stream,
-		  VEC(tree,heap) *v, bitmap seen)
+write_symbol (struct lto_streamer_cache_d *cache,
+	      struct lto_output_stream *stream,
+	      tree t, bitmap seen, bool alias)
 {
-  tree t;
-  int index;
+  const char *name;
+  enum gcc_plugin_symbol_kind kind;
+  enum gcc_plugin_symbol_visibility visibility;
+  int slot_num;
+  uint64_t size;
+  const char *comdat;
+
+  /* None of the following kinds of symbols are needed in the
+     symbol table.  */
+  if (!TREE_PUBLIC (t)
+      || is_builtin_fn (t)
+      || DECL_ABSTRACT (t)
+      || TREE_CODE (t) == RESULT_DECL)
+    return;
+
+  gcc_assert (TREE_CODE (t) == VAR_DECL
+	      || TREE_CODE (t) == FUNCTION_DECL);
+
+  name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (t));
+
+  /* FIXME lto: this is from assemble_name_raw in varasm.c. For some
+     architectures we might have to do the same name manipulations that
+     ASM_OUTPUT_LABELREF does. */
+  if (name[0] == '*')
+    name = &name[1];
+
+  lto_streamer_cache_lookup (cache, t, &slot_num);
+  gcc_assert (slot_num >= 0);
+
+  /* Avoid duplicate symbols. */
+  if (bitmap_bit_p (seen, slot_num))
+    return;
+  else
+    bitmap_set_bit (seen, slot_num);
 
-  for (index = 0; VEC_iterate(tree, v, index, t); index++)
+  if (DECL_EXTERNAL (t))
     {
-      const char *name;
-      enum gcc_plugin_symbol_kind kind;
-      enum gcc_plugin_symbol_visibility visibility;
-      int slot_num;
-      uint64_t size;
-      const char *comdat;
-
-      /* None of the following kinds of symbols are needed in the
-	 symbol table.  */
-      if (!TREE_PUBLIC (t)
-	  || is_builtin_fn (t)
-	  || DECL_ABSTRACT (t)
-	  || TREE_CODE (t) == RESULT_DECL)
-	continue;
-
-      gcc_assert (TREE_CODE (t) == VAR_DECL
-		  || TREE_CODE (t) == FUNCTION_DECL);
-
-      name = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (t));
-
-      /* FIXME lto: this is from assemble_name_raw in varasm.c. For some
-	 architectures we might have to do the same name manipulations that
-	 ASM_OUTPUT_LABELREF does. */
-      if (name[0] == '*')
-	name = &name[1];
-
-      lto_streamer_cache_lookup (cache, t, &slot_num);
-      gcc_assert (slot_num >= 0);
-
-      /* Avoid duplicate symbols. */
-      if (bitmap_bit_p (seen, slot_num))
-	continue;
+      if (DECL_WEAK (t))
+	kind = GCCPK_WEAKUNDEF;
       else
-        bitmap_set_bit (seen, slot_num);
-
-      if (DECL_EXTERNAL (t))
-	{
-	  if (DECL_WEAK (t))
-	    kind = GCCPK_WEAKUNDEF;
-	  else
-	    kind = GCCPK_UNDEF;
-	}
-      else
-	{
-	  if (DECL_WEAK (t))
-	    kind = GCCPK_WEAKDEF;
-	  else if (DECL_COMMON (t))
-	    kind = GCCPK_COMMON;
-	  else
-	    kind = GCCPK_DEF;
-	}
-
-      switch (DECL_VISIBILITY(t))
-	{
-	case VISIBILITY_DEFAULT:
-	  visibility = GCCPV_DEFAULT;
-	  break;
-	case VISIBILITY_PROTECTED:
-	  visibility = GCCPV_PROTECTED;
-	  break;
-	case VISIBILITY_HIDDEN:
-	  visibility = GCCPV_HIDDEN;
-	  break;
-	case VISIBILITY_INTERNAL:
-	  visibility = GCCPV_INTERNAL;
-	  break;
-	}
-
-      if (kind == GCCPK_COMMON
-	  && DECL_SIZE (t)
-	  && TREE_CODE (DECL_SIZE (t)) == INTEGER_CST)
-	size = (((uint64_t) TREE_INT_CST_HIGH (DECL_SIZE (t))) << 32)
-	  | TREE_INT_CST_LOW (DECL_SIZE (t));
-      else
-	size = 0;
-
-      if (DECL_ONE_ONLY (t))
-	comdat = IDENTIFIER_POINTER (DECL_COMDAT_GROUP (t));
+	kind = GCCPK_UNDEF;
+    }
+  else
+    {
+      if (DECL_WEAK (t))
+	kind = GCCPK_WEAKDEF;
+      else if (DECL_COMMON (t))
+	kind = GCCPK_COMMON;
       else
-	comdat = "";
+	kind = GCCPK_DEF;
 
-      lto_output_data_stream (stream, name, strlen (name) + 1);
-      lto_output_data_stream (stream, comdat, strlen (comdat) + 1);
-      lto_output_data_stream (stream, &kind, 1);
-      lto_output_data_stream (stream, &visibility, 1);
-      lto_output_data_stream (stream, &size, 8);
-      lto_output_data_stream (stream, &slot_num, 4);
+      /* When something is defined, it should have node attached.  */
+      gcc_assert (alias || TREE_CODE (t) != VAR_DECL
+		  || varpool_get_node (t)->finalized);
+      gcc_assert (alias || TREE_CODE (t) != FUNCTION_DECL
+		  || (cgraph_get_node (t)
+		      && cgraph_get_node (t)->analyzed));
     }
-}
-
 
-/* Write IL symbols of KIND.  CACHE is the streamer cache with all the
-   pickled nodes.  SEEN is a bitmap of symbols written so far.  */
-
-static void
-write_symbols_of_kind (lto_decl_stream_e_t kind,
-		       struct lto_streamer_cache_d *cache, bitmap seen)
-{
-  struct lto_out_decl_state *out_state;
-  struct lto_output_stream stream;
-  unsigned num_fns =
-    VEC_length (lto_out_decl_state_ptr, lto_function_decl_states);
-  unsigned idx;
-
-  memset (&stream, 0, sizeof (stream));
-  out_state = lto_get_out_decl_state ();
-  write_symbol_vec (cache, &stream, out_state->streams[kind].trees, seen);
-
-  for (idx = 0; idx < num_fns; idx++)
+  switch (DECL_VISIBILITY(t))
     {
-      out_state =
-	VEC_index (lto_out_decl_state_ptr, lto_function_decl_states, idx);
-      write_symbol_vec (cache, &stream, out_state->streams[kind].trees, seen);
+    case VISIBILITY_DEFAULT:
+      visibility = GCCPV_DEFAULT;
+      break;
+    case VISIBILITY_PROTECTED:
+      visibility = GCCPV_PROTECTED;
+      break;
+    case VISIBILITY_HIDDEN:
+      visibility = GCCPV_HIDDEN;
+      break;
+    case VISIBILITY_INTERNAL:
+      visibility = GCCPV_INTERNAL;
+      break;
     }
 
-  lto_write_stream (&stream);
+  if (kind == GCCPK_COMMON
+      && DECL_SIZE (t)
+      && TREE_CODE (DECL_SIZE (t)) == INTEGER_CST)
+    size = (((uint64_t) TREE_INT_CST_HIGH (DECL_SIZE (t))) << 32)
+      | TREE_INT_CST_LOW (DECL_SIZE (t));
+  else
+    size = 0;
+
+  if (DECL_ONE_ONLY (t))
+    comdat = IDENTIFIER_POINTER (DECL_COMDAT_GROUP (t));
+  else
+    comdat = "";
+
+  lto_output_data_stream (stream, name, strlen (name) + 1);
+  lto_output_data_stream (stream, comdat, strlen (comdat) + 1);
+  lto_output_data_stream (stream, &kind, 1);
+  lto_output_data_stream (stream, &visibility, 1);
+  lto_output_data_stream (stream, &size, 8);
+  lto_output_data_stream (stream, &slot_num, 4);
 }
 
 
-/* Write an IL symbol table.  CACHE is the streamer cache with all the
-   pickled nodes.  */
+/* Write an IL symbol table to OB.  */
 
 static void
-produce_symtab (struct lto_streamer_cache_d *cache)
+produce_symtab (struct output_block *ob,
+	        cgraph_node_set set, varpool_node_set vset)
 {
+  struct lto_streamer_cache_d *cache = ob->writer_cache;
   char *section_name = lto_get_section_name (LTO_section_symtab, NULL);
   bitmap seen;
+  struct cgraph_node *node, *alias;
+  struct varpool_node *vnode, *valias;
+  struct lto_output_stream stream;
+  lto_varpool_encoder_t varpool_encoder = ob->decl_state->varpool_node_encoder;
+  lto_cgraph_encoder_t encoder = ob->decl_state->cgraph_node_encoder;
+  int i;
+  alias_pair *p;
 
   lto_begin_section (section_name, false);
   free (section_name);
 
   seen = lto_bitmap_alloc ();
-  write_symbols_of_kind (LTO_DECL_STREAM_FN_DECL, cache, seen);
-  write_symbols_of_kind (LTO_DECL_STREAM_VAR_DECL, cache, seen);
+  memset (&stream, 0, sizeof (stream));
+
+  /* Write all functions.  */
+  for (i = 0; i < lto_cgraph_encoder_size (encoder); i++)
+    {
+      node = lto_cgraph_encoder_deref (encoder, i);
+      write_symbol (cache, &stream, node->decl, seen, false);
+      for (alias = node->same_body; alias; alias = alias->next)
+        write_symbol (cache, &stream, alias->decl, seen, true);
+    }
+
+  /* Write all variables.  */
+  for (i = 0; i < lto_varpool_encoder_size (varpool_encoder); i++)
+    {
+      vnode = lto_varpool_encoder_deref (varpool_encoder, i);
+      write_symbol (cache, &stream, vnode->decl, seen, false);
+      for (valias = vnode->extra_name; valias; valias = valias->next)
+        write_symbol (cache, &stream, valias->decl, seen, true);
+    }
+
+  /* Write all aliases.  */
+  for (i = 0; VEC_iterate (alias_pair, alias_pairs, i, p); i++)
+    if (output_alias_pair_p (p, set, vset))
+      write_symbol (cache, &stream, p->decl, seen, true);
+
+  lto_write_stream (&stream);
   lto_bitmap_free (seen);
 
   lto_end_section ();
@@ -2513,8 +2516,10 @@ produce_asm_for_decls (cgraph_node_set s
 
   lto_end_section ();
 
-  /* Write the symbol table. */
-  produce_symtab (ob->writer_cache);
+  /* Write the symbol table.  It is used by linker to determine dependencies
+     and thus we can skip it for WPA.  */
+  if (!flag_wpa)
+    produce_symtab (ob, set, vset);
 
   /* Write command line opts.  */
   lto_write_options ();

Re: Rewrite lto symtab streaming

[Diego Novillo]

On Mon, Jul 5, 2010 at 21:56, Jan Hubicka <hubicka@ucw.cz> wrote:
>
> Hi,
> this patch fixes another problem seen in Mozilla build.  Here we optimize out
> comdat function, but its reference stays in function body in debug info.  THis
> makes produce_symtab to output it into symbol table section that makes gold
> to prevail all other real copies by this non-existent function.
>
> This patch rewrites symbol table streaming to walk callgraph, varpool and
> alias pairs and output only what is really there.
>
> Bootstrapped/regtested x86_64-linux, tested on mozilla build, OK?
>
> Honza
>
>        * lto-streamer.c (write_symbol_vec): Rename to ...
>        (write_symbol) ... this one; write only symbol given and when
>        present in cache. Sanity check that what is defined is present
>        in cgraph/varpool with body/finalized decl.
>        (write_symbols_of_kind): Remove.
>        (produce_symtab): Take outputblock and sets; use cgraph/varpool/alias
>        pairs to produce symtab.
>        (produce_asm_for_decls): Update call of produce_symtab; don't do so
>        when doing WPA streaming.

Nice cleanup!  OK with some minor comments:

> -/* Helper function of write_symbols_of_kind.  CACHE is the streamer
> -   cache with all the pickled nodes.  STREAM is the stream where to
> -   write the table.  V is a vector with the DECLs that should be on
> -   the table.  SEEN is a bitmap of symbols written so far.  */
> +/* Write symbol T into STREAM in CACHE. SEEN specify symbols we wrote so far.
> +   */

Watch line wrapping.

> +      /* When something is defined, it should have node attached.  */

s/have node/have a node/

I wonder if these should be error/fatal_error instead of assertions.

> +/* Write an IL symbol table to OB.  */
>
>  static void
> -produce_symtab (struct lto_streamer_cache_d *cache)
> +produce_symtab (struct output_block *ob,
> +               cgraph_node_set set, varpool_node_set vset)

VSET and SET need documentation.


Diego.

Re: Rewrite lto symtab streaming

[Jan Hubicka]

> On Mon, Jul 5, 2010 at 21:56, Jan Hubicka <hubicka@ucw.cz> wrote:
> >
> > Hi,
> > this patch fixes another problem seen in Mozilla build.  Here we optimize out
> > comdat function, but its reference stays in function body in debug info.  THis
> > makes produce_symtab to output it into symbol table section that makes gold
> > to prevail all other real copies by this non-existent function.
> >
> > This patch rewrites symbol table streaming to walk callgraph, varpool and
> > alias pairs and output only what is really there.
> >
> > Bootstrapped/regtested x86_64-linux, tested on mozilla build, OK?
> >
> > Honza
> >
> >        * lto-streamer.c (write_symbol_vec): Rename to ...
> >        (write_symbol) ... this one; write only symbol given and when
> >        present in cache. Sanity check that what is defined is present
> >        in cgraph/varpool with body/finalized decl.
> >        (write_symbols_of_kind): Remove.
> >        (produce_symtab): Take outputblock and sets; use cgraph/varpool/alias
> >        pairs to produce symtab.
> >        (produce_asm_for_decls): Update call of produce_symtab; don't do so
> >        when doing WPA streaming.
> 
> Nice cleanup!  OK with some minor comments:
> 
> > -/* Helper function of write_symbols_of_kind.  CACHE is the streamer
> > -   cache with all the pickled nodes.  STREAM is the stream where to
> > -   write the table.  V is a vector with the DECLs that should be on
> > -   the table.  SEEN is a bitmap of symbols written so far.  */
> > +/* Write symbol T into STREAM in CACHE. SEEN specify symbols we wrote so far.
> > +   */
> 
> Watch line wrapping.
> 
> > +      /* When something is defined, it should have node attached.  */
> 
> s/have node/have a node/
> 
> I wonder if these should be error/fatal_error instead of assertions.

Well, those are internal consistency checks rather than something user can
get wrong.   We have a lot of assterts that should be fatals at LTO reading
(and a lot of unchecked errors).  I guess we will need to revamp streaming
sooner or later anyway.

Thanks!
Honza