Re: [PATCH] c++: NON_DEPENDENT_EXPR is not potentially constant [PR104507]

[Patrick Palka <ppalka@redhat.com>]

On Wed, Feb 16, 2022 at 2:47 PM Patrick Palka <ppalka@redhat.com> wrote:
>
> On Tue, 15 Feb 2022, Jason Merrill wrote:
>
> > On 2/15/22 17:00, Patrick Palka wrote:
> > > On Tue, 15 Feb 2022, Jason Merrill wrote:
> > >
> > > > On 2/15/22 15:13, Patrick Palka wrote:
> > > > > On Tue, 15 Feb 2022, Patrick Palka wrote:
> > > > >
> > > > > > Here we're crashing from potential_constant_expression because it
> > > > > > tries
> > > > > > to perform trial evaluation of the first operand '(bool)__r' of the
> > > > > > conjunction (which is overall wrapped in a NON_DEPENDENT_EXPR), but
> > > > > > cxx_eval_constant_expression ICEs on unhandled trees (of which
> > > > > > CAST_EXPR
> > > > > > is one).
> > > > > >
> > > > > > Since cxx_eval_constant_expression always treats NON_DEPENDENT_EXPR
> > > > > > as non-constant, and since NON_DEPENDENT_EXPR is also opaque to
> > > > > > instantiate_non_dependent_expr, it seems futile to have p_c_e_1 ever
> > > > > > return true for NON_DEPENDENT_EXPR, so let's just instead return false
> > > > > > and avoid recursing.
> > > >
> > > > Well, in a template we use pce1 to decide whether to complain about
> > > > something
> > > > that needs to be constant but can't be.  We aren't trying to get a value
> > > > yet.
> > >
> > > Makes sense.. though for NON_DEPENDENT_EXPR in particular, ISTM this
> > > tree is always used in a context where a constant expression isn't
> > > required, e.g. in the build_x_* functions.
> >
> > Fair enough.  The patch is OK with a comment to that effect.
>
> Thanks, I committed the following as r12-7264:

Would it be OK to backport this now for 11.3, or shall we wait until
after 11.3 is released?  It's been two months, and the only reported
fallout from this patch was PR104620, where we began to fail to
diagnose an invalid consteval call within a template ahead of time
(and it turned out we previously were diagnosing it only by accident).
The patch also fixed PR103443 (non-dependent consteval call
incorrectly rejected).

>
> -- >8 --
>
> Subject: [PATCH] c++: treat NON_DEPENDENT_EXPR as not potentially constant
>  [PR104507]
>
> Here we're crashing from potential_constant_expression because it tries
> to perform trial evaluation of the first operand '(bool)__r' of the
> conjunction (which is overall wrapped in a NON_DEPENDENT_EXPR), but
> cxx_eval_constant_expression ICEs on unsupported trees (of which CAST_EXPR
> is one).  The sequence of events is:
>
>   1. build_non_dependent_expr for the array subscript yields
>      NON_DEPENDENT_EXPR<<<(bool)__r && __s>>> ? 1 : 2
>   2. cp_build_array_ref calls fold_non_dependent_expr on this subscript
>      (after this point, processing_template_decl is cleared)
>   3. during which, the COND_EXPR case of tsubst_copy_and_build calls
>      fold_non_dependent_expr on the first operand
>   4. during which, we crash from p_c_e_1 because it attempts trial
>      evaluation of the CAST_EXPR '(bool)__r'.
>
> Note that even if this crash didn't happen, fold_non_dependent_expr
> from cp_build_array_ref would still ultimately be one big no-op here
> since neither constexpr evaluation nor tsubst handle NON_DEPENDENT_EXPR.
>
> In light of this and of the observation that we should never see
> NON_DEPENDENT_EXPR in a context where a constant expression is needed
> (it's used primarily in the build_x_* family of functions), it seems
> futile for p_c_e_1 to ever return true for NON_DEPENDENT_EXPR.  And the
> otherwise inconsistent handling of NON_DEPENDENT_EXPR between p_c_e_1,
> cxx_evaluate_constexpr_expression and tsubst apparently leads to weird
> bugs such as this one.
>
>         PR c++/104507
>
> gcc/cp/ChangeLog:
>
>         * constexpr.cc (potential_constant_expression_1)
>         <case NON_DEPENDENT_EXPR>: Return false instead of recursing.
>         Assert tf_error isn't set.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.dg/template/non-dependent21.C: New test.
> ---
>  gcc/cp/constexpr.cc                             | 9 ++++++++-
>  gcc/testsuite/g++.dg/template/non-dependent21.C | 9 +++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/template/non-dependent21.C
>
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 7274c3b760e..4716694cb71 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -9065,6 +9065,14 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>      case BIND_EXPR:
>        return RECUR (BIND_EXPR_BODY (t), want_rval);
>
> +    case NON_DEPENDENT_EXPR:
> +      /* Treat NON_DEPENDENT_EXPR as non-constant: it's not handled by
> +        constexpr evaluation or tsubst, so fold_non_dependent_expr can't
> +        do anything useful with it.  And we shouldn't see it in a context
> +        where a constant expression is strictly required, hence the assert.  */
> +      gcc_checking_assert (!(flags & tf_error));
> +      return false;
> +
>      case CLEANUP_POINT_EXPR:
>      case MUST_NOT_THROW_EXPR:
>      case TRY_CATCH_EXPR:
> @@ -9072,7 +9080,6 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>      case EH_SPEC_BLOCK:
>      case EXPR_STMT:
>      case PAREN_EXPR:
> -    case NON_DEPENDENT_EXPR:
>        /* For convenience.  */
>      case LOOP_EXPR:
>      case EXIT_EXPR:
> diff --git a/gcc/testsuite/g++.dg/template/non-dependent21.C b/gcc/testsuite/g++.dg/template/non-dependent21.C
> new file mode 100644
> index 00000000000..89900837b8b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/non-dependent21.C
> @@ -0,0 +1,9 @@
> +// PR c++/104507
> +
> +extern const char *_k_errmsg[];
> +
> +template<class>
> +const char* DoFoo(int __r, int __s) {
> +  const char* n = _k_errmsg[(bool)__r && __s ? 1 : 2];
> +  return n;
> +}
> --
> 2.35.1.129.gb80121027d
>