Re: [PATCH] c++: wrong error with constexpr array and value-init [PR108158]

[Jason Merrill <jason@redhat.com>]

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