On Thu, Jan 09, 2025 at 05:41:07PM -0500, Patrick Palka wrote:
> 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.
>
Happy to use copy_decl instead; I'll retest tonight and push tomorrow if
there's no issues. Thanks!
> > + }
> > +
> > *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
> >
> >
>
