diff mbox series

[C++] Fix constexpr OBJ_TYPE_REF handling on array elts (PR c++/87398)

Message ID 20180925071754.GX8250@tucnak
State New
Headers show
Series [C++] Fix constexpr OBJ_TYPE_REF handling on array elts (PR c++/87398) | expand

Commit Message

Jakub Jelinek Sept. 25, 2018, 7:17 a.m. UTC 
Hi!

ARRAY_REF is a handled component, so when a virtual call is on an array
element (or some component thereof), the loop to look through handled
components will look through the ARRAY_REFs too and then TREE_TYPE (obj)
might be an ARRAY_TYPE.  The code wants to look at the class type instead
though.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-09-25  Jakub Jelinek  <jakub@redhat.com>

	PR c++/87398
	* constexpr.c (cxx_eval_constant_expression) <case OBJ_TYPE_REF>: Use
	strip_array_types before checking TYPE_BINFO.

	* g++.dg/other/pr87398.C: New test.
	* g++.dg/cpp2a/constexpr-virtual10.C: New test.


	Jakub

Comments

Marek Polacek Sept. 25, 2018, 2:30 p.m. UTC | #1 
On Tue, Sep 25, 2018 at 09:17:54AM +0200, Jakub Jelinek wrote:
> Hi!
> 
> ARRAY_REF is a handled component, so when a virtual call is on an array
> element (or some component thereof), the loop to look through handled
> components will look through the ARRAY_REFs too and then TREE_TYPE (obj)
> might be an ARRAY_TYPE.  The code wants to look at the class type instead
> though.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2018-09-25  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/87398
> 	* constexpr.c (cxx_eval_constant_expression) <case OBJ_TYPE_REF>: Use
> 	strip_array_types before checking TYPE_BINFO.
> 
> 	* g++.dg/other/pr87398.C: New test.
> 	* g++.dg/cpp2a/constexpr-virtual10.C: New test.

Can't approve but this looks fine to me, thanks!

Marek
Jason Merrill Sept. 25, 2018, 2:42 p.m. UTC | #2 
On Tue, Sep 25, 2018 at 3:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> ARRAY_REF is a handled component, so when a virtual call is on an array
> element (or some component thereof), the loop to look through handled
> components will look through the ARRAY_REFs too and then TREE_TYPE (obj)
> might be an ARRAY_TYPE.  The code wants to look at the class type instead
> though.

Yeah, I think looping through handled components is wrong here; given
something like

struct A
{
  virtual void f();
};

struct B
{
  A a;
} b;

int main()
{
  b.a.f();
}

Looking through all handled components would give us "b", which has no
virtual functions.

I think we need to be more specific about what we're expecting here.

Jason
Jakub Jelinek Sept. 25, 2018, 2:57 p.m. UTC | #3 
On Tue, Sep 25, 2018 at 10:42:08AM -0400, Jason Merrill wrote:
> On Tue, Sep 25, 2018 at 3:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > ARRAY_REF is a handled component, so when a virtual call is on an array
> > element (or some component thereof), the loop to look through handled
> > components will look through the ARRAY_REFs too and then TREE_TYPE (obj)
> > might be an ARRAY_TYPE.  The code wants to look at the class type instead
> > though.
> 
> Yeah, I think looping through handled components is wrong here; given
> something like
> 
> struct A
> {
>   virtual void f();
> };
> 
> struct B
> {
>   A a;
> } b;
> 
> int main()
> {
>   b.a.f();
> }
> 
> Looking through all handled components would give us "b", which has no
> virtual functions.

You're right.  So like this (only look through COMPONENT_REFs where the
FIELD_DECL is DECL_FIELD_IS_BASE)?
Only checked
make check-c++-all RUNTESTFLAGS="dg.exp='constexpr-virt* pr87398*'"
so far, but it also fixes the PR87425 testcase and doesn't regress anything.

2018-09-25  Jakub Jelinek  <jakub@redhat.com>

	PR c++/87398
	* constexpr.c (cxx_eval_constant_expression) <case OBJ_TYPE_REF>: Only
	look through COMPONENT_REFs with DECL_FIELD_IS_BASE FIELD_DECLs.

	* g++.dg/other/pr87398.C: New test.
	* g++.dg/cpp2a/constexpr-virtual10.C: New test.

