[v2] c++: Don't ICE due to artificial constructor parameters [PR116722]

Hi Jason,

On 20 Sep 2024, at 18:01, Jason Merrill wrote:

> On 9/20/24 5:21 PM, Simon Martin wrote:
>> The following code triggers an ICE
>>
>> === cut here ===
>> class base {};
>> class derived : virtual public base {
>> public:
>>    template<typename Arg> constexpr derived(Arg) {}
>> };
>> int main() {
>>    derived obj(1.);
>> }
>> === cut here ===
>>
>> The problem is that cxx_bind_parameters_in_call ends up attempting to

>> convert a REAL_CST (the first non artificial parameter) to 
>> INTEGER_TYPE
>> (the type of the __in_chrg parameter), which ICEs.
>>
>> This patch teaches cxx_bind_parameters_in_call to handle the 
>> __in_chrg
>> and __vtt_parm parameters that {con,de}structors might have.
>>
>> Note that in the test case, the constructor is not 
>> constexpr-suitable,
>> however it's OK since it's a template according to my read of 
>> paragraph
>> (3) of [dcl.constexpr].
>
> Agreed.
>
> It looks like your patch doesn't correct the mismatching of arguments 
> to parameters that you describe, but at least for now it should be 
> enough to set *non_constant_p and return if we see a VTT or in-charge 
> parameter.
>
Thanks, it’s true that my initial patch was wrong in that we’d leave 
cxx_bind_parameters_in_call thinking the expression was actually a 
constant expression :-/

The attached revised patch follows your suggestion (thanks!). 
Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

Thanks,
   Simon
From 12d818220d4addc76f0d8e1fdf8feba336fb9b04 Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Wed, 18 Sep 2024 12:35:27 +0200
Subject: [PATCH] c++: Don't ICE due to artificial constructor parameters [PR116722]

The following code triggers an ICE

=== cut here ===
class base {};
class derived : virtual public base {
public:
  template<typename Arg> constexpr derived(Arg) {}
};
int main() {
  derived obj(1.);
}
=== cut here ===

The problem is that cxx_bind_parameters_in_call ends up attempting to
convert a REAL_CST (the first non artificial parameter) to INTEGER_TYPE
(the type of the __in_chrg parameter), which ICEs.

This patch changes cxx_bind_parameters_in_call to return early if it's
called with a *structor that has an __in_chrg or __vtt_parm parameter
since the expression won't be a constant expression.

Note that in the test case, the constructor is not constexpr-suitable,
however it's OK since it's a template according to my read of paragraph
(3) of [dcl.constexpr].

Successfully tested on x86_64-pc-linux-gnu.

	PR c++/116722

gcc/cp/ChangeLog:

	* constexpr.cc (cxx_bind_parameters_in_call): Leave early for
	{con,de}structors of classes with virtual bases.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp0x/constexpr-ctor22.C: New test.

---
 gcc/cp/constexpr.cc                           | 11 ++++++++++-
 gcc/testsuite/g++.dg/cpp0x/constexpr-ctor22.C | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-ctor22.C

On 9/23/24 10:44 AM, Simon Martin wrote:
> Hi Jason,
> 
> On 20 Sep 2024, at 18:01, Jason Merrill wrote:
> 
>> On 9/20/24 5:21 PM, Simon Martin wrote:
>>> The following code triggers an ICE
>>>
>>> === cut here ===
>>> class base {};
>>> class derived : virtual public base {
>>> public:
>>>     template<typename Arg> constexpr derived(Arg) {}
>>> };
>>> int main() {
>>>     derived obj(1.);
>>> }
>>> === cut here ===
>>>
>>> The problem is that cxx_bind_parameters_in_call ends up attempting to
> 
>>> convert a REAL_CST (the first non artificial parameter) to
>>> INTEGER_TYPE
>>> (the type of the __in_chrg parameter), which ICEs.
>>>
>>> This patch teaches cxx_bind_parameters_in_call to handle the
>>> __in_chrg
>>> and __vtt_parm parameters that {con,de}structors might have.
>>>
>>> Note that in the test case, the constructor is not
>>> constexpr-suitable,
>>> however it's OK since it's a template according to my read of
>>> paragraph
>>> (3) of [dcl.constexpr].
>>
>> Agreed.
>>
>> It looks like your patch doesn't correct the mismatching of arguments
>> to parameters that you describe, but at least for now it should be
>> enough to set *non_constant_p and return if we see a VTT or in-charge
>> parameter.
>>
> Thanks, it’s true that my initial patch was wrong in that we’d leave
> cxx_bind_parameters_in_call thinking the expression was actually a
> constant expression :-/
> 
> The attached revised patch follows your suggestion (thanks!).
> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

