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
gcc/cp/ChangeLog:
* class.cc (copy_fndecl_with_name): Mark clones as non-abstract.
* cp-tree.h (setup_explicit_instantiation_definition_linkage):
Declare new function.
* module.cc (trees_in::read_var_def): Use it.
(module_state::read_cluster): Likewise.
* pt.cc (setup_explicit_instantiation_definition_linkage): New
function.
(mark_decl_instantiated): Use it.
gcc/testsuite/ChangeLog:
* g++.dg/modules/lto-1.h: New test.
* g++.dg/modules/lto-1_a.H: New test.
* g++.dg/modules/lto-1_b.C: New test.
* g++.dg/modules/lto-1_c.C: New test.
* g++.dg/modules/lto-2_a.H: New test.
* g++.dg/modules/lto-2_b.C: New test.
* g++.dg/modules/lto-3_a.H: New test.
* g++.dg/modules/lto-3_b.C: New test.
Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
---
gcc/cp/class.cc | 1 +
gcc/cp/cp-tree.h | 1 +
gcc/cp/module.cc | 9 +++++++++
gcc/cp/pt.cc | 28 +++++++++++++++++---------
gcc/testsuite/g++.dg/modules/lto-1.h | 13 ++++++++++++
gcc/testsuite/g++.dg/modules/lto-1_a.H | 9 +++++++++
gcc/testsuite/g++.dg/modules/lto-1_b.C | 9 +++++++++
gcc/testsuite/g++.dg/modules/lto-1_c.C | 8 ++++++++
gcc/testsuite/g++.dg/modules/lto-2_a.H | 11 ++++++++++
gcc/testsuite/g++.dg/modules/lto-2_b.C | 9 +++++++++
gcc/testsuite/g++.dg/modules/lto-3_a.H | 6 ++++++
gcc/testsuite/g++.dg/modules/lto-3_b.C | 10 +++++++++
12 files changed, 105 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/modules/lto-1.h
create mode 100644 gcc/testsuite/g++.dg/modules/lto-1_a.H
create mode 100644 gcc/testsuite/g++.dg/modules/lto-1_b.C
create mode 100644 gcc/testsuite/g++.dg/modules/lto-1_c.C
create mode 100644 gcc/testsuite/g++.dg/modules/lto-2_a.H
create mode 100644 gcc/testsuite/g++.dg/modules/lto-2_b.C
create mode 100644 gcc/testsuite/g++.dg/modules/lto-3_a.H
create mode 100644 gcc/testsuite/g++.dg/modules/lto-3_b.C
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index d5ae69b0fdf..2b694b98e56 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -5169,6 +5169,7 @@ copy_fndecl_with_name (tree fn, tree name, tree_code code,
set_constraints (clone, copy_node (ci));
SET_DECL_ASSEMBLER_NAME (clone, NULL_TREE);
+ DECL_ABSTRACT_P (clone) = false;
/* There's no pending inline data for this function. */
DECL_PENDING_INLINE_INFO (clone) = NULL;
DECL_PENDING_INLINE_P (clone) = 0;
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 6f0f5cc14a2..74e89a415be 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7692,6 +7692,7 @@ extern tree fn_type_unification (tree,
tree, tree,
tree, unification_kind_t, int,
struct conversion **,
bool, bool);
+extern void setup_explicit_instantiation_definition_linkage (tree);
extern void mark_decl_instantiated (tree, int);
extern int more_specialized_fn (tree, tree, int);
extern tree type_targs_deducible_from (tree, tree);
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index 214fb918e8d..894c70f7225 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -12737,6 +12737,9 @@ 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);
+ if (DECL_EXPLICIT_INSTANTIATION (decl)
+ && !DECL_EXTERNAL (decl))
+ setup_explicit_instantiation_definition_linkage (decl);
if (DECL_IMPLICIT_INSTANTIATION (decl)
|| (DECL_EXPLICIT_INSTANTIATION (decl)
&& !DECL_EXTERNAL (decl))
@@ -16657,6 +16660,12 @@ module_state::read_cluster (unsigned snum)
cfun->language->returns_abnormally = pdata.returns_abnormally;
cfun->language->infinite_loop = pdata.infinite_loop;
+ /* 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 (abstract)
;
else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl))
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 39c0ee610bb..a816b814517 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -25963,6 +25963,24 @@ mark_definable (tree decl)
DECL_NOT_REALLY_EXTERN (clone) = 1;
}
+/* 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
+ linkonce sections. */
+ else if (TREE_PUBLIC (decl))
+ maybe_make_one_only (decl);
+}
+
/* Called if RESULT is explicitly instantiated, or is a member of an
explicitly instantiated class. */
@@ -26000,16 +26018,8 @@ mark_decl_instantiated (tree result, int extern_p)
}
else
{
- mark_definable (result);
- mark_needed (result);
set_instantiating_module (result);
- /* Always make artificials weak. */
- if (DECL_ARTIFICIAL (result) && flag_weak)
- comdat_linkage (result);
- /* For WIN32 we also want to put explicit instantiations in
- linkonce sections. */
- else if (TREE_PUBLIC (result))
- maybe_make_one_only (result);
+ setup_explicit_instantiation_definition_linkage (result);
if (TREE_CODE (result) == FUNCTION_DECL
&& DECL_TEMPLATE_INSTANTIATED (result))
/* If the function has already been instantiated, clear DECL_EXTERNAL,
diff --git a/gcc/testsuite/g++.dg/modules/lto-1.h
b/gcc/testsuite/g++.dg/modules/lto-1.h
new file mode 100644
index 00000000000..935f1de16ef
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lto-1.h
@@ -0,0 +1,13 @@
+template <typename> struct S {
+ S() {}
+};
+template <typename> inline int x = 0;
+
+extern template struct S<char>;
+extern template int x<char>;
+
+template <typename> int* foo() {
+ static int x;
+ return &x;
+};
+extern template int* foo<char>();
diff --git a/gcc/testsuite/g++.dg/modules/lto-1_a.H
b/gcc/testsuite/g++.dg/modules/lto-1_a.H
new file mode 100644
index 00000000000..6ea294db6fe
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lto-1_a.H
@@ -0,0 +1,9 @@
+// PR c++/118961
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+// Test explicit instantiations get emitted with LTO
+
+#include "lto-1.h"
+template struct S<char>;
+template int x<char>;
+template int* foo<char>();
diff --git a/gcc/testsuite/g++.dg/modules/lto-1_b.C
b/gcc/testsuite/g++.dg/modules/lto-1_b.C
new file mode 100644
index 00000000000..75d9a8013b4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lto-1_b.C
@@ -0,0 +1,9 @@
+// PR c++/118961
+// { dg-require-effective-target lto }
+// { dg-additional-options "-fmodules -flto" }
+
+#include "lto-1.h"
+
+S<char> s;
+int y = x<char>;
+int* p = foo<char>();
diff --git a/gcc/testsuite/g++.dg/modules/lto-1_c.C
b/gcc/testsuite/g++.dg/modules/lto-1_c.C
new file mode 100644
index 00000000000..ffd4595883f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lto-1_c.C
@@ -0,0 +1,8 @@
+// PR c++/118961
+// { dg-module-do link }
+// { dg-require-effective-target lto }
+// { dg-additional-options "-fmodules -fno-module-lazy -flto" }
+
+#include "lto-1_a.H"
+
+int main() {}
diff --git a/gcc/testsuite/g++.dg/modules/lto-2_a.H
b/gcc/testsuite/g++.dg/modules/lto-2_a.H
new file mode 100644
index 00000000000..f817329b564
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lto-2_a.H
@@ -0,0 +1,11 @@
+// PR c++/118961
+// { dg-additional-options "-fmodule-header -std=c++20" }
+// { dg-module-cmi {} }
+// Test we correctly emit the bodies of cloned constructors.
+
+template <typename>
+struct S {
+ S() requires true {}
+};
+
+inline S<int> foo() { return {}; }
diff --git a/gcc/testsuite/g++.dg/modules/lto-2_b.C
b/gcc/testsuite/g++.dg/modules/lto-2_b.C
new file mode 100644
index 00000000000..340ff48141f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lto-2_b.C
@@ -0,0 +1,9 @@
+// PR c++/118961
+// { dg-module-do link }
+// { dg-require-effective-target lto }
+// { dg-additional-options "-fmodules -flto -std=c++20" }
+
+import "lto-2_a.H";
+int main() {
+ foo();
+}
diff --git a/gcc/testsuite/g++.dg/modules/lto-3_a.H
b/gcc/testsuite/g++.dg/modules/lto-3_a.H
new file mode 100644
index 00000000000..be63699e66f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lto-3_a.H
@@ -0,0 +1,6 @@
+// PR c++/118961
+// { dg-additional-options "-fmodule-header -std=c++20" }
+// { dg-module-cmi {} }
+// We shouldn't ICE when linking against the standard library.
+
+#include <string>
diff --git a/gcc/testsuite/g++.dg/modules/lto-3_b.C
b/gcc/testsuite/g++.dg/modules/lto-3_b.C
new file mode 100644
index 00000000000..f459596f730
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/lto-3_b.C
@@ -0,0 +1,10 @@
+// PR c++/118961
+// { dg-module-do link }
+// { dg-require-effective-target lto }
+// { dg-additional-options "-fmodules -flto -static -std=c++20" }
+
+import "lto-3_a.H";
+
+int main() {
+ std::string m_message;
+}
--
2.47.0
