On Tue, Apr 15, 2025 at 11:39:21PM +1000, Nathaniel Shead wrote:
> On Tue, Apr 15, 2025 at 09:16:46AM -0400, Jason Merrill wrote:
> > On 4/15/25 2:56 AM, Nathaniel Shead wrote:
> > > On Mon, Apr 14, 2025 at 05:33:05PM -0400, Jason Merrill wrote:
> > > > On 4/13/25 6:32 AM, Nathaniel Shead wrote:
> > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> > > > >
> > > > > -- >8 --
> > > > >
> > > > > Currently, pruned lambda captures are still leftover in the function's
> > > > > BLOCK and topmost BIND_EXPR; this doesn't cause any issues for normal
> > > > > compilation, but does break modules streaming as we try to
> > > > > reconstruct a
> > > > > FIELD_DECL that no longer exists on the type itself.
> > > > >
> > > > > PR c++/119755
> > > > >
> > > > > gcc/cp/ChangeLog:
> > > > >
> > > > > * lambda.cc (prune_lambda_captures): Remove pruned capture from
> > > > > function's BLOCK_VARS and BIND_EXPR_VARS.
> > > > >
> > > > > gcc/testsuite/ChangeLog:
> > > > >
> > > > > * g++.dg/modules/lambda-10_a.H: New test.
> > > > > * g++.dg/modules/lambda-10_b.C: New test.
> > > > >
> > > > > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > > > > ---
> > > > > gcc/cp/lambda.cc | 22
> > > > > ++++++++++++++++++++++
> > > > > gcc/testsuite/g++.dg/modules/lambda-10_a.H | 17 +++++++++++++++++
> > > > > gcc/testsuite/g++.dg/modules/lambda-10_b.C | 7 +++++++
> > > > > 3 files changed, 46 insertions(+)
> > > > > create mode 100644 gcc/testsuite/g++.dg/modules/lambda-10_a.H
> > > > > create mode 100644 gcc/testsuite/g++.dg/modules/lambda-10_b.C
> > > > >
> > > > > diff --git a/gcc/cp/lambda.cc b/gcc/cp/lambda.cc
> > > > > index f0a54b60275..d01bb04cd32 100644
> > > > > --- a/gcc/cp/lambda.cc
> > > > > +++ b/gcc/cp/lambda.cc
> > > > > @@ -1858,6 +1858,14 @@ prune_lambda_captures (tree body)
> > > > > cp_walk_tree_without_duplicates (&body, mark_const_cap_r,
> > > > > &const_vars);
> > > > > + tree bind_expr = expr_single (DECL_SAVED_TREE (lambda_function
> > > > > (lam)));
> > > > > + gcc_assert (bind_expr
> > > > > + && (TREE_CODE (bind_expr) == BIND_EXPR
> > > > > + /* FIXME: In a noexcept lambda we never prune captures
> > > > > + (PR119764); when we do we need to handle this case
> > > > > + for modules streaming. */
> > > >
> > > > The attached patch seems to fix that, with the result that your patch
> > > > crashes.
> > > >
> > >
> > > Thanks. And yup, crashing was deliberate here as I wasn't 100% sure
> > > what the tree would look like for this case after an appropriate fix.
> > >
> > > One quick question about your patch, since it could in theory affect ABI
> > > (the size of the lambdas change) should the pruning of such lambdas be
> > > dependent on an ABI version check?
> >
> > Indeed, perhaps this is too late in the 15 cycle for such a change.
> >
> > > Otherwise here's an updated patch that relies on your patch.
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk along
> > > with yours? (Or if the potential ABI concerns mean that your change
> > > isn't appropriate for GCC15, would the old version of my patch still be
> > > OK for GCC15 to get 'import std' working again for C++26?)
> >
> > For 15 please adjust this patch to be more fault-tolerant:
> >
> > > -- >8 --
> > >
> > > Currently, pruned lambda captures are still leftover in the function's
> > > BLOCK and topmost BIND_EXPR; this doesn't cause any issues for normal
> > > compilation, but does break modules streaming as we try to reconstruct a
> > > FIELD_DECL that no longer exists on the type itself.
> > >
> > > PR c++/119755
> > >
> > > gcc/cp/ChangeLog:
> > >
> > > * lambda.cc (prune_lambda_captures): Remove pruned capture from
> > > function's BLOCK_VARS and BIND_EXPR_VARS.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > * g++.dg/modules/lambda-10_a.H: New test.
> > > * g++.dg/modules/lambda-10_b.C: New test.
> > >
> > > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > > Reviewed-by: Jason Merrill <ja...@redhat.com>
> > > ---
> > > gcc/cp/lambda.cc | 19 +++++++++++++++++++
> > > gcc/testsuite/g++.dg/modules/lambda-10_a.H | 17 +++++++++++++++++
> > > gcc/testsuite/g++.dg/modules/lambda-10_b.C | 7 +++++++
> > > 3 files changed, 43 insertions(+)
> > > create mode 100644 gcc/testsuite/g++.dg/modules/lambda-10_a.H
> > > create mode 100644 gcc/testsuite/g++.dg/modules/lambda-10_b.C
> > >
> > > diff --git a/gcc/cp/lambda.cc b/gcc/cp/lambda.cc
> > > index c6308b941d3..7bb88a900d5 100644
> > > --- a/gcc/cp/lambda.cc
> > > +++ b/gcc/cp/lambda.cc
> > > @@ -1862,6 +1862,11 @@ prune_lambda_captures (tree body)
> > > cp_walk_tree_without_duplicates (&body, mark_const_cap_r,
> > > &const_vars);
> > > + tree bind_expr = expr_single (DECL_SAVED_TREE (lambda_function (lam)));
> > > + if (bind_expr && TREE_CODE (bind_expr) == MUST_NOT_THROW_EXPR)
> > > + bind_expr = expr_single (TREE_OPERAND (bind_expr, 0));
> > > + gcc_assert (bind_expr && TREE_CODE (bind_expr) == BIND_EXPR);
> >
> > i.e. here clear bind_expr if it isn't a BIND_EXPR...
> >
> > > tree *fieldp = &TYPE_FIELDS (LAMBDA_EXPR_CLOSURE (lam));
> > > for (tree *capp = &LAMBDA_EXPR_CAPTURE_LIST (lam); *capp; )
> > > {
> > > @@ -1883,6 +1888,20 @@ prune_lambda_captures (tree body)
> > > fieldp = &DECL_CHAIN (*fieldp);
> > > *fieldp = DECL_CHAIN (*fieldp);
> > > + /* And out of the bindings for the function. */
> > > + tree *blockp = &BLOCK_VARS (current_binding_level->blocks);
> > > + while (*blockp != DECL_EXPR_DECL (**use))
> > > + blockp = &DECL_CHAIN (*blockp);
> > > + *blockp = DECL_CHAIN (*blockp);
> > > +
> > > + /* And maybe out of the vars declared in the containing
> > > + BIND_EXPR, if it's listed there. */
> > > + tree *bindp = &BIND_EXPR_VARS (bind_expr);
> > > + while (*bindp && *bindp != DECL_EXPR_DECL (**use))
> > > + bindp = &DECL_CHAIN (*bindp);
> > > + if (*bindp)
> > > + *bindp = DECL_CHAIN (*bindp);
> >
> > ...and only do this if bind_expr is set.
> >
> > Then it should work with or without my patch, right?
>
> Right; here's a slight variation on that (taking advantage of the fact
> that the bug means that 'expr_single' will return NULL_TREE if the
> variables got incorrectly pushed into that statement list). Tested so
> far on dg.exp=*lambda* modules.exp=*lambda*, OK for trunk if full
> bootstrap+regtest passes?
To clarify; this subset of tests passes both with and without your patch
for PR119764 applied.
>
> -- >8 --
>
> Currently, pruned lambda captures are still leftover in the function's
> BLOCK and topmost BIND_EXPR; this doesn't cause any issues for normal
> compilation, but does break modules streaming as we try to reconstruct a
> FIELD_DECL that no longer exists on the type itself.
>
> PR c++/119755
>
> gcc/cp/ChangeLog:
>
> * lambda.cc (prune_lambda_captures): Remove pruned capture from
> function's BLOCK_VARS and BIND_EXPR_VARS.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/lambda-10_a.H: New test.
> * g++.dg/modules/lambda-10_b.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> Reviewed-by: Jason Merrill <ja...@redhat.com>
> ---
> gcc/cp/lambda.cc | 24 ++++++++++++++++++++++
> gcc/testsuite/g++.dg/modules/lambda-10_a.H | 17 +++++++++++++++
> gcc/testsuite/g++.dg/modules/lambda-10_b.C | 7 +++++++
> 3 files changed, 48 insertions(+)
> create mode 100644 gcc/testsuite/g++.dg/modules/lambda-10_a.H
> create mode 100644 gcc/testsuite/g++.dg/modules/lambda-10_b.C
>
> diff --git a/gcc/cp/lambda.cc b/gcc/cp/lambda.cc
> index f0a54b60275..b2e0ecdd67e 100644
> --- a/gcc/cp/lambda.cc
> +++ b/gcc/cp/lambda.cc
> @@ -1858,6 +1858,13 @@ prune_lambda_captures (tree body)
>
> cp_walk_tree_without_duplicates (&body, mark_const_cap_r, &const_vars);
>
> + tree bind_expr = expr_single (DECL_SAVED_TREE (lambda_function (lam)));
> + if (bind_expr && TREE_CODE (bind_expr) == MUST_NOT_THROW_EXPR)
> + bind_expr = expr_single (TREE_OPERAND (bind_expr, 0));
> + /* FIXME: We don't currently handle noexcept lambda captures correctly,
> + so bind_expr may not be set; see PR c++/119764. */
> + gcc_assert (!bind_expr || TREE_CODE (bind_expr) == BIND_EXPR);
> +
> tree *fieldp = &TYPE_FIELDS (LAMBDA_EXPR_CLOSURE (lam));
> for (tree *capp = &LAMBDA_EXPR_CAPTURE_LIST (lam); *capp; )
> {
> @@ -1879,6 +1886,23 @@ prune_lambda_captures (tree body)
> fieldp = &DECL_CHAIN (*fieldp);
> *fieldp = DECL_CHAIN (*fieldp);
>
> + /* And out of the bindings for the function. */
> + tree *blockp = &BLOCK_VARS (current_binding_level->blocks);
> + while (*blockp != DECL_EXPR_DECL (**use))
> + blockp = &DECL_CHAIN (*blockp);
> + *blockp = DECL_CHAIN (*blockp);
> +
> + /* And maybe out of the vars declared in the containing
> + BIND_EXPR, if it's listed there. */
> + if (bind_expr)
> + {
> + tree *bindp = &BIND_EXPR_VARS (bind_expr);
> + while (*bindp && *bindp != DECL_EXPR_DECL (**use))
> + bindp = &DECL_CHAIN (*bindp);
> + if (*bindp)
> + *bindp = DECL_CHAIN (*bindp);
> + }
> +
> /* And remove the capture proxy declaration. */
> **use = void_node;
> continue;
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-10_a.H
> b/gcc/testsuite/g++.dg/modules/lambda-10_a.H
> new file mode 100644
> index 00000000000..1ad1a808c07
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-10_a.H
> @@ -0,0 +1,17 @@
> +// PR c++/119755
> +// { dg-additional-options "-fmodule-header" }
> +// { dg-module-cmi {} }
> +
> +template <typename _Out> void format(_Out) {
> + constexpr int __term = 1;
> + [&] { __term; };
> + [&] { const int outer = __term; { __term; } };
> + [&]() noexcept { __term; };
> + [&]() noexcept { const int outer = __term; { __term; } };
> + [&](auto) { int n[__term]; }(0);
> + [&](auto) noexcept { int n[__term]; }(0);
> +}
> +
> +inline void vformat() {
> + format(0);
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-10_b.C
> b/gcc/testsuite/g++.dg/modules/lambda-10_b.C
> new file mode 100644
> index 00000000000..3556bcee02c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-10_b.C
> @@ -0,0 +1,7 @@
> +// PR c++/119755
> +// { dg-additional-options "-fmodules" }
> +
> +import "lambda-10_a.H";
> +int main() {
> + vformat();
> +}
> --
> 2.47.0
>
