* [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).