On Tue, May 13, 2025 at 12:40:30PM -0400, Jason Merrill wrote:
> On 5/9/25 11:48 AM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/15?
> >
> > One slight concern I have is why we end up in 'maybe_thunk_body' to
> > start with: the imported constructor isn't DECL_ONE_ONLY (as its
> > external) and so 'can_alias_cdtor' returns false.
>
> That does seem like a problem; can_alias_cdtor shouldn't change due to
> explicit instantiation.
>
> > The change in
> > write_function_def (which I believe is necessary regardless) hides this
> > because we never actually emit the function definitions, but I worry
> > that this might somehow affect LTO, though I haven't been able to
> > construct a testcase which fails. To avoid the potential of that we
> > could do something like this to mark such functions as DECL_ONE_ONLY
> > just in case; thoughts?
> >
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index e7782627a49..c96e81aceef 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -16738,9 +16738,15 @@ module_state::read_cluster (unsigned snum)
> > /* Make sure we emit explicit instantiations.
> > FIXME do we want to do this in expand_or_defer_fn instead? */
> > - if (DECL_EXPLICIT_INSTANTIATION (decl)
> > - && !DECL_EXTERNAL (decl))
> > - setup_explicit_instantiation_definition_linkage (decl);
> > + if (DECL_EXPLICIT_INSTANTIATION (decl))
> > + {
> > + if (DECL_DECLARED_INLINE_P (decl)
> > + && TREE_PUBLIC (decl)
> > + && DECL_MAYBE_IN_CHARGE_CDTOR_P (decl))
> > + maybe_make_one_only (decl);
>
> This seems fragile, we shouldn't need to reproduce this specific behavior
> here.
>
Agreed.
> How is this case different from an extern template in a single TU?
>
It looks like the difference is that in a single TU, an extern template
is never marked as DECL_WEAK, and so in can_alias_cdtor we check
DECL_INTERFACE_KNOWN && !DECL_ONE_ONLY instead. In the modules case,
the explicit instantiation definition is marked as DECL_WEAK by
make_decl_one_only (in setup_explicit_instantiation_definition_linkage),
which we inherit.
So perhaps we should additionally clear DECL_WEAK on export for explicit
instantiations? Or maybe DECL_WEAK should only be set when we import
the definition, as with DECL_NOT_REALLY_EXTERN. I'm not sure if any
other of the visibility flags will also need special treatment (maybe
DECL_COMDAT?).
> > + if (!DECL_EXTERNAL (decl))
> > + setup_explicit_instantiation_definition_linkage (decl);
> > + }
> > if (abstract)
> > ;
> >
> >
> > This bug also affects 14 after r14-11743-g6d5a6a26e28d15; a minimal fix
> > for the ICE that seems straight-forwardly correct is to just do the
> > optimize.cc hunk, and I think that might be more appropriate given we
> > didn't handle explicit instantiations specially then anyway. Would such
> > a change (with appropriate xfailed tests) be OK?
>
> For 14, yes.
>
> > Maybe this would even be better for 15 also?
>
> I think go with this full patch for 15 and trunk. (OK)
>
Thanks. Here's what I pushed for r14-11775-g6b1a99c3e2cf0f:
-- >8 --
Subject: [PATCH] c++/modules: Fix handling of -fdeclone-ctor-dtor with
explicit instantiations [PR120125]
The attached testcase ICEs in maybe_thunk_body because we haven't
created a node in the cgraph for an imported explicit instantiation yet.
We in fact really shouldn't be emitting calls at all, since an imported
explicit instantiation always exists in the TU we imported it from. But
the required logic for that doesn't exist in this branch, so we'll just
xfail the relevant check.
PR c++/120125
gcc/cp/ChangeLog:
* optimize.cc (maybe_thunk_body): Don't assume 'fn' has a cgraph
node created.
gcc/testsuite/ChangeLog:
* g++.dg/modules/clone-4_a.C: New test.
* g++.dg/modules/clone-4_b.C: New test.
Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
Reviewed-by: Jason Merrill <ja...@redhat.com>
---
gcc/cp/optimize.cc | 4 ++--
gcc/testsuite/g++.dg/modules/clone-4_a.C | 12 ++++++++++++
gcc/testsuite/g++.dg/modules/clone-4_b.C | 12 ++++++++++++
3 files changed, 26 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/clone-4_a.C
create mode 100644 gcc/testsuite/g++.dg/modules/clone-4_b.C
diff --git a/gcc/cp/optimize.cc b/gcc/cp/optimize.cc
index 8429d856728..e8e707516d6 100644
--- a/gcc/cp/optimize.cc
+++ b/gcc/cp/optimize.cc
@@ -309,8 +309,8 @@ maybe_thunk_body (tree fn, bool force)
defer_mangling_aliases = save_defer_mangling_aliases;
cgraph_node::get_create (fns[0])->set_comdat_group (comdat_group);
cgraph_node::get_create (fns[1])->add_to_same_comdat_group
- (cgraph_node::get_create (fns[0]));
- symtab_node::get (fn)->add_to_same_comdat_group
+ (cgraph_node::get (fns[0]));
+ symtab_node::get_create (fn)->add_to_same_comdat_group
(symtab_node::get (fns[0]));
if (fns[2])
/* If *[CD][12]* dtors go into the *[CD]5* comdat group and dtor is
diff --git a/gcc/testsuite/g++.dg/modules/clone-4_a.C
b/gcc/testsuite/g++.dg/modules/clone-4_a.C
new file mode 100644
index 00000000000..bd74f588c7e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/clone-4_a.C
@@ -0,0 +1,12 @@
+// PR c++/120125
+// { dg-additional-options "-fmodules-ts -fdeclone-ctor-dtor" }
+// { dg-module-cmi M }
+
+export module M;
+
+void foo();
+export template <typename _Tp> struct __shared_ptr {
+ inline __shared_ptr() { foo(); }
+};
+
+template class __shared_ptr<int>;
diff --git a/gcc/testsuite/g++.dg/modules/clone-4_b.C
b/gcc/testsuite/g++.dg/modules/clone-4_b.C
new file mode 100644
index 00000000000..0487dc2e395
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/clone-4_b.C
@@ -0,0 +1,12 @@
+// PR c++/120125
+// { dg-additional-options "-fmodules-ts -fdeclone-ctor-dtor" }
+
+import M;
+
+int main() {
+ __shared_ptr<int> s1;
+ __shared_ptr<double> s2;
+}
+
+// { dg-final { scan-assembler-not {_ZNW1M12__shared_ptrIiEC[1-4]Ev:} { xfail
*-*-* } } }
+// { dg-final { scan-assembler {_ZNW1M12__shared_ptrIdEC2Ev:} } }
--
2.47.0