OK.

On 9/23/24 4:44 AM, Simon Martin wrote:
> Hi Jason,
> 
> On 20 Sep 2024, at 18:01, Jason Merrill wrote:
> 
>> On 9/20/24 5:21 PM, Simon Martin wrote:
>>> The following code triggers an ICE
>>>
>>> === cut here ===
>>> class base {};
>>> class derived : virtual public base {
>>> public:
>>>     template<typename Arg> constexpr derived(Arg) {}
>>> };
>>> int main() {
>>>     derived obj(1.);
>>> }
>>> === cut here ===
>>>
>>> The problem is that cxx_bind_parameters_in_call ends up attempting to
> 
>>> convert a REAL_CST (the first non artificial parameter) to
>>> INTEGER_TYPE
>>> (the type of the __in_chrg parameter), which ICEs.
>>>
>>> This patch teaches cxx_bind_parameters_in_call to handle the
>>> __in_chrg
>>> and __vtt_parm parameters that {con,de}structors might have.
>>>
>>> Note that in the test case, the constructor is not
>>> constexpr-suitable,
>>> however it's OK since it's a template according to my read of
>>> paragraph
>>> (3) of [dcl.constexpr].
>>
>> Agreed.
>>
>> It looks like your patch doesn't correct the mismatching of arguments
>> to parameters that you describe, but at least for now it should be
>> enough to set *non_constant_p and return if we see a VTT or in-charge
>> parameter.
>>
> Thanks, it’s true that my initial patch was wrong in that we’d leave
> cxx_bind_parameters_in_call thinking the expression was actually a
> constant expression :-/
> 
> The attached revised patch follows your suggestion (thanks!).
> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?

After this patch I'm seeing a regression on constexpr-dynamic10.C with 
-fimplicit-constexpr; we also need to give an error here when
(!ctx->quiet).

Jason

Hi Jason,

On 30 Sep 2024, at 19:56, Jason Merrill wrote:

> On 9/23/24 4:44 AM, Simon Martin wrote:
>> Hi Jason,
>>
>> On 20 Sep 2024, at 18:01, Jason Merrill wrote:
>>
>>> On 9/20/24 5:21 PM, Simon Martin wrote:
>>>> The following code triggers an ICE
>>>>
>>>> === cut here ===
>>>> class base {};
>>>> class derived : virtual public base {
>>>> public:
>>>>     template<typename Arg> constexpr derived(Arg) {}
>>>> };
>>>> int main() {
>>>>     derived obj(1.);
>>>> }
>>>> === cut here ===
>>>>
>>>> The problem is that cxx_bind_parameters_in_call ends up attempting 
>>>> to
>>
>>>> convert a REAL_CST (the first non artificial parameter) to
>>>> INTEGER_TYPE
>>>> (the type of the __in_chrg parameter), which ICEs.
>>>>
>>>> This patch teaches cxx_bind_parameters_in_call to handle the
>>>> __in_chrg
>>>> and __vtt_parm parameters that {con,de}structors might have.
>>>>
>>>> Note that in the test case, the constructor is not
>>>> constexpr-suitable,
>>>> however it's OK since it's a template according to my read of
>>>> paragraph
>>>> (3) of [dcl.constexpr].
>>>
>>> Agreed.
>>>
>>> It looks like your patch doesn't correct the mismatching of 
>>> arguments
>>> to parameters that you describe, but at least for now it should be
>>> enough to set *non_constant_p and return if we see a VTT or 
>>> in-charge
>>> parameter.
>>>
>> Thanks, it’s true that my initial patch was wrong in that we’d 
>> leave
>> cxx_bind_parameters_in_call thinking the expression was actually a
>> constant expression :-/
>>
>> The attached revised patch follows your suggestion (thanks!).
>> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?
>
> After this patch I'm seeing a regression on constexpr-dynamic10.C with 
> -fimplicit-constexpr; we also need to give an error here when
> (!ctx->quiet).
Thanks, good catch. TIL about --stds=impcx...