--- gcc/cp/constexpr.c.jj	2018-09-25 15:14:36.335386401 +0200
+++ gcc/cp/constexpr.c	2018-09-25 16:50:38.085334475 +0200
@@ -4812,7 +4812,8 @@ cxx_eval_constant_expression (const cons
 	    return t;
 	  }
 	obj = TREE_OPERAND (obj, 0);
-	while (handled_component_p (obj))
+	while (TREE_CODE (obj) == COMPONENT_REF
+	       && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1)))
 	  obj = TREE_OPERAND (obj, 0);
 	tree objtype = TREE_TYPE (obj);
 	/* Find the function decl in the virtual functions list.  TOKEN is
--- gcc/testsuite/g++.dg/other/pr87398.C.jj	2018-09-25 16:49:40.458304980 +0200
+++ gcc/testsuite/g++.dg/other/pr87398.C	2018-09-25 16:49:40.458304980 +0200
@@ -0,0 +1,12 @@
+// PR c++/87398
+// { dg-do compile }
+
+struct A { virtual int foo (); };
+
+int
+bar (int x)
+{
+  A e[5][2];
+  int f = e[4][x].foo ();
+  return f;
+}
--- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual10.C.jj	2018-09-25 16:49:40.458304980 +0200
+++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual10.C	2018-09-25 16:49:40.458304980 +0200
@@ -0,0 +1,18 @@
+// P1064R0
+// { dg-do compile }
+// { dg-options "-std=c++2a" }
+
+struct X
+{
+  constexpr virtual int f() const { return 1; };
+};
+
+struct Y : public X
+{
+  constexpr virtual int f() const { return 2; };
+};
+
+constexpr X a[2][1][3];
+constexpr Y b[3][12];
+static_assert (a[1][0][1].f() == 1);
+static_assert (b[2][11].f() == 2);


	Jakub
Jason Merrill Sept. 25, 2018, 3:07 p.m. UTC | #4 
On Tue, Sep 25, 2018 at 10:57 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Sep 25, 2018 at 10:42:08AM -0400, Jason Merrill wrote:
>> On Tue, Sep 25, 2018 at 3:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > ARRAY_REF is a handled component, so when a virtual call is on an array
>> > element (or some component thereof), the loop to look through handled
>> > components will look through the ARRAY_REFs too and then TREE_TYPE (obj)
>> > might be an ARRAY_TYPE.  The code wants to look at the class type instead
>> > though.
>>
>> Yeah, I think looping through handled components is wrong here; given
>> something like
>>
>> struct A
>> {
>>   virtual void f();
>> };
>>
>> struct B
>> {
>>   A a;
>> } b;
>>
>> int main()
>> {
>>   b.a.f();
>> }
>>
>> Looking through all handled components would give us "b", which has no
>> virtual functions.
>
> You're right.  So like this (only look through COMPONENT_REFs where the
> FIELD_DECL is DECL_FIELD_IS_BASE)?
> Only checked
> make check-c++-all RUNTESTFLAGS="dg.exp='constexpr-virt* pr87398*'"
> so far, but it also fixes the PR87425 testcase and doesn't regress anything.

Looks good.

Jason
Jakub Jelinek Sept. 25, 2018, 3:14 p.m. UTC | #5 
On Tue, Sep 25, 2018 at 11:07:19AM -0400, Jason Merrill wrote:
> > You're right.  So like this (only look through COMPONENT_REFs where the
> > FIELD_DECL is DECL_FIELD_IS_BASE)?
> > Only checked
> > make check-c++-all RUNTESTFLAGS="dg.exp='constexpr-virt* pr87398*'"
> > so far, but it also fixes the PR87425 testcase and doesn't regress anything.
> 
> Looks good.

Thanks, will bootstrap/regtest it now.  I also took your testcase and turned it into
something that ICEd without this patch too, is that testcase also ok
(constexpr-virtual11.C)?

2018-09-25  Jakub Jelinek  <jakub@redhat.com>

	PR c++/87398
	* constexpr.c (cxx_eval_constant_expression) <case OBJ_TYPE_REF>: Only
	look through COMPONENT_REFs with DECL_FIELD_IS_BASE FIELD_DECLs.

	* g++.dg/other/pr87398.C: New test.
	* g++.dg/cpp2a/constexpr-virtual10.C: New test.
	* g++.dg/cpp2a/constexpr-virtual11.C: New test.

--- gcc/cp/constexpr.c.jj	2018-09-25 15:14:36.335386401 +0200
+++ gcc/cp/constexpr.c	2018-09-25 16:50:38.085334475 +0200
@@ -4812,7 +4812,8 @@ cxx_eval_constant_expression (const cons
 	    return t;
 	  }
 	obj = TREE_OPERAND (obj, 0);
-	while (handled_component_p (obj))
+	while (TREE_CODE (obj) == COMPONENT_REF
+	       && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1)))
 	  obj = TREE_OPERAND (obj, 0);
 	tree objtype = TREE_TYPE (obj);
 	/* Find the function decl in the virtual functions list.  TOKEN is
--- gcc/testsuite/g++.dg/other/pr87398.C.jj	2018-09-25 16:49:40.458304980 +0200
+++ gcc/testsuite/g++.dg/other/pr87398.C	2018-09-25 16:49:40.458304980 +0200
@@ -0,0 +1,12 @@
+// PR c++/87398
+// { dg-do compile }
+
+struct A { virtual int foo (); };
+
+int
+bar (int x)
+{
+  A e[5][2];
+  int f = e[4][x].foo ();
+  return f;
+}
--- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual10.C.jj	2018-09-25 16:49:40.458304980 +0200
+++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual10.C	2018-09-25 16:49:40.458304980 +0200
@@ -0,0 +1,18 @@
+// P1064R0
+// { dg-do compile }
+// { dg-options "-std=c++2a" }
+
+struct X
+{
+  constexpr virtual int f() const { return 1; };
+};
+
+struct Y : public X
+{
+  constexpr virtual int f() const { return 2; };
+};
+
+constexpr X a[2][1][3];
+constexpr Y b[3][12];
+static_assert (a[1][0][1].f() == 1);
+static_assert (b[2][11].f() == 2);
--- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual11.C.jj	2018-09-01 11:09:41.506153390 +0200
+++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual11.C	2018-09-25 17:11:30.563244029 +0200
@@ -0,0 +1,26 @@
+// P1064R0
+// { dg-do compile }
+// { dg-options "-std=c++2a" }
+
+struct A
+{
+  constexpr virtual int f () const { return 1; }
+};
+
+struct B : public A
+{
+  constexpr virtual int f () const { return 2; }
+};
+
+struct C
+{
+  A a;
+  B b;
+};
+
+constexpr C c;
+constexpr const A &d = c.a;
+constexpr const A &e = c.b;
+constexpr const B &f = c.b;
+static_assert (c.a.f () == 1 && c.b.f () == 2);
+static_assert (d.f () == 1 && e.f () == 2 && f.f () == 2);


	Jakub
Jason Merrill Sept. 25, 2018, 3:40 p.m. UTC | #6 
Yes, thanks.

On Tue, Sep 25, 2018 at 11:14 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Sep 25, 2018 at 11:07:19AM -0400, Jason Merrill wrote:
>> > You're right.  So like this (only look through COMPONENT_REFs where the
>> > FIELD_DECL is DECL_FIELD_IS_BASE)?
>> > Only checked
>> > make check-c++-all RUNTESTFLAGS="dg.exp='constexpr-virt* pr87398*'"
>> > so far, but it also fixes the PR87425 testcase and doesn't regress anything.
>>
>> Looks good.
>
> Thanks, will bootstrap/regtest it now.  I also took your testcase and turned it into
> something that ICEd without this patch too, is that testcase also ok
> (constexpr-virtual11.C)?
>
> 2018-09-25  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/87398
>         * constexpr.c (cxx_eval_constant_expression) <case OBJ_TYPE_REF>: Only
>         look through COMPONENT_REFs with DECL_FIELD_IS_BASE FIELD_DECLs.
>
>         * g++.dg/other/pr87398.C: New test.
>         * g++.dg/cpp2a/constexpr-virtual10.C: New test.
>         * g++.dg/cpp2a/constexpr-virtual11.C: New test.
>
> --- gcc/cp/constexpr.c.jj       2018-09-25 15:14:36.335386401 +0200
> +++ gcc/cp/constexpr.c  2018-09-25 16:50:38.085334475 +0200
> @@ -4812,7 +4812,8 @@ cxx_eval_constant_expression (const cons
>             return t;
>           }
>         obj = TREE_OPERAND (obj, 0);
> -       while (handled_component_p (obj))
> +       while (TREE_CODE (obj) == COMPONENT_REF
> +              && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1)))
>           obj = TREE_OPERAND (obj, 0);
>         tree objtype = TREE_TYPE (obj);
>         /* Find the function decl in the virtual functions list.  TOKEN is
> --- gcc/testsuite/g++.dg/other/pr87398.C.jj     2018-09-25 16:49:40.458304980 +0200
> +++ gcc/testsuite/g++.dg/other/pr87398.C        2018-09-25 16:49:40.458304980 +0200
> @@ -0,0 +1,12 @@
> +// PR c++/87398
> +// { dg-do compile }
> +
> +struct A { virtual int foo (); };
> +
> +int
> +bar (int x)
> +{
> +  A e[5][2];
> +  int f = e[4][x].foo ();
> +  return f;
> +}
> --- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual10.C.jj 2018-09-25 16:49:40.458304980 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual10.C    2018-09-25 16:49:40.458304980 +0200
> @@ -0,0 +1,18 @@
> +// P1064R0
> +// { dg-do compile }
> +// { dg-options "-std=c++2a" }
> +
> +struct X
> +{
> +  constexpr virtual int f() const { return 1; };
> +};
> +
> +struct Y : public X
> +{
> +  constexpr virtual int f() const { return 2; };
> +};
> +
> +constexpr X a[2][1][3];
> +constexpr Y b[3][12];
> +static_assert (a[1][0][1].f() == 1);
> +static_assert (b[2][11].f() == 2);
> --- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual11.C.jj 2018-09-01 11:09:41.506153390 +0200
> +++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual11.C    2018-09-25 17:11:30.563244029 +0200
> @@ -0,0 +1,26 @@
> +// P1064R0
> +// { dg-do compile }
> +// { dg-options "-std=c++2a" }
> +
> +struct A
> +{
> +  constexpr virtual int f () const { return 1; }
> +};
> +
> +struct B : public A
> +{
> +  constexpr virtual int f () const { return 2; }
> +};
> +
> +struct C
> +{
> +  A a;
> +  B b;
> +};
> +
> +constexpr C c;
> +constexpr const A &d = c.a;
> +constexpr const A &e = c.b;
> +constexpr const B &f = c.b;
> +static_assert (c.a.f () == 1 && c.b.f () == 2);
> +static_assert (d.f () == 1 && e.f () == 2 && f.f () == 2);
>
>
>         Jakub
diff mbox series

Patch

--- gcc/cp/constexpr.c.jj	2018-09-24 10:36:53.687031261 +0200
+++ gcc/cp/constexpr.c	2018-09-24 11:01:34.035268597 +0200
@@ -4814,7 +4814,7 @@  cxx_eval_constant_expression (const cons
 	obj = TREE_OPERAND (obj, 0);
 	while (handled_component_p (obj))
 	  obj = TREE_OPERAND (obj, 0);
-	tree objtype = TREE_TYPE (obj);
+	tree objtype = strip_array_types (TREE_TYPE (obj));
 	/* Find the function decl in the virtual functions list.  TOKEN is
 	   the DECL_VINDEX that says which function we're looking for.  */
 	tree virtuals = BINFO_VIRTUALS (TYPE_BINFO (objtype));
--- gcc/testsuite/g++.dg/other/pr87398.C.jj	2018-09-24 10:53:41.605170048 +0200
+++ gcc/testsuite/g++.dg/other/pr87398.C	2018-09-24 10:53:14.642621001 +0200
@@ -0,0 +1,12 @@ 
+// PR c++/87398
+// { dg-do compile }
+
+struct A { virtual int foo (); };
+
+int
+bar (int x)
+{
+  A e[5][2];
+  int f = e[4][x].foo ();
+  return f;
+}
--- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual10.C.jj	2018-09-24 10:56:15.604594387 +0200
+++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual10.C	2018-09-24 10:56:54.882937453 +0200
@@ -0,0 +1,18 @@ 
+// P1064R0
+// { dg-do compile }
+// { dg-options "-std=c++2a" }
+
+struct X
+{
+  constexpr virtual int f() const { return 1; };
+};
+
+struct Y : public X
+{
+  constexpr virtual int f() const { return 2; };
+};
+
+constexpr X a[2][1][3];
+constexpr Y b[3][12];
+static_assert (a[1][0][1].f() == 1);
+static_assert (b[2][11].f() == 2);