[C++] Fix up constexpr virtual calls (PR c++/92695)

Message ID 20191127233859.GD10088@tucnak
State New
Headers show
Series
  • [C++] Fix up constexpr virtual calls (PR c++/92695)
Related show

Commit Message

Jakub Jelinek Nov. 27, 2019, 11:38 p.m.
Hi!

The OBJ_TYPE_REF constexpr handling looks through DECL_FIELD_IS_BASE
COMPONENT_REFs to find the actual object on which the method is called,
but as the following testcase shows, we need to do the similar thing also
for the argument passed as this, because cxx_eval_indirect_ref otherwise
fails to fold it.  It can handle going from derived to base, not not vice
versa.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2019-11-27  Jakub Jelinek  <jakub@redhat.com>

	PR c++/92695
	* constexpr.c (cxx_bind_parameters_in_call): For virtual calls,
	adjust the first argument to point to the derived object rather
	than its base.

	* g++.dg/cpp2a/constexpr-virtual14.C: New test.



	Jakub

Comments

Jason Merrill Dec. 2, 2019, 7:29 p.m. | #1
On 11/27/19 6:38 PM, Jakub Jelinek wrote:
> Hi!

> 

> The OBJ_TYPE_REF constexpr handling looks through DECL_FIELD_IS_BASE

> COMPONENT_REFs to find the actual object on which the method is called,

> but as the following testcase shows, we need to do the similar thing also

> for the argument passed as this, because cxx_eval_indirect_ref otherwise

> fails to fold it.  It can handle going from derived to base, not not vice

> versa.

> 

> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for

> trunk?


> 2019-11-27  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR c++/92695

> 	* constexpr.c (cxx_bind_parameters_in_call): For virtual calls,

> 	adjust the first argument to point to the derived object rather

> 	than its base.

> 

> 	* g++.dg/cpp2a/constexpr-virtual14.C: New test.

> 

> --- gcc/cp/constexpr.c.jj	2019-11-27 17:26:57.229014008 +0100

> +++ gcc/cp/constexpr.c	2019-11-27 17:53:37.477566346 +0100

> @@ -1441,6 +1441,24 @@ cxx_bind_parameters_in_call (const const

>   	    arg = adjust_temp_type (type, arg);

>   	  if (!TREE_CONSTANT (arg))

>   	    *non_constant_args = true;

> +	  /* For virtual calls, adjust the this argument, so that it is

> +	     the object on which the method is called, rather than

> +	     one of its bases.  */

> +	  if (i == 0 && DECL_VIRTUAL_P (fun))

> +	    {

> +	      tree addr = arg;

> +	      STRIP_NOPS (addr);

> +	      if (TREE_CODE (addr) == ADDR_EXPR)

> +		{

> +		  tree obj = TREE_OPERAND (addr, 0);

> +		  while (TREE_CODE (obj) == COMPONENT_REF

> +			 && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1)))

> +		    obj = TREE_OPERAND (obj, 0);


Shouldn't this loop stop when we reach DECL_CONTEXT (fun)?

Relatedly, I notice that the OBJ_TYPE_REF code breaks with multiple 
inheritance: Adding a virtual function to Y in constexpr-virtual6.C leads to

 > wa.ii:26:22: error: non-constant condition for static assertion

 >    26 | static_assert(r2.f() == &r2);

 >       |               ~~~~~~~^~~~~~

 > wa.ii:26:19: error: call to non-'constexpr' function 'virtual void 

Y::dummy()'
 >    26 | static_assert(r2.f() == &r2);

 >       |               ~~~~^~

 > wa.ii:12:16: note: 'virtual void Y::dummy()' declared here

 >    12 |   virtual void dummy () { }
Jakub Jelinek Dec. 2, 2019, 9:43 p.m. | #2
On Mon, Dec 02, 2019 at 02:29:56PM -0500, Jason Merrill wrote:
> On 11/27/19 6:38 PM, Jakub Jelinek wrote:

> > +	  if (i == 0 && DECL_VIRTUAL_P (fun))

> > +	    {

> > +	      tree addr = arg;

> > +	      STRIP_NOPS (addr);

> > +	      if (TREE_CODE (addr) == ADDR_EXPR)

> > +		{

> > +		  tree obj = TREE_OPERAND (addr, 0);

> > +		  while (TREE_CODE (obj) == COMPONENT_REF

> > +			 && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1)))

> > +		    obj = TREE_OPERAND (obj, 0);

> 

> Shouldn't this loop stop when we reach DECL_CONTEXT (fun)?


I guess it can, patch I can test tonight attached.  I thought it doesn't matter
much, because cxx_eval_indirect_ref would be able to go from further derived
class to the base if we want too far, but maybe that is not the case
with multiple inheritance.

> Relatedly, I notice that the OBJ_TYPE_REF code breaks with multiple

> inheritance: Adding a virtual function to Y in constexpr-virtual6.C leads to


I'm afraid I'd be lost with that.  There is another bug in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92695#c12 about constexpr
defaulted virtual destructors not being synthetized until end of TU.

2019-12-02  Jakub Jelinek  <jakub@redhat.com>

	PR c++/92695
	* constexpr.c (cxx_bind_parameters_in_call): For virtual calls,
	adjust the first argument to point to the derived object rather
	than its base.

	* g++.dg/cpp2a/constexpr-virtual14.C: New test.

--- gcc/cp/constexpr.c.jj	2019-12-02 22:30:31.608306270 +0100
+++ gcc/cp/constexpr.c	2019-12-02 22:35:42.374501609 +0100
@@ -1441,6 +1441,26 @@ cxx_bind_parameters_in_call (const const
 	    arg = adjust_temp_type (type, arg);
 	  if (!TREE_CONSTANT (arg))
 	    *non_constant_args = true;
+	  /* For virtual calls, adjust the this argument, so that it is
+	     the object on which the method is called, rather than
+	     one of its bases.  */
+	  if (i == 0 && DECL_VIRTUAL_P (fun))
+	    {
+	      tree addr = arg;
+	      STRIP_NOPS (addr);
+	      if (TREE_CODE (addr) == ADDR_EXPR)
+		{
+		  tree obj = TREE_OPERAND (addr, 0);
+		  while (TREE_CODE (obj) == COMPONENT_REF
+			 && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1))
+			 && !same_type_ignoring_top_level_qualifiers_p
+					(TREE_TYPE (obj), DECL_CONTEXT (fun)))
+		    obj = TREE_OPERAND (obj, 0);
+		  if (obj != TREE_OPERAND (addr, 0))
+		    arg = build_fold_addr_expr_with_type (obj,
+							  TREE_TYPE (arg));
+		}
+	    }
 	  TREE_VEC_ELT (binds, i) = arg;
 	}
       parms = TREE_CHAIN (parms);
--- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual14.C.jj	2019-12-02 22:34:05.554998508 +0100
+++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual14.C	2019-12-02 22:34:05.554998508 +0100
@@ -0,0 +1,27 @@
+// PR c++/92695
+// { dg-do compile { target c++2a } }
+
+struct A {
+  virtual int get () = 0;
+  virtual int set (A *o) = 0;
+};
+struct B : A {
+  constexpr int get () override { return 10; }
+  constexpr int set (A *o) override { a = o; return 20; }
+  A *a {};
+};
+constexpr auto addressof = [] (A &n) { return &n; };
+struct C {
+  B b;
+  A *c { addressof (b) };
+  constexpr int add () { return c->set (addressof (b)); }
+};
+struct D {
+  B b[2];
+  A *c { addressof (b[0]) };
+  constexpr int add () { return c->set (addressof (b[0])); }
+};
+template <typename T>
+constexpr int get () { T f; return f.add (); }
+static_assert (get<C> () == 20);
+static_assert (get<D> () == 20);


	Jakub
Jason Merrill Dec. 3, 2019, 7:10 a.m. | #3
On 12/2/19 4:43 PM, Jakub Jelinek wrote:
> On Mon, Dec 02, 2019 at 02:29:56PM -0500, Jason Merrill wrote:

>> On 11/27/19 6:38 PM, Jakub Jelinek wrote:

>>> +	  if (i == 0 && DECL_VIRTUAL_P (fun))

>>> +	    {

>>> +	      tree addr = arg;

>>> +	      STRIP_NOPS (addr);

>>> +	      if (TREE_CODE (addr) == ADDR_EXPR)

>>> +		{

>>> +		  tree obj = TREE_OPERAND (addr, 0);

>>> +		  while (TREE_CODE (obj) == COMPONENT_REF

>>> +			 && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1)))

>>> +		    obj = TREE_OPERAND (obj, 0);

>>

>> Shouldn't this loop stop when we reach DECL_CONTEXT (fun)?

> 

> I guess it can, patch I can test tonight attached.  I thought it doesn't matter

> much, because cxx_eval_indirect_ref would be able to go from further derived

> class to the base if we want too far, but maybe that is not the case

> with multiple inheritance.


OK after testing.

>> Relatedly, I notice that the OBJ_TYPE_REF code breaks with multiple

>> inheritance: Adding a virtual function to Y in constexpr-virtual6.C leads to

> 

> I'm afraid I'd be lost with that.  There is another bug in

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92695#c12 about constexpr

> defaulted virtual destructors not being synthetized until end of TU.

> 

> 2019-12-02  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR c++/92695

> 	* constexpr.c (cxx_bind_parameters_in_call): For virtual calls,

> 	adjust the first argument to point to the derived object rather

> 	than its base.

> 

> 	* g++.dg/cpp2a/constexpr-virtual14.C: New test.

> 

> --- gcc/cp/constexpr.c.jj	2019-12-02 22:30:31.608306270 +0100

> +++ gcc/cp/constexpr.c	2019-12-02 22:35:42.374501609 +0100

