Re: Improve handling of COMDAT vtables and virtual functoins

[Jan Hubicka <hubicka at ucw dot cz>]

> On 10/26/2010 09:33 PM, Jan Hubicka wrote:
>> !       if (vnode->analyzed&&  !prevailing_node->analyzed)
>> !         fatal_error ("Wrong reoslution for %s: prevailing symbol is not defined",
>> ! 		    IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (vnode->decl)));
>
> Typo.
Hi,
we still should error here (and I fixed the typo in meantime, too), however the
problem I saw was symptom of the fact that C++ might produce COMDAT vtable and
then mark is as forced for output.  WHOPR code did not understand this and it
is why we got this type mismatch.

I bootstrapped/regtested the following patch that avoid the lto-symtab.c change
and will commit it shortly.
We still don't handle correctly comdats with attribute used that I will handle incrementally
(as well as the lto-symtab diagnostics)

Honza

	* cgraph.h (cgraph_comdat_node_localizable): Declare.
	* lto-streamer-out.c (produce_symtab): Use it; do not output
	symtab entries for weak vtables.
	* ipa.c (cgraph_comdat_node_localizable): Break out from ...;
	virtual functions can be privatized.
	(cgraph_externally_visible_p): ... from here.
	(varpool_externally_visible_p): Use it.

