[PATCH] c++: Implement modules ABI for vtable emissions

[PATCH] c++: Implement modules ABI for vtable emissions

[Nathaniel Shead]

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

This patch implements the changes described in
https://github.com/itanium-cxx-abi/cxx-abi/pull/171.

One restriction that is lifted in the ABI that hasn't been updated here
is that the ABI no longer requires unique vtables to be emitted with
vague linkage.  I haven't changed this behaviour for this patch, but in
the future we should probably look into changing the relevant target
hook ('class_data_always_comdat') to default to 'false'.

Since the ABI for vtables attached to named modules no longer depends on
key methods, this also resolves the issue described in PR c++/105224.

	PR c++/105224

gcc/cp/ChangeLog:

	* class.cc (finish_struct_1): Also push classes attached to a
	module into the 'keyed_classes' list.
	* decl.cc (record_key_method_defined): Don't push classes
	attached to a named module into the 'keyed_classes' list.
	* module.cc (trees_in::read_class_def): Likewise.
	* decl2.cc (import_export_class): Uniquely emit vtables for
	non-template classes attached to a named module.
	(vtables_uniquely_emitted): New function.
	(import_export_decl): Update comments. Update with knowledge
	about new kinds of uniquely emitted vtables.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_a.C: Update linkage requirements.
	* g++.dg/modules/virt-2_b.C: Likewise.
	* g++.dg/modules/virt-2_c.C: Likewise.
	* g++.dg/modules/virt-4_a.C: New test.
	* g++.dg/modules/virt-4_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/class.cc                         |   7 +-
 gcc/cp/decl.cc                          |   8 +-
 gcc/cp/decl2.cc                         | 102 ++++++++++++++++--------
 gcc/cp/module.cc                        |  12 ++-
 gcc/testsuite/g++.dg/modules/virt-2_a.C |   3 -
 gcc/testsuite/g++.dg/modules/virt-2_b.C |   9 ++-
 gcc/testsuite/g++.dg/modules/virt-2_c.C |  10 +--
 gcc/testsuite/g++.dg/modules/virt-4_a.C |  31 +++++++
 gcc/testsuite/g++.dg/modules/virt-4_b.C |  23 ++++++
 9 files changed, 151 insertions(+), 54 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/virt-4_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/virt-4_b.C

diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 5f258729940..5ef7c71af61 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -7820,8 +7820,11 @@ finish_struct_1 (tree t)
       /* If a polymorphic class has no key method, we may emit the vtable
 	 in every translation unit where the class definition appears.  If
 	 we're devirtualizing, we can look into the vtable even if we
-	 aren't emitting it.  */
-      if (!CLASSTYPE_KEY_METHOD (t))
+	 aren't emitting it.
+
+	 Additionally, if the class is attached to a named module, make sure
+	 to always emit the vtable in this TU.  */
+      if (!CLASSTYPE_KEY_METHOD (t) || module_attach_p ())
 	vec_safe_push (keyed_classes, t);
     }
 
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index d88e0698652..573cfc7ece5 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -18471,7 +18471,13 @@ record_key_method_defined (tree fndecl)
     {
       tree fnclass = DECL_CONTEXT (fndecl);
       if (fndecl == CLASSTYPE_KEY_METHOD (fnclass))
-	vec_safe_push (keyed_classes, fnclass);
+	{
+	  tree classdecl = TYPE_NAME (fnclass);
+	  /* Classes attached to a named module are already handled.  */
+	  if (!DECL_LANG_SPECIFIC (classdecl)
+	      || !DECL_MODULE_ATTACH_P (classdecl))
+	    vec_safe_push (keyed_classes, fnclass);
+	}
     }
 }
 
