| Message ID | 20220215175430.632649-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: NON_DEPENDENT_EXPR is not potentially constant [PR104507] | expand |
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. > > Alternatively p_c_e_1 could continue to recurse into NON_DEPENDENT_EXPR, > but with trial evaluation disabled by setting processing_template_decl, > but as mentioned it seems pointless for p_c_e_1 to ever return true for > NON_DEPENDENT_EXPR. > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk and perhaps 10/11? > > PR c++/104507 > > gcc/cp/ChangeLog: > > * constexpr.cc (potential_constant_expression_1) > <case NON_DEPENDENT_EXPR>: Return false rather than recursing. > > gcc/testsuite/ChangeLog: > > * g++.dg/template/non-dependent21.C: New test. > --- > gcc/cp/constexpr.cc | 4 +++- > gcc/testsuite/g++.dg/template/non-dependent21.C | 9 +++++++++ > 2 files changed, 12 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..c0523551f7b 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -9065,6 +9065,9 @@ 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: > + return false; Since we're not issuing a diagnostic in this case, I suppose we should also assert that tf_error isn't set. Bootstrapped and regtested on x86_64-pc-linux-gnu. -- >8 -- 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 | 5 ++++- gcc/testsuite/g++.dg/template/non-dependent21.C | 9 +++++++++ 2 files changed, 13 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..b363ef08411 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -9065,6 +9065,10 @@ 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: + gcc_checking_assert (!(flags & tf_error)); + return false; + case CLEANUP_POINT_EXPR: case MUST_NOT_THROW_EXPR: case TRY_CATCH_EXPR: @@ -9072,7 +9076,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; +}
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. Actually, why are we seeing a NON_DEPENDENT_EXPR here? Did it leak into the AST somehow? They should all be temporary within build_x_whatever functions. >> Alternatively p_c_e_1 could continue to recurse into NON_DEPENDENT_EXPR, >> but with trial evaluation disabled by setting processing_template_decl, >> but as mentioned it seems pointless for p_c_e_1 to ever return true for >> NON_DEPENDENT_EXPR. >... > Since we're not issuing a diagnostic in this case, I suppose we should > also assert that tf_error isn't set. Bootstrapped and regtested on > x86_64-pc-linux-gnu. > > -- >8 -- > > 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 | 5 ++++- > gcc/testsuite/g++.dg/template/non-dependent21.C | 9 +++++++++ > 2 files changed, 13 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..b363ef08411 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -9065,6 +9065,10 @@ 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: > + gcc_checking_assert (!(flags & tf_error)); > + return false; > + > case CLEANUP_POINT_EXPR: > case MUST_NOT_THROW_EXPR: > case TRY_CATCH_EXPR: > @@ -9072,7 +9076,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; > +}
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. And if something is required to be a constant expression and we're inside a template, then it seems at that point we're dealing with the final templated form of that thing (which doesn't contain NON_DEPENDENT_EXPR), which I suppose explains why the patch can get away with asserting !(flags & tf_error) inside the NON_DEPENDENT_EXPR case of p_c_e_1. So I guess I don't fully understand the purpose of NON_DEPENDENT_EXPR or how it should interact with fold_non_dependent_expr and constant evaluation... > > Actually, why are we seeing a NON_DEPENDENT_EXPR here? Did it leak into the > AST somehow? They should all be temporary within build_x_whatever functions. Hmm yes, kind of. The unusual thing about this testcase is that build_non_dependent_expr (called from grok_array_decl) for the COND_EXPR (bool)__r && __s ? 1 : 2 wraps only the condition operand, yielding NON_DEPENDENT_EXPR<<<(bool)__r && __s>>> ? 1 : 2 // #1 rather than wrapping the whole thing i.e. NON_DEPENDENT_EXPR<<<(bool)__r && __s ? 1 : 2>>> // #2 cp_build_array_ref then tries to speculatively fold the non-dependent #1 as a whole, during which the COND_EXPR case of tsubst_copy_and_build tries to speculative fold #1's condition NON_DEPENDENT_EXPR<<<(bool)__r && __s>>> on its own, which ends in a crash from p_c_e_1 because at this point processing_template_decl is cleared so p_c_e_1 attempts trial evaluation of the CAST_EXPR (bool)__r. If instead build_non_dependent_expr yielded #2, speculative folding of #2 as a whole just yield #2 since tsubst doesn't look through NON_DEPENDENT_EXPR. > > > > Alternatively p_c_e_1 could continue to recurse into NON_DEPENDENT_EXPR, > > > but with trial evaluation disabled by setting processing_template_decl, > > > but as mentioned it seems pointless for p_c_e_1 to ever return true for > > > NON_DEPENDENT_EXPR. > > ... Since we're not issuing a diagnostic in this case, I suppose we should > > also assert that tf_error isn't set. Bootstrapped and regtested on > > x86_64-pc-linux-gnu. > > > > -- >8 -- > > > > 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 | 5 ++++- > > gcc/testsuite/g++.dg/template/non-dependent21.C | 9 +++++++++ > > 2 files changed, 13 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..b363ef08411 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -9065,6 +9065,10 @@ 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: > > + gcc_checking_assert (!(flags & tf_error)); > > + return false; > > + > > case CLEANUP_POINT_EXPR: > > case MUST_NOT_THROW_EXPR: > > case TRY_CATCH_EXPR: > > @@ -9072,7 +9076,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; > > +} > >
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. > And if something is required to be a constant expression and we're > inside a template, then it seems at that point we're dealing with the > final templated form of that thing (which doesn't contain > NON_DEPENDENT_EXPR), which I suppose explains why the patch can get away > with asserting !(flags & tf_error) inside the NON_DEPENDENT_EXPR case of > p_c_e_1. > > So I guess I don't fully understand the purpose of NON_DEPENDENT_EXPR > or how it should interact with fold_non_dependent_expr and constant > evaluation... > >> >> Actually, why are we seeing a NON_DEPENDENT_EXPR here? Did it leak into the >> AST somehow? They should all be temporary within build_x_whatever functions. > > Hmm yes, kind of. The unusual thing about this testcase is that > build_non_dependent_expr (called from grok_array_decl) for the > COND_EXPR > > (bool)__r && __s ? 1 : 2 > > wraps only the condition operand, yielding > > NON_DEPENDENT_EXPR<<<(bool)__r && __s>>> ? 1 : 2 // #1 > > rather than wrapping the whole thing i.e. > > NON_DEPENDENT_EXPR<<<(bool)__r && __s ? 1 : 2>>> // #2 > > cp_build_array_ref then tries to speculatively fold the non-dependent #1 > as a whole, during which the COND_EXPR case of tsubst_copy_and_build > tries to speculative fold #1's condition NON_DEPENDENT_EXPR<<<(bool)__r && __s>>> > on its own, which ends in a crash from p_c_e_1 because at this point > processing_template_decl is cleared so p_c_e_1 attempts trial evaluation > of the CAST_EXPR (bool)__r. > > If instead build_non_dependent_expr yielded #2, speculative folding of > #2 as a whole just yield #2 since tsubst doesn't look through > NON_DEPENDENT_EXPR. > >>>> Alternatively p_c_e_1 could continue to recurse into NON_DEPENDENT_EXPR, >>>> but with trial evaluation disabled by setting processing_template_decl, >>>> but as mentioned it seems pointless for p_c_e_1 to ever return true for >>>> NON_DEPENDENT_EXPR. >>> ... Since we're not issuing a diagnostic in this case, I suppose we should >>> also assert that tf_error isn't set. Bootstrapped and regtested on >>> x86_64-pc-linux-gnu. >>> >>> -- >8 -- >>> >>> 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 | 5 ++++- >>> gcc/testsuite/g++.dg/template/non-dependent21.C | 9 +++++++++ >>> 2 files changed, 13 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..b363ef08411 100644 >>> --- a/gcc/cp/constexpr.cc >>> +++ b/gcc/cp/constexpr.cc >>> @@ -9065,6 +9065,10 @@ 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: >>> + gcc_checking_assert (!(flags & tf_error)); >>> + return false; >>> + >>> case CLEANUP_POINT_EXPR: >>> case MUST_NOT_THROW_EXPR: >>> case TRY_CATCH_EXPR: >>> @@ -9072,7 +9076,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; >>> +} >> >> >
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: -- >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; +}
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 >
On 4/12/22 15:48, Patrick Palka wrote: > 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). OK. >> >> -- >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 >> >
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc index 7274c3b760e..c0523551f7b 100644 --- a/gcc/cp/constexpr.cc +++ b/gcc/cp/constexpr.cc @@ -9065,6 +9065,9 @@ 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: + return false; + case CLEANUP_POINT_EXPR: case MUST_NOT_THROW_EXPR: case TRY_CATCH_EXPR: @@ -9072,7 +9075,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; +}