diff mbox

Fix wrong assumption in contains_type_p (PR ipa/71207).

Message ID 61727576-2f83-3155-c6dc-655e4770d79e@suse.cz
State New
Headers show

Commit Message

Martin Liška Jan. 13, 2017, 3:02 p.m. UTC 
Hello.

As mentioned in the PR, having a diamond virtual inheritance can cause a wrong
assumption done in contains_type_p. I also decided to rename one argument of the
function as otr_type and outer_type names are very confusing. Apart from what was
written in bugzilla I also verified that after the hunk removal, there's no situation
on Firefox where the function would return true, which used to return false.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

Comments

Jan Hubicka Jan. 17, 2017, 10:43 a.m. UTC | #1 
> 

> gcc/testsuite/ChangeLog:

> 

> 2017-01-12  Martin Liska  <mliska@suse.cz>

> 

> 	PR ipa/71207

> 	* g++.dg/ipa/pr71207.C: New test.

> 

> gcc/ChangeLog:

> 

> 2017-01-12  Martin Liska  <mliska@suse.cz>

> 

> 	PR ipa/71207

> 	* ipa-polymorphic-call.c (contains_type_p): Fix wrong

> 	assumption, add comment and renamed otr_type to inner_type.

> ---

>  gcc/ipa-polymorphic-call.c         | 22 ++++++++++----------

>  gcc/testsuite/g++.dg/ipa/pr71207.C | 42 ++++++++++++++++++++++++++++++++++++++

>  2 files changed, 53 insertions(+), 11 deletions(-)

>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr71207.C

> 

> diff --git a/gcc/ipa-polymorphic-call.c b/gcc/ipa-polymorphic-call.c

> index da64ce4c6e0..c13fc858c86 100644

> --- a/gcc/ipa-polymorphic-call.c

> +++ b/gcc/ipa-polymorphic-call.c

> @@ -446,15 +446,15 @@ no_useful_type_info:

>      }

>  }

>  

> -/* Return true if OUTER_TYPE contains OTR_TYPE at OFFSET.

> -   CONSIDER_PLACEMENT_NEW makes function to accept cases where OTR_TYPE can

> +/* Return true if OUTER_TYPE contains INNER_TYPE at OFFSET.

> +   CONSIDER_PLACEMENT_NEW makes function to accept cases where INNER_TYPE can

>     be built within OUTER_TYPE by means of placement new.  CONSIDER_BASES makes

> -   function to accept cases where OTR_TYPE appears as base of OUTER_TYPE or as

> +   function to accept cases where INNER_TYPE appears as base of OUTER_TYPE or as

>     base of one of fields of OUTER_TYPE.  */

>  

>  static bool

>  contains_type_p (tree outer_type, HOST_WIDE_INT offset,

> -		 tree otr_type,

> +		 tree inner_type,


I would actually keep otr_type (or change it consistently in all cases).
otr comes from OBJ_TYPE_REF and is used thorough the code (not my invention,
comming from original binfo walking routine).

OK with that change.  I bleive I added the size check only to cut the recurision
early which is not a big deal.
>  		 bool consider_placement_new,

>  		 bool consider_bases)