diff --git a/gcc/cp/decl2.cc b/gcc/cp/decl2.cc
index 806a2a4bc69..a883131d175 100644
--- a/gcc/cp/decl2.cc
+++ b/gcc/cp/decl2.cc
@@ -2426,17 +2426,26 @@ import_export_class (tree ctype)
       import_export = -1;
   else if (TYPE_POLYMORPHIC_P (ctype))
     {
-      /* The ABI specifies that the virtual table and associated
-	 information are emitted with the key method, if any.  */
-      tree method = CLASSTYPE_KEY_METHOD (ctype);
-      /* If weak symbol support is not available, then we must be
-	 careful not to emit the vtable when the key function is
-	 inline.  An inline function can be defined in multiple
-	 translation units.  If we were to emit the vtable in each
-	 translation unit containing a definition, we would get
-	 multiple definition errors at link-time.  */
-      if (method && (flag_weak || ! DECL_DECLARED_INLINE_P (method)))
-	import_export = (DECL_REALLY_EXTERN (method) ? -1 : 1);
+      tree cdecl = TYPE_NAME (ctype);
+      if (DECL_LANG_SPECIFIC (cdecl) && DECL_MODULE_ATTACH_P (cdecl))
+	/* For class types attached to a named module, the ABI specifies
+	   that the tables are uniquely emitted in the object for the
+	   module unit in which it is defined.  */
+	import_export = (DECL_MODULE_IMPORT_P (cdecl) ? -1 : 1);
+      else
+	{
+	  /* The ABI specifies that the virtual table and associated
+	     information are emitted with the key method, if any.  */
+	  tree method = CLASSTYPE_KEY_METHOD (ctype);
+	  /* If weak symbol support is not available, then we must be
+	     careful not to emit the vtable when the key function is
+	     inline.  An inline function can be defined in multiple
+	     translation units.  If we were to emit the vtable in each
+	     translation unit containing a definition, we would get
+	     multiple definition errors at link-time.  */
+	  if (method && (flag_weak || ! DECL_DECLARED_INLINE_P (method)))
+	    import_export = (DECL_REALLY_EXTERN (method) ? -1 : 1);
+	}
     }
 
   /* When MULTIPLE_SYMBOL_SPACES is set, we cannot count on seeing
@@ -3329,6 +3338,35 @@ tentative_decl_linkage (tree decl)
     }
 }
 
+/* For a polymorphic class type CTYPE, whether its vtables are emitted in a
+   unique object as per the ABI.  */
+
+static bool
+vtables_uniquely_emitted (tree ctype)
+{
+  /* If the class is templated, the tables are emitted in every object that
+     references any of them.  */
+  if (CLASSTYPE_USE_TEMPLATE (ctype))
+    return false;
+
+  /* Otherwise, if the class is attached to a module, the tables are uniquely
+     emitted in the object for the module unit in which it is defined.  */
+  tree cdecl = TYPE_NAME (ctype);
+  if (DECL_LANG_SPECIFIC (cdecl) && DECL_MODULE_ATTACH_P (cdecl))
+    return true;
+
+  /* Otherwise, if the class has a key function, the tables are emitted in the
+     object for the TU containing the definition of the key function.  This is
+     unique if the key function is not inline.  */
+  tree key_method = CLASSTYPE_KEY_METHOD (ctype);
+  if (key_method && !DECL_DECLARED_INLINE_P (key_method))
+    return true;
+
+  /* Otherwise, the tables are emitted in every object that references
+     any of them.  */
+  return false;
+}
+
 /* DECL is a FUNCTION_DECL or VAR_DECL.  If the object file linkage
    for DECL has not already been determined, do so now by setting
    DECL_EXTERNAL, DECL_COMDAT and other related flags.  Until this
@@ -3403,10 +3441,6 @@ import_export_decl (tree decl)
      unit.  */
   import_p = false;
 
