diff mbox

Fix devirtualization wrong code in Firefox

Message ID 20160113084200.GE5527@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka Jan. 13, 2016, 8:42 a.m. UTC
Hi,
this patch fix wrong code with Firefox and LTO which Martin Liska tracked down.
The problem is that unlike DECL_ABSTRACT_ORIGIN that always point to transitive
closure of origin, BLOCK_ABSTRACT_ORIGIN can iterate. In firefox we see a
function split by ipa-split which contains a construction we inlined from.
Consequently, the BLOCK_ABSTRACT_ORIGIN is an another block whose
BLOCK_ABSTRACT_ORIGIN is the ctor instead of BLOCK_ABSTRACT_ORIGIN beaing ctor
idrectly as exxpected by inlined_polymorphic_ctor_dtor_block_p.

Bootstrapped/regtested x86_64-linux, will commit it tonight if there are no
complains.  Note that the function block_origin is different from 
ultimate_block_origin used by dwarf2out because it walks abstract blocks that
is needed, because the blocks we seek for are abstract.

I think the release branches are also affected.

Honza

	* ipa-polymorphic-call.c (block_origin): New function.
	(inlined_polymorphic_ctor_dtor_block_p,
	noncall_stmt_may_be_vtbl_ptr_store): Use it.

Comments

Richard Biener Jan. 13, 2016, 12:37 p.m. UTC | #1
On Wed, Jan 13, 2016 at 9:42 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> this patch fix wrong code with Firefox and LTO which Martin Liska tracked down.
> The problem is that unlike DECL_ABSTRACT_ORIGIN that always point to transitive
> closure of origin, BLOCK_ABSTRACT_ORIGIN can iterate. In firefox we see a
> function split by ipa-split which contains a construction we inlined from.
> Consequently, the BLOCK_ABSTRACT_ORIGIN is an another block whose
> BLOCK_ABSTRACT_ORIGIN is the ctor instead of BLOCK_ABSTRACT_ORIGIN beaing ctor
> idrectly as exxpected by inlined_polymorphic_ctor_dtor_block_p.
>
> Bootstrapped/regtested x86_64-linux, will commit it tonight if there are no
> complains.  Note that the function block_origin is different from
> ultimate_block_origin used by dwarf2out because it walks abstract blocks that
> is needed, because the blocks we seek for are abstract.
>
> I think the release branches are also affected.
>
> Honza
>
>         * ipa-polymorphic-call.c (block_origin): New function.
>         (inlined_polymorphic_ctor_dtor_block_p,
>         noncall_stmt_may_be_vtbl_ptr_store): Use it.
> Index: ipa-polymorphic-call.c
> ===================================================================
> --- ipa-polymorphic-call.c      (revision 232293)
> +++ ipa-polymorphic-call.c      (working copy)
> @@ -477,6 +477,20 @@ contains_type_p (tree outer_type, HOST_W
>                                           consider_bases);
>  }
>
> +/* Unlike ABSTRACT_ORIGIN, the BLOCK_ABSTRACT_ORIGIN can iterate. Look up
> +   the real origin of BLOCK.  */
> +
> +static tree
> +block_origin (tree block)
> +{
> +  if (!BLOCK_ABSTRACT_ORIGIN (block))
> +    return NULL;
> +  while (TREE_CODE (block) == BLOCK && BLOCK_ABSTRACT_ORIGIN (block)
> +        && BLOCK_ABSTRACT_ORIGIN (block) != block)
> +    block = BLOCK_ABSTRACT_ORIGIN (block);
> +  return DECL_P (block) ? DECL_ORIGIN (block) : block;
> +}

block_ultimate_origin ()?

> +
>
>  /* Return a FUNCTION_DECL if BLOCK represents a constructor or destructor.
>     If CHECK_CLONES is true, also check for clones of ctor/dtors.  */
> @@ -484,7 +498,7 @@ contains_type_p (tree outer_type, HOST_W
>  tree
>  inlined_polymorphic_ctor_dtor_block_p (tree block, bool check_clones)
>  {
> -  tree fn = BLOCK_ABSTRACT_ORIGIN (block);
> +  tree fn = block_origin (block);
>    if (fn == NULL || TREE_CODE (fn) != FUNCTION_DECL)
>      return NULL_TREE;
>
> @@ -1143,7 +1157,7 @@ noncall_stmt_may_be_vtbl_ptr_store (gimp
>    for (tree block = gimple_block (stmt); block && TREE_CODE (block) == BLOCK;
>         block = BLOCK_SUPERCONTEXT (block))
>      if (BLOCK_ABSTRACT_ORIGIN (block)
> -       && TREE_CODE (BLOCK_ABSTRACT_ORIGIN (block)) == FUNCTION_DECL)
> +       && TREE_CODE (block_origin (block)) == FUNCTION_DECL)
>        return inlined_polymorphic_ctor_dtor_block_p (block, false);
>    return (TREE_CODE (TREE_TYPE (current_function_decl)) == METHOD_TYPE
>           && (DECL_CXX_CONSTRUCTOR_P (current_function_decl)
diff mbox

Patch

Index: ipa-polymorphic-call.c
===================================================================
--- ipa-polymorphic-call.c	(revision 232293)
+++ ipa-polymorphic-call.c	(working copy)
@@ -477,6 +477,20 @@  contains_type_p (tree outer_type, HOST_W
 					  consider_bases);
 }
 
+/* Unlike ABSTRACT_ORIGIN, the BLOCK_ABSTRACT_ORIGIN can iterate. Look up
+   the real origin of BLOCK.  */
+
+static tree
+block_origin (tree block)
+{
+  if (!BLOCK_ABSTRACT_ORIGIN (block))
+    return NULL;
+  while (TREE_CODE (block) == BLOCK && BLOCK_ABSTRACT_ORIGIN (block)
+	 && BLOCK_ABSTRACT_ORIGIN (block) != block)
+    block = BLOCK_ABSTRACT_ORIGIN (block);
+  return DECL_P (block) ? DECL_ORIGIN (block) : block;
+}
+
 
 /* Return a FUNCTION_DECL if BLOCK represents a constructor or destructor.
    If CHECK_CLONES is true, also check for clones of ctor/dtors.  */
@@ -484,7 +498,7 @@  contains_type_p (tree outer_type, HOST_W
 tree
 inlined_polymorphic_ctor_dtor_block_p (tree block, bool check_clones)
 {
-  tree fn = BLOCK_ABSTRACT_ORIGIN (block);
+  tree fn = block_origin (block);
   if (fn == NULL || TREE_CODE (fn) != FUNCTION_DECL)
     return NULL_TREE;
 
@@ -1143,7 +1157,7 @@  noncall_stmt_may_be_vtbl_ptr_store (gimp
   for (tree block = gimple_block (stmt); block && TREE_CODE (block) == BLOCK;
        block = BLOCK_SUPERCONTEXT (block))
     if (BLOCK_ABSTRACT_ORIGIN (block)
-	&& TREE_CODE (BLOCK_ABSTRACT_ORIGIN (block)) == FUNCTION_DECL)
+	&& TREE_CODE (block_origin (block)) == FUNCTION_DECL)
       return inlined_polymorphic_ctor_dtor_block_p (block, false);
   return (TREE_CODE (TREE_TYPE (current_function_decl)) == METHOD_TYPE
 	  && (DECL_CXX_CONSTRUCTOR_P (current_function_decl)