> @@ -1441,6 +1441,26 @@ cxx_bind_parameters_in_call (const const

>   	    arg = adjust_temp_type (type, arg);

>   	  if (!TREE_CONSTANT (arg))

>   	    *non_constant_args = true;

> +	  /* For virtual calls, adjust the this argument, so that it is

> +	     the object on which the method is called, rather than

> +	     one of its bases.  */

> +	  if (i == 0 && DECL_VIRTUAL_P (fun))

> +	    {

> +	      tree addr = arg;

> +	      STRIP_NOPS (addr);

> +	      if (TREE_CODE (addr) == ADDR_EXPR)

> +		{

> +		  tree obj = TREE_OPERAND (addr, 0);

> +		  while (TREE_CODE (obj) == COMPONENT_REF

> +			 && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1))

> +			 && !same_type_ignoring_top_level_qualifiers_p

> +					(TREE_TYPE (obj), DECL_CONTEXT (fun)))

> +		    obj = TREE_OPERAND (obj, 0);

> +		  if (obj != TREE_OPERAND (addr, 0))

> +		    arg = build_fold_addr_expr_with_type (obj,

> +							  TREE_TYPE (arg));

> +		}

> +	    }

>   	  TREE_VEC_ELT (binds, i) = arg;

>   	}

>         parms = TREE_CHAIN (parms);

> --- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual14.C.jj	2019-12-02 22:34:05.554998508 +0100

> +++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual14.C	2019-12-02 22:34:05.554998508 +0100

> @@ -0,0 +1,27 @@

> +// PR c++/92695

> +// { dg-do compile { target c++2a } }

> +

> +struct A {

> +  virtual int get () = 0;

> +  virtual int set (A *o) = 0;

> +};

> +struct B : A {

> +  constexpr int get () override { return 10; }

> +  constexpr int set (A *o) override { a = o; return 20; }

> +  A *a {};

> +};

> +constexpr auto addressof = [] (A &n) { return &n; };

> +struct C {

> +  B b;

> +  A *c { addressof (b) };

> +  constexpr int add () { return c->set (addressof (b)); }

> +};

> +struct D {

> +  B b[2];

> +  A *c { addressof (b[0]) };

> +  constexpr int add () { return c->set (addressof (b[0])); }

> +};

> +template <typename T>

> +constexpr int get () { T f; return f.add (); }

> +static_assert (get<C> () == 20);

> +static_assert (get<D> () == 20);

> 

> 

> 	Jakub

>

Patch

--- gcc/cp/constexpr.c.jj	2019-11-27 17:26:57.229014008 +0100
+++ gcc/cp/constexpr.c	2019-11-27 17:53:37.477566346 +0100
@@ -1441,6 +1441,24 @@  cxx_bind_parameters_in_call (const const
 	    arg = adjust_temp_type (type, arg);
 	  if (!TREE_CONSTANT (arg))
 	    *non_constant_args = true;
+	  /* For virtual calls, adjust the this argument, so that it is
+	     the object on which the method is called, rather than
+	     one of its bases.  */
+	  if (i == 0 && DECL_VIRTUAL_P (fun))
+	    {
+	      tree addr = arg;
+	      STRIP_NOPS (addr);
+	      if (TREE_CODE (addr) == ADDR_EXPR)
+		{
+		  tree obj = TREE_OPERAND (addr, 0);
+		  while (TREE_CODE (obj) == COMPONENT_REF
+			 && DECL_FIELD_IS_BASE (TREE_OPERAND (obj, 1)))
+		    obj = TREE_OPERAND (obj, 0);
+		  if (obj != TREE_OPERAND (addr, 0))
+		    arg = build_fold_addr_expr_with_type (obj,
+							  TREE_TYPE (arg));
+		}
+	    }
 	  TREE_VEC_ELT (binds, i) = arg;
 	}
       parms = TREE_CHAIN (parms);
--- gcc/testsuite/g++.dg/cpp2a/constexpr-virtual14.C.jj	2019-11-27 18:01:17.181532622 +0100
+++ gcc/testsuite/g++.dg/cpp2a/constexpr-virtual14.C	2019-11-27 18:00:49.620954471 +0100
@@ -0,0 +1,27 @@ 
+// PR c++/92695
+// { dg-do compile { target c++2a } }
+
+struct A {
+  virtual int get () = 0;
+  virtual int set (A *o) = 0;
+};
+struct B : A {
+  constexpr int get () override { return 10; }
+  constexpr int set (A *o) override { a = o; return 20; }
+  A *a {};
+};
+constexpr auto addressof = [] (A &n) { return &n; };
+struct C {
+  B b;
+  A *c { addressof (b) };
+  constexpr int add () { return c->set (addressof (b)); }
+};
+struct D {
+  B b[2];
+  A *c { addressof (b[0]) };
+  constexpr int add () { return c->set (addressof (b[0])); }
+};
+template <typename T>
+constexpr int get () { T f; return f.add (); }
+static_assert (get<C> () == 20);
+static_assert (get<D> () == 20);