-  /* FIXME: Since https://github.com/itanium-cxx-abi/cxx-abi/pull/171,
-     the ABI specifies that classes attached to named modules should
-     have their vtables uniquely emitted in the object for the module
-     unit in which it is defined.  And similarly for RTTI structures.  */
   if (VAR_P (decl) && DECL_VTABLE_OR_VTT_P (decl))
     {
       class_type = DECL_CONTEXT (decl);
@@ -3415,15 +3449,13 @@ import_export_decl (tree decl)
 	  && CLASSTYPE_INTERFACE_ONLY (class_type))
 	import_p = true;
       else if ((!flag_weak || TARGET_WEAK_NOT_IN_ARCHIVE_TOC)
-	       && !CLASSTYPE_USE_TEMPLATE (class_type)
-	       && CLASSTYPE_KEY_METHOD (class_type)
-	       && !DECL_DECLARED_INLINE_P (CLASSTYPE_KEY_METHOD (class_type)))
-	/* The ABI requires that all virtual tables be emitted with
-	   COMDAT linkage.  However, on systems where COMDAT symbols
-	   don't show up in the table of contents for a static
-	   archive, or on systems without weak symbols (where we
-	   approximate COMDAT linkage by using internal linkage), the
-	   linker will report errors about undefined symbols because
+	       && !vtables_uniquely_emitted (class_type))
+	/* The ABI historically required that all virtual tables be
+	   emitted with COMDAT linkage.  However, on systems where
+	   COMDAT symbols don't show up in the table of contents for
+	   a static archive, or on systems without weak symbols (where
+	   we approximate COMDAT linkage by using internal linkage),
+	   the linker will report errors about undefined symbols because
 	   it will not see the virtual table definition.  Therefore,
 	   in the case that we know that the virtual table will be
 	   emitted in only one translation unit, we make the virtual
@@ -3444,16 +3476,17 @@ import_export_decl (tree decl)
 	    DECL_EXTERNAL (decl) = 0;
 	  else
 	    {
-	      /* The generic C++ ABI says that class data is always
-		 COMDAT, even if there is a key function.  Some
-		 variants (e.g., the ARM EABI) says that class data
-		 only has COMDAT linkage if the class data might be
-		 emitted in more than one translation unit.  When the
-		 key method can be inline and is inline, we still have
-		 to arrange for comdat even though
+	      /* The generic C++ ABI used to say that class data is always
+		 COMDAT, even if emitted in a unique object.  This is no
+		 longer true, but for now we continue to do so for
+		 compatibility with programs that are not strictly valid.
+		 However, some variants (e.g., the ARM EABI) explicitly say
+		 that class data only has COMDAT linkage if the class data
+		 might be emitted in more than one translation unit.
+		 When the key method can be inline and is inline, we still
+		 have to arrange for comdat even though
 		 class_data_always_comdat is false.  */
