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

[Martin Jambor <mjambor at suse dot cz>]

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: fsf-gcc-vtbl-assign.patch
Description: Binary data