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

[ChuanqiXu]

Summary

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.

[ChuanqiXu]

clang/lib/CodeGen/CGVTables.cpp

1154–1155

The patch won't affect MSVC due to the check here.

[dblaikie]

clang/lib/CodeGen/CGVTables.cpp

1182–1185

Where's the code that "homes" the vtable into the module definition's object file? Is it possible to share these checks so they're more closely matched/harder to get out of sync?

[ChuanqiXu]

clang/lib/CodeGen/CGVTables.cpp

1182–1185

I think this function (isVTableExternal) is the proper place since other wordings of 5.2.3 are addressed by the function too. Or do I misunderstand your meanings?

[ChuanqiXu]

Rebase with main.

[ChuanqiXu]

@rjmccall @dblaikie @iains ping!

[iains]

This looks reasonable to me - but the vtable stuff is not an area that I am familiar with, so I'll defer to the other reviewers.

clang/lib/CodeGen/CGVTables.cpp

1180–1181

This just read a little awkwardly to me...

it is agreed that "another module unit" does include the specific case (but it also includes TUs from other modules). Really, this is a case where we just need to have a test that says "not in the current TU".

[ChuanqiXu]

clang/lib/CodeGen/CGVTables.cpp

1180–1181

The suggested comment looks not so correct. Since if the key function comes from another translation unit of a different module, the vtable should be defined there too.

Really, this is a case where we just need to have a test that says "not in the current TU".

I don't understand this a lot. Do you mean we need a new test to specify the case "not in the current TU"? (But I feel this is already covered.)

[iains]

clang/lib/CodeGen/CGVTables.cpp

1180–1181

The suggested comment looks not so correct. Since if the key function comes from another translation unit of a different module, the vtable should be defined there too.

Ah, OK, if that is a viable scenario, then my comment is wrong - it's not immediately obvious to me how we can have a class in one module with a key function in a different one (but probably i am missing some case).

[dblaikie]

clang/lib/CodeGen/CGVTables.cpp

1182–1185

Yeah, I'm trying to understand: Why were we getting a duplicate symbol before? I guess maybe there's some other code somewhere that's choosing the linkage of the vtable? And it's choosing strong linkage when the definition goes into the module?

[ChuanqiXu]

clang/lib/CodeGen/CGVTables.cpp

1182–1185

Yeah, understood. It looks better to answer the second question first:

I guess maybe there's some other code somewhere that's choosing the linkage of the vtable?