Index: cgraph.h
===================================================================
*** cgraph.h	(revision 165990)
--- cgraph.h	(working copy)
*************** void varpool_node_set_remove (varpool_no
*** 695,700 ****
--- 695,701 ----
  void dump_varpool_node_set (FILE *, varpool_node_set);
  void debug_varpool_node_set (varpool_node_set);
  void ipa_discover_readonly_nonaddressable_vars (void);
+ bool cgraph_comdat_node_localizable (struct cgraph_node *);
  
  /* In predict.c  */
  bool cgraph_maybe_hot_edge_p (struct cgraph_edge *e);
Index: lto-streamer-out.c
===================================================================
*** lto-streamer-out.c	(revision 165990)
--- lto-streamer-out.c	(working copy)
*************** produce_symtab (struct output_block *ob,
*** 2487,2493 ****
        if (DECL_EXTERNAL (node->decl))
  	continue;
        if (DECL_COMDAT (node->decl)
! 	  && cgraph_can_remove_if_no_direct_calls_p (node))
  	continue;
        if (node->alias || node->global.inlined_to)
  	continue;
--- 2487,2493 ----
        if (DECL_EXTERNAL (node->decl))
  	continue;
        if (DECL_COMDAT (node->decl)
! 	  && cgraph_comdat_node_localizable (node))
  	continue;
        if (node->alias || node->global.inlined_to)
  	continue;
*************** produce_symtab (struct output_block *ob,
*** 2501,2507 ****
        if (!DECL_EXTERNAL (node->decl))
  	continue;
        if (DECL_COMDAT (node->decl)
! 	  && cgraph_can_remove_if_no_direct_calls_p (node))
  	continue;
        if (node->alias || node->global.inlined_to)
  	continue;
--- 2501,2507 ----
        if (!DECL_EXTERNAL (node->decl))
  	continue;
        if (DECL_COMDAT (node->decl)
! 	  && cgraph_comdat_node_localizable (node))
  	continue;
        if (node->alias || node->global.inlined_to)
  	continue;
*************** produce_symtab (struct output_block *ob,
*** 2516,2521 ****
--- 2516,2528 ----
        vnode = lto_varpool_encoder_deref (varpool_encoder, i);
        if (DECL_EXTERNAL (vnode->decl))
  	continue;
+       /* COMDAT virtual tables can be unshared.  Do not declare them
+ 	 in the LTO symbol table to prevent linker from forcing them
+ 	 into the output. */
+       if (DECL_COMDAT (vnode->decl)
+ 	  && !vnode->force_output
+ 	  && (!vnode->finalized || DECL_VIRTUAL_P (vnode->decl)))
+ 	continue;
        if (vnode->alias)
  	continue;
        write_symbol (cache, &stream, vnode->decl, seen, false);
*************** produce_symtab (struct output_block *ob,
*** 2527,2532 ****
--- 2534,2543 ----
        vnode = lto_varpool_encoder_deref (varpool_encoder, i);
        if (!DECL_EXTERNAL (vnode->decl))
  	continue;
+       if (DECL_COMDAT (vnode->decl)
+ 	  && !vnode->force_output
+ 	  && (!vnode->finalized || DECL_VIRTUAL_P (vnode->decl)))
+ 	continue;
        if (vnode->alias)
  	continue;
        write_symbol (cache, &stream, vnode->decl, seen, false);
Index: ipa.c
===================================================================
*** ipa.c	(revision 165990)
--- ipa.c	(working copy)
*************** ipa_discover_readonly_nonaddressable_var
*** 597,602 ****
--- 597,632 ----
      fprintf (dump_file, "\n");
  }
  
+ /* COMDAT functions must be shared only if they have address taken,
+    otherwise we can produce our own private implementation with
+    -fwhole-program.  
+    Return true when turning COMDAT functoin static can not lead to wrong
+    code when the resulting object links with a library defining same COMDAT.
+ 
+    Virtual functions do have their addresses taken from the vtables,
+    but in C++ there is no way to compare their addresses for equality.  */
+ 
+ bool
+ cgraph_comdat_node_localizable (struct cgraph_node *node)
+ {
+   if ((node->address_taken && !DECL_VIRTUAL_P (node->decl))
+       || !node->analyzed)
+     return false;
+   if (node->same_comdat_group)
+     {
+       struct cgraph_node *next;
+ 
+       /* If more than one function is in the same COMDAT group, it must
+          be shared even if just one function in the comdat group has
+          address taken.  */
+       for (next = node->same_comdat_group;
+ 	   next != node; next = next->same_comdat_group)
+ 	if (next->address_taken && !DECL_VIRTUAL_P (next->decl))
+ 	  return false;
+     }
+   return true;
+ }
+ 
  /* Return true when function NODE should be considered externally visible.  */
  
  static bool
*************** cgraph_externally_visible_p (struct cgra
*** 622,627 ****
--- 652,666 ----
    if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES (node->decl)))
      return true;
  
+   /* When doing LTO or whole program, we can bring COMDAT functoins static.
+      This improves code quality and we know we will duplicate them at most twice
+      (in the case that we are not using plugin and link with object file
+       implementing same COMDAT)  */
+   if ((in_lto_p || whole_program)
+       && DECL_COMDAT (node->decl)
+       && cgraph_comdat_node_localizable (node))
+     return false;
+ 
    /* See if we have linker information about symbol not being used or
       if we need to make guess based on the declaration.
  
*************** cgraph_externally_visible_p (struct cgra
*** 644,670 ****
      ;
    else if (!whole_program)
      return true;
-   /* COMDAT functions must be shared only if they have address taken,
-      otherwise we can produce our own private implementation with
-      -fwhole-program.  */
-   else if (DECL_COMDAT (node->decl))
-     {
-       if (node->address_taken || !node->analyzed)
- 	return true;
-       if (node->same_comdat_group)
- 	{
- 	  struct cgraph_node *next;
- 
- 	  /* If more than one function is in the same COMDAT group, it must
- 	     be shared even if just one function in the comdat group has
- 	     address taken.  */
- 	  for (next = node->same_comdat_group;
- 	       next != node;
- 	       next = next->same_comdat_group)
- 	    if (next->address_taken || !next->analyzed)
- 	      return true;
- 	}
-     }
  
    if (MAIN_NAME_P (DECL_NAME (node->decl)))
      return true;
--- 683,688 ----
*************** varpool_externally_visible_p (struct var
*** 710,715 ****
--- 728,743 ----
    if (!alias && vnode->resolution == LDPR_PREVAILING_DEF_IRONLY)
      return false;
  
+   /* As a special case, the COMDAT virutal tables can be unshared.
+      In LTO mode turn vtables into static variables.  The variable is readonly,
+      so this does not enable more optimization, but referring static var
+      is faster for dynamic linking.  Also this match logic hidding vtables
+      from LTO symbol tables.  */
+   if ((in_lto_p || flag_whole_program)
+       && !vnode->force_output
+       && DECL_COMDAT (vnode->decl) && DECL_VIRTUAL_P (vnode->decl))
+     return false;
+ 
    /* When doing link time optimizations, hidden symbols become local.  */
    if (in_lto_p
        && (DECL_VISIBILITY (vnode->decl) == VISIBILITY_HIDDEN