[C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes #75912
Conversation
llvmbot
added
clang
ChuanqiXu9
added
ABI
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesClose #70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless. Full diff: https://github.com/llvm/llvm-project/pull/75912.diff 5 Files Affected:
diff --git a/clang/lib/CodeGen/CGVTables.cpp b/clang/lib/CodeGen/CGVTables.cpp
index 27a2cab4f75319..06b02df93b26d8 100644
--- a/clang/lib/CodeGen/CGVTables.cpp
+++ b/clang/lib/CodeGen/CGVTables.cpp
@@ -1045,6 +1045,15 @@ llvm::GlobalVariable::LinkageTypes
CodeGenModule::getVTableLinkage(const CXXRecordDecl *RD) {
if (!RD->isExternallyVisible())
return llvm::GlobalVariable::InternalLinkage;
+
+ // Previously we'll decide the linkage of the vtable by the linkage
+ // of the key function. But within modules, the concept of key functions
+ // becomes meaningless. So the linkage of the vtable should always be
+ // external if the class is externally visible.
+ //
+ // TODO: How about the case of AppleKext, DLLExportAttr and DLLImportAttr.
+ if (Module *M = RD->getOwningModule(); M && M->isNamedModule())
+ return llvm::GlobalVariable::ExternalLinkage;
// We're at the end of the translation unit, so the current key
// function is fully correct.
@@ -1180,6 +1189,16 @@ bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) {
TSK == TSK_ExplicitInstantiationDefinition)
return false;
+ // Itanium C++ ABI [5.2.3]:
+ // Virtual tables for dynamic classes are emitted as follows:
+ //
+ // - If the class is templated, the tables are emitted in every object that references any of them.
+ // - 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.
+ // - Otherwise, if the class has a key function (see below), the tables are emitted in the object for the translation unit containing the definition of the key function. This is unique if the key function is not inline.
+ // - Otherwise, the tables are emitted in every object that references any of them.
+ if (Module *M = RD->getOwningModule(); M && M->isNamedModule())
+ return M != CGM.getContext().getCurrentNamedModule();
+
// 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);
@@ -1189,13 +1208,7 @@ bool CodeGenVTables::isVTableExternal(const CXXRecordDecl *RD) {
const FunctionDecl *Def;
// Otherwise, if we don't have a definition of the key function, the
// vtable must be defined somewhere else.
- 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();
+ return !keyFunction->hasBody(Def);
}
/// Given that we're currently at the end of the translation unit, and
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index d173806ec8ce53..7e2c1191071c84 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -1801,6 +1801,12 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT,
if (VTable->hasInitializer())
return;
+ // If the class are attached to a C++ named module other than the one
+ // we're currently compiling, the vtable should be defined there.
+ if (Module *M = RD->getOwningModule(); M && M->isNamedModule() &&
+ M != CGM.getContext().getCurrentNamedModule())
+ return;
+
ItaniumVTableContext &VTContext = CGM.getItaniumVTableContext();
const VTableLayout &VTLayout = VTContext.getVTableLayout(RD);
llvm::GlobalVariable::LinkageTypes Linkage = CGM.getVTableLinkage(RD);
diff --git a/clang/lib/CodeGen/ModuleBuilder.cpp b/clang/lib/CodeGen/ModuleBuilder.cpp
index 3594f4c66e6774..76175a157ba7bb 100644
--- a/clang/lib/CodeGen/ModuleBuilder.cpp
+++ b/clang/lib/CodeGen/ModuleBuilder.cpp
@@ -317,7 +317,7 @@ namespace {
void HandleVTable(CXXRecordDecl *RD) override {
if (Diags.hasErrorOccurred())
return;
-
+
Builder->EmitVTable(RD);
}
};
diff --git a/clang/test/CodeGenCXX/modules-vtable.cppm b/clang/test/CodeGenCXX/modules-vtable.cppm
index fb179b1de4880b..abbbc403ace243 100644
--- a/clang/test/CodeGenCXX/modules-vtable.cppm
+++ b/clang/test/CodeGenCXX/modules-vtable.cppm
@@ -24,6 +24,8 @@
// 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
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 \
+// RUN: %t/M-A.pcm -emit-llvm -o - | FileCheck %t/M-A.cppm
//--- Mod.cppm
export module Mod;
@@ -41,9 +43,10 @@ Base::~Base() {}
// 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
+// With the new Itanium C++ ABI, the linkage of vtables in modules don't need to be linkonce ODR.
+// CHECK-INLINE: @_ZTVW3Mod4Base = {{.*}}unnamed_addr constant
+// CHECK-INLINE: @_ZTSW3Mod4Base = {{.*}}constant
+// CHECK-INLINE: @_ZTIW3Mod4Base = {{.*}}constant
module :private;
int private_use() {
@@ -60,11 +63,11 @@ int use() {
// CHECK-NOT: @_ZTSW3Mod4Base = constant
// CHECK-NOT: @_ZTIW3Mod4Base = constant
-// CHECK: @_ZTVW3Mod4Base = external unnamed_addr
+// CHECK: @_ZTVW3Mod4Base = external
-// CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant
-// CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant
-// CHECK-INLINE: @_ZTIW3Mod4Base = linkonce_odr {{.*}}constant
+// CHECK-INLINE-NOT: @_ZTSW3Mod4Base = constant
+// CHECK-INLINE-NOT: @_ZTIW3Mod4Base = constant
+// CHECK-INLINE: @_ZTVW3Mod4Base = external
// 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
@@ -82,6 +85,10 @@ int a_use() {
return 43;
}
+// CHECK: @_ZTVW1M1C = unnamed_addr constant
+// CHECK: @_ZTSW1M1C = constant
+// CHECK: @_ZTIW1M1C = constant
+
//--- M-B.cppm
export module M:B;
import :A;
@@ -93,6 +100,6 @@ int b_use() {
return 43;
}
-// CHECK: @_ZTVW1M1C = unnamed_addr constant
-// CHECK: @_ZTSW1M1C = constant
-// CHECK: @_ZTIW1M1C = constant
+// CHECK: @_ZTVW1M1C = external
+// CHECK-NOT: @_ZTSW1M1C = constant
+// CHECK-NOT: @_ZTIW1M1C = constant
diff --git a/clang/test/CodeGenCXX/pr70585.cppm b/clang/test/CodeGenCXX/pr70585.cppm
new file mode 100644
index 00000000000000..5c8245c0efbf80
--- /dev/null
+++ b/clang/test/CodeGenCXX/pr70585.cppm
@@ -0,0 +1,50 @@
+// REQUIRES: !system-windows
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/layer1.cppm -triple %itanium_abi_triple \
+// RUN: -emit-module-interface -o %t/foo-layer1.pcm
+// RUN: %clang_cc1 -std=c++20 %t/layer2.cppm -triple %itanium_abi_triple \
+// RUN: -emit-module-interface -fmodule-file=foo:layer1=%t/foo-layer1.pcm \
+// RUN: -o %t/foo-layer2.pcm
+// RUN: %clang_cc1 -std=c++20 %t/foo-layer1.pcm -S -emit-llvm -o - | FileCheck %t/layer1.cppm
+// RUN: %clang_cc1 -std=c++20 %t/foo-layer2.pcm -S -emit-llvm -o - \
+// RUN: -fmodule-file=foo:layer1=%t/foo-layer1.pcm | FileCheck %t/layer2.cppm
+
+//--- layer1.cppm
+export module foo:layer1;
+struct Fruit {
+ virtual ~Fruit() = default;
+ virtual void eval() = 0;
+};
+struct Banana : public Fruit {
+ Banana() {}
+ void eval() override;
+};
+
+// CHECK-DAG: @_ZTVW3foo6Banana = unnamed_addr constant
+// CHECK-DAG: @_ZTSW3foo6Banana = constant
+// CHECK-DAG: @_ZTIW3foo6Banana = constant
+//
+// CHECK-DAG: @_ZTVW3foo5Fruit = unnamed_addr constant
+// CHECK-DAG: @_ZTSW3foo5Fruit = constant
+// CHECK-DAG: @_ZTIW3foo5Fruit = constant
+
+//--- layer2.cppm
+export module foo:layer2;
+import :layer1;
+export void layer2_fun() {
+ Banana *b = new Banana();
+ b->eval();
+}
+void Banana::eval() {
+}
+
+// CHECK-NOT: @_ZTVW3foo6Banana
+// CHECK-NOT: @_ZTSW3foo6Banana
+// CHECK-NOT: @_ZTIW3foo6Banana
+// CHECK-NOT: @_ZTVW3foo5Fruit
+// CHECK-NOT: @_ZTSW3foo5Fruit
+// CHECK-NOT: @_ZTIW3foo5Fruit
|
ChuanqiXu9
force-pushed
the
ModulesVtable
branch
from
38b3178
to
6429682
Compare
llvmbot
added
clang
clang/test/CodeGenCXX/pr70585.cppm
Outdated
| struct Fruit { | ||
| virtual ~Fruit() = default; | ||
| virtual void eval() = 0; | ||
| }; | ||
| struct Banana : public Fruit { | ||
| Banana() {} | ||
| void eval() override; | ||
| }; | ||
|
|
There was a problem hiding this comment.
What different cases do these two classes test? Would one class be sufficient?
There was a problem hiding this comment.
This comes from the original reproducer while the base class is a pure virtual class that can't be instantiated. While it may be sufficient to reduce them to a single class, I feel slightly better to keep the form since it is not complex.
There was a problem hiding this comment.
Not complex, but does obscure the intent of the test somewhat - keeping things as reduced as possible makes the test easier to understand - having superfluous details makes it harder to read/understand the purpose of the test.
There was a problem hiding this comment.
Great suggestion. I caught another bug after I removed Banana: previously we would only generate vtable if it is used. And my draft to fix that is ChuanqiXu9@f6a2284. It touches Sema and Serializer. So I feel it is a little bit complex even if it is small. Personally, I prefer to separate that into 2 patches later instead of combining everything in the current single patch. How do you feel about the direction? @dwblaikie @rjmccall
There was a problem hiding this comment.
I've just tried to update it here. If you feel it is too sparse, I can try to make it into 2 patches.
clang/test/CodeGenCXX/pr70585.cppm
Outdated
| // CHECK-DAG: @_ZTVW3foo6Banana = unnamed_addr constant | ||
| // CHECK-DAG: @_ZTSW3foo6Banana = constant | ||
| // CHECK-DAG: @_ZTIW3foo6Banana = constant | ||
| // | ||
| // CHECK-DAG: @_ZTVW3foo5Fruit = unnamed_addr constant | ||
| // CHECK-DAG: @_ZTSW3foo5Fruit = constant | ||
| // CHECK-DAG: @_ZTIW3foo5Fruit = constant | ||
|
|
There was a problem hiding this comment.
Should the linkage and be checked here?
There was a problem hiding this comment.
Yeah, the linkage is already tested. Currently we're checking the linkage is external (default linkage). For example, if the linkage is linkonce_odr, the output will be something like: // CHECK-DAG: @_ZTSW3foo5Fruit = linkonce_odr constant
clang/test/CodeGenCXX/pr70585.cppm
Outdated
| export void layer2_fun() { | ||
| Banana *b = new Banana(); | ||
| b->eval(); | ||
| } | ||
| void Banana::eval() { | ||
| } | ||
|
|
There was a problem hiding this comment.
What do these two uses check (I guess one invokes the ctor to check if the vtable is emitted wherever the ctor is, and the other defines a virtual member, to check if the key function triggers vtable emission?)? Perhaps they could be simpler and/or more explicit (maybe comments to document the cases they're trying to test) - but probably one case is sufficient? (key function, I guess)
There was a problem hiding this comment.
We're testing:
(1) The use of virtual functions won't produce the vtable.
(2) The definition of key functions won't produce the vtable.
I'll try to add a comment here.
clang/test/CodeGenCXX/pr70585.cppm
Outdated
| // CHECK-NOT: @_ZTVW3foo6Banana | ||
| // CHECK-NOT: @_ZTSW3foo6Banana | ||
| // CHECK-NOT: @_ZTIW3foo6Banana | ||
| // CHECK-NOT: @_ZTVW3foo5Fruit | ||
| // CHECK-NOT: @_ZTSW3foo5Fruit | ||
| // CHECK-NOT: @_ZTIW3foo5Fruit |
There was a problem hiding this comment.
Perhaps this could be generalized to // CHECK-NOT: @_Z{{.*(Banana|Fruit)}} or something like that? (even simpler if only one type is used here)
There was a problem hiding this comment.
I prefer the current pattern so that other developers can understand what we're testing by llvm-cxxfilt.
ChuanqiXu9
force-pushed
the
ModulesVtable
branch
from
6429682
to
908a028
Compare
|
@dwblaikie @rjmccall ping~ |
clang/lib/CodeGen/CGVTables.cpp
Outdated
| // becomes meaningless. So the linkage of the vtable should always be | ||
| // external if the class is externally visible. | ||
| // | ||
| // TODO: How about the case of AppleKext, DLLExportAttr and DLLImportAttr. |
There was a problem hiding this comment.
Comments should be timeless: they exist to explain the current code, not the patch you're making. So don't say stuff like "previously". You can just "V-tables for non-template classes with an owning module are always uniquely emitted in that module."
It's correct for this to apply even for the Apple kext, dllexport, and dllimport cases. But I think you do actually need to handle the template case here; we can't emit v-tables for template instantiations with strong linkage.
I think this function could probably be significantly simplified to eliminate a lot of the redundancy; it's essentially:
- Use internal linkage if the class isn't externally visible.
- Decide whether the v-table has vague linkage.
- If not, it gets either external or available-externally linkage based on a few different criteria.
- If so, there's a bunch of platform/ABI-specific decisions that affect the exact LLVM linkage we pick.
There was a problem hiding this comment.
I've updated the comments. And I'd like to leave the simplification to later patches.
ChuanqiXu9
force-pushed
the
ModulesVtable
branch
from
908a028
to
630e597
Compare
llvmbot
added
the
clang:frontend
ChuanqiXu9
force-pushed
the
ModulesVtable
branch
from
630e597
to
7a5c4cc
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@rjmccall @dwblaikie ping |
| // CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant | ||
| // CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant | ||
| // CHECK-INLINE: @_ZTIW3Mod4Base = linkonce_odr {{.*}}constant | ||
| // With the new Itanium C++ ABI, the linkage of vtables in modules don't need to be linkonce ODR. |
There was a problem hiding this comment.
Would they benefit from still being linkonce? What if the vtable is never used? Or do we leave that up to --gc-sections linker behavior? (I'm not clear on when we use linkage to let things be dropped, and when we use the linker function-sections/data-sections/gc-sections behavior to drop unused stuff)
There was a problem hiding this comment.
If it is linkonce, as long as I understand correctly, if the vtable is not used in the current TU, the middle end is free to remove the unused linkonce entity. But this is not wanted. Since all the things in the module purview is potentially used. Do I misunderstand anything?
There was a problem hiding this comment.
right, sorry, was misreading the linkages - I guess weak might be applicable?
“weak” linkage has the same merging semantics as linkonce linkage, except that unreferenced globals with weak linkage may not be discarded.
But maybe just normal external linkage is the right thing & we let the linker GC things if the user asks it to. Fair enough.
There was a problem hiding this comment.
While I agree weak may work here, I feel it is odd. I feel the default strong linkage seems better when it is applicable. What's the advance to use weak linkage here?
| // CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant | ||
| // CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant | ||
| // CHECK-INLINE: @_ZTIW3Mod4Base = linkonce_odr {{.*}}constant | ||
| // CHECK-INLINE-NOT: @_ZTSW3Mod4Base = constant |
There was a problem hiding this comment.
NOT checks should be pretty broad - for instance these NOT checks would've passed with the old code too (since it was linkonce_odr, not constant) - perhaps these should just be non-NOT checks, affirmatively checking for the linkage of these globals (which I assume they are emitted, as declarations - if they aren't emitted at all, then NOT: @foo might be enough, without the = constant?)
There was a problem hiding this comment.
They should be emitted in other TU and not emitted at all in the current TU. So I updated the test to // CHECK-INLINE-NOT: @_ZTSW3Mod4Base as suggested.
ChuanqiXu9
force-pushed
the
ModulesVtable
branch
from
7a5c4cc
to
f05b4e6
Compare
There was a problem hiding this comment.
I am going to defer to @rjmccall and @dwblaikie on this one (the comments so far seem reasonable to me).
ChuanqiXu9
force-pushed
the
ModulesVtable
branch
from
08dd476
to
5401fe6
Compare
xfails: * clang/test/CodeGenCXX/modules-vtable.cppm * clang/test/CodeGenCXX/pr70585.cppm cause: llvm#75912 [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes Change-Id: I7c502ffc1fe6bc7f98f3a60bc6c7bfe0af52b497
named modules Close #97313 In the previous patch (#75912), I made an oversight that I ignored the templates in named module when calculating the linkage for the vtables. In this patch, I tried to correct the behavior by merging the logics to calculate the linkage with key functions with named modules.
named modules Close llvm#97313 In the previous patch (llvm#75912), I made an oversight that I ignored the templates in named module when calculating the linkage for the vtables. In this patch, I tried to correct the behavior by merging the logics to calculate the linkage with key functions with named modules.
named modules Close llvm#97313 In the previous patch (llvm#75912), I made an oversight that I ignored the templates in named module when calculating the linkage for the vtables. In this patch, I tried to correct the behavior by merging the logics to calculate the linkage with key functions with named modules.
|
FYI, the commit 99873b3, which lists this PR as a motivator, causes breakage building a project, which I am still looking for a reduced test case. The commit is not NFC despite what is says, just by cursory inspection of the change. Please avoid adding NFC tag to such commits in the future and create a pull request. |
Oh, sorry, I took another look at the commit and it looks the change makes it not a NFC change is this line: 99873b3#diff-6fe53759e8d797c328c73ada5f3324c6248a8634ef36131c7eb2b9d89192bb64R6514 This shouldn't be in that commit but in this commit. It is not intentional. I guess we can't observe that if we put that in this PR too. And that change looks not bad. So maybe it makes something already bad to show up. |
|
The reproducer turned out to be pretty simple: export module a;
module :private;
static void f() {}
void g() {
f();
}Compiles without that patch, otherwise produces:
Even without that change, the other changes are too complex, and there are other suspicious things you wouldn't expect in an NFC commit which doesn't call this out specifically, like the removal of that whole block with the FIXME included. I think the appropriate tag for such commits would be NFCI, and should still require PR and review. |
Thanks for the reproducer and the information about NFCI. It looks like I took an oversight in the refactoring of |
Clarifying a couple of things here
But, yeah, bit more caution in the future, and probably smaller/more isolated patches would be good in any case. |
I think we need the distinction between simple NFC, for trivial, non-controversial and non-surprising changes like some renames, small whitespace / formatting change, documentation changes, and complex NFC, which requires some thinking and has more risk of not actually being NFC. So in practical terms, I propose a complex NFC change should only be excused of changes to functional testing, nothing else. Also, I think we presently have sufficient review bandwidth in clang proper, even including the area of modules, so that any non-trivial change can be reviewed, and I also propose that we abstain from trying to directly commit those. |
That's a pretty substantial policy change to propose, and this probably isn't the place to propose/discuss it. If that's your intent, probably best to take that up on discord. NFC, to me, has always been basically "this has no observable change/can't be tested" - it's not a bar on whether something should be reviewed. (lots of NFC patches get sent for review, but not all of them to be sure - and equally not all non-NFC patches get sent for review) (FWIW, check some of the recent modules changes @ChuanqiXu9 has been working on to see that reviewer bandwidth here is pretty thin (& my experience in LLVM in general, including clang, is that reviewer bandwidth is pretty thin - though it is something we should address & I do think it might be time to change LLVM's post-commit review policy, but I think it'll be a substantial amount of work)) |
…dule unit of dynamic classes (llvm#75912)" This reverts commit 18f3bcb, 15bb026 and 99873b3. See the post commit message in llvm#75912 to see the reasons.
Due to llvm#75912 is reverted and llvm#70585 is reopened. It looks riskful to fix the issue correctly before 19. So we provide a workaround here to help people in this trouble as much as possible.
I am not proposing a policy change. I believe the current policy is aimed at giving an escape hatch for projects which there is basically one or two active developers. I am pointing out that I believe we don't need for that escape hatch to apply to any parts of clang currently.
If you feel bandwidth for modules is pretty thin, I put myself available as a reviewer, so feel free to ping me. |
Thanks. I'll try to add you for future reviewings.
To be honest, this is not what I feel at least for coroutines and modules for a long time. I had multiple experience that some patches doesn't get reviewed for months. I still believe it will work better if I can push some simple patches directly. |
With a fair bit of experience on this project and the norms - I can say pretty confidently that was/is not the intent of the current policy. Especially for code owners, there's an expectation they probably know better than anyone else & can make summary judgments on direction in many cases. Every now and then they'll ask for second opinions on uncertain directions, but can probably pretty well determine what's acceptable for most work without a second opinion. (honestly, moving to all changes being reviewed might be good for the project - though I think we'd need to work more on ownership and reviewer availability/authority/process, but it's not where we are today in either case (policy or socially)) The dev policy says "Smaller patches (or patches where the developer owns the component) that meet likely-community-consensus requirements (as apply to all patch approvals) can be committed prior to an explicit review. " (in this case @ChuanqiXu9 is the owner of clang's modules support)
While I appreciate the offer and would encourage you to review these changes - I'll point out that one of the reasons these reviews haven't been forthcoming is that the feature area is fairly complicated and nuanced. |
Close #70585 and reflect itanium-cxx-abi/cxx-abi#170.
The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.