On Fri, 10 Jan 2025, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14?
Nice approach, thanks for fixing this!
>
> -- >8 --
>
> In the linked testcase, an ICE occurs because when reading the
> (duplicate) function definition for _M_do_parse from module Y, the local
> type definitions have already been streamed from module X and setup as
> regular backreferences, rather than being found with find_duplicate,
> causing issues with managing DECL_CHAIN.
>
> It is tempting to just skip setting up the DECL_CHAIN for this case.
> However, for the future it would be best to ensure that the block vars
> for the duplicate definition are accurate, so that we could implement
> ODR checking on function definitions at some point.
>
> So to solve this, this patch creates a copy of the streamed-in local
> type and chains that; it will be discarded along with the rest of the
> duplicate function after we've finished processing.
>
> A couple of suggested implementations from the discussion on the PR that
> don't work:
>
> - Replacing the `DECL_CHAIN` assertion with `(*chain && *chain != decl)`
> doesn't handle the case where type definitions are followed by regular
> local variables, since those won't have been imported as separate
> backreferences and so the chains will diverge.
>
> - Correcting the purviewness of GMF template instantiations to force Y
> to emit copies of the local types rather than backreferences into X is
> insufficient, as it's still possible that the local types got streamed
> in a separate cluster to the function definition, and so will be again
> referred to via regular backreferences when importing.
>
> - Likewise, preventing the emission of function definitions where an
> import has already provided that same definition also is insufficient,
> for much the same reason.
>
> PR c++/114630
>
> gcc/cp/ChangeLog:
>
> * module.cc (trees_in::core_vals) <BLOCK>: Chain a new node if
> DECL_CHAIN already is set.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/modules/pr114630.h: New test.
> * g++.dg/modules/pr114630_a.C: New test.
> * g++.dg/modules/pr114630_b.C: New test.
> * g++.dg/modules/pr114630_c.C: New test.
>
> Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> ---
> gcc/cp/module.cc | 14 +++++++++++++-
> gcc/testsuite/g++.dg/modules/pr114630.h | 11 +++++++++++
> gcc/testsuite/g++.dg/modules/pr114630_a.C | 7 +++++++
> gcc/testsuite/g++.dg/modules/pr114630_b.C | 8 ++++++++
> gcc/testsuite/g++.dg/modules/pr114630_c.C | 4 ++++
> 5 files changed, 43 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630.h
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_a.C
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_b.C
> create mode 100644 gcc/testsuite/g++.dg/modules/pr114630_c.C
>
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 5350e6c4bad..ff2683de73e 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -6928,11 +6928,23 @@ trees_in::core_vals (tree t)
> body) anyway. */
> decl = maybe_duplicate (decl);
>
> - if (!DECL_P (decl) || DECL_CHAIN (decl))
> + if (!DECL_P (decl))
> {
> set_overrun ();
> break;
> }
> +
> + /* If DECL_CHAIN is already set then this was a backreference to a
> + local type or enumerator from a previous read (PR c++/114630).
> + Let's copy the node so we can keep building the chain for ODR
> + checking later. */
> + if (DECL_CHAIN (decl))
> + {
> + gcc_checking_assert (TREE_CODE (decl) == TYPE_DECL
> + && find_duplicate (DECL_CONTEXT (decl)));
> + decl = copy_node (decl);
Shall we use copy_decl here instead so that any DECL_LANG_SPECIFIC node
is copied as well? IIUC we usually don't share DECL_LANG_SPECIFIC
between decls.
> + }
> +
> *chain = decl;
> chain = &DECL_CHAIN (decl);
> }
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630.h
> b/gcc/testsuite/g++.dg/modules/pr114630.h
> new file mode 100644
> index 00000000000..8730007f59f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630.h
> @@ -0,0 +1,11 @@
> +template <typename _CharT>
> +void _M_do_parse() {
> + struct A {};
> + struct B {};
> + int x;
> +}
> +
> +template <typename> struct formatter;
> +template <> struct formatter<int> {
> + void parse() { _M_do_parse<int>(); }
> +};
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_a.C
> b/gcc/testsuite/g++.dg/modules/pr114630_a.C
> new file mode 100644
> index 00000000000..ecfd7ca0b28
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_a.C
> @@ -0,0 +1,7 @@
> +// { dg-additional-options "-fmodules" }
> +// { dg-module-cmi X }
> +
> +module;
> +#include "pr114630.h"
> +export module X;
> +formatter<int> a;
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_b.C
> b/gcc/testsuite/g++.dg/modules/pr114630_b.C
> new file mode 100644
> index 00000000000..52fe04e2ce0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_b.C
> @@ -0,0 +1,8 @@
> +// { dg-additional-options "-fmodules" }
> +// { dg-module-cmi Y }
> +
> +module;
> +#include "pr114630.h"
> +export module Y;
> +import X;
> +formatter<int> b;
> diff --git a/gcc/testsuite/g++.dg/modules/pr114630_c.C
> b/gcc/testsuite/g++.dg/modules/pr114630_c.C
> new file mode 100644
> index 00000000000..54a21f08057
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr114630_c.C
> @@ -0,0 +1,4 @@
> +// { dg-additional-options "-fmodules" }
> +
> +#include "pr114630.h"
> +import Y;
> --
> 2.47.0
>
>
