Re: [PATCH] Don't assume that constants can clobber vtbl

[Martin Jambor <mjambor@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

Hi,

sorry that taking care of the devirtualization patches takes me longer
than expected for various reasons.  But I have not forgotten about
this.

On Sat, Oct 01, 2011 at 01:58:57PM +1300, Maxim Kuvyrkov wrote:
> This patch makes detect_type_change analysis assume that only ADDR_EXPRs can be assigned to vtable entries.
> 
> Initially, the patch made a less strict assumption that constants
> are not assigned to vtables.  I then bumped the assumption to "only
> ADDR_EXPRs can be assigned to vtables".  I have this patch since GCC
> 4.6 and did not came across a testcase that would invalidate either
> of the assumptions.
> 

> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 3e56e70..491bb11 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -320,6 +320,7 @@ stmt_may_be_vtbl_ptr_store (gimple stmt)
>    else if (is_gimple_assign (stmt))
>      {
>        tree lhs = gimple_assign_lhs (stmt);
> +      tree rhs;
>                 
>        if (!AGGREGATE_TYPE_P (TREE_TYPE (lhs)))
>         {
> @@ -334,6 +335,13 @@ stmt_may_be_vtbl_ptr_store (gimple stmt)
>              if there is a field corresponding to the offset and if
>         so, procee> d
>              almost like if it was a component ref.  */
>         }

Just before this, there is the following test (guarded by
flag_strict_aliasing per explicit Richi's request). 

	  if (flag_strict_aliasing
	      && !POINTER_TYPE_P (TREE_TYPE (lhs)))
	    return false;

Why is it not enough to catch integer constants?

I'd be OK with ignoring invariants which are not pointers but those
should already be handled by the test above.  Your code also ignores
even variable right hand sides which I think might be dangerous,
e.g. if SRA ever decided to copy an object by copying all fields.

Thanks,

Martin


> +
> +      /* Assume that only ADDR_EXPRs can be assigned to vtables.
> +        In particular, ignore *ptr = <const_int> when searching for aliasing
> +        entries.  */
> +      rhs = gimple_assign_rhs1 (stmt);
> +      if (TREE_CODE (rhs) != ADDR_EXPR)
> +       return false;
>      }
>    return true;
>  }
> 


> 
> Martin, you are the author of stmt_may_be_vtbl_ptr_store; is there
> any reaso> n to assume that something other than ADDR_EXPR can be
> assigned to a vtable?
> 
> Bootstrapped and regtested on x86_64-linux-gnu {-m64/-m32} with no regressions.
> 
> OK for trunk?
> 
> Thank you,
> 
> --
> Maxim Kuvyrkov
> CodeSourcery / Mentor Graphics
> 
> 


[-- Attachment #2: fsf-gcc-vtbl-assign.patch --]
[-- Type: application/octet-stream, Size: 974 bytes --]

commit 443712c075426eec018222b0147f7f5350f3c0b3
Author: Maxim Kuvyrkov <maxim@codesourcery.com>
Date:   Thu Mar 17 08:27:33 2011 -0700

    Improve detect_type_change

diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 3e56e70..491bb11 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -320,6 +320,7 @@ stmt_may_be_vtbl_ptr_store (gimple stmt)
   else if (is_gimple_assign (stmt))
     {
       tree lhs = gimple_assign_lhs (stmt);
+      tree rhs;
 
       if (!AGGREGATE_TYPE_P (TREE_TYPE (lhs)))
 	{
@@ -334,6 +335,13 @@ stmt_may_be_vtbl_ptr_store (gimple stmt)
 	     if there is a field corresponding to the offset and if so, proceed
 	     almost like if it was a component ref.  */
 	}
+
+      /* Assume that only ADDR_EXPRs can be assigned to vtables.
+	 In particular, ignore *ptr = <const_int> when searching for aliasing
+	 entries.  */
+      rhs = gimple_assign_rhs1 (stmt);
+      if (TREE_CODE (rhs) != ADDR_EXPR)
+	return false;
     }
   return true;
 }