c++/modules: ICE with lambda initializing local var [PR105322]

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?

-- >8 --

For a local variable initialized by a lambda:

  auto f = []{};

The corresponding BLOCK_VARS contains the variable declaration first,
followed by the closure type declaration, consistent with the
syntactical order.  This however means that a use of the closure type
appears (in the variable type/initializer) before the declaration of the
type.  This ends up causing an ICE when streaming the BLOCK_VARS of f1
below because we stream (by value) the CONSTRUCTOR initializer of g1 --
which contains components of the closure type -- before we've streamed
the declaration defining the closure type.  The following comment in
module.cc seems relevant:

  /* We want to stream the type of a expression-like nodes /after/
     we've streamed the operands.  The type often contains (bits
     of the) types of the operands, and with things like decltype
     and noexcept in play, we really want to stream the decls
     defining the type before we try and stream the type on its
     own.  Otherwise we can find ourselves trying to read in a
     decl, when we're already partially reading in a component of
     its type.  And that's bad.  */

This patch narrowly fixes this issue by special casing closure type
declarations in add_decl_to_level.  (A loop is needed since there could
be multiple variable declarations with an unprocessed initializer in
light of structured bindings.)

	PR c++/105322

gcc/cp/ChangeLog:

	* name-lookup.cc (add_decl_to_level): When adding a closure
	type declaration to a block scope, add it before rather than
	after any variable declarations whose initializer we're still
	processing.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/lambda-5_a.C: New test.
	* g++.dg/modules/lambda-5_b.C: New test.
---
 gcc/cp/name-lookup.cc                     | 19 ++++++++++++++++---
 gcc/testsuite/g++.dg/modules/lambda-5_a.C | 23 +++++++++++++++++++++++
 gcc/testsuite/g++.dg/modules/lambda-5_b.C | 10 ++++++++++
 3 files changed, 49 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/lambda-5_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/lambda-5_b.C

On Wed, 18 Oct 2023, Patrick Palka wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?

Note that this doesn't fix the other testcase in the PR, which doesn't use any
lambdas and which ICEs in the same way:

    export module pr105322;

    auto f() {
      struct A { int m; };
      return A{};
    }

    export
    inline void g() {
      auto r = decltype(f()){0};
    }

Here when streaming the CONSTRUCTOR initializer of r, we end up streaming
components of f()::A before ever streaming the declaration/definition of
f()::A.  I suspect a separate fix might be needed for this testcase?
The narrow fix for the lambda testcase still seems useful nonetheless.

