On Mon, 13 Jan 2025, Jason Merrill wrote:
> On 1/10/25 2:20 PM, Patrick Palka wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> > OK for trunk?
> >
> > The documentation for LAMBDA_EXPR_THIS_CAPTURE seems outdated because
> > it says the field is only used at parse time, but apparently it's also
> > used at instantiation time.
> >
> > Non-'this' captures don't seem to be affected, because there is no
> > corresponding LAMBDA_EXPR field that gets clobbered, and instead their
> > uses get resolved via the local specialization mechanism which is
> > recursion aware.
> >
> > The bug also disappears if we explicitly use this in the openSeries call,
> > i.e. this->openSeries(...), because that sidesteps the use of
> > maybe_resolve_dummy / LAMBDA_EXPR_THIS_CAPTURE for resolving the
> > implicit object, and instead gets resolved via the local mechanism
> > specialization.
> >
> > Maybe this suggests that there's a better way to fix this, but I'm not
> > sure...
>
> That does sound like an interesting direction. Maybe for a generic lambda,
> LAMBDA_EXPR_THIS_CAPTURE could just refer to the captured parameter, and we
> use retrieve_local_specialization to find the proxy?
Like so? Tested on x86_64-pc-linux-gnu, full bootstrap+regtest in
progress.
-- >8 --
Subject: [PATCH v2] c++: 'this' capture clobbered during recursive inst
[PR116756]
Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
OK for trunk?
-- >8 --
Here during instantiation of generic lambda's op() [with I = 0] we
substitute into the call self(self, cst<1>{}) which requires recursive
instantiation of the same op() [with I = 1] (which isn't deferred due to
lambda's deduced return type. During this recursive instantiation, the
DECL_EXPR case of tsubst_stmt clobbers LAMBDA_EXPR_THIS_CAPTURE to point
to the child op()'s specialized capture proxy instead of the parent's,
and the original value is never restored.
So later when substituting into the openSeries call in the parent op()
maybe_resolve_dummy uses the 'this' proxy belonging to the child op(),
which leads to a context mismatch ICE during gimplification of the
proxy.
An earlier version of this patch fixed this by making instantiate_body
save/restore LAMBDA_EXPR_THIS_CAPTURE during a lambda op() instantiation.
But it seems cleaner to avoid overwriting LAMBDA_EXPR_THIS_CAPTURE in the
first place by making it point to the non-specialized capture proxy, and
instead call retrieve_local_specialization as needed, which is what this
patch implements. It's simpler then to not clear LAMBDA_EXPR_THIS_CAPTURE
after parsing/regenerating a lambda.
PR c++/116756
gcc/cp/ChangeLog:
* lambda.cc (lambda_expr_this_capture): Call
retrieve_local_specialization on the result of
LAMBDA_EXPR_THIS_CAPTURE for a generic lambda.
* parser.cc (cp_parser_lambda_expression): Don't clear
LAMBDA_EXPR_THIS_CAPTURE.
* pt.cc (tsubst_stmt) <case DECL_EXPR>: Don't overwrite
LAMBDA_EXPR_THIS_CAPTURE.
(tsubst_lambda_expr): Don't clear LAMBDA_EXPR_THIS_CAPTURE
afterward.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1z/constexpr-if-lambda7.C: New test.
---
gcc/cp/lambda.cc | 6 +++++
gcc/cp/parser.cc | 3 ---
gcc/cp/pt.cc | 11 +--------
.../g++.dg/cpp1z/constexpr-if-lambda7.C | 24 +++++++++++++++++++
4 files changed, 31 insertions(+), 13 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda7.C
diff --git a/gcc/cp/lambda.cc b/gcc/cp/lambda.cc
index be8a0fe01cb..4ee8f6c745d 100644
--- a/gcc/cp/lambda.cc
+++ b/gcc/cp/lambda.cc
@@ -785,6 +785,12 @@ lambda_expr_this_capture (tree lambda, int add_capture_p)
tree result;
tree this_capture = LAMBDA_EXPR_THIS_CAPTURE (lambda);
+ if (this_capture)
+ if (tree spec = retrieve_local_specialization (this_capture))
+ {
+ gcc_checking_assert (generic_lambda_fn_p (lambda_function (lambda)));
+ this_capture = spec;
+ }
/* In unevaluated context this isn't an odr-use, so don't capture. */
if (cp_unevaluated_operand)
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 74f4f7cd6d8..16bbb87a815 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -11723,9 +11723,6 @@ cp_parser_lambda_expression (cp_parser* parser)
parser->omp_array_section_p = saved_omp_array_section_p;
}
- /* This field is only used during parsing of the lambda. */
- LAMBDA_EXPR_THIS_CAPTURE (lambda_expr) = NULL_TREE;
-
/* This lambda shouldn't have any proxies left at this point. */
gcc_assert (LAMBDA_EXPR_PENDING_PROXIES (lambda_expr) == NULL);
/* And now that we're done, push proxies for an enclosing lambda. */
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 961696f333e..64c7d3da405 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -18938,12 +18938,6 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t
complain, tree in_decl)
else if (is_capture_proxy (DECL_EXPR_DECL (t)))
{
DECL_CONTEXT (decl) = current_function_decl;
- if (DECL_NAME (decl) == this_identifier)
- {
- tree lam = DECL_CONTEXT (current_function_decl);
- lam = CLASSTYPE_LAMBDA_EXPR (lam);
- LAMBDA_EXPR_THIS_CAPTURE (lam) = decl;
- }
insert_capture_proxy (decl);
}
else if (DECL_IMPLICIT_TYPEDEF_P (t))
@@ -20146,8 +20140,7 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t
complain, tree in_decl)
LAMBDA_EXPR_REGEN_INFO (r)
= build_template_info (t, preserve_args (args));
- gcc_assert (LAMBDA_EXPR_THIS_CAPTURE (t) == NULL_TREE
- && LAMBDA_EXPR_PENDING_PROXIES (t) == NULL);
+ gcc_assert (LAMBDA_EXPR_PENDING_PROXIES (t) == NULL);
vec<tree,va_gc>* field_packs = NULL;
unsigned name_independent_cnt = 0;
@@ -20362,8 +20355,6 @@ tsubst_lambda_expr (tree t, tree args, tsubst_flags_t
complain, tree in_decl)
LAMBDA_EXPR_CAPTURE_LIST (r)
= nreverse (LAMBDA_EXPR_CAPTURE_LIST (r));
- LAMBDA_EXPR_THIS_CAPTURE (r) = NULL_TREE;
-
maybe_add_lambda_conv_op (type);
}
diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda7.C
b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda7.C
new file mode 100644
index 00000000000..8304c8f22e3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda7.C
@@ -0,0 +1,24 @@
+// PR c++/116756
+// { dg-do compile { target c++17 } }
+
+template<int N> struct cst { static constexpr int value = N; };
+
+struct Store {
+ void openDF() {
+ auto lambda = [this](auto& self, auto I) {
+ if constexpr (I.value == 0) {
+ auto next = [&self] { self(self, cst<1>{}); };
+ openSeries(next);
+ } else {
+ openSeries(0);
+ }
+ };
+ lambda(lambda, cst<0>{});
+ }
+ template<class T> void openSeries(T) { }
+};
+
+int main() {
+ Store store;
+ store.openDF();
+}
--
2.48.1.2.g757161efcc
>
> > -- >8 --
> >
> > Here during instantiation of lambda::op() [with I = 0] we substitute
> > into the call self(self, cst<1>{}) which requires recursive instantiation
> > of the same lambda::op() [with I = 1] (which isn't deferred due to
> > lambda's deduced return type. During this recursive instantiation, the
> > DECL_EXPR case of tsubst_stmt clobbers LAMBDA_EXPR_THIS_CAPTURE to point
> > to the inner lambda::op()'s capture proxy instead of the outer
> > lambda::op(), and the original value is never restored.
> >
> > So later during substitution into the openSeries call in the outer
> > lambda::op() maybe_resolve_dummy uses the 'this' proxy belonging to the
> > inner lambda::op(), which leads to an context mismatch ICE during
> > gimplification of the proxy.
> >
> > This patch naively fixes this by making us restore LAMBDA_EXPR_THIS_CAPTURE
> > after instantiating a lambda's op().
> >
> > PR c++/116756
> >
> > gcc/cp/ChangeLog:
> >
> > * pt.cc (instantiate_body): Restore LAMBDA_EXPR_THIS_CAPTURE
> > after instantiating a lambda's op().
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/cpp1z/constexpr-if-lambda7.C: New test.
> > ---
> > gcc/cp/pt.cc | 11 +++++++++
> > .../g++.dg/cpp1z/constexpr-if-lambda7.C | 24 +++++++++++++++++++
> > 2 files changed, 35 insertions(+)
> > create mode 100644 gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda7.C
> >
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index b22129d8a46..a141de56446 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -27512,6 +27512,13 @@ instantiate_body (tree pattern, tree args, tree d,
> > bool nested_p)
> > local_specialization_stack lss (push_to_top ? lss_blank : lss_copy);
> > tree block = NULL_TREE;
> > + tree saved_this_capture = NULL_TREE;
> > + if (LAMBDA_FUNCTION_P (d))
> > + /* Save/restore the 'this' capture, which gets clobbered by
> > tsubst_stmt,
> > + which causes problems in case of recursive op() instantiation. */
> > + saved_this_capture
> > + = LAMBDA_EXPR_THIS_CAPTURE (CLASSTYPE_LAMBDA_EXPR (DECL_CONTEXT
> > (d)));
> > +
> > /* Set up context. */
> > if (nested_p)
> > block = push_stmt_list ();
> > @@ -27555,6 +27562,10 @@ instantiate_body (tree pattern, tree args, tree d,
> > bool nested_p)
> > if (DECL_OMP_DECLARE_REDUCTION_P (code_pattern))
> > cp_check_omp_declare_reduction (d);
> > +
> > + if (LAMBDA_FUNCTION_P (d))
> > + LAMBDA_EXPR_THIS_CAPTURE (CLASSTYPE_LAMBDA_EXPR (DECL_CONTEXT (d)))
> > + = saved_this_capture;
> > }
> > /* We're not deferring instantiation any more. */
> > diff --git a/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda7.C
> > b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda7.C
> > new file mode 100644
> > index 00000000000..8304c8f22e3
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp1z/constexpr-if-lambda7.C
> > @@ -0,0 +1,24 @@
> > +// PR c++/116756
> > +// { dg-do compile { target c++17 } }
> > +
> > +template<int N> struct cst { static constexpr int value = N; };
> > +
> > +struct Store {
> > + void openDF() {
> > + auto lambda = [this](auto& self, auto I) {
> > + if constexpr (I.value == 0) {
> > + auto next = [&self] { self(self, cst<1>{}); };
> > + openSeries(next);
> > + } else {
> > + openSeries(0);
> > + }
> > + };
> > + lambda(lambda, cst<0>{});
> > + }
> > + template<class T> void openSeries(T) { }
> > +};
> > +
> > +int main() {
> > + Store store;
> > + store.openDF();
> > +}
>
>