>  {

> @@ -463,18 +463,18 @@ contains_type_p (tree outer_type, HOST_WIDE_INT offset,

>    /* Check that type is within range.  */

>    if (offset < 0)

>      return false;

> -  if (TYPE_SIZE (outer_type) && TYPE_SIZE (otr_type)

> -      && TREE_CODE (TYPE_SIZE (outer_type)) == INTEGER_CST

> -      && TREE_CODE (TYPE_SIZE (otr_type)) == INTEGER_CST

> -      && wi::ltu_p (wi::to_offset (TYPE_SIZE (outer_type)),

> -		    (wi::to_offset (TYPE_SIZE (otr_type)) + offset)))

> -    return false;

> +

> +  /* PR ipa/71207

> +     As OUTER_TYPE can be a type which has a diamond virtual inheritance,

> +     it's not necessary that INNER_TYPE will fit within OUTER_TYPE with

> +     a given offset.  It can happen that INNER_TYPE also contains a base object,

> +     however it would point to the same instance in the OUTER_TYPE.  */

>  

>    context.offset = offset;

>    context.outer_type = TYPE_MAIN_VARIANT (outer_type);

>    context.maybe_derived_type = false;

>    context.dynamic = false;

> -  return context.restrict_to_inner_class (otr_type, consider_placement_new,

> +  return context.restrict_to_inner_class (inner_type, consider_placement_new,

>  					  consider_bases);

>  }

>  

> diff --git a/gcc/testsuite/g++.dg/ipa/pr71207.C b/gcc/testsuite/g++.dg/ipa/pr71207.C

> new file mode 100644

> index 00000000000..19a03998460

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/ipa/pr71207.C

> @@ -0,0 +1,42 @@

> +/* PR ipa/71207 */

> +/* { dg-do run } */

> +

> +class Class1

> +{

> +public:

> +  Class1() {};

> +  virtual ~Class1() {};

> +

> +protected:

> +  unsigned Field1;

> +};

> +

> +class Class2 : public virtual Class1

> +{

> +};

> +

> +class Class3 : public virtual Class1

> +{

> +public:

> +  virtual void Method1() = 0;

> +

> +  void Method2()

> +  {

> +    Method1();

> +  }

> +};

> +

> +class Class4 : public Class2, public virtual Class3

> +{

> +public:

> +  Class4() {};

> +  virtual void Method1() {};

> +};

> +

> +int main()

> +{

> +  Class4 var1;

> +  var1.Method2();

> +

> +  return 0;

> +}

> -- 

> 2.11.0

>
Martin Liška Jan. 17, 2017, 3:12 p.m. UTC | #2 
On 01/17/2017 11:43 AM, Jan Hubicka wrote:
>>

>> gcc/testsuite/ChangeLog:

>>

>> 2017-01-12  Martin Liska  <mliska@suse.cz>

>>

>> 	PR ipa/71207

>> 	* g++.dg/ipa/pr71207.C: New test.

>>

>> gcc/ChangeLog:

>>

>> 2017-01-12  Martin Liska  <mliska@suse.cz>

>>

>> 	PR ipa/71207

>> 	* ipa-polymorphic-call.c (contains_type_p): Fix wrong

>> 	assumption, add comment and renamed otr_type to inner_type.

>> ---

>>  gcc/ipa-polymorphic-call.c         | 22 ++++++++++----------

>>  gcc/testsuite/g++.dg/ipa/pr71207.C | 42 ++++++++++++++++++++++++++++++++++++++

>>  2 files changed, 53 insertions(+), 11 deletions(-)

>>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr71207.C

>>

>> diff --git a/gcc/ipa-polymorphic-call.c b/gcc/ipa-polymorphic-call.c

>> index da64ce4c6e0..c13fc858c86 100644

>> --- a/gcc/ipa-polymorphic-call.c

>> +++ b/gcc/ipa-polymorphic-call.c

>> @@ -446,15 +446,15 @@ no_useful_type_info:

>>      }

>>  }

>>  

>> -/* Return true if OUTER_TYPE contains OTR_TYPE at OFFSET.

>> -   CONSIDER_PLACEMENT_NEW makes function to accept cases where OTR_TYPE can

>> +/* Return true if OUTER_TYPE contains INNER_TYPE at OFFSET.

>> +   CONSIDER_PLACEMENT_NEW makes function to accept cases where INNER_TYPE can

>>     be built within OUTER_TYPE by means of placement new.  CONSIDER_BASES makes

>> -   function to accept cases where OTR_TYPE appears as base of OUTER_TYPE or as

>> +   function to accept cases where INNER_TYPE appears as base of OUTER_TYPE or as

>>     base of one of fields of OUTER_TYPE.  */

>>  

>>  static bool

>>  contains_type_p (tree outer_type, HOST_WIDE_INT offset,

>> -		 tree otr_type,

>> +		 tree inner_type,

> 

> I would actually keep otr_type (or change it consistently in all cases).

> otr comes from OBJ_TYPE_REF and is used thorough the code (not my invention,

> comming from original binfo walking routine).

> 

> OK with that change.  I bleive I added the size check only to cut the recurision

> early which is not a big deal.


Ok, applied without the renaming as r244530. I guess you added that to cut the recursion.

Would it be fine to install the patch to active branches after proper testing?
Thanks,
Martin

>>  		 bool consider_placement_new,

>>  		 bool consider_bases)

