On 3/28/25 8:54 AM, Nathaniel Shead wrote:
On Thu, Mar 27, 2025 at 10:38:02AM -0400, Jason Merrill wrote:
On 3/27/25 3:35 AM, Nathaniel Shead wrote:
Bootstrapped and regtested (so far just dg.exp and modules.exp) on
x86_64-pc-linux-gnu, OK for trunk if full regtest succeeds?
Rather than updating copy_fndecl_with_name, we could also just fix
modules specifically by overwriting DECL_ABSTRACT_P before calling
build_cdtor_clones in trees_in::decl_value, or by forcing it to 0 for
DECL_MAYBE_IN_CHARGE_CDTOR during tree streaming, if you prefer, since
it'll always be set again by expand_or_defer_fn anyway.
-- >8 --
This patch makes some adjustments required to get a simple modules
testcase working with LTO. There are two main issues fixed.
Firstly, modules only streams the maybe-in-charge constructor, and any
clones are recreated on stream-in. These clones are copied from the
existing function decl and then adjusted. This caused issues because
the clones were getting incorrectly marked as abstract, since after
clones have been created (in the imported file) the maybe-in-charge decl
gets marked as abstract. So this patch just ensures that clones are
always created as non-abstract.
Sounds good.
The second issue is that we need to explicitly tell cgraph that explicit
instantiations need to be emitted, otherwise LTO will elide them (as
they don't necessarily appear to be used directly) and cause link
errors.
Makes sense. Maybe you want to check get_importer_interface == always_emit
instead of specifically for explicit inst?
...except I see that it returns that value for internal decls, which don't
actually always need to be emitted; there seems to be a missing distinction
between "considered to be defined in this TU" and actually "always emit".
Right; I've reworked this function a little for future-proofing, but I
didn't end up using it, so I'll send that as a separate change.
Additionally, expand_or_defer_fn doesn't setup comdat groups
for explicit instantiations, so we need to do that here as well.
Hmm, that inconsistency seems worth fixing in expand_or_defer_fn, though
it's fine to leave that for later and just add a FIXME comment to your
change.
Added a comment. That existing logic makes sense since the only place
that explicit instantiations can occur is 'mark_decl_instantiated' which
does all the linkage handling itself, but maybe it would be reasonable
to generalise in the future; I've split out the relevant parts of that
function to reduce code duplication at least for now.
PR c++/118961
gcc/cp/ChangeLog:
* class.cc (copy_fndecl_with_name): Mark clones as non-abstract.
* module.cc (trees_in::read_var_def): Explicit instantiation
definitions of variables must be emitted, and are COMDAT.
(module_state::read_cluster): Likewise for functions.
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 2cded878c64..f9f48bb2421 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -12684,6 +12684,13 @@ trees_in::read_var_def (tree decl, tree maybe_template)
if (maybe_dup && DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P
(maybe_dup))
DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P (decl) = true;
tentative_decl_linkage (decl);
Doesn't this handle the comdat for variables?
Not for external instantiations, for two reasons: it doesn't do anything
if DECL_INTERFACE_KNOWN is set (as is the case here), and even if it
did, maybe_commonize_var only sets comdat for function-scope variables
or variables declared 'inline' (which an explicit instantiation is not
necessarily).
In some ways I think explicit instantiations are quite special with
needing to be emitted in a translation unit without necessarily being
referenced, but also needing COMDAT linkage, so I think it's OK for now
to keep them as special cases until we find another case that this is
needed for. Thoughts?
+ if (DECL_EXPLICIT_INSTANTIATION (decl)
+ && !DECL_EXTERNAL (decl))
+ {
+ mark_needed (decl);
+ if (TREE_PUBLIC (decl))
+ maybe_make_one_only (decl);
+ }
if (DECL_IMPLICIT_INSTANTIATION (decl)
|| (DECL_EXPLICIT_INSTANTIATION (decl)
&& !DECL_EXTERNAL (decl))
Jason
Here's an updated version of the patch; bootstrapped and regtested on
x86_64-pc-linux-gnu (so far just modules.exp), OK for trunk if full
regtest succeeds?
-- >8 --
This patch makes some adjustments required to get a simple modules
testcase working with LTO. There are two main issues fixed.
Firstly, modules only streams the maybe-in-charge constructor, and any
clones are recreated on stream-in. These clones are copied from the
existing function decl and then adjusted. This caused issues because
the clones were getting incorrectly marked as abstract, since after
clones have been created (in the imported file) the maybe-in-charge decl
gets marked as abstract. So this patch just ensures that clones are
always created as non-abstract.
The second issue is that we need to explicitly tell cgraph that explicit
instantiations need to be emitted, otherwise LTO will elide them (as
they don't necessarily appear to be used directly) and cause link
errors. Additionally, expand_or_defer_fn doesn't setup comdat groups
for explicit instantiations, so we need to do that here as well.
Currently this is all handled in 'mark_decl_instantiated'; this patch
splits out the linkage handling into a separate function that we can
call from modules code, maybe in GCC16 we could move this somewhere more
central.
PR c++/118961
+/* DECL is an explicit instantiation definition, ensure that it will
+ be written out here and that it won't clash with other instantiations
+ in other translation units. */
+
+void
+setup_explicit_instantiation_definition_linkage (tree decl)
+{
+ mark_definable (decl);
+ mark_needed (decl);
+ /* Always make artificials weak. */
+ if (DECL_ARTIFICIAL (decl) && flag_weak)
+ comdat_linkage (decl);
+ /* For WIN32 we also want to put explicit instantiations in
Since we're moving this comment, let's drop the obsolete "For WIN32".
OK with that tweak.
Jason