The code to decide the linkage of the vtable is in CodeGenModule::getVTableLinkage. The logic is simply, (let's skip some details for simplicity)
(1) If the linkage of the class is not externally visible, the linkage of the vtable should be internal;
(2) If the key function is inline, the linkage of the vtable is linkonce linkage.
(3) Otherwise it is strong linkage.

And it's choosing strong linkage when the definition goes into the module?

Yes, and this may be a correct behavior in the reproducers since the key functions are not inline.

---

Why were we getting a duplicate symbol before?

The fundamental reason was that we didn't care about names modules when we was generating the tables. Previously we decided if the key function lives in the current TU by searching its body in the current ASTContext. This was correct since both clang modules and pch don't have TU's semantics. But this changes after we introduce named modules.

See the reproducer: https://godbolt.org/z/jha8rYeoo. Both fmt.cppm and main.cpp contain the strong definition for vtable. It is good that the fmt.cppm contains a strong definition. But it is not good for main.cpp to contain the strong definition too. Since the key function doesn't live in the TU of main.cpp semantically. The compiler was just able to find its definition during the compilation. And this patch fixes this so that the linkage of vtable will be extern in main.cpp. So we avoid the problem.

[ChuanqiXu]

clang/lib/CodeGen/CGVTables.cpp

1182–1185

So my summary is:
(1) if the key function is inline, the linkage of the vtable is linkonce in every used TU.
(2) Otherwise the key function should only live in the TU that the key function lives in.

[ChuanqiXu]

Oh, I got your points. I didn't handle the case that the key function lived in other module unit is inlined. This revision handles the case.

[ChuanqiXu]

@dblaikie @rjmccall gentle ping~

[iains]

although I would welcome input from the ABI owners, testing shows that this patch does appear to DTRT and brings clang in line with the operation of GCC in this area. It would be good to land it (or decide what other action is needed) before 17 branches

[rjmccall]

I had to go learn how modules work in order to review this; pity me.

clang/lib/CodeGen/CGVTables.cpp

1182–1185

Please avoid calling hasBody twice; if we might need the definition, we can just save it the first time.

We need to check specifically whether the key function *definition* is in another module unit, right? getCurrentKeyFunction will return the declaration from the class, and that might be from a different module unit even if the definition is not (maybe also vice-versa?).

You don't need the logic with AllowInlineKeyFunctions here: if we got an inline key function definition, we know the target allows inline key functions. So this can just be Def->isInAnotherModuleUnit() && !Def->isInlineSpecified().

Technically, I think we actually *could* emit inline definitions in module interface units as strong definitions, since there's a unique TU that we could make responsible for emitting them. But I don't think we should go down that road without Itanium ABI agreement; what we're doing here is more consistent with the current ABI.

[rjmccall]

On a more general point, I'm sorry the review of this patch was slow, but please continue to hold basic ABI questions up for code owner review; it's important not to be cavalier about these things.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

On a more general point, I'm sorry the review of this patch was slow, but please continue to hold basic ABI questions up for code owner review; it's important not to be cavalier about these things.

Yeah. But it is good that we know the things are keeping going forward. Slowness is much better than stop.

clang/lib/CodeGen/CGVTables.cpp

1182–1185

We need to check specifically whether the key function *definition* is in another module unit, right?

Yes. Great catch. I've added the case to the tests.

Technically, I think we actually *could* emit inline definitions in module interface units as strong definitions, since there's a unique TU that we could make responsible for emitting them. But I don't think we should go down that road without Itanium ABI agreement; what we're doing here is more consistent with the current ABI.

Agreed. We can't do this now.

[rjmccall]

Thanks, LGTM.

[thakis]

Looks like this breaks check-clang on Mac: http://45.33.8.238/macm1/62779/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[rZhBoYao]

Looks like this breaks check-clang on Mac: http://45.33.8.238/macm1/62779/step_7.txt

Same here. Kindly ping @ChuanqiXu .

[thakis]

Reverted in dbdd6372b7af2f6df5f41d19d966e6bac1b30208 for now.

[ChuanqiXu]

Thanks for keeping the bot green. I see the failure comes from an ABI difference showed up in the test case. I'll try to address it and land it again later.

clang/lib/CodeGen/CGVTables.cpp

 /// not contain a vtable definition when that vtable is defined
 /// strongly elsewhere.  Otherwise, we'd just like to avoid emitting
 /// vtables when unnecessary.
 bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) {
   assert(RD->isDynamicClass() && "Non-dynamic classes have no VTable.");
 
   // We always synthesize vtables if they are needed in the MS ABI. MSVC doesn't
   // emit them even if there is an explicit template instantiation.
   if (CGM.getTarget().getCXXABI().isMicrosoft())
     return false;
 
   // If we have an explicit instantiation declaration (and not a
   // definition), the vtable is defined elsewhere.
   TemplateSpecializationKind TSK = RD->getTemplateSpecializationKind();
   if (TSK == TSK_ExplicitInstantiationDeclaration)
     return true;
 
   // Otherwise, if the class is an instantiated template, the
   // vtable must be defined here.
   if (TSK == TSK_ImplicitInstantiation ||
       TSK == TSK_ExplicitInstantiationDefinition)
     return false;
 
   // Otherwise, if the class doesn't have a key function (possibly
   // anymore), the vtable must be defined here.
   const CXXMethodDecl *keyFunction = CGM.getContext().getCurrentKeyFunction(RD);
   if (!keyFunction)
     return false;
 
   // 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())
+    return true;
+
+  // If the key function comes from another module unit, the vtable must be
+  // defined there.
+  if (keyFunction->isInAnotherModuleUnit())
+    return true;
+
+  return false;
 }
 
 /// Given that we're currently at the end of the translation unit, and
 /// we've emitted a reference to the vtable for this class, should
 /// we define that vtable?
 static bool shouldEmitVTableAtEndOfTranslationUnit(CodeGenModule &CGM,
                                                    const CXXRecordDecl *RD) {
   // If vtable is internal then it has to be done.

clang/test/CodeGenCXX/modules-vtable.cppm

+// 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/Base.cppm -o %t/Base.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -emit-module-interface \
+// RUN:     %t/Sub.cppm -o %t/Sub.pcm -fmodule-file=Mod:Base=%t/Base.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 -emit-module-interface \
+// RUN:     %t/Mod.cppm -o %t/Mod.pcm -fmodule-file=Mod:Base=%t/Base.pcm \
+// RUN:     -fmodule-file=Mod:Sub=%t/Sub.pcm
+//
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %t/Base.pcm \
+// RUN:     -emit-llvm -o - | FileCheck %t/Base.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
+
+//--- Base.cppm
+export module Mod:Base;
+export class Base {
+public:
+    Base();
+    virtual ~Base();
+};
+Base::Base() {}
+Base::~Base() {}
+
+// CHECK: @_ZTVW3Mod4Base = unnamed_addr constant
+// CHECK: @_ZTSW3Mod4Base = constant
+// CHECK: @_ZTIW3Mod4Base = constant
+
+//--- Sub.cppm
+export module Mod:Sub;
+import :Base;
+export class Sub : public Base {
+
+};
+
+// CHECK
+
+//--- Mod.cppm
+export module Mod;
+export import :Base;
+export import :Sub;
+
+//--- Use.cpp
+import Mod;
+int use() {
+    Base *b = new Sub();
+}
+
+// CHECK-NOT: @_ZTSW3Mod4Base = constant
+// CHECK-NOT: @_ZTIW3Mod4Base = constant
+
+// CHECK-DAG: @_ZTVW3Mod3Sub = linkonce_odr unnamed_addr
+// CHECK-DAG: @_ZTSW3Mod3Sub = linkonce_odr
+// CHECK-DAG: @_ZTIW3Mod4Base = external constant ptr
+// CHECK-DAG: @_ZTIW3Mod3Sub = linkonce_odr