[ItaniumMangle] Make sure class types are added to the dictionary of substitution candidates when compiling for older ABIs #138947
Conversation
substitution candidates when compiling for older ABIs llvm#132401 made changes to the function that mangles member pointer types, which caused substitutions to be mangled incorrectly. Make sure addSubstitution is called in mangleCXXRecordDecl unless vtables are being mangled (see llvm#109970 for why that is needed). rdar://149307496
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Akira Hatanaka (ahatanak) Changes#132401 made changes to the function that mangles member pointer types, which caused substitutions to be mangled incorrectly. Make sure addSubstitution is called in mangleCXXRecordDecl unless vtables are being mangled (see #109970 for why that is needed). rdar://149307496 Full diff: https://github.com/llvm/llvm-project/pull/138947.diff 3 Files Affected:
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 33a8728728574..f5a1c4332ef80 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -455,7 +455,8 @@ class CXXNameMangler {
void mangleSeqID(unsigned SeqID);
void mangleName(GlobalDecl GD);
void mangleType(QualType T);
- void mangleCXXRecordDecl(const CXXRecordDecl *Record);
+ void mangleCXXRecordDecl(const CXXRecordDecl *Record,
+ bool IsManglingVTable = false);
void mangleLambdaSig(const CXXRecordDecl *Lambda);
void mangleModuleNamePrefix(StringRef Name, bool IsPartition = false);
void mangleVendorQualifier(StringRef Name);
@@ -3102,11 +3103,15 @@ void CXXNameMangler::mangleType(QualType T) {
addSubstitution(T);
}
-void CXXNameMangler::mangleCXXRecordDecl(const CXXRecordDecl *Record) {
+void CXXNameMangler::mangleCXXRecordDecl(const CXXRecordDecl *Record,
+ bool IsManglingVTable) {
if (mangleSubstitution(Record))
return;
mangleName(Record);
- if (isCompatibleWith(LangOptions::ClangABI::Ver19))
+ // If we are mangling vtables, return early without adding the record to the
+ // dictionary of substitution candidates to maintain compatibility with older
+ // ABIs.
+ if (IsManglingVTable && isCompatibleWith(LangOptions::ClangABI::Ver19))
return;
addSubstitution(Record);
}
@@ -7501,7 +7506,7 @@ void ItaniumMangleContextImpl::mangleCXXVTable(const CXXRecordDecl *RD,
// <special-name> ::= TV <type> # virtual table
CXXNameMangler Mangler(*this, Out);
Mangler.getStream() << "_ZTV";
- Mangler.mangleCXXRecordDecl(RD);
+ Mangler.mangleCXXRecordDecl(RD, /*IsManglingVTable=*/true);
}
void ItaniumMangleContextImpl::mangleCXXVTT(const CXXRecordDecl *RD,
@@ -7509,7 +7514,7 @@ void ItaniumMangleContextImpl::mangleCXXVTT(const CXXRecordDecl *RD,
// <special-name> ::= TT <type> # VTT structure
CXXNameMangler Mangler(*this, Out);
Mangler.getStream() << "_ZTT";
- Mangler.mangleCXXRecordDecl(RD);
+ Mangler.mangleCXXRecordDecl(RD, /*IsManglingVTable=*/true);
}
void ItaniumMangleContextImpl::mangleCXXCtorVTable(const CXXRecordDecl *RD,
@@ -7519,10 +7524,10 @@ void ItaniumMangleContextImpl::mangleCXXCtorVTable(const CXXRecordDecl *RD,
// <special-name> ::= TC <type> <offset number> _ <base type>
CXXNameMangler Mangler(*this, Out);
Mangler.getStream() << "_ZTC";
- Mangler.mangleCXXRecordDecl(RD);
+ Mangler.mangleCXXRecordDecl(RD, /*IsManglingVTable=*/true);
Mangler.getStream() << Offset;
Mangler.getStream() << '_';
- Mangler.mangleCXXRecordDecl(Type);
+ Mangler.mangleCXXRecordDecl(Type, /*IsManglingVTable=*/true);
}
void ItaniumMangleContextImpl::mangleCXXRTTI(QualType Ty, raw_ostream &Out) {
diff --git a/clang/test/CodeGenCXX/mangle-itanium-ptrauth.cpp b/clang/test/CodeGenCXX/mangle-itanium-ptrauth.cpp
index 88d80423c3764..155b766803a0a 100644
--- a/clang/test/CodeGenCXX/mangle-itanium-ptrauth.cpp
+++ b/clang/test/CodeGenCXX/mangle-itanium-ptrauth.cpp
@@ -1,5 +1,19 @@
// RUN: %clang_cc1 -std=c++11 -fptrauth-intrinsics -fptrauth-calls -emit-llvm -o - -triple=arm64-apple-ios %s | FileCheck %s
// RUN: %clang_cc1 -std=c++11 -fptrauth-intrinsics -fptrauth-calls -emit-llvm -o - -triple=aarch64-linux-gnu %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -fptrauth-intrinsics -fptrauth-calls -emit-llvm -o - -triple=arm64-apple-ios -fclang-abi-compat=4 %s | FileCheck %s
+
+// clang previously emitted an incorrect discriminator for the member function
+// pointer because of a bug in the mangler.
+
+// CHECK: @_ZN17test_substitution5funcsE = global [1 x { i64, i64 }] [{ i64, i64 } { i64 ptrtoint (ptr ptrauth (ptr @_ZN17test_substitution1S1fEPvS1_, i32 0, i64 48995) to i64), i64 0 }], align 8
+namespace test_substitution {
+struct S { int f(void *, void *); };
+
+typedef int (S::*s_func)(void *, void *);
+
+s_func funcs[] = { (s_func)(&S::f) };
+}
+
// CHECK: define {{.*}}void @_Z3fooPU9__ptrauthILj3ELb1ELj234EEPi(
void foo(int * __ptrauth(3, 1, 234) *) {}
diff --git a/clang/test/CodeGenCXX/mangle.cpp b/clang/test/CodeGenCXX/mangle.cpp
index f4dc17bc4561e..bf9a4f6187778 100644
--- a/clang/test/CodeGenCXX/mangle.cpp
+++ b/clang/test/CodeGenCXX/mangle.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin9 -fblocks -std=c++11 | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin9 -fblocks -std=c++11 | FileCheck --check-prefixes=CHECK,CHECK-ABI-LATEST %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin9 -fblocks -std=c++11 -fclang-abi-compat=4 | FileCheck %s
struct X { };
struct Y { };
@@ -1176,7 +1177,7 @@ namespace test56 {
namespace test57 {
struct X { template <int N> int f(); } x;
template<int N> void f(decltype(x.f<0>() + N)) {}
- // CHECK-LABEL: @_ZN6test571fILi0EEEvDTplcldtL_ZNS_1xEE1fILi0EEET_E
+ // CHECK-ABI-LATEST: @_ZN6test571fILi0EEEvDTplcldtL_ZNS_1xEE1fILi0EEET_E
template void f<0>(int);
}
@@ -1217,7 +1218,7 @@ namespace test61 {
};
};
template <typename T> void f(typename T::Y::a, typename T::Y::b) {}
- // CHECK-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
+ // CHECK-ABI-LATEST-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
template void f<X>(int, int);
}
@@ -1247,3 +1248,15 @@ namespace test63 {
// CHECK-LABEL: @_ZN6test6312_GLOBAL__N_11fIiEEvNS0_4_AndINS0_17integral_constantIivEENS0_7_OrImplIXsr17integral_constantIT_iEE5valueEEEEE
void g() { f<int>({}); }
} // namespace test63
+
+namespace test_substitution {
+struct S { int f(void *, void *); };
+
+typedef int (S::*s_func)(void *, void *);
+
+// clang previously emitted 'S0_' for the second 'void *' parameter type because
+// of a bug in the mangler.
+
+// CHECK-LABEL: define void @_ZN17test_substitution4foo1EMNS_1SEFiPvS1_E(
+void foo1(s_func s) {}
+}
|
|
Can you confirm that the intent here is to restore mangler behavior to what it was prior to #132401, making the pair of PRs ABI-neutral? |
|
This change seems to impact the path add for #108015 which was explicitly related to vtable mangling, are we sure we're not regressing that bug? |
|
(sorry for the noise, I see the above references are PRs rather than the issue # so they're all the same thing :D) |
|
Given that the original PR broke pointer auth discriminators for function pointers, can we verify that discriminators for vtable pointers and vtable pointer slots are still correct as well? I think what we might actually need is for mangling to be aware that it is being used for the purpose of constructing a pointer auth discriminator- (this would also make it possible for us to handle the problem of abi versions being included in the mangling of vtable pointer slots) |
|
I'd like to have @tcwzxx look at this as well |
I a bit confused here as well. The description on the PR doesn't say this, but the patch seems to claim that #132401 fixed a bug here, but it was an ABI breaking change, so it wishes to preserve the bug for ABI < 19. |
clang/lib/AST/ItaniumMangle.cpp
Outdated
| // If we are mangling vtables, return early without adding the record to the | ||
| // dictionary of substitution candidates to maintain compatibility with older | ||
| // ABIs. | ||
| if (IsManglingVTable && isCompatibleWith(LangOptions::ClangABI::Ver19)) |
There was a problem hiding this comment.
So the previous buggy behavior is a bit interesting.
You are saying we used to mangle a substitution here, but not actually consider this site a user of the type for future substitutions?
Do you have a specific test case for that?
|
Ok, I went over this with Akira, and verified the discriminators for vtable pointers and vtable pointer slots have remained consistent. I think we should consider adding additional run commands to some of the existing pointer auth tests to explicitly run with abi-compatibility=4 and latest to ensure that both modes always produce the same discriminators. That's not necessary in this PR though. |
Sorry for the confusion. #132401 caused clang to emit the wrong mangling. Because of the changes made to |
Ah okay, reading #109970 again, it seems you wish to selectively not apply that workaround when dealing with the member pointer class type. |
|
Yes, #109970 preserved the buggy behavior for older ABIs to maintain ABI compatibility and this PR doesn't change that. |
|
When targeting older ABIs, |
|
Given that the mangleCXXRecordDecl function is used outside of the |
Could you elaborate on how to separate just the ABI compatibility logic from Wouldn't you have to move the code to |
This compatibility occurs only here. My last patch wasn't perfect. void mangleCXXRecordDecl(const CXXRecordDecl *Record, bool DontAddSubstitutionForCompat = false);
void ItaniumMangleContextImpl::mangleCXXCtorVTable(const CXXRecordDecl *RD,
int64_t Offset,
const CXXRecordDecl *Type,
raw_ostream &Out) {
// <special-name> ::= TC <type> <offset number> _ <base type>
CXXNameMangler Mangler(*this, Out);
Mangler.getStream() << "_ZTC";
bool CompatibilityForV19 = getASTContext().getLangOpts().getClangABICompat() <=
clang::LangOptionsBase::ClangABI::Ver19;
Mangler.mangleCXXRecordDecl(RD, CompatibilityForV19);
Mangler.getStream() << Offset;
Mangler.getStream() << '_';
Mangler.mangleCXXRecordDecl(Type);
}
|
#132401 made changes to the function that mangles member pointer types, which caused substitutions to be mangled incorrectly.
Make sure addSubstitution is called in mangleCXXRecordDecl unless vtables are being mangled (see #109970 for why that is needed).
rdar://149307496