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

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

[Maxim Kuvyrkov]

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

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.

Martin, you are the author of stmt_may_be_vtbl_ptr_store; is there any reason 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.ChangeLog --]
[-- Type: application/octet-stream, Size: 150 bytes --]

2011-09-30  Maxim Kuvyrkov  <maxim@codesourcery.com>

	* ipa-prop.c (stmt_may_be_vtbl_ptr_store): Assume only ADDR_EXPRs can
	be assigned to vtables.

[-- Attachment #3: 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;
 }

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

[Martin Jambor]

[-- 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;
 }

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

[Martin Jambor]

On Mon, Oct 10, 2011 at 05:05:14PM +0200, Martin Jambor wrote:
> Hi,
> 
> sorry that taking care of the devirtualization patches takes me longer
> than expected for various reasons.  But I have not forgotten about
> this.

Ah, please ignore the attachment, I was trying to persuade mutt to add
it to the original email so that I could comment on it inline but it
re-attached it instead, for some weird reason.

Thanks,

Martin

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

[Maxim Kuvyrkov]

On 11/10/2011, at 4:05 AM, Martin Jambor wrote:

> 
> 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?

It appears to be enough.  The patch I sent was developed against GCC 4.6 and I didn't notice your change when updated it for 4.7.  Thanks for fixing this.

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics