diff mbox

Fix omnetpp miscompilation

Message ID 20151214180940.GN5527@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka Dec. 14, 2015, 6:09 p.m. UTC 
Hi,
this patch is not so nice fix for quite nasty problem with devirtualization and
decl merging.  Once we determine the call target, we use
possible_polymorphic_call_target_p to check if this is type consistent and if
not we redirect to builtin_unreachable and do not account this as a hint
to the inlining/cloning.

THe testing is done by collecing all polymorphic target of a given virtual call
and checking that the determined target is in the list.  What happens in
omnetpp is that it has external vtables which do not have symbol nodes
associated with them (as they are not use by the code only by the devirt
machinery). Consequently they are not seeen by lto-symtab.c and kept unmerged.
Now ipa-cp has different copy of same vtable than what is used by the the
ipa-devirt and each of them has different function unmerged external decls in
them.  Since ipa-cp pulls out one decl and possible_polymorphic_call_target_p
has other decl and we do not have the the alias link between them, we
incorrectly think the function is not in the list.

The patch makes the virtuals to be always merged, so ODR rule is represented
correctly. I am also working on patch that makes the trensparent alias links
for duplicated decls at node creation time that will also fix the wrong code,
but this patch also improves the hitrate of devirtualization code because
it now can get to the vtable constructors more often (on Firefox from 800
to 3900 devirtualizations), so this patch still makes sense.

An alternative solution I will try again (probably only next stage1) is to
record all those vtables into symbol table at streaming time. I did that in
past but the extra streaming overhead did not seem to pay back for the extra
devirtualizaitons achieved.

Note that the bug to some degree exists at the release branches too.  These
still won't merge declarations that have no symbol tables at all.  I will try
to make a testcase.

Bootstrapped/regtested x86_64-linux, will commit it after re-testing at Firefox
and libreoffice.

Honza
	PR lto/68878
	* lto-symtab.c (lto_symtab_prevailing_virtual_decl): New function.
	* lto-symtab.h (lto_symtab_prevailing_virtual_decl): Declare.
	(lto_symtab_prevailing_decl): Use it.
diff mbox

Patch

Index: lto/lto-symtab.c
===================================================================
--- lto/lto-symtab.c	(revision 231581)
+++ lto/lto-symtab.c	(working copy)
@@ -968,3 +975,33 @@  lto_symtab_merge_symbols (void)
 	}
     }
 }
+
+/* Virtual tables may matter for code generation even if they are not
+   directly refernced by the code because they may be used for devirtualizaiton.
+   For this reason it is important to merge even virtual tables that have no
+   associated symbol table entries.  Without doing so we lose optimization
+   oppurtunities by losing track of the vtable constructor.
+   FIXME: we probably ought to introduce explicit symbol table entries for
+   those before streaming.  */
+
+tree
+lto_symtab_prevailing_virtual_decl (tree decl)
+{
+  gcc_checking_assert (!type_in_anonymous_namespace_p (DECL_CONTEXT (decl))
+		       && DECL_ASSEMBLER_NAME_SET_P (decl));
+
+  symtab_node *n = symtab_node::get_for_asmname
+		     (DECL_ASSEMBLER_NAME (decl));
+  while (n && ((!DECL_EXTERNAL (n->decl) && !TREE_PUBLIC (n->decl))
+	       || !DECL_VIRTUAL_P (n->decl)))
+    n = n->next_sharing_asm_name;
+  if (n)
+    {
+      lto_symtab_prevail_decl (n->decl, decl);
+      decl = n->decl;
+    }
+  else
+    symtab_node::get_create (decl);
+
+  return decl;
+}
Index: lto/lto-symtab.h
===================================================================
--- lto/lto-symtab.h	(revision 231581)
+++ lto/lto-symtab.h	(working copy)
@@ -20,6 +20,7 @@  along with GCC; see the file COPYING3.
 extern void lto_symtab_merge_decls (void);
 extern void lto_symtab_merge_symbols (void);
 extern tree lto_symtab_prevailing_decl (tree decl);
+extern tree lto_symtab_prevailing_virtual_decl (tree decl);
 
 /* Mark DECL to be previailed by PREVAILING.
    Use DECL_ABSTRACT_ORIGIN and DECL_CHAIN as special markers; those do not
@@ -31,6 +32,7 @@  inline void
 lto_symtab_prevail_decl (tree prevailing, tree decl)
 {
   gcc_checking_assert (DECL_ABSTRACT_ORIGIN (decl) != error_mark_node);
+  gcc_assert (TREE_PUBLIC (decl) || DECL_EXTERNAL (decl));
   DECL_CHAIN (decl) = prevailing;
   DECL_ABSTRACT_ORIGIN (decl) = error_mark_node;
 }
@@ -43,5 +45,12 @@  lto_symtab_prevailing_decl (tree decl)
   if (DECL_ABSTRACT_ORIGIN (decl) == error_mark_node)
     return DECL_CHAIN (decl);
   else
-    return decl;
+    {
+      if ((TREE_CODE (decl) == VAR_DECL || TREE_CODE (decl) == FUNCTION_DECL)
+	  && DECL_VIRTUAL_P (decl)
+	  && (TREE_PUBLIC (decl) || DECL_EXTERNAL (decl))
+	  && !symtab_node::get (decl))
+	return lto_symtab_prevailing_virtual_decl (decl);
+      return decl;
+    }
 }