>>  {

>> @@ -463,18 +463,18 @@ contains_type_p (tree outer_type, HOST_WIDE_INT offset,

>>    /* Check that type is within range.  */

>>    if (offset < 0)

>>      return false;

>> -  if (TYPE_SIZE (outer_type) && TYPE_SIZE (otr_type)

>> -      && TREE_CODE (TYPE_SIZE (outer_type)) == INTEGER_CST

>> -      && TREE_CODE (TYPE_SIZE (otr_type)) == INTEGER_CST

>> -      && wi::ltu_p (wi::to_offset (TYPE_SIZE (outer_type)),

>> -		    (wi::to_offset (TYPE_SIZE (otr_type)) + offset)))

>> -    return false;

>> +

>> +  /* PR ipa/71207

>> +     As OUTER_TYPE can be a type which has a diamond virtual inheritance,

>> +     it's not necessary that INNER_TYPE will fit within OUTER_TYPE with

>> +     a given offset.  It can happen that INNER_TYPE also contains a base object,

>> +     however it would point to the same instance in the OUTER_TYPE.  */

>>  

>>    context.offset = offset;

>>    context.outer_type = TYPE_MAIN_VARIANT (outer_type);

>>    context.maybe_derived_type = false;

>>    context.dynamic = false;

>> -  return context.restrict_to_inner_class (otr_type, consider_placement_new,

>> +  return context.restrict_to_inner_class (inner_type, consider_placement_new,

>>  					  consider_bases);

>>  }

>>  

>> diff --git a/gcc/testsuite/g++.dg/ipa/pr71207.C b/gcc/testsuite/g++.dg/ipa/pr71207.C

>> new file mode 100644

>> index 00000000000..19a03998460

>> --- /dev/null

>> +++ b/gcc/testsuite/g++.dg/ipa/pr71207.C

>> @@ -0,0 +1,42 @@

>> +/* PR ipa/71207 */

>> +/* { dg-do run } */

>> +

>> +class Class1

>> +{

>> +public:

>> +  Class1() {};

>> +  virtual ~Class1() {};

>> +

>> +protected:

>> +  unsigned Field1;

>> +};

>> +

>> +class Class2 : public virtual Class1

>> +{

>> +};

>> +

>> +class Class3 : public virtual Class1

>> +{

>> +public:

>> +  virtual void Method1() = 0;

>> +

>> +  void Method2()

>> +  {

>> +    Method1();

>> +  }

>> +};

>> +

>> +class Class4 : public Class2, public virtual Class3

>> +{

>> +public:

>> +  Class4() {};

>> +  virtual void Method1() {};

>> +};

>> +

>> +int main()

>> +{

>> +  Class4 var1;

>> +  var1.Method2();

>> +

>> +  return 0;

>> +}

>> -- 

>> 2.11.0

>>

>
Jan Hubicka Jan. 17, 2017, 5:19 p.m. UTC | #3 
> 

> Ok, applied without the renaming as r244530. I guess you added that to cut the recursion.

> 

> Would it be fine to install the patch to active branches after proper testing?

OK
Honza
> Thanks,

> Martin

> 

> >>  		 bool consider_placement_new,

> >>  		 bool consider_bases)

> >>  {

> >> @@ -463,18 +463,18 @@ contains_type_p (tree outer_type, HOST_WIDE_INT offset,

> >>    /* Check that type is within range.  */

> >>    if (offset < 0)

> >>      return false;

> >> -  if (TYPE_SIZE (outer_type) && TYPE_SIZE (otr_type)

> >> -      && TREE_CODE (TYPE_SIZE (outer_type)) == INTEGER_CST

> >> -      && TREE_CODE (TYPE_SIZE (otr_type)) == INTEGER_CST

> >> -      && wi::ltu_p (wi::to_offset (TYPE_SIZE (outer_type)),

> >> -		    (wi::to_offset (TYPE_SIZE (otr_type)) + offset)))

> >> -    return false;

> >> +

> >> +  /* PR ipa/71207

> >> +     As OUTER_TYPE can be a type which has a diamond virtual inheritance,

> >> +     it's not necessary that INNER_TYPE will fit within OUTER_TYPE with

> >> +     a given offset.  It can happen that INNER_TYPE also contains a base object,

> >> +     however it would point to the same instance in the OUTER_TYPE.  */

> >>  

> >>    context.offset = offset;

> >>    context.outer_type = TYPE_MAIN_VARIANT (outer_type);

> >>    context.maybe_derived_type = false;

> >>    context.dynamic = false;

> >> -  return context.restrict_to_inner_class (otr_type, consider_placement_new,

> >> +  return context.restrict_to_inner_class (inner_type, consider_placement_new,

> >>  					  consider_bases);

> >>  }

> >>  

> >> diff --git a/gcc/testsuite/g++.dg/ipa/pr71207.C b/gcc/testsuite/g++.dg/ipa/pr71207.C

> >> new file mode 100644

> >> index 00000000000..19a03998460

> >> --- /dev/null

> >> +++ b/gcc/testsuite/g++.dg/ipa/pr71207.C

> >> @@ -0,0 +1,42 @@

> >> +/* PR ipa/71207 */

> >> +/* { dg-do run } */

> >> +

> >> +class Class1

> >> +{

> >> +public:

> >> +  Class1() {};

> >> +  virtual ~Class1() {};

> >> +

> >> +protected:

> >> +  unsigned Field1;

> >> +};

> >> +

> >> +class Class2 : public virtual Class1

> >> +{

> >> +};

> >> +

> >> +class Class3 : public virtual Class1

> >> +{

> >> +public:

> >> +  virtual void Method1() = 0;

> >> +

> >> +  void Method2()

> >> +  {

> >> +    Method1();

> >> +  }

> >> +};

> >> +

> >> +class Class4 : public Class2, public virtual Class3

> >> +{

> >> +public:

> >> +  Class4() {};

> >> +  virtual void Method1() {};

> >> +};

> >> +

> >> +int main()

> >> +{

> >> +  Class4 var1;

> >> +  var1.Method2();

> >> +

> >> +  return 0;

> >> +}

> >> -- 

> >> 2.11.0

> >>

> >
diff mbox

Patch

From 47e06bcf33719df390b9c49328235d9cbb48e4a7 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 12 Jan 2017 15:55:42 +0100
Subject: [PATCH] Fix wrong assumption in contains_type_p (PR ipa/71207).

gcc/testsuite/ChangeLog:

2017-01-12  Martin Liska  <mliska@suse.cz>

	PR ipa/71207
	* g++.dg/ipa/pr71207.C: New test.

gcc/ChangeLog:

2017-01-12  Martin Liska  <mliska@suse.cz>

	PR ipa/71207
	* ipa-polymorphic-call.c (contains_type_p): Fix wrong
	assumption, add comment and renamed otr_type to inner_type.
---
 gcc/ipa-polymorphic-call.c         | 22 ++++++++++----------
 gcc/testsuite/g++.dg/ipa/pr71207.C | 42 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr71207.C

diff --git a/gcc/ipa-polymorphic-call.c b/gcc/ipa-polymorphic-call.c
index da64ce4c6e0..c13fc858c86 100644
--- a/gcc/ipa-polymorphic-call.c
+++ b/gcc/ipa-polymorphic-call.c
@@ -446,15 +446,15 @@  no_useful_type_info:
     }
 }
 
-/* Return true if OUTER_TYPE contains OTR_TYPE at OFFSET.
-   CONSIDER_PLACEMENT_NEW makes function to accept cases where OTR_TYPE can
+/* Return true if OUTER_TYPE contains INNER_TYPE at OFFSET.
+   CONSIDER_PLACEMENT_NEW makes function to accept cases where INNER_TYPE can
    be built within OUTER_TYPE by means of placement new.  CONSIDER_BASES makes
-   function to accept cases where OTR_TYPE appears as base of OUTER_TYPE or as
+   function to accept cases where INNER_TYPE appears as base of OUTER_TYPE or as
    base of one of fields of OUTER_TYPE.  */
 
 static bool
 contains_type_p (tree outer_type, HOST_WIDE_INT offset,
-		 tree otr_type,
+		 tree inner_type,
 		 bool consider_placement_new,
 		 bool consider_bases)
 {
@@ -463,18 +463,18 @@  contains_type_p (tree outer_type, HOST_WIDE_INT offset,
   /* Check that type is within range.  */
   if (offset < 0)
     return false;
-  if (TYPE_SIZE (outer_type) && TYPE_SIZE (otr_type)
-      && TREE_CODE (TYPE_SIZE (outer_type)) == INTEGER_CST
-      && TREE_CODE (TYPE_SIZE (otr_type)) == INTEGER_CST
-      && wi::ltu_p (wi::to_offset (TYPE_SIZE (outer_type)),
-		    (wi::to_offset (TYPE_SIZE (otr_type)) + offset)))
-    return false;
+
+  /* PR ipa/71207
+     As OUTER_TYPE can be a type which has a diamond virtual inheritance,
+     it's not necessary that INNER_TYPE will fit within OUTER_TYPE with
+     a given offset.  It can happen that INNER_TYPE also contains a base object,
+     however it would point to the same instance in the OUTER_TYPE.  */
 
   context.offset = offset;
   context.outer_type = TYPE_MAIN_VARIANT (outer_type);
   context.maybe_derived_type = false;
   context.dynamic = false;
-  return context.restrict_to_inner_class (otr_type, consider_placement_new,
+  return context.restrict_to_inner_class (inner_type, consider_placement_new,
 					  consider_bases);
 }
 
diff --git a/gcc/testsuite/g++.dg/ipa/pr71207.C b/gcc/testsuite/g++.dg/ipa/pr71207.C
new file mode 100644
index 00000000000..19a03998460
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr71207.C
@@ -0,0 +1,42 @@ 
+/* PR ipa/71207 */
+/* { dg-do run } */
+
+class Class1
+{
+public:
+  Class1() {};
+  virtual ~Class1() {};
+
+protected:
+  unsigned Field1;
+};
+
+class Class2 : public virtual Class1
+{
+};
+
+class Class3 : public virtual Class1
+{
+public:
+  virtual void Method1() = 0;
+
+  void Method2()
+  {
+    Method1();
+  }
+};
+
+class Class4 : public Class2, public virtual Class3
+{
+public:
+  Class4() {};
+  virtual void Method1() {};
+};
+
+int main()
+{
+  Class4 var1;
+  var1.Method2();
+
+  return 0;
+}
-- 
2.11.0