> 
> -- >8 --
> 
> For a local variable initialized by a lambda:
> 
>   auto f = []{};
> 
> The corresponding BLOCK_VARS contains the variable declaration first,
> followed by the closure type declaration, consistent with the
> syntactical order.  This however means that a use of the closure type
> appears (in the variable type/initializer) before the declaration of the
> type.  This ends up causing an ICE when streaming the BLOCK_VARS of f1
> below because we stream (by value) the CONSTRUCTOR initializer of g1 --
> which contains components of the closure type -- before we've streamed
> the declaration defining the closure type.  The following comment in
> module.cc seems relevant:
> 
>   /* We want to stream the type of a expression-like nodes /after/
>      we've streamed the operands.  The type often contains (bits
>      of the) types of the operands, and with things like decltype
>      and noexcept in play, we really want to stream the decls
>      defining the type before we try and stream the type on its
>      own.  Otherwise we can find ourselves trying to read in a
>      decl, when we're already partially reading in a component of
>      its type.  And that's bad.  */
> 
> This patch narrowly fixes this issue by special casing closure type
> declarations in add_decl_to_level.  (A loop is needed since there could
> be multiple variable declarations with an unprocessed initializer in
> light of structured bindings.)
> 
> 	PR c++/105322
> 
> gcc/cp/ChangeLog:
> 
> 	* name-lookup.cc (add_decl_to_level): When adding a closure
> 	type declaration to a block scope, add it before rather than
> 	after any variable declarations whose initializer we're still
> 	processing.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/lambda-5_a.C: New test.
> 	* g++.dg/modules/lambda-5_b.C: New test.
> ---
>  gcc/cp/name-lookup.cc                     | 19 ++++++++++++++++---
>  gcc/testsuite/g++.dg/modules/lambda-5_a.C | 23 +++++++++++++++++++++++
>  gcc/testsuite/g++.dg/modules/lambda-5_b.C | 10 ++++++++++
>  3 files changed, 49 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/lambda-5_a.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/lambda-5_b.C
> 
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index a8b9229b29e..bb00baaf9f4 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -391,9 +391,22 @@ add_decl_to_level (cp_binding_level *b, tree decl)
>    gcc_assert (b->names != decl);
>  
>    /* We build up the list in reverse order, and reverse it later if
> -     necessary.  */
> -  TREE_CHAIN (decl) = b->names;
> -  b->names = decl;
> +     necessary.  If we're adding a lambda closure type to a block
> +     scope as part of a local variable initializer, then make sure
> +     we declare the type before the variable; modules expects that
> +     we see a type declaration before a use of the type.  */
> +  tree *prev = &b->names;
> +  if (b->kind == sk_block
> +      && !processing_template_decl
> +      && TREE_CODE (decl) == TYPE_DECL
> +      && LAMBDA_TYPE_P (TREE_TYPE (decl)))
> +    while (*prev && VAR_P (*prev)
> +	   && !DECL_EXTERNAL (*prev)
> +	   && !DECL_INITIALIZED_P (*prev))
> +      prev = &TREE_CHAIN (*prev);
> +
> +  TREE_CHAIN (decl) = *prev;
> +  *prev = decl;
>  
>    /* If appropriate, add decl to separate list of statics.  We include
>       extern variables because they might turn out to be static later.
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-5_a.C b/gcc/testsuite/g++.dg/modules/lambda-5_a.C
> new file mode 100644
> index 00000000000..6b54c8e3173
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-5_a.C
> @@ -0,0 +1,23 @@
> +// PR c++/105322
> +// { dg-additional-options -fmodules-ts }
> +// { dg-module-cmi pr105322 }
> +
> +export module pr105322;
> +
> +struct A { };
> +
> +export
> +inline void f1() {
> +  A a;
> +  auto g1 = [a] { }; // used to ICE here during stream out
> +}
> +
> +export
> +template<class...>
> +void f2() {
> +  A a;
> +  auto g2 = [a] { };
> +}
> +
> +export
> +inline auto g3 = [a=A{}] { };
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-5_b.C b/gcc/testsuite/g++.dg/modules/lambda-5_b.C
> new file mode 100644
> index 00000000000..e25a913b726
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-5_b.C
> @@ -0,0 +1,10 @@
> +// PR c++/105322
> +// { dg-additional-options -fmodules-ts }
> +
> +import pr105322;
> +
> +int main() {
> +  f1();
> +  f2();
> +  g3();
> +}
> -- 
> 2.42.0.398.ga9ecda2788
> 
>

Thanks for looking at this, but your patch is essentially papering over the problem.

It took me a while to figure out, but the clue was that things like 
'decltype(f()).m' worked, but 'decltype(f()){0}' did not.  The CONSTRUCTOR node 
is the exception to the rule that required an expression node's type to be 
streamed after the node's operands.  We want the opposite for CTORS.

I'll commit this once bootstrapped.

nathan

On 10/18/23 12:28, Patrick Palka wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
> 
> -- >8 --
> 
> For a local variable initialized by a lambda:
> 
>    auto f = []{};
> 
> The corresponding BLOCK_VARS contains the variable declaration first,
> followed by the closure type declaration, consistent with the
> syntactical order.  This however means that a use of the closure type
> appears (in the variable type/initializer) before the declaration of the
> type.  This ends up causing an ICE when streaming the BLOCK_VARS of f1
> below because we stream (by value) the CONSTRUCTOR initializer of g1 --
> which contains components of the closure type -- before we've streamed
> the declaration defining the closure type.  The following comment in
> module.cc seems relevant:
> 
>    /* We want to stream the type of a expression-like nodes /after/
>       we've streamed the operands.  The type often contains (bits
>       of the) types of the operands, and with things like decltype
>       and noexcept in play, we really want to stream the decls
>       defining the type before we try and stream the type on its
>       own.  Otherwise we can find ourselves trying to read in a
>       decl, when we're already partially reading in a component of
>       its type.  And that's bad.  */
> 
> This patch narrowly fixes this issue by special casing closure type
> declarations in add_decl_to_level.  (A loop is needed since there could
> be multiple variable declarations with an unprocessed initializer in
> light of structured bindings.)
> 
> 	PR c++/105322
> 
> gcc/cp/ChangeLog:
> 
> 	* name-lookup.cc (add_decl_to_level): When adding a closure
> 	type declaration to a block scope, add it before rather than
> 	after any variable declarations whose initializer we're still
> 	processing.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/lambda-5_a.C: New test.
> 	* g++.dg/modules/lambda-5_b.C: New test.
> ---
>   gcc/cp/name-lookup.cc                     | 19 ++++++++++++++++---
>   gcc/testsuite/g++.dg/modules/lambda-5_a.C | 23 +++++++++++++++++++++++
>   gcc/testsuite/g++.dg/modules/lambda-5_b.C | 10 ++++++++++
>   3 files changed, 49 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/lambda-5_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/lambda-5_b.C
> 
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index a8b9229b29e..bb00baaf9f4 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -391,9 +391,22 @@ add_decl_to_level (cp_binding_level *b, tree decl)
>     gcc_assert (b->names != decl);
>   
>     /* We build up the list in reverse order, and reverse it later if
> -     necessary.  */
> -  TREE_CHAIN (decl) = b->names;
> -  b->names = decl;
> +     necessary.  If we're adding a lambda closure type to a block
> +     scope as part of a local variable initializer, then make sure
> +     we declare the type before the variable; modules expects that
> +     we see a type declaration before a use of the type.  */
> +  tree *prev = &b->names;
> +  if (b->kind == sk_block
> +      && !processing_template_decl
> +      && TREE_CODE (decl) == TYPE_DECL
> +      && LAMBDA_TYPE_P (TREE_TYPE (decl)))
> +    while (*prev && VAR_P (*prev)
> +	   && !DECL_EXTERNAL (*prev)
> +	   && !DECL_INITIALIZED_P (*prev))
> +      prev = &TREE_CHAIN (*prev);
> +
> +  TREE_CHAIN (decl) = *prev;
> +  *prev = decl;
>   
>     /* If appropriate, add decl to separate list of statics.  We include
>        extern variables because they might turn out to be static later.
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-5_a.C b/gcc/testsuite/g++.dg/modules/lambda-5_a.C
> new file mode 100644
> index 00000000000..6b54c8e3173
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-5_a.C
> @@ -0,0 +1,23 @@
> +// PR c++/105322
> +// { dg-additional-options -fmodules-ts }
> +// { dg-module-cmi pr105322 }
> +
> +export module pr105322;
> +
> +struct A { };
> +
> +export
> +inline void f1() {
> +  A a;
> +  auto g1 = [a] { }; // used to ICE here during stream out
> +}
> +
> +export
> +template<class...>
> +void f2() {
> +  A a;
> +  auto g2 = [a] { };
> +}
> +
> +export
> +inline auto g3 = [a=A{}] { };
> diff --git a/gcc/testsuite/g++.dg/modules/lambda-5_b.C b/gcc/testsuite/g++.dg/modules/lambda-5_b.C
> new file mode 100644
> index 00000000000..e25a913b726
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/lambda-5_b.C
> @@ -0,0 +1,10 @@
> +// PR c++/105322
> +// { dg-additional-options -fmodules-ts }
> +
> +import pr105322;
> +
> +int main() {
> +  f1();
> +  f2();
> +  g3();
> +}

On Fri, 20 Oct 2023, Nathan Sidwell wrote:

> Thanks for looking at this, but your patch is essentially papering over the
> problem.
> 
> It took me a while to figure out, but the clue was that things like
> 'decltype(f()).m' worked, but 'decltype(f()){0}' did not.  The CONSTRUCTOR
> node is the exception to the rule that required an expression node's type to
> be streamed after the node's operands.  We want the opposite for CTORS.
> 
> I'll commit this once bootstrapped.

Very interesting, thanks a lot!  It's awesome that both testcases could
be fixed simultaneously after all :)

> 
> nathan
> 
> On 10/18/23 12:28, Patrick Palka wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?
> > 
> > -- >8 --
> > 
> > For a local variable initialized by a lambda:
> > 
> >    auto f = []{};
> > 
> > The corresponding BLOCK_VARS contains the variable declaration first,
> > followed by the closure type declaration, consistent with the
> > syntactical order.  This however means that a use of the closure type
> > appears (in the variable type/initializer) before the declaration of the
> > type.  This ends up causing an ICE when streaming the BLOCK_VARS of f1
> > below because we stream (by value) the CONSTRUCTOR initializer of g1 --
> > which contains components of the closure type -- before we've streamed
> > the declaration defining the closure type.  The following comment in
> > module.cc seems relevant:
> > 
> >    /* We want to stream the type of a expression-like nodes /after/
> >       we've streamed the operands.  The type often contains (bits
> >       of the) types of the operands, and with things like decltype
> >       and noexcept in play, we really want to stream the decls
> >       defining the type before we try and stream the type on its
> >       own.  Otherwise we can find ourselves trying to read in a
> >       decl, when we're already partially reading in a component of
> >       its type.  And that's bad.  */
> > 
> > This patch narrowly fixes this issue by special casing closure type
> > declarations in add_decl_to_level.  (A loop is needed since there could
> > be multiple variable declarations with an unprocessed initializer in
> > light of structured bindings.)
> > 
> > 	PR c++/105322
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* name-lookup.cc (add_decl_to_level): When adding a closure
> > 	type declaration to a block scope, add it before rather than
> > 	after any variable declarations whose initializer we're still
> > 	processing.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/modules/lambda-5_a.C: New test.
> > 	* g++.dg/modules/lambda-5_b.C: New test.
> > ---
> >   gcc/cp/name-lookup.cc                     | 19 ++++++++++++++++---
> >   gcc/testsuite/g++.dg/modules/lambda-5_a.C | 23 +++++++++++++++++++++++
> >   gcc/testsuite/g++.dg/modules/lambda-5_b.C | 10 ++++++++++
> >   3 files changed, 49 insertions(+), 3 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/modules/lambda-5_a.C
> >   create mode 100644 gcc/testsuite/g++.dg/modules/lambda-5_b.C
> > 
> > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> > index a8b9229b29e..bb00baaf9f4 100644
> > --- a/gcc/cp/name-lookup.cc
> > +++ b/gcc/cp/name-lookup.cc
> > @@ -391,9 +391,22 @@ add_decl_to_level (cp_binding_level *b, tree decl)
> >     gcc_assert (b->names != decl);
> >       /* We build up the list in reverse order, and reverse it later if
> > -     necessary.  */
> > -  TREE_CHAIN (decl) = b->names;
> > -  b->names = decl;
> > +     necessary.  If we're adding a lambda closure type to a block
> > +     scope as part of a local variable initializer, then make sure
> > +     we declare the type before the variable; modules expects that
> > +     we see a type declaration before a use of the type.  */
> > +  tree *prev = &b->names;
> > +  if (b->kind == sk_block
> > +      && !processing_template_decl
> > +      && TREE_CODE (decl) == TYPE_DECL
> > +      && LAMBDA_TYPE_P (TREE_TYPE (decl)))
> > +    while (*prev && VAR_P (*prev)
> > +	   && !DECL_EXTERNAL (*prev)
> > +	   && !DECL_INITIALIZED_P (*prev))
> > +      prev = &TREE_CHAIN (*prev);
> > +
> > +  TREE_CHAIN (decl) = *prev;
> > +  *prev = decl;
> >       /* If appropriate, add decl to separate list of statics.  We include
> >        extern variables because they might turn out to be static later.
> > diff --git a/gcc/testsuite/g++.dg/modules/lambda-5_a.C
> > b/gcc/testsuite/g++.dg/modules/lambda-5_a.C
> > new file mode 100644
> > index 00000000000..6b54c8e3173
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/lambda-5_a.C
> > @@ -0,0 +1,23 @@
> > +// PR c++/105322
> > +// { dg-additional-options -fmodules-ts }
> > +// { dg-module-cmi pr105322 }
> > +
> > +export module pr105322;
> > +
> > +struct A { };
> > +
> > +export
> > +inline void f1() {
> > +  A a;
> > +  auto g1 = [a] { }; // used to ICE here during stream out
> > +}
> > +
> > +export
> > +template<class...>
> > +void f2() {
> > +  A a;
> > +  auto g2 = [a] { };
> > +}
> > +
> > +export
> > +inline auto g3 = [a=A{}] { };
> > diff --git a/gcc/testsuite/g++.dg/modules/lambda-5_b.C
> > b/gcc/testsuite/g++.dg/modules/lambda-5_b.C
> > new file mode 100644
> > index 00000000000..e25a913b726
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/lambda-5_b.C
> > @@ -0,0 +1,10 @@
> > +// PR c++/105322
> > +// { dg-additional-options -fmodules-ts }
> > +
> > +import pr105322;
> > +
> > +int main() {
> > +  f1();
> > +  f2();
> > +  g3();
> > +}
> 
> -- 
> Nathan Sidwell
>