| Message ID | 20240311165307.3930810-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: recalculating local specs via build_extra_args [PR114303] | expand |
On 3/11/24 12:53, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > OK for trunk and release branches? > > -- >8 -- > > r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts > first so that we prefer processing a local specialization in an evaluated > context even if its first use is in an unevaluated context. But this > means we need to avoid walking a tree that already has extra args/specs > saved because the list of saved specs appears to be an evaluated > context. It seems then that we should be calculating the saved specs > from scratch each time, rather than potentially walking the saved specs > list from an earlier partial instantiation when calling build_extra_args > a second time around. Makes sense, but I wonder if we want to approach that by avoiding walking into *_EXTRA_ARGS in extract_locals_r? Or do we still want to walk into any nested extra args? And if so, will we run into this same problem then? > PR c++/114303 > > gcc/cp/ChangeLog: > > * constraint.cc (tsubst_requires_expr): Clear > REQUIRES_EXPR_EXTRA_ARGS before calling build_extra_args. > * pt.cc (tsubst_stmt) <case IF_STMT>: Call build_extra_args > on the new IF_STMT instead of t which might already have > IF_STMT_EXTRA_ARGS. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1z/constexpr-if-lambda6.C: New test. > --- > gcc/cp/constraint.cc | 1 + > gcc/cp/pt.cc | 2 +- > .../g++.dg/cpp1z/constexpr-if-lambda6.C | 16 ++++++++++++++++ > 3 files changed, 18 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc > index 49de3211d4c..8a3b5d80ba7 100644 > --- a/gcc/cp/constraint.cc > +++ b/gcc/cp/constraint.cc > @@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info info) > matching or dguide constraint rewriting), in which case we need > to partially substitute. */ > t = copy_node (t); > + REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE; > REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, info.complain); > return t; > } > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index 8cf0d5b7a8d..37f2392d035 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -18718,7 +18718,7 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl) > IF_COND (stmt) = IF_COND (t); > THEN_CLAUSE (stmt) = THEN_CLAUSE (t); > ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); > - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); > + IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain); > add_stmt (stmt); > break; > } > diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > new file mode 100644 > index 00000000000..038c2a41210 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > @@ -0,0 +1,16 @@ > +// PR c++/114303 > +// { dg-do compile { target c++17 } } > + > +struct A { static constexpr bool value = true; }; > + > +int main() { > + [](auto x1) { > + return [&](auto) { > + return [&](auto x3) { > + if constexpr (decltype(x3)::value) { > + static_assert(decltype(x1)::value); > + } > + }(A{}); > + }(0); > + }(A{}); > +}
On Tue, 12 Mar 2024, Jason Merrill wrote: > On 3/11/24 12:53, Patrick Palka wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > > OK for trunk and release branches? > > > > -- >8 -- > > > > r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts > > first so that we prefer processing a local specialization in an evaluated > > context even if its first use is in an unevaluated context. But this > > means we need to avoid walking a tree that already has extra args/specs > > saved because the list of saved specs appears to be an evaluated > > context. It seems then that we should be calculating the saved specs > > from scratch each time, rather than potentially walking the saved specs > > list from an earlier partial instantiation when calling build_extra_args > > a second time around. > > Makes sense, but I wonder if we want to approach that by avoiding walking into > *_EXTRA_ARGS in extract_locals_r? Or do we still want to walk into any nested > extra args? And if so, will we run into this same problem then? I'm not sure totally but I'd expect a nested extra-args tree to always have empty *_EXTRA_ARGS since the outer extra-args tree should intercept any substitution before the inner extra-args tree can see it? > > > PR c++/114303 > > > > gcc/cp/ChangeLog: > > > > * constraint.cc (tsubst_requires_expr): Clear > > REQUIRES_EXPR_EXTRA_ARGS before calling build_extra_args. > > * pt.cc (tsubst_stmt) <case IF_STMT>: Call build_extra_args > > on the new IF_STMT instead of t which might already have > > IF_STMT_EXTRA_ARGS. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp1z/constexpr-if-lambda6.C: New test. > > --- > > gcc/cp/constraint.cc | 1 + > > gcc/cp/pt.cc | 2 +- > > .../g++.dg/cpp1z/constexpr-if-lambda6.C | 16 ++++++++++++++++ > > 3 files changed, 18 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc > > index 49de3211d4c..8a3b5d80ba7 100644 > > --- a/gcc/cp/constraint.cc > > +++ b/gcc/cp/constraint.cc > > @@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info > > info) > > matching or dguide constraint rewriting), in which case we need > > to partially substitute. */ > > t = copy_node (t); > > + REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE; > > REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, > > info.complain); > > return t; > > } > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > > index 8cf0d5b7a8d..37f2392d035 100644 > > --- a/gcc/cp/pt.cc > > +++ b/gcc/cp/pt.cc > > @@ -18718,7 +18718,7 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t > > complain, tree in_decl) > > IF_COND (stmt) = IF_COND (t); > > THEN_CLAUSE (stmt) = THEN_CLAUSE (t); > > ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); > > - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); > > + IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain); > > add_stmt (stmt); > > break; > > } > > diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > new file mode 100644 > > index 00000000000..038c2a41210 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > @@ -0,0 +1,16 @@ > > +// PR c++/114303 > > +// { dg-do compile { target c++17 } } > > + > > +struct A { static constexpr bool value = true; }; > > + > > +int main() { > > + [](auto x1) { > > + return [&](auto) { > > + return [&](auto x3) { > > + if constexpr (decltype(x3)::value) { > > + static_assert(decltype(x1)::value); > > + } > > + }(A{}); > > + }(0); > > + }(A{}); > > +} > >
On Tue, 12 Mar 2024, Patrick Palka wrote: > On Tue, 12 Mar 2024, Jason Merrill wrote: > > > On 3/11/24 12:53, Patrick Palka wrote: > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > > > OK for trunk and release branches? > > > > > > -- >8 -- > > > > > > r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts > > > first so that we prefer processing a local specialization in an evaluated > > > context even if its first use is in an unevaluated context. But this > > > means we need to avoid walking a tree that already has extra args/specs > > > saved because the list of saved specs appears to be an evaluated > > > context. It seems then that we should be calculating the saved specs > > > from scratch each time, rather than potentially walking the saved specs > > > list from an earlier partial instantiation when calling build_extra_args > > > a second time around. > > > > Makes sense, but I wonder if we want to approach that by avoiding walking into > > *_EXTRA_ARGS in extract_locals_r? Or do we still want to walk into any nested > > extra args? And if so, will we run into this same problem then? > > I'm not sure totally but I'd expect a nested extra-args tree to always > have empty *_EXTRA_ARGS since the outer extra-args tree should intercept > any substitution before the inner extra-args tree can see it? ... and so in extract_locals_r I think we can assume *_EXTRA_ARGS is empty, and not have to explicitly avoid walking it. > > > > > > PR c++/114303 > > > > > > gcc/cp/ChangeLog: > > > > > > * constraint.cc (tsubst_requires_expr): Clear > > > REQUIRES_EXPR_EXTRA_ARGS before calling build_extra_args. > > > * pt.cc (tsubst_stmt) <case IF_STMT>: Call build_extra_args > > > on the new IF_STMT instead of t which might already have > > > IF_STMT_EXTRA_ARGS. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * g++.dg/cpp1z/constexpr-if-lambda6.C: New test. > > > --- > > > gcc/cp/constraint.cc | 1 + > > > gcc/cp/pt.cc | 2 +- > > > .../g++.dg/cpp1z/constexpr-if-lambda6.C | 16 ++++++++++++++++ > > > 3 files changed, 18 insertions(+), 1 deletion(-) > > > create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > > > > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc > > > index 49de3211d4c..8a3b5d80ba7 100644 > > > --- a/gcc/cp/constraint.cc > > > +++ b/gcc/cp/constraint.cc > > > @@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info > > > info) > > > matching or dguide constraint rewriting), in which case we need > > > to partially substitute. */ > > > t = copy_node (t); > > > + REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE; > > > REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, > > > info.complain); > > > return t; > > > } > > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > > > index 8cf0d5b7a8d..37f2392d035 100644 > > > --- a/gcc/cp/pt.cc > > > +++ b/gcc/cp/pt.cc > > > @@ -18718,7 +18718,7 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t > > > complain, tree in_decl) > > > IF_COND (stmt) = IF_COND (t); > > > THEN_CLAUSE (stmt) = THEN_CLAUSE (t); > > > ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); > > > - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); > > > + IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain); > > > add_stmt (stmt); > > > break; > > > } > > > diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > > b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > > new file mode 100644 > > > index 00000000000..038c2a41210 > > > --- /dev/null > > > +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > > @@ -0,0 +1,16 @@ > > > +// PR c++/114303 > > > +// { dg-do compile { target c++17 } } > > > + > > > +struct A { static constexpr bool value = true; }; > > > + > > > +int main() { > > > + [](auto x1) { > > > + return [&](auto) { > > > + return [&](auto x3) { > > > + if constexpr (decltype(x3)::value) { > > > + static_assert(decltype(x1)::value); > > > + } > > > + }(A{}); > > > + }(0); > > > + }(A{}); > > > +} > > > > >
On Mon, 11 Mar 2024, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > OK for trunk and release branches? Ping. > > -- >8 -- > > r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts > first so that we prefer processing a local specialization in an evaluated > context even if its first use is in an unevaluated context. But this > means we need to avoid walking a tree that already has extra args/specs > saved because the list of saved specs appears to be an evaluated > context. It seems then that we should be calculating the saved specs > from scratch each time, rather than potentially walking the saved specs > list from an earlier partial instantiation when calling build_extra_args > a second time around. > > PR c++/114303 > > gcc/cp/ChangeLog: > > * constraint.cc (tsubst_requires_expr): Clear > REQUIRES_EXPR_EXTRA_ARGS before calling build_extra_args. > * pt.cc (tsubst_stmt) <case IF_STMT>: Call build_extra_args > on the new IF_STMT instead of t which might already have > IF_STMT_EXTRA_ARGS. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1z/constexpr-if-lambda6.C: New test. > --- > gcc/cp/constraint.cc | 1 + > gcc/cp/pt.cc | 2 +- > .../g++.dg/cpp1z/constexpr-if-lambda6.C | 16 ++++++++++++++++ > 3 files changed, 18 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc > index 49de3211d4c..8a3b5d80ba7 100644 > --- a/gcc/cp/constraint.cc > +++ b/gcc/cp/constraint.cc > @@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info info) > matching or dguide constraint rewriting), in which case we need > to partially substitute. */ > t = copy_node (t); > + REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE; > REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, info.complain); > return t; > } > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index 8cf0d5b7a8d..37f2392d035 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -18718,7 +18718,7 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl) > IF_COND (stmt) = IF_COND (t); > THEN_CLAUSE (stmt) = THEN_CLAUSE (t); > ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); > - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); > + IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain); > add_stmt (stmt); > break; > } > diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > new file mode 100644 > index 00000000000..038c2a41210 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > @@ -0,0 +1,16 @@ > +// PR c++/114303 > +// { dg-do compile { target c++17 } } > + > +struct A { static constexpr bool value = true; }; > + > +int main() { > + [](auto x1) { > + return [&](auto) { > + return [&](auto x3) { > + if constexpr (decltype(x3)::value) { > + static_assert(decltype(x1)::value); > + } > + }(A{}); > + }(0); > + }(A{}); > +} > -- > 2.44.0.165.ge09f1254c5 > >
On Tue, 26 Mar 2024, Patrick Palka wrote: > On Mon, 11 Mar 2024, Patrick Palka wrote: > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look > > OK for trunk and release branches? > > Ping. Ping. > > > > > -- >8 -- > > > > r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts > > first so that we prefer processing a local specialization in an evaluated > > context even if its first use is in an unevaluated context. But this > > means we need to avoid walking a tree that already has extra args/specs > > saved because the list of saved specs appears to be an evaluated > > context. It seems then that we should be calculating the saved specs > > from scratch each time, rather than potentially walking the saved specs > > list from an earlier partial instantiation when calling build_extra_args > > a second time around. > > > > PR c++/114303 > > > > gcc/cp/ChangeLog: > > > > * constraint.cc (tsubst_requires_expr): Clear > > REQUIRES_EXPR_EXTRA_ARGS before calling build_extra_args. > > * pt.cc (tsubst_stmt) <case IF_STMT>: Call build_extra_args > > on the new IF_STMT instead of t which might already have > > IF_STMT_EXTRA_ARGS. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp1z/constexpr-if-lambda6.C: New test. > > --- > > gcc/cp/constraint.cc | 1 + > > gcc/cp/pt.cc | 2 +- > > .../g++.dg/cpp1z/constexpr-if-lambda6.C | 16 ++++++++++++++++ > > 3 files changed, 18 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > > > diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc > > index 49de3211d4c..8a3b5d80ba7 100644 > > --- a/gcc/cp/constraint.cc > > +++ b/gcc/cp/constraint.cc > > @@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info info) > > matching or dguide constraint rewriting), in which case we need > > to partially substitute. */ > > t = copy_node (t); > > + REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE; > > REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, info.complain); > > return t; > > } > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > > index 8cf0d5b7a8d..37f2392d035 100644 > > --- a/gcc/cp/pt.cc > > +++ b/gcc/cp/pt.cc > > @@ -18718,7 +18718,7 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl) > > IF_COND (stmt) = IF_COND (t); > > THEN_CLAUSE (stmt) = THEN_CLAUSE (t); > > ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); > > - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); > > + IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain); > > add_stmt (stmt); > > break; > > } > > diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > new file mode 100644 > > index 00000000000..038c2a41210 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C > > @@ -0,0 +1,16 @@ > > +// PR c++/114303 > > +// { dg-do compile { target c++17 } } > > + > > +struct A { static constexpr bool value = true; }; > > + > > +int main() { > > + [](auto x1) { > > + return [&](auto) { > > + return [&](auto x3) { > > + if constexpr (decltype(x3)::value) { > > + static_assert(decltype(x1)::value); > > + } > > + }(A{}); > > + }(0); > > + }(A{}); > > +} > > -- > > 2.44.0.165.ge09f1254c5 > > > > >
On 3/12/24 10:51, Patrick Palka wrote: > On Tue, 12 Mar 2024, Patrick Palka wrote: >> On Tue, 12 Mar 2024, Jason Merrill wrote: >>> On 3/11/24 12:53, Patrick Palka wrote: >>>> >>>> r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts >>>> first so that we prefer processing a local specialization in an evaluated >>>> context even if its first use is in an unevaluated context. But this >>>> means we need to avoid walking a tree that already has extra args/specs >>>> saved because the list of saved specs appears to be an evaluated >>>> context. It seems then that we should be calculating the saved specs >>>> from scratch each time, rather than potentially walking the saved specs >>>> list from an earlier partial instantiation when calling build_extra_args >>>> a second time around. >>> >>> Makes sense, but I wonder if we want to approach that by avoiding walking into >>> *_EXTRA_ARGS in extract_locals_r? Or do we still want to walk into any nested >>> extra args? And if so, will we run into this same problem then? >> >> I'm not sure totally but I'd expect a nested extra-args tree to always >> have empty *_EXTRA_ARGS since the outer extra-args tree should intercept >> any substitution before the inner extra-args tree can see it? > > ... and so in extract_locals_r I think we can assume *_EXTRA_ARGS is > empty, and not have to explicitly avoid walking it. It seems more robust to me to handle _EXTRA_ARGS appropriately in build_extra_args rather than expect callers to know that they shouldn't pass in a tree with _EXTRA_ARGS set. At least check and abort in that case? Jason
On Wed, 10 Apr 2024, Jason Merrill wrote: > On 3/12/24 10:51, Patrick Palka wrote: > > On Tue, 12 Mar 2024, Patrick Palka wrote: > > > On Tue, 12 Mar 2024, Jason Merrill wrote: > > > > On 3/11/24 12:53, Patrick Palka wrote: > > > > > > > > > > r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts > > > > > first so that we prefer processing a local specialization in an > > > > > evaluated > > > > > context even if its first use is in an unevaluated context. But this > > > > > means we need to avoid walking a tree that already has extra > > > > > args/specs > > > > > saved because the list of saved specs appears to be an evaluated > > > > > context. It seems then that we should be calculating the saved specs > > > > > from scratch each time, rather than potentially walking the saved > > > > > specs > > > > > list from an earlier partial instantiation when calling > > > > > build_extra_args > > > > > a second time around. > > > > > > > > Makes sense, but I wonder if we want to approach that by avoiding > > > > walking into > > > > *_EXTRA_ARGS in extract_locals_r? Or do we still want to walk into any > > > > nested > > > > extra args? And if so, will we run into this same problem then? > > > > > > I'm not sure totally but I'd expect a nested extra-args tree to always > > > have empty *_EXTRA_ARGS since the outer extra-args tree should intercept > > > any substitution before the inner extra-args tree can see it? > > > > ... and so in extract_locals_r I think we can assume *_EXTRA_ARGS is > > empty, and not have to explicitly avoid walking it. > > It seems more robust to me to handle _EXTRA_ARGS appropriately in > build_extra_args rather than expect callers to know that they shouldn't pass > in a tree with _EXTRA_ARGS set. At least check and abort in that case? Sounds good. That IMHO seems simpler than actually avoiding walking into *_EXTRA_ARGS from extract_locals_r because we'd have to repeat the walking logic from cp_walk_subtree modulo the *_EXTRA_ARGS walk. How does the following look? Bootstraped and regtested on x86_64-pc-linux-gnu. -- > 8-- Subject: [PATCH] c++: build_extra_args recapturing local specs [PR114303] PR c++/114303 gcc/cp/ChangeLog: * constraint.cc (tsubst_requires_expr): Clear REQUIRES_EXPR_EXTRA_ARGS before calling build_extra_args. * pt.cc (extract_locals_r): Assert *_EXTRA_ARGS is empty. (tsubst_stmt) <case IF_STMT>: Call build_extra_args on the new IF_STMT instead of t which might already have IF_STMT_EXTRA_ARGS. gcc/testsuite/ChangeLog: * g++.dg/cpp1z/constexpr-if-lambda6.C: New test. --- gcc/cp/constraint.cc | 1 + gcc/cp/pt.cc | 16 +++++++++++++++- .../g++.dg/cpp1z/constexpr-if-lambda6.C | 16 ++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc index 49de3211d4c..8a3b5d80ba7 100644 --- a/gcc/cp/constraint.cc +++ b/gcc/cp/constraint.cc @@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info info) matching or dguide constraint rewriting), in which case we need to partially substitute. */ t = copy_node (t); + REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE; REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, info.complain); return t; } diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index c38594cd862..6cc9b95fc06 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -13310,6 +13310,19 @@ extract_locals_r (tree *tp, int *walk_subtrees, void *data_) /* Remember local typedefs (85214). */ tp = &TYPE_NAME (*tp); + if (has_extra_args_mechanism_p (*tp)) + { + if (PACK_EXPANSION_P (*tp)) + gcc_checking_assert (!PACK_EXPANSION_EXTRA_ARGS (*tp)); + else if (TREE_CODE (*tp) == REQUIRES_EXPR) + gcc_checking_assert (!REQUIRES_EXPR_EXTRA_ARGS (*tp)); + else if (TREE_CODE (*tp) == IF_STMT + && IF_STMT_CONSTEXPR_P (*tp)) + gcc_checking_assert (!IF_STMT_EXTRA_ARGS (*tp)); + else + gcc_unreachable (); + } + if (TREE_CODE (*tp) == DECL_EXPR) { tree decl = DECL_EXPR_DECL (*tp); @@ -18738,7 +18751,8 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl) IF_COND (stmt) = IF_COND (t); THEN_CLAUSE (stmt) = THEN_CLAUSE (t); ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); + IF_SCOPE (stmt) = NULL_TREE; + IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain); add_stmt (stmt); break; } diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C new file mode 100644 index 00000000000..038c2a41210 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C @@ -0,0 +1,16 @@ +// PR c++/114303 +// { dg-do compile { target c++17 } } + +struct A { static constexpr bool value = true; }; + +int main() { + [](auto x1) { + return [&](auto) { + return [&](auto x3) { + if constexpr (decltype(x3)::value) { + static_assert(decltype(x1)::value); + } + }(A{}); + }(0); + }(A{}); +}
On 4/10/24 17:39, Patrick Palka wrote: > On Wed, 10 Apr 2024, Jason Merrill wrote: > >> On 3/12/24 10:51, Patrick Palka wrote: >>> On Tue, 12 Mar 2024, Patrick Palka wrote: >>>> On Tue, 12 Mar 2024, Jason Merrill wrote: >>>>> On 3/11/24 12:53, Patrick Palka wrote: >>>>>> >>>>>> r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated contexts >>>>>> first so that we prefer processing a local specialization in an >>>>>> evaluated >>>>>> context even if its first use is in an unevaluated context. But this >>>>>> means we need to avoid walking a tree that already has extra >>>>>> args/specs >>>>>> saved because the list of saved specs appears to be an evaluated >>>>>> context. It seems then that we should be calculating the saved specs >>>>>> from scratch each time, rather than potentially walking the saved >>>>>> specs >>>>>> list from an earlier partial instantiation when calling >>>>>> build_extra_args >>>>>> a second time around. >>>>> >>>>> Makes sense, but I wonder if we want to approach that by avoiding >>>>> walking into >>>>> *_EXTRA_ARGS in extract_locals_r? Or do we still want to walk into any >>>>> nested >>>>> extra args? And if so, will we run into this same problem then? >>>> >>>> I'm not sure totally but I'd expect a nested extra-args tree to always >>>> have empty *_EXTRA_ARGS since the outer extra-args tree should intercept >>>> any substitution before the inner extra-args tree can see it? >>> >>> ... and so in extract_locals_r I think we can assume *_EXTRA_ARGS is >>> empty, and not have to explicitly avoid walking it. >> >> It seems more robust to me to handle _EXTRA_ARGS appropriately in >> build_extra_args rather than expect callers to know that they shouldn't pass >> in a tree with _EXTRA_ARGS set. At least check and abort in that case? > > Sounds good. That IMHO seems simpler than actually avoiding walking > into *_EXTRA_ARGS from extract_locals_r because we'd have to repeat > the walking logic from cp_walk_subtree modulo the *_EXTRA_ARGS walk. > > How does the following look? Bootstraped and regtested on > x86_64-pc-linux-gnu. > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > index c38594cd862..6cc9b95fc06 100644 > --- a/gcc/cp/pt.cc > +++ b/gcc/cp/pt.cc > @@ -13310,6 +13310,19 @@ extract_locals_r (tree *tp, int *walk_subtrees, void *data_) > /* Remember local typedefs (85214). */ > tp = &TYPE_NAME (*tp); > Please add a comment explaining why it needs to be null. Also, how about a generic _EXTRA_ARGS accessor so places like this don't need to check each code themselves? > + if (has_extra_args_mechanism_p (*tp)) > + { > + if (PACK_EXPANSION_P (*tp)) > + gcc_checking_assert (!PACK_EXPANSION_EXTRA_ARGS (*tp)); > + else if (TREE_CODE (*tp) == REQUIRES_EXPR) > + gcc_checking_assert (!REQUIRES_EXPR_EXTRA_ARGS (*tp)); > + else if (TREE_CODE (*tp) == IF_STMT > + && IF_STMT_CONSTEXPR_P (*tp)) > + gcc_checking_assert (!IF_STMT_EXTRA_ARGS (*tp)); > + else > + gcc_unreachable (); > + } > + > if (TREE_CODE (*tp) == DECL_EXPR) > { > tree decl = DECL_EXPR_DECL (*tp); > @@ -18738,7 +18751,8 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl) > IF_COND (stmt) = IF_COND (t); > THEN_CLAUSE (stmt) = THEN_CLAUSE (t); > ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); > - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); > + IF_SCOPE (stmt) = NULL_TREE; What does IF_SCOPE have to do with this? Jason
On Wed, 10 Apr 2024, Jason Merrill wrote: > On 4/10/24 17:39, Patrick Palka wrote: > > On Wed, 10 Apr 2024, Jason Merrill wrote: > > > > > On 3/12/24 10:51, Patrick Palka wrote: > > > > On Tue, 12 Mar 2024, Patrick Palka wrote: > > > > > On Tue, 12 Mar 2024, Jason Merrill wrote: > > > > > > On 3/11/24 12:53, Patrick Palka wrote: > > > > > > > > > > > > > > r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated > > > > > > > contexts > > > > > > > first so that we prefer processing a local specialization in an > > > > > > > evaluated > > > > > > > context even if its first use is in an unevaluated context. But > > > > > > > this > > > > > > > means we need to avoid walking a tree that already has extra > > > > > > > args/specs > > > > > > > saved because the list of saved specs appears to be an evaluated > > > > > > > context. It seems then that we should be calculating the saved > > > > > > > specs > > > > > > > from scratch each time, rather than potentially walking the saved > > > > > > > specs > > > > > > > list from an earlier partial instantiation when calling > > > > > > > build_extra_args > > > > > > > a second time around. > > > > > > > > > > > > Makes sense, but I wonder if we want to approach that by avoiding > > > > > > walking into > > > > > > *_EXTRA_ARGS in extract_locals_r? Or do we still want to walk into > > > > > > any > > > > > > nested > > > > > > extra args? And if so, will we run into this same problem then? > > > > > > > > > > I'm not sure totally but I'd expect a nested extra-args tree to always > > > > > have empty *_EXTRA_ARGS since the outer extra-args tree should > > > > > intercept > > > > > any substitution before the inner extra-args tree can see it? > > > > > > > > ... and so in extract_locals_r I think we can assume *_EXTRA_ARGS is > > > > empty, and not have to explicitly avoid walking it. > > > > > > It seems more robust to me to handle _EXTRA_ARGS appropriately in > > > build_extra_args rather than expect callers to know that they shouldn't > > > pass > > > in a tree with _EXTRA_ARGS set. At least check and abort in that case? > > > > Sounds good. That IMHO seems simpler than actually avoiding walking > > into *_EXTRA_ARGS from extract_locals_r because we'd have to repeat > > the walking logic from cp_walk_subtree modulo the *_EXTRA_ARGS walk. > > > > How does the following look? Bootstraped and regtested on > > x86_64-pc-linux-gnu. > > > > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc > > index c38594cd862..6cc9b95fc06 100644 > > --- a/gcc/cp/pt.cc > > +++ b/gcc/cp/pt.cc > > @@ -13310,6 +13310,19 @@ extract_locals_r (tree *tp, int *walk_subtrees, > > void *data_) > > /* Remember local typedefs (85214). */ > > tp = &TYPE_NAME (*tp); > > > > Please add a comment explaining why it needs to be null. > > Also, how about a generic _EXTRA_ARGS accessor so places like this don't need > to check each code themselves? Sounds good. > > > + if (has_extra_args_mechanism_p (*tp)) > > + { > > + if (PACK_EXPANSION_P (*tp)) > > + gcc_checking_assert (!PACK_EXPANSION_EXTRA_ARGS (*tp)); > > + else if (TREE_CODE (*tp) == REQUIRES_EXPR) > > + gcc_checking_assert (!REQUIRES_EXPR_EXTRA_ARGS (*tp)); > > + else if (TREE_CODE (*tp) == IF_STMT > > + && IF_STMT_CONSTEXPR_P (*tp)) > > + gcc_checking_assert (!IF_STMT_EXTRA_ARGS (*tp)); > > + else > > + gcc_unreachable (); > > + } > > + > > if (TREE_CODE (*tp) == DECL_EXPR) > > { > > tree decl = DECL_EXPR_DECL (*tp); > > @@ -18738,7 +18751,8 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t > > complain, tree in_decl) > > IF_COND (stmt) = IF_COND (t); > > THEN_CLAUSE (stmt) = THEN_CLAUSE (t); > > ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); > > - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); > > + IF_SCOPE (stmt) = NULL_TREE; > > What does IF_SCOPE have to do with this? IF_SCOPE is the same field as IF_STMT_EXTRA_ARGS so we need to clear it before calling build_extra_args to avoid tripping over the added assert. How does the following look? -- >8 -- Subject: [PATCH] c++: build_extra_args recapturing local specs [PR114303] PR c++/114303 gcc/cp/ChangeLog: * constraint.cc (tsubst_requires_expr): Clear REQUIRES_EXPR_EXTRA_ARGS before calling build_extra_args. * pt.cc (tree_extra_args): Define. (extract_locals_r): Assert *_EXTRA_ARGS is empty. (tsubst_stmt) <case IF_STMT>: Clear IF_SCOPE on the new IF_STMT. Call build_extra_args on the new IF_STMT instead of t which might already have IF_STMT_EXTRA_ARGS. gcc/testsuite/ChangeLog: * g++.dg/cpp1z/constexpr-if-lambda6.C: New test. --- gcc/cp/constraint.cc | 1 + gcc/cp/pt.cc | 31 ++++++++++++++++++- .../g++.dg/cpp1z/constexpr-if-lambda6.C | 16 ++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc index 49de3211d4c..8a3b5d80ba7 100644 --- a/gcc/cp/constraint.cc +++ b/gcc/cp/constraint.cc @@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info info) matching or dguide constraint rewriting), in which case we need to partially substitute. */ t = copy_node (t); + REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE; REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, info.complain); return t; } diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index c999e2d9baa..1b17784f6b5 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -3859,6 +3859,25 @@ has_extra_args_mechanism_p (const_tree t) || TREE_CODE (t) == LAMBDA_EXPR); /* LAMBDA_EXPR_EXTRA_ARGS */ } +/* Return *_EXTRA_ARGS of the given supported tree T. */ + +static tree& +tree_extra_args (tree t) +{ + gcc_checking_assert (has_extra_args_mechanism_p (t)); + + if (PACK_EXPANSION_P (t)) + return PACK_EXPANSION_EXTRA_ARGS (t); + else if (TREE_CODE (t) == REQUIRES_EXPR) + return REQUIRES_EXPR_EXTRA_ARGS (t); + else if (TREE_CODE (t) == IF_STMT + && IF_STMT_CONSTEXPR_P (t)) + return IF_STMT_EXTRA_ARGS (t); + else if (TREE_CODE (t) == LAMBDA_EXPR) + return LAMBDA_EXPR_EXTRA_ARGS (t); + gcc_unreachable (); +} + /* Structure used to track the progress of find_parameter_packs_r. */ struct find_parameter_pack_data { @@ -13292,6 +13311,15 @@ extract_locals_r (tree *tp, int *walk_subtrees, void *data_) /* Remember local typedefs (85214). */ tp = &TYPE_NAME (*tp); + if (has_extra_args_mechanism_p (*tp)) + /* We don't want to walk *_EXTRA_ARGS to avoid seeing a captured + local in an evaluated context that's otherwise used only in an + unevaluated context (PR114303). So callers should ensure the + *_EXTRA_ARGS of the outermost tree is empty. (Nested *_EXTRA_ARGS + should naturally be empty since the outermost (extra-args) tree + will intercept any substitution.) */ + gcc_checking_assert (!tree_extra_args (*tp)); + if (TREE_CODE (*tp) == DECL_EXPR) { tree decl = DECL_EXPR_DECL (*tp); @@ -18720,7 +18748,8 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl) IF_COND (stmt) = IF_COND (t); THEN_CLAUSE (stmt) = THEN_CLAUSE (t); ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); + IF_SCOPE (stmt) = NULL_TREE; + IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain); add_stmt (stmt); break; } diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C new file mode 100644 index 00000000000..038c2a41210 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C @@ -0,0 +1,16 @@ +// PR c++/114303 +// { dg-do compile { target c++17 } } + +struct A { static constexpr bool value = true; }; + +int main() { + [](auto x1) { + return [&](auto) { + return [&](auto x3) { + if constexpr (decltype(x3)::value) { + static_assert(decltype(x1)::value); + } + }(A{}); + }(0); + }(A{}); +}
On 4/10/24 20:00, Patrick Palka wrote: > On Wed, 10 Apr 2024, Jason Merrill wrote: > >> On 4/10/24 17:39, Patrick Palka wrote: >>> On Wed, 10 Apr 2024, Jason Merrill wrote: >>> >>>> On 3/12/24 10:51, Patrick Palka wrote: >>>>> On Tue, 12 Mar 2024, Patrick Palka wrote: >>>>>> On Tue, 12 Mar 2024, Jason Merrill wrote: >>>>>>> On 3/11/24 12:53, Patrick Palka wrote: >>>>>>>> >>>>>>>> r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated >>>>>>>> contexts >>>>>>>> first so that we prefer processing a local specialization in an >>>>>>>> evaluated >>>>>>>> context even if its first use is in an unevaluated context. But >>>>>>>> this >>>>>>>> means we need to avoid walking a tree that already has extra >>>>>>>> args/specs >>>>>>>> saved because the list of saved specs appears to be an evaluated >>>>>>>> context. It seems then that we should be calculating the saved >>>>>>>> specs >>>>>>>> from scratch each time, rather than potentially walking the saved >>>>>>>> specs >>>>>>>> list from an earlier partial instantiation when calling >>>>>>>> build_extra_args >>>>>>>> a second time around. >>>>>>> >>>>>>> Makes sense, but I wonder if we want to approach that by avoiding >>>>>>> walking into >>>>>>> *_EXTRA_ARGS in extract_locals_r? Or do we still want to walk into >>>>>>> any >>>>>>> nested >>>>>>> extra args? And if so, will we run into this same problem then? >>>>>> >>>>>> I'm not sure totally but I'd expect a nested extra-args tree to always >>>>>> have empty *_EXTRA_ARGS since the outer extra-args tree should >>>>>> intercept >>>>>> any substitution before the inner extra-args tree can see it? >>>>> >>>>> ... and so in extract_locals_r I think we can assume *_EXTRA_ARGS is >>>>> empty, and not have to explicitly avoid walking it. >>>> >>>> It seems more robust to me to handle _EXTRA_ARGS appropriately in >>>> build_extra_args rather than expect callers to know that they shouldn't >>>> pass >>>> in a tree with _EXTRA_ARGS set. At least check and abort in that case? >>> >>> Sounds good. That IMHO seems simpler than actually avoiding walking >>> into *_EXTRA_ARGS from extract_locals_r because we'd have to repeat >>> the walking logic from cp_walk_subtree modulo the *_EXTRA_ARGS walk. >>> >>> How does the following look? Bootstraped and regtested on >>> x86_64-pc-linux-gnu. >>> >>> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc >>> index c38594cd862..6cc9b95fc06 100644 >>> --- a/gcc/cp/pt.cc >>> +++ b/gcc/cp/pt.cc >>> @@ -13310,6 +13310,19 @@ extract_locals_r (tree *tp, int *walk_subtrees, >>> void *data_) >>> /* Remember local typedefs (85214). */ >>> tp = &TYPE_NAME (*tp); >>> >> >> Please add a comment explaining why it needs to be null. >> >> Also, how about a generic _EXTRA_ARGS accessor so places like this don't need >> to check each code themselves? > > Sounds good. > >> >>> + if (has_extra_args_mechanism_p (*tp)) >>> + { >>> + if (PACK_EXPANSION_P (*tp)) >>> + gcc_checking_assert (!PACK_EXPANSION_EXTRA_ARGS (*tp)); >>> + else if (TREE_CODE (*tp) == REQUIRES_EXPR) >>> + gcc_checking_assert (!REQUIRES_EXPR_EXTRA_ARGS (*tp)); >>> + else if (TREE_CODE (*tp) == IF_STMT >>> + && IF_STMT_CONSTEXPR_P (*tp)) >>> + gcc_checking_assert (!IF_STMT_EXTRA_ARGS (*tp)); >>> + else >>> + gcc_unreachable (); >>> + } >>> + >>> if (TREE_CODE (*tp) == DECL_EXPR) >>> { >>> tree decl = DECL_EXPR_DECL (*tp); >>> @@ -18738,7 +18751,8 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t >>> complain, tree in_decl) >>> IF_COND (stmt) = IF_COND (t); >>> THEN_CLAUSE (stmt) = THEN_CLAUSE (t); >>> ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); >>> - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); >>> + IF_SCOPE (stmt) = NULL_TREE; >> >> What does IF_SCOPE have to do with this? > > IF_SCOPE is the same field as IF_STMT_EXTRA_ARGS so we need to clear it > before calling build_extra_args to avoid tripping over the added assert. Let's clear it a few lines earlier, then, immediately after the poplevel; OK with that change. finish_if_stmt clears it even before calling poplevel, but that doesn't seem necessary. Jason
diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc index 49de3211d4c..8a3b5d80ba7 100644 --- a/gcc/cp/constraint.cc +++ b/gcc/cp/constraint.cc @@ -2362,6 +2362,7 @@ tsubst_requires_expr (tree t, tree args, sat_info info) matching or dguide constraint rewriting), in which case we need to partially substitute. */ t = copy_node (t); + REQUIRES_EXPR_EXTRA_ARGS (t) = NULL_TREE; REQUIRES_EXPR_EXTRA_ARGS (t) = build_extra_args (t, args, info.complain); return t; } diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc index 8cf0d5b7a8d..37f2392d035 100644 --- a/gcc/cp/pt.cc +++ b/gcc/cp/pt.cc @@ -18718,7 +18718,7 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t complain, tree in_decl) IF_COND (stmt) = IF_COND (t); THEN_CLAUSE (stmt) = THEN_CLAUSE (t); ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t); - IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain); + IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (stmt, args, complain); add_stmt (stmt); break; } diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C new file mode 100644 index 00000000000..038c2a41210 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda6.C @@ -0,0 +1,16 @@ +// PR c++/114303 +// { dg-do compile { target c++17 } } + +struct A { static constexpr bool value = true; }; + +int main() { + [](auto x1) { + return [&](auto) { + return [&](auto x3) { + if constexpr (decltype(x3)::value) { + static_assert(decltype(x1)::value); + } + }(A{}); + }(0); + }(A{}); +}