diff mbox

Proper fix for PR58477

Message ID 20140708003041.GF12716@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka July 8, 2014, 12:30 a.m. UTC
Hi,
I had to revert change to ipa-prop.c that made stmt_may_be_vtbl_ptr_store
to know that clobbers can not be such stores.

This triggered byg in detect_type_change_from_memory_writes. This function does
two things. First is that it tries to prove that vtbl pointer was not modified
in the function body. Second it tries to prove that the modification is a known
type.  For this it needs to find proper vtbl store along every path to the call.
This is not what happens because it only checks that all vtbl stores it found
are of the same type.  The object may come pre-initialized to the function.

The bug on stopping of clobbers sort of fixed the issue, because we consider only
object in declarations and those are constructed in a dominating block, but it
works by accident.

This is patch we discussed back then adding interface to walk_aliased_vdefs
declaring whether entry of function was reached and avoids stmt_may_be_vtbl_ptr_store
to jump into conclussion about the dynamic type in that case.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* tree-ssa-alias.c (walk_aliased_vdefs_1): Add FUNCTION_ENTRY_REACHED
	parameter.
	(walk_aliased_vdefs): Likewise.
	* tree-ssa-alias.h (walk_aliased_vdefs): Likewise.
	* ipa-prop.c (stmt_may_be_vtbl_ptr_store): Skip clobbers
	(detect_type_change_from_memory_writes): Check if entry was reached.

Comments

Richard Biener July 8, 2014, 8:46 a.m. UTC | #1
On Tue, 8 Jul 2014, Jan Hubicka wrote:

> Hi,
> I had to revert change to ipa-prop.c that made stmt_may_be_vtbl_ptr_store
> to know that clobbers can not be such stores.
> 
> This triggered byg in detect_type_change_from_memory_writes. This function does
> two things. First is that it tries to prove that vtbl pointer was not modified
> in the function body. Second it tries to prove that the modification is a known
> type.  For this it needs to find proper vtbl store along every path to the call.
> This is not what happens because it only checks that all vtbl stores it found
> are of the same type.  The object may come pre-initialized to the function.
> 
> The bug on stopping of clobbers sort of fixed the issue, because we consider only
> object in declarations and those are constructed in a dominating block, but it
> works by accident.
> 
> This is patch we discussed back then adding interface to walk_aliased_vdefs
> declaring whether entry of function was reached and avoids stmt_may_be_vtbl_ptr_store
> to jump into conclussion about the dynamic type in that case.
> 
> Bootstrapped/regtested x86_64-linux, OK?

Ok.

Thanks,
Richard.

