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
From fb65e961231943ecc68988f38a9ec78b0c93e5df Mon Sep 17 00:00:00 2001
From: Nathan Sidwell <nat...@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