The attached patch fixes the issue, and was successfully tested on 
x86_64-pc-linux-gnu, including with “make -C gcc -k check-c++-all”. 
OK for trunk?

Note that it includes a new test that’s basically a copy of 
constexpr-dynamic10.C, with -fimplicit-constexpr. Is that the right 
thing to do or should I just leave the test suite as is, knowing that 

someone/something will run the test suite with --stds=impcx at some 

point before a release?

And the next natural question is whether I should have also tested with 
--stds=impcx before my initial submission? 
https://gcc.gnu.org/contribute.html#testing advises to test with “make 
-k check”; maybe we need to also mention check-c++-all for changes to 
the C++ front-end?

Thanks again, Simon
From c724f939881c6808d2fc36b63e0093ec59af3a40 Mon Sep 17 00:00:00 2001
From: Simon Martin <simon@nasilyan.com>
Date: Tue, 1 Oct 2024 11:07:51 +0200
Subject: [PATCH] c++: Fix regression introduced by r15-3796 [PR116722]

Jason pointed out that the fix I made for PR116722 via r15-3796
introduces a regression when running constexpr-dynamic10.C with
-fimplicit-constexpr.

The problem is that my change makes us leave cxx_eval_call_expression
early, and bypass the call to cxx_eval_thunk_call (through a recursive
call to cxx_eval_call_expression) that used to emit an error for that
testcase with -fimplicit-constexpr.

This patch emits the error if !ctx->quiet before bailing out because the
{con,de}structor belongs to a class with virtual bases.

Successfully tested on x86_64-pc-linux-gnu and also with --stds=impcx,
that showed a failure and does not anymore.

	PR c++/116722

gcc/cp/ChangeLog:

	* constexpr.cc (cxx_bind_parameters_in_call): When !ctx->quiet,
	emit error before bailing out due to a call to {con,de}structor
	for a class with virtual bases.

gcc/testsuite/ChangeLog:

	* g++.dg/ext/fimplicit-constexpr2.C: New test.

---
 gcc/cp/constexpr.cc                             |  5 +++++
 gcc/testsuite/g++.dg/ext/fimplicit-constexpr2.C | 14 ++++++++++++++
 2 files changed, 19 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/ext/fimplicit-constexpr2.C

diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 5c6696740fc..36ad894cefb 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -1867,6 +1867,11 @@ cxx_bind_parameters_in_call (const constexpr_ctx *ctx, tree t, tree fun,
      with virtual bases.  */
   if (DECL_HAS_IN_CHARGE_PARM_P (fun) || DECL_HAS_VTT_PARM_P (fun))
     {
+      if (!ctx->quiet
+	  && constexpr_error (cp_expr_loc_or_input_loc (t),
+			      /*constexpr_fundef_p=*/false,
+			      "call to non-%<constexpr%> function %qD", fun))
+	explain_invalid_constexpr_fn (fun);
       *non_constant_p = true;
       return binds;
     }
diff --git a/gcc/testsuite/g++.dg/ext/fimplicit-constexpr2.C b/gcc/testsuite/g++.dg/ext/fimplicit-constexpr2.C
new file mode 100644
index 00000000000..76c050973a9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/fimplicit-constexpr2.C
@@ -0,0 +1,14 @@
+// PR c++/116722
+// Same as constexpr-dynamic10.C, but with -fimplicit-constexpr
+// { dg-do compile { target c++20 } }
+// { dg-additional-options -fimplicit-constexpr }
+
+// Virtual base.
+
+struct C { virtual void a(); };
+struct B { virtual void b(); };
+struct A : virtual B, C { virtual void c(); }; // { dg-error ".struct A. has virtual base classes" }
+
+constexpr A a; // { dg-error "call" }
+
+constexpr bool b1 = (dynamic_cast<C&>((B&)a), false);

On 10/1/24 12:44 PM, Simon Martin wrote:
> Hi Jason,
> 
> On 30 Sep 2024, at 19:56, Jason Merrill wrote:
> 
>> On 9/23/24 4:44 AM, Simon Martin wrote:
>>> Hi Jason,
>>>
>>> On 20 Sep 2024, at 18:01, Jason Merrill wrote:
>>>
>>>> On 9/20/24 5:21 PM, Simon Martin wrote:
>>>>> The following code triggers an ICE
>>>>>
>>>>> === cut here ===
>>>>> class base {};
>>>>> class derived : virtual public base {
>>>>> public:
>>>>>      template<typename Arg> constexpr derived(Arg) {}
>>>>> };
>>>>> int main() {
>>>>>      derived obj(1.);
>>>>> }
>>>>> === cut here ===
>>>>>
>>>>> The problem is that cxx_bind_parameters_in_call ends up attempting
>>>>> to
>>>
>>>>> convert a REAL_CST (the first non artificial parameter) to
>>>>> INTEGER_TYPE
>>>>> (the type of the __in_chrg parameter), which ICEs.
>>>>>
>>>>> This patch teaches cxx_bind_parameters_in_call to handle the
>>>>> __in_chrg
>>>>> and __vtt_parm parameters that {con,de}structors might have.
>>>>>
>>>>> Note that in the test case, the constructor is not
>>>>> constexpr-suitable,
>>>>> however it's OK since it's a template according to my read of
>>>>> paragraph
>>>>> (3) of [dcl.constexpr].
>>>>
>>>> Agreed.
>>>>
>>>> It looks like your patch doesn't correct the mismatching of
>>>> arguments
>>>> to parameters that you describe, but at least for now it should be
>>>> enough to set *non_constant_p and return if we see a VTT or
>>>> in-charge
>>>> parameter.
>>>>
>>> Thanks, it’s true that my initial patch was wrong in that we’d
>>> leave
>>> cxx_bind_parameters_in_call thinking the expression was actually a
>>> constant expression :-/
>>>
>>> The attached revised patch follows your suggestion (thanks!).
>>> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?
>>
>> After this patch I'm seeing a regression on constexpr-dynamic10.C with
>> -fimplicit-constexpr; we also need to give an error here when
>> (!ctx->quiet).
> Thanks, good catch. TIL about --stds=impcx...
> 
> The attached patch fixes the issue, and was successfully tested on
> x86_64-pc-linux-gnu, including with “make -C gcc -k check-c++-all”.
> OK for trunk?
> 
> Note that it includes a new test that’s basically a copy of
> constexpr-dynamic10.C, with -fimplicit-constexpr. Is that the right
> thing to do or should I just leave the test suite as is, knowing that
> someone/something will run the test suite with --stds=impcx at some
> point before a release?

I think leave it as is.

> And the next natural question is whether I should have also tested with
> --stds=impcx before my initial submission?

Probably a good idea when messing with constexpr.

> https://gcc.gnu.org/contribute.html#testing advises to test with “make
> -k check”; maybe we need to also mention check-c++-all for changes to
> the C++ front-end?

Sounds good.

> +	  && constexpr_error (cp_expr_loc_or_input_loc (t),
> +			      /*constexpr_fundef_p=*/false,
> +			      "call to non-%<constexpr%> function %qD", fun))

Let's use "error_at" here, like in cxx_eval_call_expression; 
constexpr_error with fundef_p false is equivalent.

OK with those adjustments.

Jason

Hi Jason,

On 2 Oct 2024, at 0:18, Jason Merrill wrote:

