subject:"\[PATCH\] Fix wrong assumption in contains_type

Re: [PATCH] Fix wrong assumption in contains_type_p (PR ipa/71207).

2017-01-17 Thread Jan Hubicka

> 
> 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 000..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
> >>
> >

Re: [PATCH] Fix wrong assumption in contains_type_p (PR ipa/71207).

2017-01-17 Thread Martin Liška

On 01/17/2017 11:43 AM, Jan Hubicka wrote:
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-01-12  Martin Liska  
>>
>>  PR ipa/71207
>>  * g++.dg/ipa/pr71207.C: New test.
>>
>> gcc/ChangeLog:
>>
>> 2017-01-12  Martin Liska  
>>
>>  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 000..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
>>
>

Re: [PATCH] Fix wrong assumption in contains_type_p (PR ipa/71207).

2017-01-17 Thread Jan Hubicka

> 
> gcc/testsuite/ChangeLog:
> 
> 2017-01-12  Martin Liska  
> 
>   PR ipa/71207
>   * g++.dg/ipa/pr71207.C: New test.
> 
> gcc/ChangeLog:
> 
> 2017-01-12  Martin Liska  
> 
>   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 000..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
>

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

2017-01-13 Thread Martin Liška

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
>From 47e06bcf33719df390b9c49328235d9cbb48e4a7 Mon Sep 17 00:00:00 2001
From: marxin <mli...@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  <mli...@suse.cz>

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

gcc/ChangeLog:

2017-01-12  Martin Liska  <mli...@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 000..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

Re: [PATCH] Fix wrong assumption in contains_type_p (PR ipa/71207).

Re: [PATCH] Fix wrong assumption in contains_type_p (PR ipa/71207).

Re: [PATCH] Fix wrong assumption in contains_type_p (PR ipa/71207).

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

4 matches

Site Navigation

Mail list logo

Footer information