Re: [PATCH] c++/modules: Fix divergence in streaming/non-streaming tree walks [PR119608]

[Jason Merrill]

On 4/3/25 9:09 PM, Nathaniel Shead wrote:

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

OK.

-- >8 --

Modules streaming walks decls multiple times, first as a non-streaming
walk to find dependencies, and then later to actually emit the decls.
The first walk needs to be done to note locations that will be emitted.

In the PR we are getting a checking ICE because we are streaming a decl
that we didn't initially walk when collecting dependencies, so the
location isn't in the noted locations map.  This is because in decl_node
we have a branch where a PARM_DECL that hasn't previously been
referenced gets walked by value only if 'streaming_p ()' is true.

The true root cause here is that the decltype(v) in the testcase refers
to a different PARM_DECL from the one in the declaration that we're
streaming; it's the PARM_DECL from the initial forward-declaration, that
we're not streaming.  A proper fix would be to ensure that it gets
remapped to the decl in the definition we're actually emitting, but for
now this workaround fixes the bug (and any other bugs that might
manifest similarly).

        PR c++/119608

gcc/cp/ChangeLog:

        * module.cc (trees_out::decl_node): Maybe require by-value
        walking not just when streaming.

gcc/testsuite/ChangeLog:

        * g++.dg/modules/pr119608_a.C: New test.
        * g++.dg/modules/pr119608_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
---
  gcc/cp/module.cc                          | 39 ++++++++++++-----------
  gcc/testsuite/g++.dg/modules/pr119608_a.C | 16 ++++++++++
  gcc/testsuite/g++.dg/modules/pr119608_b.C |  8 +++++
  3 files changed, 45 insertions(+), 18 deletions(-)
  create mode 100644 gcc/testsuite/g++.dg/modules/pr119608_a.C
  create mode 100644 gcc/testsuite/g++.dg/modules/pr119608_b.C

diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index ce22b2ece3f..f60d7cb8e12 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -9004,25 +9004,28 @@ trees_out::decl_node (tree decl, walk_kind ref)
        if (streaming_p ())
          i (tt_parm);
        tree_node (DECL_CONTEXT (decl));
-       if (streaming_p ())
-         {
-           /* That must have put this in the map.  */
-           walk_kind ref = ref_node (decl);
-           if (ref != WK_none)
-             // FIXME:OPTIMIZATION We can wander into bits of the
-             // template this was instantiated from.  For instance
-             // deferred noexcept and default parms.  Currently we'll
-             // end up cloning those bits of tree.  It would be nice
-             // to reference those specific nodes.  I think putting
-             // those things in the map when we reference their
-             // template by name.  See the note in add_indirects.
-             return true;

- dump (dumper::TREE)

-             && dump ("Wrote %s reference %N",
-                      TREE_CODE (decl) == PARM_DECL ? "parameter" : "result",
-                      decl);
-         }
+       /* That must have put this in the map.  */
+       walk_kind ref = ref_node (decl);
+       if (ref != WK_none)
+         // FIXME:OPTIMIZATION We can wander into bits of the
+         // template this was instantiated from, for instance
+         // deferred noexcept and default parms, or references
+         // to parms from earlier forward-decls (PR c++/119608).
+         //
+         // Currently we'll end up cloning those bits of tree.
+         // It would be nice to reference those specific nodes.
+         // I think putting those things in the map when we
+         // reference their template by name.
+         //
+         // See the note in add_indirects.
+         return true;
+
+       if (streaming_p ())
+         dump (dumper::TREE)
+           && dump ("Wrote %s reference %N",
+                    TREE_CODE (decl) == PARM_DECL ? "parameter" : "result",
+                    decl);
        }
        return false;

diff --git a/gcc/testsuite/g++.dg/modules/pr119608_a.C b/gcc/testsuite/g++.dg/modules/pr119608_a.C

new file mode 100644
index 00000000000..4e7b359ac58
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr119608_a.C
@@ -0,0 +1,16 @@
+// PR c++/119608
+// { dg-additional-options "-fmodules" }
+// { dg-module-cmi repro }
+
+export module repro;
+
+export template<class Visitor>
+auto
+visit(
+  Visitor v
+) -> decltype(
+  v);
+
+export template<class Visitor> auto visit(Visitor v) -> decltype(v) {
+  return {};
+}
diff --git a/gcc/testsuite/g++.dg/modules/pr119608_b.C 
b/gcc/testsuite/g++.dg/modules/pr119608_b.C
new file mode 100644
index 00000000000..023d20adf4c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr119608_b.C
@@ -0,0 +1,8 @@
+// PR c++/119608
+// { dg-additional-options "-fmodules" }
+
+import repro;
+
+int main() {
+  visit(123);
+}