> On 10/1/24 12:44 PM, Simon Martin wrote:
>> Hi Jason,
>>
>> On 30 Sep 2024, at 19:56, Jason Merrill wrote:
>>
>>> On 9/23/24 4:44 AM, Simon Martin wrote:
>>>> Hi Jason,
>>>>
>>>> On 20 Sep 2024, at 18:01, Jason Merrill wrote:
>>>>
>>>>> On 9/20/24 5:21 PM, Simon Martin wrote:
>>>>>> The following code triggers an ICE
>>>>>>
>>>>>> === cut here ===
>>>>>> class base {};
>>>>>> class derived : virtual public base {
>>>>>> public:
>>>>>>      template<typename Arg> constexpr derived(Arg) {}
>>>>>> };
>>>>>> int main() {
>>>>>>      derived obj(1.);
>>>>>> }
>>>>>> === cut here ===
>>>>>>
>>>>>> The problem is that cxx_bind_parameters_in_call ends up 
>>>>>> attempting
>>>>>> to
>>>>
>>>>>> convert a REAL_CST (the first non artificial parameter) to
>>>>>> INTEGER_TYPE
>>>>>> (the type of the __in_chrg parameter), which ICEs.
>>>>>>
>>>>>> This patch teaches cxx_bind_parameters_in_call to handle the
>>>>>> __in_chrg
>>>>>> and __vtt_parm parameters that {con,de}structors might have.
>>>>>>
>>>>>> Note that in the test case, the constructor is not
>>>>>> constexpr-suitable,
>>>>>> however it's OK since it's a template according to my read of
>>>>>> paragraph
>>>>>> (3) of [dcl.constexpr].
>>>>>
>>>>> Agreed.
>>>>>
>>>>> It looks like your patch doesn't correct the mismatching of
>>>>> arguments
>>>>> to parameters that you describe, but at least for now it should be
>>>>> enough to set *non_constant_p and return if we see a VTT or
>>>>> in-charge
>>>>> parameter.
>>>>>
>>>> Thanks, it’s true that my initial patch was wrong in that we’d
>>>> leave
>>>> cxx_bind_parameters_in_call thinking the expression was actually a
>>>> constant expression :-/
>>>>
>>>> The attached revised patch follows your suggestion (thanks!).
>>>> Successfully tested on x86_64-pc-linux-gnu. OK for trunk?
>>>
>>> After this patch I'm seeing a regression on constexpr-dynamic10.C 
>>> with
>>> -fimplicit-constexpr; we also need to give an error here when
>>> (!ctx->quiet).
>> Thanks, good catch. TIL about --stds=impcx...
>>
>> The attached patch fixes the issue, and was successfully tested on
>> x86_64-pc-linux-gnu, including with “make -C gcc -k 
>> check-c++-all”.
>> OK for trunk?
>>
>> Note that it includes a new test that’s basically a copy of
>> constexpr-dynamic10.C, with -fimplicit-constexpr. Is that the right
>> thing to do or should I just leave the test suite as is, knowing that
>> someone/something will run the test suite with --stds=impcx at some
>> point before a release?
>
> I think leave it as is.
OK, it’s simpler :-)
>
>> And the next natural question is whether I should have also tested 
>> with
>> --stds=impcx before my initial submission?
>
> Probably a good idea when messing with constexpr.
ACK.
>
>> https://gcc.gnu.org/contribute.html#testing advises to test with 
>> “make
>> -k check”; maybe we need to also mention check-c++-all for changes 
>> to
>> the C++ front-end?
>
> Sounds good.
I sent 
https://gcc.gnu.org/pipermail/gcc-patches/2024-October/664275.html for 
that.
>
>> +	  && constexpr_error (cp_expr_loc_or_input_loc (t),
>> +			      /*constexpr_fundef_p=*/false,
>> +			      "call to non-%<constexpr%> function %qD", fun))
>
> Let's use "error_at" here, like in cxx_eval_call_expression; 
> constexpr_error with fundef_p false is equivalent.
>
> OK with those adjustments.
Thanks. Merged as r15—4026.

Simon