* [PATCH] c++: wrong error with constexpr array and value-init [PR108158]
@ 2023-01-31 2:35 Marek Polacek
2023-02-02 22:29 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Marek Polacek @ 2023-01-31 2:35 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
In this test case, we find ourselves evaluating 't' which is
((const struct carray *) this)->data_[VIEW_CONVERT_EXPR<long int>(index)]
in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so
we replace it with 't':
new_ctx.object = t; // result_decl replaced
and then we go to cxx_eval_constant_expression to evaluate an
AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the
body of the constructor for seed_or_index):
((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0>
whereupon in cxx_eval_store_expression we go to the probe loop
where the 'this' is evaluated to
ze_set.tables_.first_table_.data_[0]
so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr
so we fail with a bogus error. ze_set is not there because it comes
from a different constexpr context (it's not in cv_cache either).
The problem started with r12-2304 where I added the new_ctx.object
replacement. That was to prevent a type mismatch: the type of 't'
and ctx.object were different.
It seems clear that we shouldn't have replaced ctx.object here.
The cxx_eval_array_reference I mentioned earlier is called from
cxx_eval_store_expression:
6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue,
6258 non_constant_p, overflow_p);
which already created a new context, whose .object we should be
using unless, for instance, INIT contained a.b and we're evaluating
the 'a' part, which I think was the case for r12-2304; in that case
ctx.object has to be something different.
A relatively safe fix should be to check the types before replacing
ctx.object, as in the below.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
PR c++/108158
gcc/cp/ChangeLog:
* constexpr.cc (cxx_eval_array_reference): Don't replace
new_ctx.object if its type is the same as elem_type.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1y/constexpr-108158.C: New test.
---
gcc/cp/constexpr.cc | 8 +++--
gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C | 32 +++++++++++++++++++
2 files changed, 38 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index be99bec17e7..00582cfffe2 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -4301,9 +4301,13 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
if (!SCALAR_TYPE_P (elem_type))
{
new_ctx = *ctx;
- if (ctx->object)
+ if (ctx->object
+ && !same_type_ignoring_top_level_qualifiers_p
+ (elem_type, TREE_TYPE (ctx->object)))
/* If there was no object, don't add one: it could confuse us
- into thinking we're modifying a const object. */
+ into thinking we're modifying a const object. Similarly, if
+ the types are the same, replacing .object could lead to a
+ failure to evaluate it (c++/108158). */
new_ctx.object = t;
new_ctx.ctor = build_constructor (elem_type, NULL);
ctx = &new_ctx;
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
new file mode 100644
index 00000000000..e5f5e9954e5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
@@ -0,0 +1,32 @@
+// PR c++/108158
+// { dg-do compile { target c++14 } }
+
+template <class T, int N> struct carray {
+ T data_[N]{};
+ constexpr T operator[](long index) const { return data_[index]; }
+};
+struct seed_or_index {
+private:
+ long value_ = 0;
+};
+template <int M> struct pmh_tables {
+ carray<seed_or_index, M> first_table_;
+ template <typename KeyType, typename HasherType>
+ constexpr void lookup(KeyType, HasherType) const {
+ first_table_[0];
+ }
+};
+template <int N> struct unordered_set {
+ int equal_;
+ carray<int, N> keys_;
+ pmh_tables<N> tables_;
+ constexpr unordered_set() : equal_{} {}
+ template <class KeyType, class Hasher>
+ constexpr auto lookup(KeyType key, Hasher hash) const {
+ tables_.lookup(key, hash);
+ return keys_;
+ }
+};
+constexpr unordered_set<3> ze_set;
+constexpr auto nocount = ze_set.lookup(4, int());
+constexpr auto nocount2 = unordered_set<3>{}.lookup(4, int());
base-commit: 897a0502056e6cc6613f26e0b22d1c1e06b1490f
--
2.39.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] c++: wrong error with constexpr array and value-init [PR108158]
2023-01-31 2:35 [PATCH] c++: wrong error with constexpr array and value-init [PR108158] Marek Polacek
@ 2023-02-02 22:29 ` Jason Merrill
2023-02-03 18:08 ` [PATCH v2] " Marek Polacek
0 siblings, 1 reply; 5+ messages in thread
From: Jason Merrill @ 2023-02-02 22:29 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
On 1/30/23 21:35, Marek Polacek wrote:
> In this test case, we find ourselves evaluating 't' which is
> ((const struct carray *) this)->data_[VIEW_CONVERT_EXPR<long int>(index)]
> in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so
> we replace it with 't':
>
> new_ctx.object = t; // result_decl replaced
>
> and then we go to cxx_eval_constant_expression to evaluate an
> AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the
> body of the constructor for seed_or_index):
>
> ((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0>
>
> whereupon in cxx_eval_store_expression we go to the probe loop
> where the 'this' is evaluated to
>
> ze_set.tables_.first_table_.data_[0]
>
> so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr
> so we fail with a bogus error. ze_set is not there because it comes
> from a different constexpr context (it's not in cv_cache either).
>
> The problem started with r12-2304 where I added the new_ctx.object
> replacement. That was to prevent a type mismatch: the type of 't'
> and ctx.object were different.
>
> It seems clear that we shouldn't have replaced ctx.object here.
> The cxx_eval_array_reference I mentioned earlier is called from
> cxx_eval_store_expression:
> 6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue,
> 6258 non_constant_p, overflow_p);
> which already created a new context, whose .object we should be
> using unless, for instance, INIT contained a.b and we're evaluating
> the 'a' part, which I think was the case for r12-2304; in that case
> ctx.object has to be something different.
>
> A relatively safe fix should be to check the types before replacing
> ctx.object, as in the below.
Agreed. I'm trying to understand when the replacement could ever make
sense, since 't' is not the target, it's the initializer. The
replacement comes from Patrick's fix for 98295, but that testcase no
longer hits that code (likely due to changes in empty class handling).
If you add a gcc_checking_assert (false) to the replacement, does
anything trip it?
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>
> PR c++/108158
>
> gcc/cp/ChangeLog:
>
> * constexpr.cc (cxx_eval_array_reference): Don't replace
> new_ctx.object if its type is the same as elem_type.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp1y/constexpr-108158.C: New test.
> ---
> gcc/cp/constexpr.cc | 8 +++--
> gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C | 32 +++++++++++++++++++
> 2 files changed, 38 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
>
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index be99bec17e7..00582cfffe2 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -4301,9 +4301,13 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
> if (!SCALAR_TYPE_P (elem_type))
> {
> new_ctx = *ctx;
> - if (ctx->object)
> + if (ctx->object
> + && !same_type_ignoring_top_level_qualifiers_p
> + (elem_type, TREE_TYPE (ctx->object)))
> /* If there was no object, don't add one: it could confuse us
> - into thinking we're modifying a const object. */
> + into thinking we're modifying a const object. Similarly, if
> + the types are the same, replacing .object could lead to a
> + failure to evaluate it (c++/108158). */
> new_ctx.object = t;
> new_ctx.ctor = build_constructor (elem_type, NULL);
> ctx = &new_ctx;
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
> new file mode 100644
> index 00000000000..e5f5e9954e5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
> @@ -0,0 +1,32 @@
> +// PR c++/108158
> +// { dg-do compile { target c++14 } }
> +
> +template <class T, int N> struct carray {
> + T data_[N]{};
> + constexpr T operator[](long index) const { return data_[index]; }
> +};
> +struct seed_or_index {
> +private:
> + long value_ = 0;
> +};
> +template <int M> struct pmh_tables {
> + carray<seed_or_index, M> first_table_;
> + template <typename KeyType, typename HasherType>
> + constexpr void lookup(KeyType, HasherType) const {
> + first_table_[0];
> + }
> +};
> +template <int N> struct unordered_set {
> + int equal_;
> + carray<int, N> keys_;
> + pmh_tables<N> tables_;
> + constexpr unordered_set() : equal_{} {}
> + template <class KeyType, class Hasher>
> + constexpr auto lookup(KeyType key, Hasher hash) const {
> + tables_.lookup(key, hash);
> + return keys_;
> + }
> +};
> +constexpr unordered_set<3> ze_set;
> +constexpr auto nocount = ze_set.lookup(4, int());
> +constexpr auto nocount2 = unordered_set<3>{}.lookup(4, int());
>
> base-commit: 897a0502056e6cc6613f26e0b22d1c1e06b1490f
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] c++: wrong error with constexpr array and value-init [PR108158]
2023-02-02 22:29 ` Jason Merrill
@ 2023-02-03 18:08 ` Marek Polacek
2023-02-03 18:53 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Marek Polacek @ 2023-02-03 18:08 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Thu, Feb 02, 2023 at 05:29:44PM -0500, Jason Merrill wrote:
> On 1/30/23 21:35, Marek Polacek wrote:
> > In this test case, we find ourselves evaluating 't' which is
> > ((const struct carray *) this)->data_[VIEW_CONVERT_EXPR<long int>(index)]
> > in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so
> > we replace it with 't':
> >
> > new_ctx.object = t; // result_decl replaced
> >
> > and then we go to cxx_eval_constant_expression to evaluate an
> > AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the
> > body of the constructor for seed_or_index):
> >
> > ((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0>
> >
> > whereupon in cxx_eval_store_expression we go to the probe loop
> > where the 'this' is evaluated to
> >
> > ze_set.tables_.first_table_.data_[0]
> >
> > so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr
> > so we fail with a bogus error. ze_set is not there because it comes
> > from a different constexpr context (it's not in cv_cache either).
> >
> > The problem started with r12-2304 where I added the new_ctx.object
> > replacement. That was to prevent a type mismatch: the type of 't'
> > and ctx.object were different.
> >
> > It seems clear that we shouldn't have replaced ctx.object here.
> > The cxx_eval_array_reference I mentioned earlier is called from
> > cxx_eval_store_expression:
> > 6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue,
> > 6258 non_constant_p, overflow_p);
> > which already created a new context, whose .object we should be
> > using unless, for instance, INIT contained a.b and we're evaluating
> > the 'a' part, which I think was the case for r12-2304; in that case
> > ctx.object has to be something different.
> >
> > A relatively safe fix should be to check the types before replacing
> > ctx.object, as in the below.
>
> Agreed. I'm trying to understand when the replacement could ever make
> sense, since 't' is not the target, it's the initializer. The replacement
> comes from Patrick's fix for 98295, but that testcase no longer hits that
> code (likely due to changes in empty class handling).
>
> If you add a gcc_checking_assert (false) to the replacement, does anything
> trip it?
It would trip in constexpr-101371.C, added in r12-2304. BUT, and I would
have sworn that it ICEd when I tried, it's not necessary anymore. So it
looks like we can simply remove the new_ctx.object line. At least for
trunk, maybe 12 too.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >8 --
In this test case, we find ourselves evaluating 't' which is
((const struct carray *) this)->data_[VIEW_CONVERT_EXPR<long int>(index)]
in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so
we replace it with 't':
new_ctx.object = t; // result_decl replaced
and then we go to cxx_eval_constant_expression to evaluate an
AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the
body of the constructor for seed_or_index):
((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0>
whereupon in cxx_eval_store_expression we go to the probe loop
where the 'this' is evaluated to
ze_set.tables_.first_table_.data_[0]
so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr
so we fail with a bogus error. ze_set is not there because it comes
from a different constexpr context (it's not in cv_cache either).
The problem started with r12-2304 where I added the new_ctx.object
replacement. That was to prevent a type mismatch: the type of 't'
and ctx.object were different.
It seems clear that we shouldn't have replaced ctx.object here.
The cxx_eval_array_reference I mentioned earlier is called from
cxx_eval_store_expression:
6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue,
6258 non_constant_p, overflow_p);
which already created a new context, whose .object we should be
using unless, for instance, INIT contained a.b and we're evaluating
the 'a' part, which I think was the case for r12-2304; in that case
ctx.object has to be something different.
It no longer seems necessary to replace new_ctx.object (likely due to
changes in empty class handling).
PR c++/108158
gcc/cp/ChangeLog:
* constexpr.cc (cxx_eval_array_reference): Don't replace
new_ctx.object.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1y/constexpr-108158.C: New test.
---
gcc/cp/constexpr.cc | 4 ---
gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C | 32 +++++++++++++++++++
2 files changed, 32 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 5b31f9c27d1..564766c8a00 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -4301,10 +4301,6 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
if (!SCALAR_TYPE_P (elem_type))
{
new_ctx = *ctx;
- if (ctx->object)
- /* If there was no object, don't add one: it could confuse us
- into thinking we're modifying a const object. */
- new_ctx.object = t;
new_ctx.ctor = build_constructor (elem_type, NULL);
ctx = &new_ctx;
}
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
new file mode 100644
index 00000000000..e5f5e9954e5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
@@ -0,0 +1,32 @@
+// PR c++/108158
+// { dg-do compile { target c++14 } }
+
+template <class T, int N> struct carray {
+ T data_[N]{};
+ constexpr T operator[](long index) const { return data_[index]; }
+};
+struct seed_or_index {
+private:
+ long value_ = 0;
+};
+template <int M> struct pmh_tables {
+ carray<seed_or_index, M> first_table_;
+ template <typename KeyType, typename HasherType>
+ constexpr void lookup(KeyType, HasherType) const {
+ first_table_[0];
+ }
+};
+template <int N> struct unordered_set {
+ int equal_;
+ carray<int, N> keys_;
+ pmh_tables<N> tables_;
+ constexpr unordered_set() : equal_{} {}
+ template <class KeyType, class Hasher>
+ constexpr auto lookup(KeyType key, Hasher hash) const {
+ tables_.lookup(key, hash);
+ return keys_;
+ }
+};
+constexpr unordered_set<3> ze_set;
+constexpr auto nocount = ze_set.lookup(4, int());
+constexpr auto nocount2 = unordered_set<3>{}.lookup(4, int());
base-commit: c9aef107ce697f58a34734d82f8d2514405c9be0
--
2.39.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] c++: wrong error with constexpr array and value-init [PR108158]
2023-02-03 18:08 ` [PATCH v2] " Marek Polacek
@ 2023-02-03 18:53 ` Jason Merrill
2023-02-03 18:56 ` Marek Polacek
0 siblings, 1 reply; 5+ messages in thread
From: Jason Merrill @ 2023-02-03 18:53 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On 2/3/23 13:08, Marek Polacek wrote:
> On Thu, Feb 02, 2023 at 05:29:44PM -0500, Jason Merrill wrote:
>> On 1/30/23 21:35, Marek Polacek wrote:
>>> In this test case, we find ourselves evaluating 't' which is
>>> ((const struct carray *) this)->data_[VIEW_CONVERT_EXPR<long int>(index)]
>>> in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so
>>> we replace it with 't':
>>>
>>> new_ctx.object = t; // result_decl replaced
>>>
>>> and then we go to cxx_eval_constant_expression to evaluate an
>>> AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the
>>> body of the constructor for seed_or_index):
>>>
>>> ((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0>
>>>
>>> whereupon in cxx_eval_store_expression we go to the probe loop
>>> where the 'this' is evaluated to
>>>
>>> ze_set.tables_.first_table_.data_[0]
>>>
>>> so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr
>>> so we fail with a bogus error. ze_set is not there because it comes
>>> from a different constexpr context (it's not in cv_cache either).
>>>
>>> The problem started with r12-2304 where I added the new_ctx.object
>>> replacement. That was to prevent a type mismatch: the type of 't'
>>> and ctx.object were different.
>>>
>>> It seems clear that we shouldn't have replaced ctx.object here.
>>> The cxx_eval_array_reference I mentioned earlier is called from
>>> cxx_eval_store_expression:
>>> 6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue,
>>> 6258 non_constant_p, overflow_p);
>>> which already created a new context, whose .object we should be
>>> using unless, for instance, INIT contained a.b and we're evaluating
>>> the 'a' part, which I think was the case for r12-2304; in that case
>>> ctx.object has to be something different.
>>>
>>> A relatively safe fix should be to check the types before replacing
>>> ctx.object, as in the below.
>>
>> Agreed. I'm trying to understand when the replacement could ever make
>> sense, since 't' is not the target, it's the initializer. The replacement
>> comes from Patrick's fix for 98295, but that testcase no longer hits that
>> code (likely due to changes in empty class handling).
>>
>> If you add a gcc_checking_assert (false) to the replacement, does anything
>> trip it?
>
> It would trip in constexpr-101371.C, added in r12-2304. BUT, and I would
> have sworn that it ICEd when I tried, it's not necessary anymore. So it
> looks like we can simply remove the new_ctx.object line. At least for
> trunk, maybe 12 too.
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
OK, thanks. Let's go with your original patch for 11/12.
> -- >8 --
> In this test case, we find ourselves evaluating 't' which is
> ((const struct carray *) this)->data_[VIEW_CONVERT_EXPR<long int>(index)]
> in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so
> we replace it with 't':
>
> new_ctx.object = t; // result_decl replaced
>
> and then we go to cxx_eval_constant_expression to evaluate an
> AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the
> body of the constructor for seed_or_index):
>
> ((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0>
>
> whereupon in cxx_eval_store_expression we go to the probe loop
> where the 'this' is evaluated to
>
> ze_set.tables_.first_table_.data_[0]
>
> so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr
> so we fail with a bogus error. ze_set is not there because it comes
> from a different constexpr context (it's not in cv_cache either).
>
> The problem started with r12-2304 where I added the new_ctx.object
> replacement. That was to prevent a type mismatch: the type of 't'
> and ctx.object were different.
>
> It seems clear that we shouldn't have replaced ctx.object here.
> The cxx_eval_array_reference I mentioned earlier is called from
> cxx_eval_store_expression:
> 6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue,
> 6258 non_constant_p, overflow_p);
> which already created a new context, whose .object we should be
> using unless, for instance, INIT contained a.b and we're evaluating
> the 'a' part, which I think was the case for r12-2304; in that case
> ctx.object has to be something different.
>
> It no longer seems necessary to replace new_ctx.object (likely due to
> changes in empty class handling).
>
> PR c++/108158
>
> gcc/cp/ChangeLog:
>
> * constexpr.cc (cxx_eval_array_reference): Don't replace
> new_ctx.object.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp1y/constexpr-108158.C: New test.
> ---
> gcc/cp/constexpr.cc | 4 ---
> gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C | 32 +++++++++++++++++++
> 2 files changed, 32 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
>
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 5b31f9c27d1..564766c8a00 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -4301,10 +4301,6 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
> if (!SCALAR_TYPE_P (elem_type))
> {
> new_ctx = *ctx;
> - if (ctx->object)
> - /* If there was no object, don't add one: it could confuse us
> - into thinking we're modifying a const object. */
> - new_ctx.object = t;
> new_ctx.ctor = build_constructor (elem_type, NULL);
> ctx = &new_ctx;
> }
> diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
> new file mode 100644
> index 00000000000..e5f5e9954e5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-108158.C
> @@ -0,0 +1,32 @@
> +// PR c++/108158
> +// { dg-do compile { target c++14 } }
> +
> +template <class T, int N> struct carray {
> + T data_[N]{};
> + constexpr T operator[](long index) const { return data_[index]; }
> +};
> +struct seed_or_index {
> +private:
> + long value_ = 0;
> +};
> +template <int M> struct pmh_tables {
> + carray<seed_or_index, M> first_table_;
> + template <typename KeyType, typename HasherType>
> + constexpr void lookup(KeyType, HasherType) const {
> + first_table_[0];
> + }
> +};
> +template <int N> struct unordered_set {
> + int equal_;
> + carray<int, N> keys_;
> + pmh_tables<N> tables_;
> + constexpr unordered_set() : equal_{} {}
> + template <class KeyType, class Hasher>
> + constexpr auto lookup(KeyType key, Hasher hash) const {
> + tables_.lookup(key, hash);
> + return keys_;
> + }
> +};
> +constexpr unordered_set<3> ze_set;
> +constexpr auto nocount = ze_set.lookup(4, int());
> +constexpr auto nocount2 = unordered_set<3>{}.lookup(4, int());
>
> base-commit: c9aef107ce697f58a34734d82f8d2514405c9be0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] c++: wrong error with constexpr array and value-init [PR108158]
2023-02-03 18:53 ` Jason Merrill
@ 2023-02-03 18:56 ` Marek Polacek
0 siblings, 0 replies; 5+ messages in thread
From: Marek Polacek @ 2023-02-03 18:56 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Fri, Feb 03, 2023 at 01:53:48PM -0500, Jason Merrill wrote:
> On 2/3/23 13:08, Marek Polacek wrote:
> > On Thu, Feb 02, 2023 at 05:29:44PM -0500, Jason Merrill wrote:
> > > On 1/30/23 21:35, Marek Polacek wrote:
> > > > In this test case, we find ourselves evaluating 't' which is
> > > > ((const struct carray *) this)->data_[VIEW_CONVERT_EXPR<long int>(index)]
> > > > in cxx_eval_array_reference. ctx->object is non-null, a RESULT_DECL, so
> > > > we replace it with 't':
> > > >
> > > > new_ctx.object = t; // result_decl replaced
> > > >
> > > > and then we go to cxx_eval_constant_expression to evaluate an
> > > > AGGR_INIT_EXPR, where we end up evaluating an INIT_EXPR (which is in the
> > > > body of the constructor for seed_or_index):
> > > >
> > > > ((struct seed_or_index *) this)->value_ = NON_LVALUE_EXPR <0>
> > > >
> > > > whereupon in cxx_eval_store_expression we go to the probe loop
> > > > where the 'this' is evaluated to
> > > >
> > > > ze_set.tables_.first_table_.data_[0]
> > > >
> > > > so the 'object' is ze_set, but that isn't in ctx->global->get_value_ptr
> > > > so we fail with a bogus error. ze_set is not there because it comes
> > > > from a different constexpr context (it's not in cv_cache either).
> > > >
> > > > The problem started with r12-2304 where I added the new_ctx.object
> > > > replacement. That was to prevent a type mismatch: the type of 't'
> > > > and ctx.object were different.
> > > >
> > > > It seems clear that we shouldn't have replaced ctx.object here.
> > > > The cxx_eval_array_reference I mentioned earlier is called from
> > > > cxx_eval_store_expression:
> > > > 6257 init = cxx_eval_constant_expression (&new_ctx, init, vc_prvalue,
> > > > 6258 non_constant_p, overflow_p);
> > > > which already created a new context, whose .object we should be
> > > > using unless, for instance, INIT contained a.b and we're evaluating
> > > > the 'a' part, which I think was the case for r12-2304; in that case
> > > > ctx.object has to be something different.
> > > >
> > > > A relatively safe fix should be to check the types before replacing
> > > > ctx.object, as in the below.
> > >
> > > Agreed. I'm trying to understand when the replacement could ever make
> > > sense, since 't' is not the target, it's the initializer. The replacement
> > > comes from Patrick's fix for 98295, but that testcase no longer hits that
> > > code (likely due to changes in empty class handling).
> > >
> > > If you add a gcc_checking_assert (false) to the replacement, does anything
> > > trip it?
> >
> > It would trip in constexpr-101371.C, added in r12-2304. BUT, and I would
> > have sworn that it ICEd when I tried, it's not necessary anymore. So it
> > looks like we can simply remove the new_ctx.object line. At least for
> > trunk, maybe 12 too.
> >
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>
> OK, thanks. Let's go with your original patch for 11/12.
Will do, thanks. I think I'll wait for a few days before backporting.
Marek
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-02-03 18:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 2:35 [PATCH] c++: wrong error with constexpr array and value-init [PR108158] Marek Polacek
2023-02-02 22:29 ` Jason Merrill
2023-02-03 18:08 ` [PATCH v2] " Marek Polacek
2023-02-03 18:53 ` Jason Merrill
2023-02-03 18:56 ` Marek Polacek
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).