> Honza
> 
> 	* tree-ssa-alias.c (walk_aliased_vdefs_1): Add FUNCTION_ENTRY_REACHED
> 	parameter.
> 	(walk_aliased_vdefs): Likewise.
> 	* tree-ssa-alias.h (walk_aliased_vdefs): Likewise.
> 	* ipa-prop.c (stmt_may_be_vtbl_ptr_store): Skip clobbers
> 	(detect_type_change_from_memory_writes): Check if entry was reached.
> Index: tree-ssa-alias.c
> ===================================================================
> --- tree-ssa-alias.c	(revision 212339)
> +++ tree-ssa-alias.c	(working copy)
> @@ -2643,6 +2643,9 @@ walk_non_aliased_vuses (ao_ref *ref, tre
>     WALKER is called with REF, the current vdef and DATA.  If WALKER
>     returns true the walk is stopped, otherwise it continues.
>  
> +   If function entry is reached, FUNCTION_ENTRY_REACHED is set to true.
> +   The pointer may be NULL and then we do not track this information.
> +
>     At PHI nodes walk_aliased_vdefs forks into one walk for reach
>     PHI argument (but only one walk continues on merge points), the
>     return value is true if any of the walks was successful.
> @@ -2652,8 +2655,11 @@ walk_non_aliased_vuses (ao_ref *ref, tre
>  static unsigned int
>  walk_aliased_vdefs_1 (ao_ref *ref, tree vdef,
>  		      bool (*walker)(ao_ref *, tree, void *), void *data,
> -		      bitmap *visited, unsigned int cnt)
> +		      bitmap *visited, unsigned int cnt,
> +		      bool *function_entry_reached)
>  {
> +  if (function_entry_reached)
> +    *function_entry_reached = false;
>    do
>      {
>        gimple def_stmt = SSA_NAME_DEF_STMT (vdef);
> @@ -2663,7 +2669,11 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
>  	return cnt;
>  
>        if (gimple_nop_p (def_stmt))
> -	return cnt;
> +	{
> +	  if (function_entry_reached)
> +	    *function_entry_reached = true;
> +	  return cnt;
> +	}
>        else if (gimple_code (def_stmt) == GIMPLE_PHI)
>  	{
>  	  unsigned i;
> @@ -2671,7 +2681,8 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
>  	    *visited = BITMAP_ALLOC (NULL);
>  	  for (i = 0; i < gimple_phi_num_args (def_stmt); ++i)
>  	    cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i),
> -					 walker, data, visited, 0);
> +					 walker, data, visited, 0,
> +					 function_entry_reached);
>  	  return cnt;
>  	}
>  
> @@ -2690,7 +2701,8 @@ walk_aliased_vdefs_1 (ao_ref *ref, tree
>  unsigned int
>  walk_aliased_vdefs (ao_ref *ref, tree vdef,
>  		    bool (*walker)(ao_ref *, tree, void *), void *data,
> -		    bitmap *visited)
> +		    bitmap *visited,
> +		    bool *function_entry_reached)
>  {
>    bitmap local_visited = NULL;
>    unsigned int ret;
> @@ -2698,7 +2710,8 @@ walk_aliased_vdefs (ao_ref *ref, tree vd
>    timevar_push (TV_ALIAS_STMT_WALK);
>  
>    ret = walk_aliased_vdefs_1 (ref, vdef, walker, data,
> -			      visited ? visited : &local_visited, 0);
> +			      visited ? visited : &local_visited, 0,
> +			      function_entry_reached);
>    if (local_visited)
>      BITMAP_FREE (local_visited);
>  
> Index: tree-ssa-alias.h
> ===================================================================
> --- tree-ssa-alias.h	(revision 212339)
> +++ tree-ssa-alias.h	(working copy)
> @@ -123,7 +123,8 @@ extern void *walk_non_aliased_vuses (ao_
>  				     void *);
>  extern unsigned int walk_aliased_vdefs (ao_ref *, tree,
>  					bool (*)(ao_ref *, tree, void *),
> -					void *, bitmap *);
> +					void *, bitmap *,
> +					bool *function_entry_reached = NULL);
>  extern void dump_alias_info (FILE *);
>  extern void debug_alias_info (void);
>  extern void dump_points_to_solution (FILE *, struct pt_solution *);
> Index: ipa-prop.c
> ===================================================================
> --- ipa-prop.c	(revision 212339)
> +++ ipa-prop.c	(working copy)
> @@ -638,7 +638,8 @@ stmt_may_be_vtbl_ptr_store (gimple stmt)
>  {
>    if (is_gimple_call (stmt))
>      return false;
> -  /* TODO: Skip clobbers, doing so triggers problem in PR60306.  */
> +  if (gimple_clobber_p (stmt))
> +    return false;
>    else if (is_gimple_assign (stmt))
>      {
>        tree lhs = gimple_assign_lhs (stmt);
> @@ -817,6 +818,7 @@ detect_type_change_from_memory_writes (t
>  {
>    struct type_change_info tci;
>    ao_ref ao;
> +  bool entry_reached = false;
>  
>    gcc_checking_assert (DECL_P (arg)
>  		       || TREE_CODE (arg) == MEM_REF
> @@ -847,13 +849,16 @@ detect_type_change_from_memory_writes (t
>    tci.multiple_types_encountered = false;
>  
>    walk_aliased_vdefs (&ao, gimple_vuse (call), check_stmt_for_type_change,
> -		      &tci, NULL);
> +		      &tci, NULL, &entry_reached);
>    if (!tci.type_maybe_changed)
>      return false;
>  
>    if (!tci.known_current_type
>        || tci.multiple_types_encountered
> -      || offset != 0)
> +      || offset != 0
> +      /* When the walk reached function entry, it means that type
> +	 is set along some paths but not along others.  */
> +      || entry_reached)
>      jfunc->type = IPA_JF_UNKNOWN;
>    else
>      ipa_set_jf_known_type (jfunc, 0, tci.known_current_type, comp_type);
> 
>
diff mbox

Patch

Index: tree-ssa-alias.c
===================================================================
--- tree-ssa-alias.c	(revision 212339)
+++ tree-ssa-alias.c	(working copy)
@@ -2643,6 +2643,9 @@  walk_non_aliased_vuses (ao_ref *ref, tre
    WALKER is called with REF, the current vdef and DATA.  If WALKER
    returns true the walk is stopped, otherwise it continues.
 
+   If function entry is reached, FUNCTION_ENTRY_REACHED is set to true.
+   The pointer may be NULL and then we do not track this information.
+
    At PHI nodes walk_aliased_vdefs forks into one walk for reach
    PHI argument (but only one walk continues on merge points), the
    return value is true if any of the walks was successful.
@@ -2652,8 +2655,11 @@  walk_non_aliased_vuses (ao_ref *ref, tre
 static unsigned int
 walk_aliased_vdefs_1 (ao_ref *ref, tree vdef,
 		      bool (*walker)(ao_ref *, tree, void *), void *data,
-		      bitmap *visited, unsigned int cnt)
+		      bitmap *visited, unsigned int cnt,
+		      bool *function_entry_reached)
 {
+  if (function_entry_reached)
+    *function_entry_reached = false;
   do
     {
       gimple def_stmt = SSA_NAME_DEF_STMT (vdef);
@@ -2663,7 +2669,11 @@  walk_aliased_vdefs_1 (ao_ref *ref, tree
 	return cnt;
 
       if (gimple_nop_p (def_stmt))
-	return cnt;
+	{
+	  if (function_entry_reached)
+	    *function_entry_reached = true;
+	  return cnt;
+	}
       else if (gimple_code (def_stmt) == GIMPLE_PHI)
 	{
 	  unsigned i;
@@ -2671,7 +2681,8 @@  walk_aliased_vdefs_1 (ao_ref *ref, tree
 	    *visited = BITMAP_ALLOC (NULL);
 	  for (i = 0; i < gimple_phi_num_args (def_stmt); ++i)
 	    cnt += walk_aliased_vdefs_1 (ref, gimple_phi_arg_def (def_stmt, i),
-					 walker, data, visited, 0);
+					 walker, data, visited, 0,
+					 function_entry_reached);
 	  return cnt;
 	}
 
@@ -2690,7 +2701,8 @@  walk_aliased_vdefs_1 (ao_ref *ref, tree
 unsigned int
 walk_aliased_vdefs (ao_ref *ref, tree vdef,
 		    bool (*walker)(ao_ref *, tree, void *), void *data,
-		    bitmap *visited)
+		    bitmap *visited,
+		    bool *function_entry_reached)
 {
   bitmap local_visited = NULL;
   unsigned int ret;
@@ -2698,7 +2710,8 @@  walk_aliased_vdefs (ao_ref *ref, tree vd
   timevar_push (TV_ALIAS_STMT_WALK);
 
   ret = walk_aliased_vdefs_1 (ref, vdef, walker, data,
-			      visited ? visited : &local_visited, 0);
+			      visited ? visited : &local_visited, 0,
+			      function_entry_reached);
   if (local_visited)
     BITMAP_FREE (local_visited);
 
Index: tree-ssa-alias.h
===================================================================
--- tree-ssa-alias.h	(revision 212339)
+++ tree-ssa-alias.h	(working copy)
@@ -123,7 +123,8 @@  extern void *walk_non_aliased_vuses (ao_
 				     void *);
 extern unsigned int walk_aliased_vdefs (ao_ref *, tree,
 					bool (*)(ao_ref *, tree, void *),
-					void *, bitmap *);
+					void *, bitmap *,
+					bool *function_entry_reached = NULL);
 extern void dump_alias_info (FILE *);
 extern void debug_alias_info (void);
 extern void dump_points_to_solution (FILE *, struct pt_solution *);
Index: ipa-prop.c
===================================================================
--- ipa-prop.c	(revision 212339)
+++ ipa-prop.c	(working copy)
@@ -638,7 +638,8 @@  stmt_may_be_vtbl_ptr_store (gimple stmt)
 {
   if (is_gimple_call (stmt))
     return false;
-  /* TODO: Skip clobbers, doing so triggers problem in PR60306.  */
+  if (gimple_clobber_p (stmt))
+    return false;
   else if (is_gimple_assign (stmt))
     {
       tree lhs = gimple_assign_lhs (stmt);
@@ -817,6 +818,7 @@  detect_type_change_from_memory_writes (t
 {
   struct type_change_info tci;
   ao_ref ao;
+  bool entry_reached = false;
 
   gcc_checking_assert (DECL_P (arg)
 		       || TREE_CODE (arg) == MEM_REF
@@ -847,13 +849,16 @@  detect_type_change_from_memory_writes (t
   tci.multiple_types_encountered = false;
 
   walk_aliased_vdefs (&ao, gimple_vuse (call), check_stmt_for_type_change,
-		      &tci, NULL);
+		      &tci, NULL, &entry_reached);
   if (!tci.type_maybe_changed)
     return false;
 
   if (!tci.known_current_type
       || tci.multiple_types_encountered
-      || offset != 0)
+      || offset != 0
+      /* When the walk reached function entry, it means that type
+	 is set along some paths but not along others.  */
+      || entry_reached)
     jfunc->type = IPA_JF_UNKNOWN;
   else
     ipa_set_jf_known_type (jfunc, 0, tci.known_current_type, comp_type);