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

[Nathan Sidwell <nathan@acm.org>]

[-- Attachment #1: Type: text/plain, Size: 5078 bytes --]

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();
> +}

-- 
Nathan Sidwell

[-- Attachment #2: 0001-c-Constructor-streaming-PR105322.patch --]
[-- Type: text/x-patch, Size: 5504 bytes --]

From fb65e961231943ecc68988f38a9ec78b0c93e5df Mon Sep 17 00:00:00 2001
From: Nathan Sidwell <nathan@acm.org>
Date: Fri, 20 Oct 2023 12:20:37 -0400
Subject: [PATCH] c++: Constructor streaming [PR105322]

An expresion node's type is streamed after the expression's operands,
because the type can come from some aspect of an operand (for instance
decltype and noexcept). There's a comment in the code explaining that.

But that doesn't work for constructors, which can directly reference
components of their type (eg FIELD_DECLS). If this is a
type-introducing CONSTRUCTOR, we need to ensure the type has been
streamed first. So move CONSTRUCTOR stream to after the type streaming.

The reason things like COMPONENT_REF work is that they stream their
first operand first, and that introduces the type that their second
operand looks up a field in.
---
 gcc/cp/module.cc                            | 58 ++++++++++++---------
 gcc/testsuite/g++.dg/modules/decltype-1_a.C | 28 ++++++++++
 gcc/testsuite/g++.dg/modules/decltype-1_b.C | 10 ++++
 gcc/testsuite/g++.dg/modules/lambda-5_a.C   | 24 +++++++++
 gcc/testsuite/g++.dg/modules/lambda-5_b.C   | 10 ++++
 5 files changed, 105 insertions(+), 25 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/decltype-1_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/decltype-1_b.C
 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/module.cc b/gcc/cp/module.cc
index bbb1e20b55b..539518d7923 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -6212,19 +6212,9 @@ trees_out::core_vals (tree t)
       break;
 
     case CONSTRUCTOR:
-      {
-	unsigned len = vec_safe_length (t->constructor.elts);
-	if (streaming_p ())
-	  WU (len);
-	if (len)
-	  for (unsigned ix = 0; ix != len; ix++)
-	    {
-	      const constructor_elt &elt = (*t->constructor.elts)[ix];
-
-	      WT (elt.index);
-	      WT (elt.value);
-	    }
-      }
+      // This must be streamed /after/ we've streamed the type,
+      // because it can directly refer to elements of the type. Eg,
+      // FIELD_DECLs of a RECORD_TYPE.
       break;
 
     case OMP_CLAUSE:
@@ -6458,6 +6448,21 @@ trees_out::core_vals (tree t)
 	WU (prec);
     }
 
+  if (TREE_CODE (t) == CONSTRUCTOR)
+    {
+      unsigned len = vec_safe_length (t->constructor.elts);
+      if (streaming_p ())
+	WU (len);
+      if (len)
+	for (unsigned ix = 0; ix != len; ix++)
+	  {
+	    const constructor_elt &elt = (*t->constructor.elts)[ix];
+
+	    WT (elt.index);
+	    WT (elt.value);
+	  }
+    }
+
 #undef WT
 #undef WU
 }
@@ -6717,18 +6722,7 @@ trees_in::core_vals (tree t)
       break;
 
     case CONSTRUCTOR:
-      if (unsigned len = u ())
-	{
-	  vec_alloc (t->constructor.elts, len);
-	  for (unsigned ix = 0; ix != len; ix++)
-	    {
-	      constructor_elt elt;
-
-	      RT (elt.index);
-	      RTU (elt.value);
-	      t->constructor.elts->quick_push (elt);
-	    }
-	}
+      // Streamed after the node's type.
       break;
 
     case OMP_CLAUSE:
@@ -6901,6 +6895,20 @@ trees_in::core_vals (tree t)
 	t->typed.type = type;
     }
 
+  if (TREE_CODE (t) == CONSTRUCTOR)
+    if (unsigned len = u ())
+      {
+	vec_alloc (t->constructor.elts, len);
+	for (unsigned ix = 0; ix != len; ix++)
+	  {
+	    constructor_elt elt;
+
+	    RT (elt.index);
+	    RTU (elt.value);
+	    t->constructor.elts->quick_push (elt);
+	  }
+      }
+
 #undef RT
 #undef RM
 #undef RU
diff --git a/gcc/testsuite/g++.dg/modules/decltype-1_a.C b/gcc/testsuite/g++.dg/modules/decltype-1_a.C
new file mode 100644
index 00000000000..ca66e8b598a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/decltype-1_a.C
@@ -0,0 +1,28 @@
+// PR c++/105322
+// { dg-module-do link
+// { dg-additional-options -fmodules-ts }
+// { dg-module-cmi pr105322.Decltype }
+
+export module pr105322.Decltype;
+
+auto f() {
+  struct A { int m;
+    int get () { return m; }
+  };
+  return A{};
+}
+
+export
+inline void g1() {
+  auto r = decltype(f()){0};
+}
+
+export
+inline void g2() {
+  auto r = f().m;
+}
+
+export
+inline void g3() {
+  auto r = f().get();
+}
diff --git a/gcc/testsuite/g++.dg/modules/decltype-1_b.C b/gcc/testsuite/g++.dg/modules/decltype-1_b.C
new file mode 100644
index 00000000000..6bebe13604f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/decltype-1_b.C
@@ -0,0 +1,10 @@
+// PR c++/105322
+// { dg-additional-options -fmodules-ts }
+
+import pr105322.Decltype;
+
+int main() {
+  g1();
+  g2();
+  g3();
+}
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..6b589d4965c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lambda-5_a.C
@@ -0,0 +1,24 @@
+// PR c++/105322
+// { dg-module-do link
+// { dg-additional-options -fmodules-ts }
+// { dg-module-cmi pr105322.Lambda }
+
+export module pr105322.Lambda;
+
+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..a7ce7098835
--- /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.Lambda;
+
+int main() {
+  f1();
+  f2();
+  g3();
+}
-- 
2.41.0