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

  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


base-commit: 897a0502056e6cc6613f26e0b22d1c1e06b1490f
  

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