* [PATCH] c++: fix ICE with constexpr ARRAY_REF [PR110382]
@ 2023-07-21 22:38 Marek Polacek
2023-07-22 4:28 ` Jason Merrill
0 siblings, 1 reply; 6+ messages in thread
From: Marek Polacek @ 2023-07-21 22:38 UTC (permalink / raw)
To: Jason Merrill, GCC Patches; +Cc: Patrick Palka
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13?
-- >8 --
This code in cxx_eval_array_reference has been hard to get right.
In r12-2304 I added some code; in r13-5693 I removed some of it.
Here the problematic line is "S s = arr[0];" which causes a crash
on the assert in verify_ctor_sanity:
gcc_assert (!ctx->object || !DECL_P (ctx->object)
|| ctx->global->get_value (ctx->object) == ctx->ctor);
ctx->object is the VAR_DECL 's', which is correct here. The second
line points to the problem: we replaced ctx->ctor in
cxx_eval_array_reference:
new_ctx.ctor = build_constructor (elem_type, NULL); // #1
which I think we shouldn't have; the CONSTRUCTOR we created in
cxx_eval_constant_expression/DECL_EXPR
new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL);
had the right type.
We still need #1 though. E.g., in constexpr-96241.C, we never
set ctx.ctor/object before calling cxx_eval_array_reference, so
we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C
we have a ctx.ctor, but it has the wrong type, so we need a new one.
PR c++/110382
gcc/cp/ChangeLog:
* constexpr.cc (cxx_eval_array_reference): Create a new constructor
only when we don't already have a matching one.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1y/constexpr-110382.C: New test.
---
gcc/cp/constexpr.cc | 5 ++++-
gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++
2 files changed, 21 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index fb94f3cefcb..518b7c7a2d5 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -4291,7 +4291,10 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
else
val = build_value_init (elem_type, tf_warning_or_error);
- if (!SCALAR_TYPE_P (elem_type))
+ if (!SCALAR_TYPE_P (elem_type)
+ /* Create a new constructor only if we don't already have one that
+ is suitable. */
+ && !(ctx->ctor && same_type_p (elem_type, TREE_TYPE (ctx->ctor))))
{
new_ctx = *ctx;
new_ctx.ctor = build_constructor (elem_type, NULL);
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C
new file mode 100644
index 00000000000..317c5ecfcd5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C
@@ -0,0 +1,17 @@
+// PR c++/110382
+// { dg-do compile { target c++14 } }
+
+struct S {
+ double a = 0;
+};
+
+constexpr double
+g ()
+{
+ S arr[1];
+ S s = arr[0];
+ (void) arr[0];
+ return s.a;
+}
+
+int main() { return g (); }
base-commit: 87516efcbe28884c39a8c68e600d11cc91ed96c7
--
2.41.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] c++: fix ICE with constexpr ARRAY_REF [PR110382] 2023-07-21 22:38 [PATCH] c++: fix ICE with constexpr ARRAY_REF [PR110382] Marek Polacek @ 2023-07-22 4:28 ` Jason Merrill 2023-07-24 22:37 ` [PATCH v2] " Marek Polacek 0 siblings, 1 reply; 6+ messages in thread From: Jason Merrill @ 2023-07-22 4:28 UTC (permalink / raw) To: Marek Polacek, GCC Patches; +Cc: Patrick Palka On 7/21/23 18:38, Marek Polacek wrote: > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13? > > -- >8 -- > > This code in cxx_eval_array_reference has been hard to get right. > In r12-2304 I added some code; in r13-5693 I removed some of it. > > Here the problematic line is "S s = arr[0];" which causes a crash > on the assert in verify_ctor_sanity: > > gcc_assert (!ctx->object || !DECL_P (ctx->object) > || ctx->global->get_value (ctx->object) == ctx->ctor); > > ctx->object is the VAR_DECL 's', which is correct here. The second > line points to the problem: we replaced ctx->ctor in > cxx_eval_array_reference: > > new_ctx.ctor = build_constructor (elem_type, NULL); // #1 ...and this code doesn't also clear(/set) new_ctx.object like everywhere else in constexpr.cc that sets new_ctx.ctor. Fixing that should make the testcase work. > which I think we shouldn't have; the CONSTRUCTOR we created in > cxx_eval_constant_expression/DECL_EXPR > > new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); > > had the right type. Indeed, and using it rather than building a new one seems like a valid optimization for trunk. I also notice that the DECL_EXPR code calls unshare_constructor, which should be unnecessary if init == ctx->ctor? > We still need #1 though. E.g., in constexpr-96241.C, we never > set ctx.ctor/object before calling cxx_eval_array_reference, so > we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C > we have a ctx.ctor, but it has the wrong type, so we need a new one. > > PR c++/110382 > > gcc/cp/ChangeLog: > > * constexpr.cc (cxx_eval_array_reference): Create a new constructor > only when we don't already have a matching one. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/constexpr-110382.C: New test. > --- > gcc/cp/constexpr.cc | 5 ++++- > gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ > 2 files changed, 21 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index fb94f3cefcb..518b7c7a2d5 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -4291,7 +4291,10 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, > else > val = build_value_init (elem_type, tf_warning_or_error); > > - if (!SCALAR_TYPE_P (elem_type)) > + if (!SCALAR_TYPE_P (elem_type) > + /* Create a new constructor only if we don't already have one that > + is suitable. */ > + && !(ctx->ctor && same_type_p (elem_type, TREE_TYPE (ctx->ctor)))) We generally use same_type_ignoring_top_level_qualifiers_p in the constexpr code. Jason ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] c++: fix ICE with constexpr ARRAY_REF [PR110382] 2023-07-22 4:28 ` Jason Merrill @ 2023-07-24 22:37 ` Marek Polacek 2023-07-25 15:15 ` Jason Merrill 0 siblings, 1 reply; 6+ messages in thread From: Marek Polacek @ 2023-07-24 22:37 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches, Patrick Palka On Sat, Jul 22, 2023 at 12:28:59AM -0400, Jason Merrill wrote: > On 7/21/23 18:38, Marek Polacek wrote: > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13? > > > > -- >8 -- > > > > This code in cxx_eval_array_reference has been hard to get right. > > In r12-2304 I added some code; in r13-5693 I removed some of it. > > > > Here the problematic line is "S s = arr[0];" which causes a crash > > on the assert in verify_ctor_sanity: > > > > gcc_assert (!ctx->object || !DECL_P (ctx->object) > > || ctx->global->get_value (ctx->object) == ctx->ctor); > > > > ctx->object is the VAR_DECL 's', which is correct here. The second > > line points to the problem: we replaced ctx->ctor in > > cxx_eval_array_reference: > > > > new_ctx.ctor = build_constructor (elem_type, NULL); // #1 > > ...and this code doesn't also clear(/set) new_ctx.object like everywhere > else in constexpr.cc that sets new_ctx.ctor. Fixing that should make the > testcase work. Right, but then we'd be back pre-r12-2304 or r13-5693... ...except it should work to always clear the object, like below. > > which I think we shouldn't have; the CONSTRUCTOR we created in > > cxx_eval_constant_expression/DECL_EXPR > > > > new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); > > > > had the right type. > > Indeed, and using it rather than building a new one seems like a valid > optimization for trunk. Agreed, I kept it. > I also notice that the DECL_EXPR code calls unshare_constructor, which > should be unnecessary if init == ctx->ctor? It looks like init == ctx->ctor only happens only with this new testcase. I'm not sure it's worth it adding code for such a rare case? > > We still need #1 though. E.g., in constexpr-96241.C, we never > > set ctx.ctor/object before calling cxx_eval_array_reference, so > > we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C > > we have a ctx.ctor, but it has the wrong type, so we need a new one. > > > > PR c++/110382 > > > > gcc/cp/ChangeLog: > > > > * constexpr.cc (cxx_eval_array_reference): Create a new constructor > > only when we don't already have a matching one. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp1y/constexpr-110382.C: New test. > > --- > > gcc/cp/constexpr.cc | 5 ++++- > > gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ > > 2 files changed, 21 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > index fb94f3cefcb..518b7c7a2d5 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -4291,7 +4291,10 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, > > else > > val = build_value_init (elem_type, tf_warning_or_error); > > - if (!SCALAR_TYPE_P (elem_type)) > > + if (!SCALAR_TYPE_P (elem_type) > > + /* Create a new constructor only if we don't already have one that > > + is suitable. */ > > + && !(ctx->ctor && same_type_p (elem_type, TREE_TYPE (ctx->ctor)))) > > We generally use same_type_ignoring_top_level_qualifiers_p in the constexpr > code. True, changed. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? For 13, I guess I should only clear the object and leave out the same_type_ bit. -- >8 -- This code in cxx_eval_array_reference has been hard to get right. In r12-2304 I added some code; in r13-5693 I removed some of it. Here the problematic line is "S s = arr[0];" which causes a crash on the assert in verify_ctor_sanity: gcc_assert (!ctx->object || !DECL_P (ctx->object) || ctx->global->get_value (ctx->object) == ctx->ctor); ctx->object is the VAR_DECL 's', which is correct here. The second line points to the problem: we replaced ctx->ctor in cxx_eval_array_reference: new_ctx.ctor = build_constructor (elem_type, NULL); // #1 which I think we shouldn't have; the CONSTRUCTOR we created in cxx_eval_constant_expression/DECL_EXPR new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); had the right type. We still need #1 though. E.g., in constexpr-96241.C, we never set ctx.ctor/object before calling cxx_eval_array_reference, so we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C we have a ctx.ctor, but it has the wrong type, so we need a new one. We can fix the problem by always clearing the object, and, as an optimization, only create/free a new ctor when actually needed. PR c++/110382 gcc/cp/ChangeLog: * constexpr.cc (cxx_eval_array_reference): Create a new constructor only when we don't already have a matching one. Clear the object when the type is non-scalar. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/constexpr-110382.C: New test. --- gcc/cp/constexpr.cc | 17 +++++++++++++++-- gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index fb94f3cefcb..054812bcb72 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -4291,15 +4291,28 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, else val = build_value_init (elem_type, tf_warning_or_error); + bool new_ctor; if (!SCALAR_TYPE_P (elem_type)) { new_ctx = *ctx; - new_ctx.ctor = build_constructor (elem_type, NULL); + /* We clear the object here. We used to replace it with T, but that + caused problems (101371, 108158); and anyway, T is the initializer, + not the target object. */ + new_ctx.object = NULL_TREE; + /* Create a new constructor only if we don't already have a suitable + one. */ + new_ctor = (!ctx->ctor + || !same_type_ignoring_top_level_qualifiers_p + (elem_type, TREE_TYPE (ctx->ctor))); + if (new_ctor) + new_ctx.ctor = build_constructor (elem_type, NULL); ctx = &new_ctx; } + else + new_ctor = false; t = cxx_eval_constant_expression (ctx, val, lval, non_constant_p, overflow_p); - if (!SCALAR_TYPE_P (elem_type) && t != ctx->ctor) + if (new_ctor && t != ctx->ctor) free_constructor (ctx->ctor); return t; } diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C new file mode 100644 index 00000000000..317c5ecfcd5 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C @@ -0,0 +1,17 @@ +// PR c++/110382 +// { dg-do compile { target c++14 } } + +struct S { + double a = 0; +}; + +constexpr double +g () +{ + S arr[1]; + S s = arr[0]; + (void) arr[0]; + return s.a; +} + +int main() { return g (); } base-commit: 96482ffe60d9bdec802fcad705c69641b2a3e040 -- 2.41.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] c++: fix ICE with constexpr ARRAY_REF [PR110382] 2023-07-24 22:37 ` [PATCH v2] " Marek Polacek @ 2023-07-25 15:15 ` Jason Merrill 2023-07-25 16:59 ` [PATCH v3] " Marek Polacek 0 siblings, 1 reply; 6+ messages in thread From: Jason Merrill @ 2023-07-25 15:15 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Patrick Palka On 7/24/23 18:37, Marek Polacek wrote: > On Sat, Jul 22, 2023 at 12:28:59AM -0400, Jason Merrill wrote: >> On 7/21/23 18:38, Marek Polacek wrote: >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13? >>> >>> -- >8 -- >>> >>> This code in cxx_eval_array_reference has been hard to get right. >>> In r12-2304 I added some code; in r13-5693 I removed some of it. >>> >>> Here the problematic line is "S s = arr[0];" which causes a crash >>> on the assert in verify_ctor_sanity: >>> >>> gcc_assert (!ctx->object || !DECL_P (ctx->object) >>> || ctx->global->get_value (ctx->object) == ctx->ctor); >>> >>> ctx->object is the VAR_DECL 's', which is correct here. The second >>> line points to the problem: we replaced ctx->ctor in >>> cxx_eval_array_reference: >>> >>> new_ctx.ctor = build_constructor (elem_type, NULL); // #1 >> >> ...and this code doesn't also clear(/set) new_ctx.object like everywhere >> else in constexpr.cc that sets new_ctx.ctor. Fixing that should make the >> testcase work. > > Right, but then we'd be back pre-r12-2304 or r13-5693... > > ...except it should work to always clear the object, like below. > >>> which I think we shouldn't have; the CONSTRUCTOR we created in >>> cxx_eval_constant_expression/DECL_EXPR >>> >>> new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); >>> >>> had the right type. >> >> Indeed, and using it rather than building a new one seems like a valid >> optimization for trunk. > > Agreed, I kept it. > >> I also notice that the DECL_EXPR code calls unshare_constructor, which >> should be unnecessary if init == ctx->ctor? > > It looks like init == ctx->ctor only happens only with this new testcase. > I'm not sure it's worth it adding code for such a rare case? > >>> We still need #1 though. E.g., in constexpr-96241.C, we never >>> set ctx.ctor/object before calling cxx_eval_array_reference, so >>> we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C >>> we have a ctx.ctor, but it has the wrong type, so we need a new one. >>> >>> PR c++/110382 >>> >>> gcc/cp/ChangeLog: >>> >>> * constexpr.cc (cxx_eval_array_reference): Create a new constructor >>> only when we don't already have a matching one. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/cpp1y/constexpr-110382.C: New test. >>> --- >>> gcc/cp/constexpr.cc | 5 ++++- >>> gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ >>> 2 files changed, 21 insertions(+), 1 deletion(-) >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C >>> >>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc >>> index fb94f3cefcb..518b7c7a2d5 100644 >>> --- a/gcc/cp/constexpr.cc >>> +++ b/gcc/cp/constexpr.cc >>> @@ -4291,7 +4291,10 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, >>> else >>> val = build_value_init (elem_type, tf_warning_or_error); >>> - if (!SCALAR_TYPE_P (elem_type)) >>> + if (!SCALAR_TYPE_P (elem_type) >>> + /* Create a new constructor only if we don't already have one that >>> + is suitable. */ >>> + && !(ctx->ctor && same_type_p (elem_type, TREE_TYPE (ctx->ctor)))) >> >> We generally use same_type_ignoring_top_level_qualifiers_p in the constexpr >> code. > > True, changed. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > For 13, I guess I should only clear the object and leave out the > same_type_ bit. > > -- >8 -- > This code in cxx_eval_array_reference has been hard to get right. > In r12-2304 I added some code; in r13-5693 I removed some of it. > > Here the problematic line is "S s = arr[0];" which causes a crash > on the assert in verify_ctor_sanity: > > gcc_assert (!ctx->object || !DECL_P (ctx->object) > || ctx->global->get_value (ctx->object) == ctx->ctor); > > ctx->object is the VAR_DECL 's', which is correct here. The second > line points to the problem: we replaced ctx->ctor in > cxx_eval_array_reference: > > new_ctx.ctor = build_constructor (elem_type, NULL); // #1 > > which I think we shouldn't have; the CONSTRUCTOR we created in > cxx_eval_constant_expression/DECL_EXPR > > new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); > > had the right type. > > We still need #1 though. E.g., in constexpr-96241.C, we never > set ctx.ctor/object before calling cxx_eval_array_reference, so > we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C > we have a ctx.ctor, but it has the wrong type, so we need a new one. > > We can fix the problem by always clearing the object, and, as an > optimization, only create/free a new ctor when actually needed. > > PR c++/110382 > > gcc/cp/ChangeLog: > > * constexpr.cc (cxx_eval_array_reference): Create a new constructor > only when we don't already have a matching one. Clear the object > when the type is non-scalar. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/constexpr-110382.C: New test. > --- > gcc/cp/constexpr.cc | 17 +++++++++++++++-- > gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ > 2 files changed, 32 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index fb94f3cefcb..054812bcb72 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -4291,15 +4291,28 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, > else > val = build_value_init (elem_type, tf_warning_or_error); > > + bool new_ctor; > if (!SCALAR_TYPE_P (elem_type)) > { > new_ctx = *ctx; > - new_ctx.ctor = build_constructor (elem_type, NULL); > + /* We clear the object here. We used to replace it with T, but that > + caused problems (101371, 108158); and anyway, T is the initializer, > + not the target object. */ > + new_ctx.object = NULL_TREE; > + /* Create a new constructor only if we don't already have a suitable > + one. */ > + new_ctor = (!ctx->ctor > + || !same_type_ignoring_top_level_qualifiers_p > + (elem_type, TREE_TYPE (ctx->ctor))); > + if (new_ctor) > + new_ctx.ctor = build_constructor (elem_type, NULL); I think clearing .object should (only) happen along with changing .ctor; if we leave .ctor alone, we should also leave the matching .object alone. > ctx = &new_ctx; > } > + else > + new_ctor = false; > t = cxx_eval_constant_expression (ctx, val, lval, non_constant_p, > overflow_p); > - if (!SCALAR_TYPE_P (elem_type) && t != ctx->ctor) > + if (new_ctor && t != ctx->ctor) > free_constructor (ctx->ctor); > return t; > } > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > new file mode 100644 > index 00000000000..317c5ecfcd5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > @@ -0,0 +1,17 @@ > +// PR c++/110382 > +// { dg-do compile { target c++14 } } > + > +struct S { > + double a = 0; > +}; > + > +constexpr double > +g () > +{ > + S arr[1]; > + S s = arr[0]; > + (void) arr[0]; > + return s.a; > +} > + > +int main() { return g (); } > > base-commit: 96482ffe60d9bdec802fcad705c69641b2a3e040 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] c++: fix ICE with constexpr ARRAY_REF [PR110382] 2023-07-25 15:15 ` Jason Merrill @ 2023-07-25 16:59 ` Marek Polacek 2023-07-25 18:01 ` Jason Merrill 0 siblings, 1 reply; 6+ messages in thread From: Marek Polacek @ 2023-07-25 16:59 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches, Patrick Palka On Tue, Jul 25, 2023 at 11:15:07AM -0400, Jason Merrill wrote: > On 7/24/23 18:37, Marek Polacek wrote: > > On Sat, Jul 22, 2023 at 12:28:59AM -0400, Jason Merrill wrote: > > > On 7/21/23 18:38, Marek Polacek wrote: > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13? > > > > > > > > -- >8 -- > > > > > > > > This code in cxx_eval_array_reference has been hard to get right. > > > > In r12-2304 I added some code; in r13-5693 I removed some of it. > > > > > > > > Here the problematic line is "S s = arr[0];" which causes a crash > > > > on the assert in verify_ctor_sanity: > > > > > > > > gcc_assert (!ctx->object || !DECL_P (ctx->object) > > > > || ctx->global->get_value (ctx->object) == ctx->ctor); > > > > > > > > ctx->object is the VAR_DECL 's', which is correct here. The second > > > > line points to the problem: we replaced ctx->ctor in > > > > cxx_eval_array_reference: > > > > > > > > new_ctx.ctor = build_constructor (elem_type, NULL); // #1 > > > > > > ...and this code doesn't also clear(/set) new_ctx.object like everywhere > > > else in constexpr.cc that sets new_ctx.ctor. Fixing that should make the > > > testcase work. > > > > Right, but then we'd be back pre-r12-2304 or r13-5693... > > > > ...except it should work to always clear the object, like below. > > > > which I think we shouldn't have; the CONSTRUCTOR we created in > > > > cxx_eval_constant_expression/DECL_EXPR > > > > > > > > new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); > > > > > > > > had the right type. > > > > > > Indeed, and using it rather than building a new one seems like a valid > > > optimization for trunk. > > Agreed, I kept it. > > > > > I also notice that the DECL_EXPR code calls unshare_constructor, which > > > should be unnecessary if init == ctx->ctor? > > > > It looks like init == ctx->ctor only happens only with this new testcase. > > I'm not sure it's worth it adding code for such a rare case? > > > > We still need #1 though. E.g., in constexpr-96241.C, we never > > > > set ctx.ctor/object before calling cxx_eval_array_reference, so > > > > we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C > > > > we have a ctx.ctor, but it has the wrong type, so we need a new one. > > > > > > > > PR c++/110382 > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * constexpr.cc (cxx_eval_array_reference): Create a new constructor > > > > only when we don't already have a matching one. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * g++.dg/cpp1y/constexpr-110382.C: New test. > > > > --- > > > > gcc/cp/constexpr.cc | 5 ++++- > > > > gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ > > > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > > > > > > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > > > index fb94f3cefcb..518b7c7a2d5 100644 > > > > --- a/gcc/cp/constexpr.cc > > > > +++ b/gcc/cp/constexpr.cc > > > > @@ -4291,7 +4291,10 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, > > > > else > > > > val = build_value_init (elem_type, tf_warning_or_error); > > > > - if (!SCALAR_TYPE_P (elem_type)) > > > > + if (!SCALAR_TYPE_P (elem_type) > > > > + /* Create a new constructor only if we don't already have one that > > > > + is suitable. */ > > > > + && !(ctx->ctor && same_type_p (elem_type, TREE_TYPE (ctx->ctor)))) > > > > > > We generally use same_type_ignoring_top_level_qualifiers_p in the constexpr > > > code. > > > > True, changed. > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > For 13, I guess I should only clear the object and leave out the > > same_type_ bit. > > > > -- >8 -- > > This code in cxx_eval_array_reference has been hard to get right. > > In r12-2304 I added some code; in r13-5693 I removed some of it. > > > > Here the problematic line is "S s = arr[0];" which causes a crash > > on the assert in verify_ctor_sanity: > > > > gcc_assert (!ctx->object || !DECL_P (ctx->object) > > || ctx->global->get_value (ctx->object) == ctx->ctor); > > > > ctx->object is the VAR_DECL 's', which is correct here. The second > > line points to the problem: we replaced ctx->ctor in > > cxx_eval_array_reference: > > > > new_ctx.ctor = build_constructor (elem_type, NULL); // #1 > > > > which I think we shouldn't have; the CONSTRUCTOR we created in > > cxx_eval_constant_expression/DECL_EXPR > > > > new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); > > > > had the right type. > > > > We still need #1 though. E.g., in constexpr-96241.C, we never > > set ctx.ctor/object before calling cxx_eval_array_reference, so > > we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C > > we have a ctx.ctor, but it has the wrong type, so we need a new one. > > > > We can fix the problem by always clearing the object, and, as an > > optimization, only create/free a new ctor when actually needed. > > > > PR c++/110382 > > > > gcc/cp/ChangeLog: > > > > * constexpr.cc (cxx_eval_array_reference): Create a new constructor > > only when we don't already have a matching one. Clear the object > > when the type is non-scalar. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp1y/constexpr-110382.C: New test. > > --- > > gcc/cp/constexpr.cc | 17 +++++++++++++++-- > > gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ > > 2 files changed, 32 insertions(+), 2 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > index fb94f3cefcb..054812bcb72 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -4291,15 +4291,28 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, > > else > > val = build_value_init (elem_type, tf_warning_or_error); > > + bool new_ctor; > > if (!SCALAR_TYPE_P (elem_type)) > > { > > new_ctx = *ctx; > > - new_ctx.ctor = build_constructor (elem_type, NULL); > > + /* We clear the object here. We used to replace it with T, but that > > + caused problems (101371, 108158); and anyway, T is the initializer, > > + not the target object. */ > > + new_ctx.object = NULL_TREE; > > + /* Create a new constructor only if we don't already have a suitable > > + one. */ > > + new_ctor = (!ctx->ctor > > + || !same_type_ignoring_top_level_qualifiers_p > > + (elem_type, TREE_TYPE (ctx->ctor))); > > + if (new_ctor) > > + new_ctx.ctor = build_constructor (elem_type, NULL); > > I think clearing .object should (only) happen along with changing .ctor; if > we leave .ctor alone, we should also leave the matching .object alone. Ah, true. Thus: Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- This code in cxx_eval_array_reference has been hard to get right. In r12-2304 I added some code; in r13-5693 I removed some of it. Here the problematic line is "S s = arr[0];" which causes a crash on the assert in verify_ctor_sanity: gcc_assert (!ctx->object || !DECL_P (ctx->object) || ctx->global->get_value (ctx->object) == ctx->ctor); ctx->object is the VAR_DECL 's', which is correct here. The second line points to the problem: we replaced ctx->ctor in cxx_eval_array_reference: new_ctx.ctor = build_constructor (elem_type, NULL); // #1 which I think we shouldn't have; the CONSTRUCTOR we created in cxx_eval_constant_expression/DECL_EXPR new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); had the right type. We still need #1 though. E.g., in constexpr-96241.C, we never set ctx.ctor/object before calling cxx_eval_array_reference, so we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C we have a ctx.ctor, but it has the wrong type, so we need a new one. We can fix the problem by always clearing the object, and, as an optimization, only create/free a new ctor when actually needed. PR c++/110382 gcc/cp/ChangeLog: * constexpr.cc (cxx_eval_array_reference): Create a new constructor only when we don't already have a matching one. Clear the object when the type is non-scalar. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/constexpr-110382.C: New test. --- gcc/cp/constexpr.cc | 13 +++++++++++-- gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index fb94f3cefcb..5e0fe93ebc0 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -4291,15 +4291,24 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, else val = build_value_init (elem_type, tf_warning_or_error); - if (!SCALAR_TYPE_P (elem_type)) + /* Create a new constructor only if we don't already have a suitable one. */ + const bool new_ctor = (!SCALAR_TYPE_P (elem_type) + && (!ctx->ctor + || !same_type_ignoring_top_level_qualifiers_p + (elem_type, TREE_TYPE (ctx->ctor)))); + if (new_ctor) { new_ctx = *ctx; + /* We clear the object here. We used to replace it with T, but that + caused problems (101371, 108158); and anyway, T is the initializer, + not the target object. */ + new_ctx.object = NULL_TREE; new_ctx.ctor = build_constructor (elem_type, NULL); ctx = &new_ctx; } t = cxx_eval_constant_expression (ctx, val, lval, non_constant_p, overflow_p); - if (!SCALAR_TYPE_P (elem_type) && t != ctx->ctor) + if (new_ctor && t != ctx->ctor) free_constructor (ctx->ctor); return t; } diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C new file mode 100644 index 00000000000..317c5ecfcd5 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C @@ -0,0 +1,17 @@ +// PR c++/110382 +// { dg-do compile { target c++14 } } + +struct S { + double a = 0; +}; + +constexpr double +g () +{ + S arr[1]; + S s = arr[0]; + (void) arr[0]; + return s.a; +} + +int main() { return g (); } base-commit: 09dda270380fe13e7b4722305cb1122df1f779a0 -- 2.41.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] c++: fix ICE with constexpr ARRAY_REF [PR110382] 2023-07-25 16:59 ` [PATCH v3] " Marek Polacek @ 2023-07-25 18:01 ` Jason Merrill 0 siblings, 0 replies; 6+ messages in thread From: Jason Merrill @ 2023-07-25 18:01 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Patrick Palka On 7/25/23 12:59, Marek Polacek wrote: > On Tue, Jul 25, 2023 at 11:15:07AM -0400, Jason Merrill wrote: >> On 7/24/23 18:37, Marek Polacek wrote: >>> On Sat, Jul 22, 2023 at 12:28:59AM -0400, Jason Merrill wrote: >>>> On 7/21/23 18:38, Marek Polacek wrote: >>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13? >>>>> >>>>> -- >8 -- >>>>> >>>>> This code in cxx_eval_array_reference has been hard to get right. >>>>> In r12-2304 I added some code; in r13-5693 I removed some of it. >>>>> >>>>> Here the problematic line is "S s = arr[0];" which causes a crash >>>>> on the assert in verify_ctor_sanity: >>>>> >>>>> gcc_assert (!ctx->object || !DECL_P (ctx->object) >>>>> || ctx->global->get_value (ctx->object) == ctx->ctor); >>>>> >>>>> ctx->object is the VAR_DECL 's', which is correct here. The second >>>>> line points to the problem: we replaced ctx->ctor in >>>>> cxx_eval_array_reference: >>>>> >>>>> new_ctx.ctor = build_constructor (elem_type, NULL); // #1 >>>> >>>> ...and this code doesn't also clear(/set) new_ctx.object like everywhere >>>> else in constexpr.cc that sets new_ctx.ctor. Fixing that should make the >>>> testcase work. >>> >>> Right, but then we'd be back pre-r12-2304 or r13-5693... >>> >>> ...except it should work to always clear the object, like below. >>>>> which I think we shouldn't have; the CONSTRUCTOR we created in >>>>> cxx_eval_constant_expression/DECL_EXPR >>>>> >>>>> new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); >>>>> >>>>> had the right type. >>>> >>>> Indeed, and using it rather than building a new one seems like a valid >>>> optimization for trunk. >>> Agreed, I kept it. >>> >>>> I also notice that the DECL_EXPR code calls unshare_constructor, which >>>> should be unnecessary if init == ctx->ctor? >>> >>> It looks like init == ctx->ctor only happens only with this new testcase. >>> I'm not sure it's worth it adding code for such a rare case? >>>>> We still need #1 though. E.g., in constexpr-96241.C, we never >>>>> set ctx.ctor/object before calling cxx_eval_array_reference, so >>>>> we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C >>>>> we have a ctx.ctor, but it has the wrong type, so we need a new one. >>>>> >>>>> PR c++/110382 >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * constexpr.cc (cxx_eval_array_reference): Create a new constructor >>>>> only when we don't already have a matching one. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * g++.dg/cpp1y/constexpr-110382.C: New test. >>>>> --- >>>>> gcc/cp/constexpr.cc | 5 ++++- >>>>> gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ >>>>> 2 files changed, 21 insertions(+), 1 deletion(-) >>>>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C >>>>> >>>>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc >>>>> index fb94f3cefcb..518b7c7a2d5 100644 >>>>> --- a/gcc/cp/constexpr.cc >>>>> +++ b/gcc/cp/constexpr.cc >>>>> @@ -4291,7 +4291,10 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, >>>>> else >>>>> val = build_value_init (elem_type, tf_warning_or_error); >>>>> - if (!SCALAR_TYPE_P (elem_type)) >>>>> + if (!SCALAR_TYPE_P (elem_type) >>>>> + /* Create a new constructor only if we don't already have one that >>>>> + is suitable. */ >>>>> + && !(ctx->ctor && same_type_p (elem_type, TREE_TYPE (ctx->ctor)))) >>>> >>>> We generally use same_type_ignoring_top_level_qualifiers_p in the constexpr >>>> code. >>> >>> True, changed. >>> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>> >>> For 13, I guess I should only clear the object and leave out the >>> same_type_ bit. >>> >>> -- >8 -- >>> This code in cxx_eval_array_reference has been hard to get right. >>> In r12-2304 I added some code; in r13-5693 I removed some of it. >>> >>> Here the problematic line is "S s = arr[0];" which causes a crash >>> on the assert in verify_ctor_sanity: >>> >>> gcc_assert (!ctx->object || !DECL_P (ctx->object) >>> || ctx->global->get_value (ctx->object) == ctx->ctor); >>> >>> ctx->object is the VAR_DECL 's', which is correct here. The second >>> line points to the problem: we replaced ctx->ctor in >>> cxx_eval_array_reference: >>> >>> new_ctx.ctor = build_constructor (elem_type, NULL); // #1 >>> >>> which I think we shouldn't have; the CONSTRUCTOR we created in >>> cxx_eval_constant_expression/DECL_EXPR >>> >>> new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); >>> >>> had the right type. >>> >>> We still need #1 though. E.g., in constexpr-96241.C, we never >>> set ctx.ctor/object before calling cxx_eval_array_reference, so >>> we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C >>> we have a ctx.ctor, but it has the wrong type, so we need a new one. >>> >>> We can fix the problem by always clearing the object, and, as an >>> optimization, only create/free a new ctor when actually needed. >>> >>> PR c++/110382 >>> >>> gcc/cp/ChangeLog: >>> >>> * constexpr.cc (cxx_eval_array_reference): Create a new constructor >>> only when we don't already have a matching one. Clear the object >>> when the type is non-scalar. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/cpp1y/constexpr-110382.C: New test. >>> --- >>> gcc/cp/constexpr.cc | 17 +++++++++++++++-- >>> gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ >>> 2 files changed, 32 insertions(+), 2 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C >>> >>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc >>> index fb94f3cefcb..054812bcb72 100644 >>> --- a/gcc/cp/constexpr.cc >>> +++ b/gcc/cp/constexpr.cc >>> @@ -4291,15 +4291,28 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, >>> else >>> val = build_value_init (elem_type, tf_warning_or_error); >>> + bool new_ctor; >>> if (!SCALAR_TYPE_P (elem_type)) >>> { >>> new_ctx = *ctx; >>> - new_ctx.ctor = build_constructor (elem_type, NULL); >>> + /* We clear the object here. We used to replace it with T, but that >>> + caused problems (101371, 108158); and anyway, T is the initializer, >>> + not the target object. */ >>> + new_ctx.object = NULL_TREE; >>> + /* Create a new constructor only if we don't already have a suitable >>> + one. */ >>> + new_ctor = (!ctx->ctor >>> + || !same_type_ignoring_top_level_qualifiers_p >>> + (elem_type, TREE_TYPE (ctx->ctor))); >>> + if (new_ctor) >>> + new_ctx.ctor = build_constructor (elem_type, NULL); >> >> I think clearing .object should (only) happen along with changing .ctor; if >> we leave .ctor alone, we should also leave the matching .object alone. > > Ah, true. Thus: > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? OK. > -- >8 -- > This code in cxx_eval_array_reference has been hard to get right. > In r12-2304 I added some code; in r13-5693 I removed some of it. > > Here the problematic line is "S s = arr[0];" which causes a crash > on the assert in verify_ctor_sanity: > > gcc_assert (!ctx->object || !DECL_P (ctx->object) > || ctx->global->get_value (ctx->object) == ctx->ctor); > > ctx->object is the VAR_DECL 's', which is correct here. The second > line points to the problem: we replaced ctx->ctor in > cxx_eval_array_reference: > > new_ctx.ctor = build_constructor (elem_type, NULL); // #1 > > which I think we shouldn't have; the CONSTRUCTOR we created in > cxx_eval_constant_expression/DECL_EXPR > > new_ctx.ctor = build_constructor (TREE_TYPE (r), NULL); > > had the right type. > > We still need #1 though. E.g., in constexpr-96241.C, we never > set ctx.ctor/object before calling cxx_eval_array_reference, so > we have to build a CONSTRUCTOR there. And in constexpr-101371-2.C > we have a ctx.ctor, but it has the wrong type, so we need a new one. > > We can fix the problem by always clearing the object, and, as an > optimization, only create/free a new ctor when actually needed. > > PR c++/110382 > > gcc/cp/ChangeLog: > > * constexpr.cc (cxx_eval_array_reference): Create a new constructor > only when we don't already have a matching one. Clear the object > when the type is non-scalar. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/constexpr-110382.C: New test. > --- > gcc/cp/constexpr.cc | 13 +++++++++++-- > gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C | 17 +++++++++++++++++ > 2 files changed, 28 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index fb94f3cefcb..5e0fe93ebc0 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -4291,15 +4291,24 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t, > else > val = build_value_init (elem_type, tf_warning_or_error); > > - if (!SCALAR_TYPE_P (elem_type)) > + /* Create a new constructor only if we don't already have a suitable one. */ > + const bool new_ctor = (!SCALAR_TYPE_P (elem_type) > + && (!ctx->ctor > + || !same_type_ignoring_top_level_qualifiers_p > + (elem_type, TREE_TYPE (ctx->ctor)))); > + if (new_ctor) > { > new_ctx = *ctx; > + /* We clear the object here. We used to replace it with T, but that > + caused problems (101371, 108158); and anyway, T is the initializer, > + not the target object. */ > + new_ctx.object = NULL_TREE; > new_ctx.ctor = build_constructor (elem_type, NULL); > ctx = &new_ctx; > } > t = cxx_eval_constant_expression (ctx, val, lval, non_constant_p, > overflow_p); > - if (!SCALAR_TYPE_P (elem_type) && t != ctx->ctor) > + if (new_ctor && t != ctx->ctor) > free_constructor (ctx->ctor); > return t; > } > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > new file mode 100644 > index 00000000000..317c5ecfcd5 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-110382.C > @@ -0,0 +1,17 @@ > +// PR c++/110382 > +// { dg-do compile { target c++14 } } > + > +struct S { > + double a = 0; > +}; > + > +constexpr double > +g () > +{ > + S arr[1]; > + S s = arr[0]; > + (void) arr[0]; > + return s.a; > +} > + > +int main() { return g (); } > > base-commit: 09dda270380fe13e7b4722305cb1122df1f779a0 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-07-25 18:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-07-21 22:38 [PATCH] c++: fix ICE with constexpr ARRAY_REF [PR110382] Marek Polacek 2023-07-22 4:28 ` Jason Merrill 2023-07-24 22:37 ` [PATCH v2] " Marek Polacek 2023-07-25 15:15 ` Jason Merrill 2023-07-25 16:59 ` [PATCH v3] " Marek Polacek 2023-07-25 18:01 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).