On Fri, 4 Apr 2025, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>
> -- >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).
Interesting! This reminds me of PR115159, which was worked around by
preventing PARM_DECL from leaking to another declaration through a
function type's noexcept-spec.
>
> 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);
> +}
> --
> 2.47.0
>
>