-	      if (!CLASSTYPE_KEY_METHOD (class_type)
-		  || DECL_DECLARED_INLINE_P (CLASSTYPE_KEY_METHOD (class_type))
+	      if (!vtables_uniquely_emitted (class_type)
 		  || targetm.cxx.class_data_always_comdat ())
 		{
 		  /* The ABI requires COMDAT linkage.  Normally, we
@@ -3493,8 +3526,7 @@ import_export_decl (tree decl)
 		  && !CLASSTYPE_INTERFACE_ONLY (type))
 		{
 		  comdat_p = (targetm.cxx.class_data_always_comdat ()
-			      || (CLASSTYPE_KEY_METHOD (type)
-				  && DECL_DECLARED_INLINE_P (CLASSTYPE_KEY_METHOD (type))));
+			      || !vtables_uniquely_emitted (class_type));
 		  mark_needed (decl);
 		  if (!flag_weak)
 		    {
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index fac0301d80e..f7725091f60 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -12502,10 +12502,14 @@ trees_in::read_class_def (tree defn, tree maybe_template)
 
 	  if (vtables)
 	    {
-	      if (!CLASSTYPE_KEY_METHOD (type)
-		  /* Sneaky user may have defined it inline
-		     out-of-class.  */
-		  || DECL_DECLARED_INLINE_P (CLASSTYPE_KEY_METHOD (type)))
+	      if ((!CLASSTYPE_KEY_METHOD (type)
+		   /* Sneaky user may have defined it inline
+		      out-of-class.  */
+		   || DECL_DECLARED_INLINE_P (CLASSTYPE_KEY_METHOD (type)))
+		  /* An imported non-template class attached to a module
+		     doesn't need to have its vtables emitted here.  */
+		  && (CLASSTYPE_USE_TEMPLATE (type)
+		      || !DECL_MODULE_ATTACH_P (defn)))
 		vec_safe_push (keyed_classes, type);
 	      unsigned len = vtables->length ();
 	      tree *chain = &CLASSTYPE_VTABLES (type);
diff --git a/gcc/testsuite/g++.dg/modules/virt-2_a.C b/gcc/testsuite/g++.dg/modules/virt-2_a.C
index b5050445c3f..580552be5a0 100644
--- a/gcc/testsuite/g++.dg/modules/virt-2_a.C
+++ b/gcc/testsuite/g++.dg/modules/virt-2_a.C
@@ -1,6 +1,3 @@
-// AAPCS overrides TARGET_CXX_KEY_METHOD_MAY_BE_INLINE,
-// in a way that invalidates this test.
-// { dg-skip-if "!TARGET_CXX_KEY_METHOD_MAY_BE_INLINE" { arm_eabi } } 
 // { dg-module-do run }
 // { dg-additional-options -fmodules-ts }
 export module foo;
diff --git a/gcc/testsuite/g++.dg/modules/virt-2_b.C b/gcc/testsuite/g++.dg/modules/virt-2_b.C
index 2bc5eced013..305ef472533 100644
--- a/gcc/testsuite/g++.dg/modules/virt-2_b.C
+++ b/gcc/testsuite/g++.dg/modules/virt-2_b.C
@@ -21,7 +21,8 @@ int main ()
   return !(Visit (&me) == 1);
 }
 
-// Again, we emit Visitor vtable and rtti here
-// { dg-final { scan-assembler {_ZTVW3foo7Visitor:} } }
-// { dg-final { scan-assembler {_ZTIW3foo7Visitor:} } }
-// { dg-final { scan-assembler {_ZTSW3foo7Visitor:} } }
+// Since https://github.com/itanium-cxx-abi/cxx-abi/pull/171
+// we only emit Visitor vtables and RTTI in its module unit
+// { dg-final { scan-assembler-not {_ZTVW3foo7Visitor:} } }
+// { dg-final { scan-assembler-not {_ZTIW3foo7Visitor:} } }
+// { dg-final { scan-assembler-not {_ZTSW3foo7Visitor:} } }
diff --git a/gcc/testsuite/g++.dg/modules/virt-2_c.C b/gcc/testsuite/g++.dg/modules/virt-2_c.C
index 7b3eeebe508..fed29cf4d54 100644
--- a/gcc/testsuite/g++.dg/modules/virt-2_c.C
+++ b/gcc/testsuite/g++.dg/modules/virt-2_c.C
@@ -9,8 +9,8 @@ int Foo ()
   return !(Visit (&v) == 0);
 }
 
