[clang] 2d8044e - Recommit [ABI] [C++20] [Modules] Don't generate vtable if the class is defined in other module unit

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2023-06-19T10:41:12+08:00
New Revision: 2d8044ee8b19f23e0a7fe5cd35876515d0d1d72e

URL: https://github.com/llvm/llvm-project/commit/2d8044ee8b19f23e0a7fe5cd35876515d0d1d72e
DIFF: https://github.com/llvm/llvm-project/commit/2d8044ee8b19f23e0a7fe5cd35876515d0d1d72e.diff

LOG: Recommit [ABI] [C++20] [Modules] Don't generate vtable if the class is defined in other module unit

Close https://github.com/llvm/llvm-project/issues/61940.

The root cause is that clang will generate vtable as strong symbol now
even if the corresponding class is defined in other module units. After
I check the wording in Itanium ABI, I find this is not inconsistent.
Itanium ABI 5.2.3
(https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable) says:

> The virtual table for a class is emitted in the same object containing
> the definition of its key function, i.e. the first non-pure virtual
> function that is not inline at the point of class definition.

So the current behavior is incorrect. This patch tries to address this.
Also I think we need to do a similar change for MSVC ABI. But I don't
find the formal wording. So I don't address this in this patch.

Reviewed By: rjmccall, iains, dblaikie

Differential Revision: https://reviews.llvm.org/D150023

Added: 
    clang/test/CodeGenCXX/modules-vtable.cppm

Modified: 
    clang/lib/CodeGen/CGVTables.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp
index 32259d1e4cbff..fb0276af57b25 100644
--- a/clang/lib/CodeGen/CGVTables.cpp
+++ b/clang/lib/CodeGen/CGVTables.cpp
@@ -1172,9 +1172,16 @@ bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) {
   if (!keyFunction)
     return false;
 
+  const FunctionDecl *Def;
   // Otherwise, if we don't have a definition of the key function, the
   // vtable must be defined somewhere else.
-  return !keyFunction->hasBody();
+  if (!keyFunction->hasBody(Def))
+    return true;
+
+  assert(Def && "The body of the key function is not assigned to Def?");
+  // If the non-inline key function comes from another module unit, the vtable
+  // must be defined there.
+  return Def->isInAnotherModuleUnit() && !Def->isInlineSpecified();
 }
 
 /// Given that we're currently at the end of the translation unit, and

diff  --git a/clang/test/CodeGenCXX/modules-vtable.cppm b/clang/test/CodeGenCXX/modules-vtable.cppm
new file mode 100644
index 0000000000000..5d13225697b4d
--- /dev/null
+++ b/clang/test/CodeGenCXX/modules-vtable.cppm
@@ -0,0 +1,98 @@
+// REQUIRES: !system-windows
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -emit-module-interface \
+// RUN:     %t/Mod.cppm -o %t/Mod.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %t/Mod.pcm \
+// RUN:     -emit-llvm -o - | FileCheck %t/Mod.cppm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -fmodule-file=Mod=%t/Mod.pcm \
+// RUN:     %t/Use.cpp  -emit-llvm -o - | FileCheck %t/Use.cpp
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -emit-module-interface \
+// RUN:     %t/Mod.cppm -o %t/Mod.pcm -DKEY_FUNCTION_INLINE
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %t/Mod.pcm \
+// RUN:     -emit-llvm -o - | FileCheck %t/Mod.cppm -check-prefix=CHECK-INLINE
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -fmodule-file=Mod=%t/Mod.pcm \
+// RUN:     %t/Use.cpp  -emit-llvm -o - | FileCheck %t/Use.cpp -check-prefix=CHECK-INLINE
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -emit-module-interface \
+// RUN:     %t/M-A.cppm -o %t/M-A.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -fmodule-file=M:A=%t/M-A.pcm \
+// RUN:     %t/M-B.cppm  -emit-llvm -o - | FileCheck %t/M-B.cppm
+
+//--- Mod.cppm
+export module Mod;
+
+export class Base {
+public:
+    virtual ~Base();
+};
+#ifdef KEY_FUNCTION_INLINE
+inline
+#endif
+Base::~Base() {}
+
+// CHECK: @_ZTVW3Mod4Base = unnamed_addr constant
+// CHECK: @_ZTSW3Mod4Base = constant
+// CHECK: @_ZTIW3Mod4Base = constant
+
+// CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant
+// CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant
+// CHECK-INLINE: @_ZTIW3Mod4Base = linkonce_odr {{.*}}constant
+
+module :private;
+int private_use() {
+    Base base;
+    return 43;
+}
+
+//--- Use.cpp
+import Mod;
+int use() {
+    Base* base = new Base();
+    return 43;
+}
+
+// CHECK-NOT: @_ZTSW3Mod4Base = constant
+// CHECK-NOT: @_ZTIW3Mod4Base = constant
+// CHECK: @_ZTVW3Mod4Base = external unnamed_addr
+
+// CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant
+// CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant
+// CHECK-INLINE: @_ZTIW3Mod4Base = linkonce_odr {{.*}}constant
+
+// Check the case that the declaration of the key function comes from another
+// module unit but the definition of the key function comes from the current
+// mdoule unit.
+
+//--- M-A.cppm
+export module M:A;
+export class C {
+public:
+    virtual ~C();
+};
+
+int a_use() {
+    C c;
+    return 43;
+}
+
+//--- M-B.cppm
+export module M:B;
+import :A;
+
+C::~C() {}
+
+int b_use() {
+    C c;
+    return 43;
+}
+
+// CHECK: @_ZTVW1M1C = unnamed_addr constant
+// CHECK: @_ZTSW1M1C = constant
+// CHECK: @_ZTIW1M1C = constant