[MS-ABI] skip generate comdat for vftable defined with internal alias. #71748
Conversation
Current compler assert with `!SI->isAtomic() && !SI->isVolatile()' failed This due to following rule: First, no exception can move in or out of _try region., i.e., no "potential faulty instruction can be moved across _try boundary. Second, the order of exceptions for instructions 'directly' under a _try must be preserved (not applied to those in callees). Finally, global states (local/global/heap variables) that can be read outside of _try region must be updated in memory (not just in register) before the subsequent exception occurs. All memory instructions inside a _try are considered as 'volatile' to assure 2nd and 3rd rules for C-code above. This is a little sub-optimized. But it's acceptable as the amount of code directly under _try is very small. However during findDominatingStoreToReturnValue: those are not allowed. To fix just skip the assertion when current function has seh try.
…alias. We got a error: LLVM ERROR: Associative COMDAT symbol '??_7?$T@V<lambda_0>@@@@6b@' is not a key for its COMDAT Current we create internal alias for vftable when lambd is used. For the test, IR generate: $"??_7?$T@V<lambda_0>@@$0A@@@6b@" = comdat any @0 = private unnamed_addr constant { [2 x ptr] } { [2 x ptr] [ptr @"??_R4?$T@V<lambda_0>@@$0A@@@6b@", ptr @"?c@b@@UEAAXXZ"] }, comdat($"??_7?$T@V<lambda_0>@@$0A@@@6b@") @"??_7?$T@V<lambda_0>@@$0A@@@6b@" = internal unnamed_addr alias ptr, getelementptr inbounds ({ [2 x ptr] }, ptr @0, i32 0, i32 0, i32 1) According LLVM language reference manual section on COMDATs: There are some restrictions on the properties of the global object. It, or an alias to it, must have the same name as the COMDAT group when targeting COFF. The contents and size of this object may be used during link-time to determine which COMDAT groups get selected depending on the selection kind. Because the name of the object must match the name of the COMDAT group, the linkage of the global object must not be local; local symbols can get renamed if a collision occurs in the symbol table. So one way to fix this is to generate unqiue virtual function table name, to aviod local symbol renaming. My fix is to generate unqiue vftable name for every TU where encoded file name in addition. after the change: <lambda_A7884B04_0> before: <lambda_0>
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: None (jyu2-git) ChangesWe got a error: @0 = private unnamed_addr constant { [2 x ptr] } { [2 x ptr] [ptr @"??_R4?$T@V<lambda_0>@@$0A@@@6b@", ptr @"?c@b@@UEAAXXZ"] }, comdat($"??_7?$T@V<lambda_0>@@$0A@@@6b@") @"??_7?$T@V<lambda_0>@@$0A@@@6b@" = internal unnamed_addr alias ptr, getelementptr inbounds ({ [2 x ptr] }, ptr @0, i32 0, i32 0, i32 1) According LLVM language reference manual section on COMDATs: So one way to fix this is to generate unqiue virtual function table name, to aviod local symbol renaming. Full diff: https://github.com/llvm/llvm-project/pull/71748.diff 6 Files Affected:
diff --git a/clang/include/clang/AST/Mangle.h b/clang/include/clang/AST/Mangle.h
index e586b0cec43dfd6..420ba52f255eb2c 100644
--- a/clang/include/clang/AST/Mangle.h
+++ b/clang/include/clang/AST/Mangle.h
@@ -239,7 +239,8 @@ class MicrosoftMangleContext : public MangleContext {
/// them correctly.
virtual void mangleCXXVFTable(const CXXRecordDecl *Derived,
ArrayRef<const CXXRecordDecl *> BasePath,
- raw_ostream &Out) = 0;
+ raw_ostream &Out,
+ bool IsLocalVFTAliasReq = false) = 0;
/// Mangle vbtable symbols. Only a subset of the bases along the path
/// to the vbtable are included in the name. It's up to the caller to pick
diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index 50ab6ea59be9d03..a84959b3680fdde 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -148,6 +148,7 @@ class MicrosoftMangleContextImpl : public MicrosoftMangleContext {
llvm::DenseMap<GlobalDecl, unsigned> SEHFilterIds;
llvm::DenseMap<GlobalDecl, unsigned> SEHFinallyIds;
SmallString<16> AnonymousNamespaceHash;
+ bool IsLocalVFTAliasReq = false;
public:
MicrosoftMangleContextImpl(ASTContext &Context, DiagnosticsEngine &Diags,
@@ -165,7 +166,8 @@ class MicrosoftMangleContextImpl : public MicrosoftMangleContext {
raw_ostream &) override;
void mangleCXXVFTable(const CXXRecordDecl *Derived,
ArrayRef<const CXXRecordDecl *> BasePath,
- raw_ostream &Out) override;
+ raw_ostream &Out,
+ bool IsLocalVFTAliaesReq = false) override;
void mangleCXXVBTable(const CXXRecordDecl *Derived,
ArrayRef<const CXXRecordDecl *> BasePath,
raw_ostream &Out) override;
@@ -211,6 +213,10 @@ class MicrosoftMangleContextImpl : public MicrosoftMangleContext {
void mangleSEHFinallyBlock(GlobalDecl EnclosingDecl,
raw_ostream &Out) override;
void mangleStringLiteral(const StringLiteral *SL, raw_ostream &Out) override;
+ void setIsLocalVFTAliasReq(bool isLocalVFTAliasReq) {
+ IsLocalVFTAliasReq = isLocalVFTAliasReq;
+ }
+ bool getIsLocalVFTAliasReq() { return IsLocalVFTAliasReq; }
bool getNextDiscriminator(const NamedDecl *ND, unsigned &disc) {
const DeclContext *DC = getEffectiveDeclContext(ND);
if (!DC->isFunctionOrMethod())
@@ -1106,6 +1112,10 @@ void MicrosoftCXXNameMangler::mangleUnqualifiedName(GlobalDecl GD,
if (const CXXRecordDecl *Record = dyn_cast<CXXRecordDecl>(TD)) {
if (Record->isLambda()) {
llvm::SmallString<10> Name("<lambda_");
+ if (Context.getIsLocalVFTAliasReq()) {
+ Name += Context.getAnonymousNamespaceHash();
+ Name += "_";
+ }
Decl *LambdaContextDecl = Record->getLambdaContextDecl();
unsigned LambdaManglingNumber = Record->getLambdaManglingNumber();
@@ -3634,11 +3644,12 @@ void MicrosoftMangleContextImpl::mangleCXXDtorThunk(
void MicrosoftMangleContextImpl::mangleCXXVFTable(
const CXXRecordDecl *Derived, ArrayRef<const CXXRecordDecl *> BasePath,
- raw_ostream &Out) {
+ raw_ostream &Out, bool IsLocalVFTAliasReq) {
// <mangled-name> ::= ?_7 <class-name> <storage-class>
// <cvr-qualifiers> [<name>] @
// NOTE: <cvr-qualifiers> here is always 'B' (const). <storage-class>
// is always '6' for vftables.
+ setIsLocalVFTAliasReq(IsLocalVFTAliasReq);
msvc_hashing_ostream MHO(Out);
MicrosoftCXXNameMangler Mangler(*this, MHO);
if (Derived->hasAttr<DLLImportAttr>())
@@ -3650,6 +3661,7 @@ void MicrosoftMangleContextImpl::mangleCXXVFTable(
for (const CXXRecordDecl *RD : BasePath)
Mangler.mangleName(RD);
Mangler.getStream() << '@';
+ setIsLocalVFTAliasReq(false);
}
void MicrosoftMangleContextImpl::mangleCXXVBTable(
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index e7951b3a3f4d855..bc367b89992bbd9 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -3507,6 +3507,9 @@ static llvm::StoreInst *findDominatingStoreToReturnValue(CodeGenFunction &CGF) {
return nullptr;
// These aren't actually possible for non-coerced returns, and we
// only care about non-coerced returns on this code path.
+ // All memory instructions inside __try block are volatile.
+ if (CGF.currentFunctionUsesSEHTry() && SI->isVolatile())
+ return SI;
assert(!SI->isAtomic() && !SI->isVolatile());
return SI;
};
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index cc7bc3b73589877..6218afebe6ff65e 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1778,9 +1778,11 @@ llvm::Value *MicrosoftCXXABI::getVTableAddressPointInStructor(
static void mangleVFTableName(MicrosoftMangleContext &MangleContext,
const CXXRecordDecl *RD, const VPtrInfo &VFPtr,
- SmallString<256> &Name) {
+ SmallString<256> &Name,
+ bool IsLocalVFTAliasReq = false) {
llvm::raw_svector_ostream Out(Name);
- MangleContext.mangleCXXVFTable(RD, VFPtr.MangledPath, Out);
+ MangleContext.mangleCXXVFTable(RD, VFPtr.MangledPath, Out,
+ IsLocalVFTAliasReq);
}
llvm::Constant *
@@ -1844,9 +1846,6 @@ llvm::GlobalVariable *MicrosoftCXXABI::getAddrOfVTable(const CXXRecordDecl *RD,
}
const std::unique_ptr<VPtrInfo> &VFPtr = *VFPtrI;
- SmallString<256> VFTableName;
- mangleVFTableName(getMangleContext(), RD, *VFPtr, VFTableName);
-
// Classes marked __declspec(dllimport) need vftables generated on the
// import-side in order to support features like constexpr. No other
// translation unit relies on the emission of the local vftable, translation
@@ -1863,6 +1862,12 @@ llvm::GlobalVariable *MicrosoftCXXABI::getAddrOfVTable(const CXXRecordDecl *RD,
bool VTableAliasIsRequred =
!VFTableComesFromAnotherTU && getContext().getLangOpts().RTTIData;
+ bool IsLocalVFTAliasReq =
+ llvm::GlobalValue::isLocalLinkage(VFTableLinkage) && VTableAliasIsRequred;
+
+ SmallString<256> VFTableName;
+ mangleVFTableName(getMangleContext(), RD, *VFPtr, VFTableName,
+ IsLocalVFTAliasReq);
if (llvm::GlobalValue *VFTable =
CGM.getModule().getNamedGlobal(VFTableName)) {
VFTablesMap[ID] = VFTable;
diff --git a/clang/test/CodeGen/windows-seh-EHa-TryInFinally.cpp b/clang/test/CodeGen/windows-seh-EHa-TryInFinally.cpp
index fab567e763df443..ce2a9528e1908a9 100644
--- a/clang/test/CodeGen/windows-seh-EHa-TryInFinally.cpp
+++ b/clang/test/CodeGen/windows-seh-EHa-TryInFinally.cpp
@@ -67,3 +67,26 @@ void foo()
}
}
}
+
+// CHECK-LABEL:@"?bar@@YAHXZ"()
+// CHECK: invoke.cont:
+// CHECK: invoke void @llvm.seh.try.begin()
+// CHECK: invoke.cont1:
+// CHECK: store volatile i32 1, ptr %cleanup.dest.slot
+// CHECK: invoke void @llvm.seh.try.end()
+// CHECK: invoke.cont2:
+// CHECK: call void @"?fin$0@0@bar@@"
+// CHECK: %cleanup.dest3 = load i32, ptr %cleanup.dest.slot
+// CHECK: return:
+// CHECK: ret i32 11
+int bar()
+{
+ int x;
+ __try {
+ return 11;
+ } __finally {
+ if (_abnormal_termination()) {
+ x = 9;
+ }
+ }
+}
diff --git a/clang/test/CodeGenCXX/ms-local-vft-alias-comdat.cpp b/clang/test/CodeGenCXX/ms-local-vft-alias-comdat.cpp
new file mode 100644
index 000000000000000..675f91384b70fda
--- /dev/null
+++ b/clang/test/CodeGenCXX/ms-local-vft-alias-comdat.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fcxx-exceptions -triple=x86_64-windows-msvc \
+// RUN: -Wmicrosoft-template -fms-compatibility -emit-llvm %s -o - \
+// RUN: | FileCheck %s
+
+template <typename a> struct T {
+ virtual void c();
+ T(a h) {}
+};
+struct m {
+ template <typename j> void ab(j ac) {
+ using ad = T<j>;
+ ad j(ac);
+ }
+};
+template <typename ae> struct n {
+ template <typename j> n(j ac) { q.ab(ac); }
+ ae q;
+};
+class s : n<m> {
+ using ag = n<m>;
+public:
+ template <typename j> s(j ac) : ag(ac) {}
+};
+struct ah {
+ ah(s);
+} a([]{});
+
+//CHECK: "??_7?$T@V
+//CHECK-SAME: <lambda_[[UNIQ:.*]]_0>
+//CHECK-NOT: <lambda_0>
+//CHECK-SAME: @@@@6B@"
|
|
Why are we creating a comdat here in the first place? Creating a comdat associated with an internal symbol doesn't do anything useful: an internal symbol can't be defined in any other translation unit, so the comdat will never be discarded. If we allow creating a comdat associated with an internal symbol, the backend should handle it correctly; it's a bug if it crashes like you've described. (I'm not sure how to reproduce the crash with your testcase, though.) |
I guess that's b2615aa... but I'm pretty sure we fixed -fdata-sections so the backend automatically generates comdats. So maybe we should just revert that.
LangRef says "Because the name of the object must match the name of the COMDAT group, the linkage of the global object must not be local; local symbols can get renamed if a collision occurs in the symbol table." I think that means the IR clang generates is illegal, but I guess nothing in the backend actually checks this. |
|
Hi @efriedma-quic, Thanks for looking into this. First for test case: Second,
Except that error we see, there is no other verification check that. Third:
Let me try to revert that, to see if I can get through all tests, just worry about ABI change. Thank you so much for looking into. Jennifer |
That seems reasonable, however, there are other QOI reasons for attempting to generate globally unique names for lambdas. The debug info which describes them relies on having unique mangled names, if there are duplicates, the debugger will not be able to find the correct type information. I think our strategy to ensure internal lambdas like this are globally unique is that they appear in some externally visible scope which can only be defined once, and that scope gets mangled into the name of the lambda. Is that happening here? I don't have an MS demangler handy to confirm against the test case. |
Sure. But we wouldn't want to special-case vftables for that.
In the testcase, the lambda is just at global scope. Not sure what you're expecting to happen here. |
I think that's what I'm trying to get at, this strategy is incomplete and didn't work. I think a principled fix to the general problem would be to incorporate the anonymous namespace hash into the names of all internal lambdas. The point where we check the lambda mangling number is the point where we know if we have an internal or external lambda and decide accordingly: llvm-project/clang/lib/AST/MicrosoftMangle.cpp Line 1125 in 11f52f7 Also this logic has been duplicated, that doesn't seem good: To your point, we shouldn't special case vftables, since this solves a more general problem of debug info quality. |
I am choosing @efriedma-quic's idea, since this has less impact for compile. I also tried @rnk's idea, I don't fell comfort by adding getAnonymousNamespaceHash.s string for all lambda name mangling. Thank you both!
|
Thanks @efriedma-quic and @rnk, both ideas should fix our problem. I am choosing @efriedma-quic's idea, since this has less impact for I also tried @rnk's idea, I don't fell comfort by adding Thanks for both of yuo!! |
This my first time using this new review. I need merge them in. I revert my first change add new one. But it seems it is not merged. |
|
The simplest thing to do is rebase and force-push. You can alternatively try to use a merge commit, but I'm not completely sure how github would react to that. |
|
Looks fine now. (You might want to change the title before merging.) |
jyu2-git
changed the title
| // MS-TYPEMETADATA: comdat($"??_7D@?A0x{{[^@]*}}@@6BB@@@"), !type [[B8]], !type [[D8:![0-9]+]] | ||
| // MS-TYPEMETADATA: comdat($"??_7D@?A0x{{[^@]*}}@@6BA@@@"), !type [[A8]] | ||
| // MS-TYPEMETADATA: comdat($"??_7FA@?1??foo@@YAXXZ@6B@"), !type [[A8]], !type [[FA8:![0-9]+]] | ||
| // MS-TYPEMETADATA: private unnamed_addr constant { [3 x ptr] } { [3 x ptr] [ptr @"??_R4D@?A0xFA4729A9@@6BB@@@", ptr @"?g@B@@UEAAXXZ", ptr @"?h@D@?A0xFA4729A9@@UEAAXXZ"] }, !type [[B8]], !type [[D8:![0-9]+]] |
There was a problem hiding this comment.
The problem is AnonymousNamespaceHas, this need to change to {{.*}}.
llvm#71748) We got a error: `LLVM ERROR: Associative COMDAT symbol '??_7?$T@V<lambda_0>@@@@6b@' is not a key for its COMDAT` Current we create internal alias for vftable when lambd is used. For the test, IR generate: ``` $"??_7?$T@V<lambda_0>@@$0A@@@6b@" = comdat any @0 = private unnamed_addr constant { [2 x ptr] } { [2 x ptr] [ptr @"??_R4?$T@V<lambda_0>@@$0A@@@6b@", ptr @"?c@b@@UEAAXXZ"] }, comdat($"??_7?$T@V<lambda_0>@@$0A@@@6b@") @"??_7?$T@V<lambda_0>@@$0A@@@6b@" = internal unnamed_addr alias ptr, getelementptr inbounds ({ [2 x ptr] }, ptr @0, i32 0, i32 0, i32 1) ``` According LLVM language reference manual section on COMDATs: There are some restrictions on the properties of the global object. It, or an alias to it, must have the same name as the COMDAT group when targeting COFF. The contents and size of this object may be used during link-time to determine which COMDAT groups get selected depending on the selection kind. Because the name of the object must match the name of the COMDAT group, the linkage of the global object must not be local; local symbols can get renamed if a collision occurs in the symbol table. So one way to fix this is to not create comdat for the alias. @0 = private unnamed_addr constant { [2 x ptr] } { [2 x ptr] [ptr @"??_R4?$T@V<lambda_0>@@@@6b@", ptr @"?c@?$T@V<lambda_0>@@@@UEAAXXZ"] }
We got a error:
LLVM ERROR: Associative COMDAT symbol '??_7?$T@V<lambda_0>@@@@6B@' is not a key for its COMDATCurrent we create internal alias for vftable when lambd is used.
For the test, IR generate:
According LLVM language reference manual section on COMDATs:
There are some restrictions on the properties of the global object. It,
or an alias to it, must have the same name as the COMDAT group when
targeting COFF. The contents and size of this object may be used during
link-time to determine which COMDAT groups get selected depending on the
selection kind. Because the name of the object must match the name of the
COMDAT group, the linkage of the global object must not be local; local
symbols can get renamed if a collision occurs in the symbol table.
So one way to fix this is to not create comdat for the alias.
@0 = private unnamed_addr constant { [2 x ptr] } { [2 x ptr] [ptr @"??_R4?$T@V<lambda_0>@@@@6b@", ptr @"?c@?$T@V<lambda_0>@@@@UEAAXXZ"] }