-// We do emit Visitor vtable
-// andl also we do emit rtti here
-// { dg-final { scan-assembler {_ZTVW3foo7Visitor:} } }
-// { dg-final { scan-assembler {_ZTIW3foo7Visitor:} } }
-// { dg-final { scan-assembler {_ZTSW3foo7Visitor:} } }
+// Since https://github.com/itanium-cxx-abi/cxx-abi/pull/171
+// we only emit Visitor vtables and RTTI in its module unit
+// { dg-final { scan-assembler-not {_ZTVW3foo7Visitor:} } }
+// { dg-final { scan-assembler-not {_ZTIW3foo7Visitor:} } }
+// { dg-final { scan-assembler-not {_ZTSW3foo7Visitor:} } }
diff --git a/gcc/testsuite/g++.dg/modules/virt-4_a.C b/gcc/testsuite/g++.dg/modules/virt-4_a.C
new file mode 100644
index 00000000000..a2691630260
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/virt-4_a.C
@@ -0,0 +1,31 @@
+// { dg-additional-options "-fmodules-ts" }
+// Test changes for https://github.com/itanium-cxx-abi/cxx-abi/pull/171
+
+export module M;
+
+// Attached to module, should only be emitted in this TU
+// { dg-final { scan-assembler {_ZTVW1M8Attached:} } }
+// { dg-final { scan-assembler {_ZTIW1M8Attached:} } }
+// { dg-final { scan-assembler {_ZTSW1M8Attached:} } }
+export struct Attached {
+  virtual void key();
+};
+
+// Not attached to module, should be emitted where key function is
+// { dg-final { scan-assembler-not {_ZTV10Unattached:} } }
+// { dg-final { scan-assembler-not {_ZTI10Unattached:} } }
+// { dg-final { scan-assembler-not {_ZTS10Unattached:} } }
+export extern "C++" struct Unattached {
+  // Key function not defined here
+  virtual void key();
+};
+
+// Template, should be emitted everywhere it's used
+export template <typename T> struct Templated {
+  virtual void key() {}
+};
+
+// { dg-final { scan-assembler {_ZTVW1M9TemplatedIiE:} } }
+// { dg-final { scan-assembler {_ZTIW1M9TemplatedIiE:} } }
+// { dg-final { scan-assembler {_ZTSW1M9TemplatedIiE:} } }
+static Templated<int> x;
diff --git a/gcc/testsuite/g++.dg/modules/virt-4_b.C b/gcc/testsuite/g++.dg/modules/virt-4_b.C
new file mode 100644
index 00000000000..999d48bfaf3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/virt-4_b.C
@@ -0,0 +1,23 @@
+// { dg-additional-options "-fmodules-ts" }
+
+module M;
+
+// Attached to module, shouldn't be defined in this TU
+// (was already emitted in interface unit)
+// { dg-final { scan-assembler-not {_ZTVW1M8Attached:} } }
+// { dg-final { scan-assembler-not {_ZTIW1M8Attached:} } }
+// { dg-final { scan-assembler-not {_ZTSW1M8Attached:} } }
+void Attached::key() {}
+
+// Not attached to module and this is the key function,
+// so vtables and RTTI should be emitted here
+// { dg-final { scan-assembler {_ZTV10Unattached:} } }
+// { dg-final { scan-assembler {_ZTI10Unattached:} } }
+// { dg-final { scan-assembler {_ZTS10Unattached:} } }
+extern "C++" void Unattached::key() {}
+
+// Template vtables should be emitted wherever it's used
+// { dg-final { scan-assembler {_ZTVW1M9TemplatedIiE:} } }
+// { dg-final { scan-assembler {_ZTIW1M9TemplatedIiE:} } }
+// { dg-final { scan-assembler {_ZTSW1M9TemplatedIiE:} } }
+static Templated<int> y;
-- 
2.43.2

Re: [PATCH] c++: Implement modules ABI for vtable emissions

[Jason Merrill]

On 5/1/24 04:37, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> -- >8 --
> 
> This patch implements the changes described in
> https://github.com/itanium-cxx-abi/cxx-abi/pull/171.
> 
> One restriction that is lifted in the ABI that hasn't been updated here
> is that the ABI no longer requires unique vtables to be emitted with
> vague linkage.  I haven't changed this behaviour for this patch, but in
> the future we should probably look into changing the relevant target
> hook ('class_data_always_comdat') to default to 'false'.

I suppose we could, but it seems unnecessary.  We've previously decided 
to use COMDAT even when we know that a vague linkage entity is only 
emitted in one TU.  For instance, sticking with COMDAT is more forgiving 
to changes in key function identification.

The patch is OK.

Jason