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{});
+}
--
2.44.0.568.g436d4e5b14
