-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[PAC] Fix address discrimination for type info vtable pointers #102199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
In llvm#99726, `-fptrauth-type-info-vtable-pointer-discrimination` was introduced, which is intended to enable type and address discrimination for type_info vtable pointers. However, some codegen logic for actually enabling address discrimination was missing. This patch addresses the issue. Fixes llvm#101716
Also tagging @ojhunt (GitHub still does not let to add as a reviewer) |
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang-codegen Author: Daniil Kovalev (kovdan01) ChangesIn #99726, Fixes #101716 Full diff: https://github.com/llvm/llvm-project/pull/102199.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 9b5227772125b2..13aeb3e75d6843 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3955,9 +3955,23 @@ void ItaniumRTTIBuilder::BuildVTablePointer(const Type *Ty) {
VTable, Two);
}
- if (auto &Schema = CGM.getCodeGenOpts().PointerAuth.CXXTypeInfoVTablePointer)
- VTable = CGM.getConstantSignedPointer(VTable, Schema, nullptr, GlobalDecl(),
- QualType(Ty, 0));
+ if (const auto &Schema =
+ CGM.getCodeGenOpts().PointerAuth.CXXTypeInfoVTablePointer) {
+ llvm::PointerType *PtrTy = llvm::PointerType::get(
+ CGM.getLLVMContext(),
+ CGM.getModule().getDataLayout().getProgramAddressSpace());
+ llvm::Constant *StorageAddress =
+ (Schema.isAddressDiscriminated()
+ ? llvm::ConstantExpr::getIntToPtr(
+ llvm::ConstantInt::get(
+ CGM.IntPtrTy,
+ llvm::ConstantPtrAuth::
+ AddrDiscriminator_CXXTypeInfoVTablePointer),
+ PtrTy)
+ : nullptr);
+ VTable = CGM.getConstantSignedPointer(VTable, Schema, StorageAddress,
+ GlobalDecl(), QualType(Ty, 0));
+ }
Fields.push_back(VTable);
}
diff --git a/clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp b/clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp
index 174aeda89d1755..61eef73b5be2a4 100644
--- a/clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp
+++ b/clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp
@@ -65,7 +65,7 @@ extern "C" int disc_std_type_info = __builtin_ptrauth_string_discriminator("_ZTV
// NODISC: @_ZTI10TestStruct = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2), ptr @_ZTS10TestStruct }, align 8
-// DISC: @_ZTI10TestStruct = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2, i64 [[STDTYPEINFO_DISC]]), ptr @_ZTS10TestStruct }, align 8
+// DISC: @_ZTI10TestStruct = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2, i64 [[STDTYPEINFO_DISC]], ptr inttoptr (i64 1 to ptr)), ptr @_ZTS10TestStruct }, align 8
struct TestStruct {
virtual ~TestStruct();
diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h
index 2788751e8b62a1..aaa1c197651a66 100644
--- a/llvm/include/llvm/IR/Constants.h
+++ b/llvm/include/llvm/IR/Constants.h
@@ -1056,12 +1056,18 @@ class ConstantPtrAuth final : public Constant {
return !getAddrDiscriminator()->isNullValue();
}
- /// A constant value for the address discriminator which has special
- /// significance to ctors/dtors lowering. Regular address discrimination can't
- /// be applied for them since uses of llvm.global_{c|d}tors are disallowed
- /// (see Verifier::visitGlobalVariable) and we can't emit getelementptr
- /// expressions referencing these special arrays.
- enum { AddrDiscriminator_CtorsDtors = 1 };
+ /// Constant values for the address discriminator which have special
+ /// significance to lowering in some contexts.
+ /// - For ctors/dtors, regular address discrimination can't
+ /// be applied for them since uses of llvm.global_{c|d}tors are disallowed
+ /// (see Verifier::visitGlobalVariable) and we can't emit getelementptr
+ /// expressions referencing these special arrays.
+ /// - For vtable pointers of std::type_info and classes derived from it,
+ /// we do not know the storage address when emitting ptrauth constant.
+ enum {
+ AddrDiscriminator_CtorsDtors = 1,
+ AddrDiscriminator_CXXTypeInfoVTablePointer = 1
+ };
/// Whether the address uses a special address discriminator.
/// These discriminators can't be used in real pointer-auth values; they
diff --git a/llvm/test/CodeGen/AArch64/ptrauth-type-info-vptr-discr.ll b/llvm/test/CodeGen/AArch64/ptrauth-type-info-vptr-discr.ll
new file mode 100644
index 00000000000000..b25a32f856b7ab
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/ptrauth-type-info-vptr-discr.ll
@@ -0,0 +1,20 @@
+; RUN: llc -mtriple aarch64-linux-gnu -mattr=+pauth -filetype=asm -o - %s | FileCheck --check-prefix=ELF %s
+; RUN: llc -mtriple aarch64-apple-darwin -mattr=+pauth -filetype=asm -o - %s | FileCheck --check-prefix=MACHO %s
+
+; ELF-LABEL: _ZTI10Disc:
+; ELF-NEXT: .xword (_ZTVN10__cxxabiv117__class_type_infoE+16)@AUTH(da,45546,addr)
+; ELF-LABEL: _ZTI10NoDisc:
+; ELF-NEXT: .xword (_ZTVN10__cxxabiv117__class_type_infoE+16)@AUTH(da,45546)
+
+; MACHO-LABEL: __ZTI10Disc:
+; MACHO-NEXT: .quad (__ZTVN10__cxxabiv117__class_type_infoE+16)@AUTH(da,45546,addr)
+; MACHO-LABEL: __ZTI10NoDisc:
+; MACHO-NEXT: .quad (__ZTVN10__cxxabiv117__class_type_infoE+16)@AUTH(da,45546)
+
+@_ZTVN10__cxxabiv117__class_type_infoE = external global [0 x ptr]
+
+@_ZTS10Disc = constant [4 x i8] c"Disc", align 1
+@_ZTI10Disc = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2, i64 45546, ptr inttoptr (i64 1 to ptr)), ptr @_ZTS10Disc }, align 8
+
+@_ZTS10NoDisc = constant [6 x i8] c"NoDisc", align 1
+@_ZTI10NoDisc = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2, i64 45546), ptr @_ZTS10NoDisc }, align 8
|
@llvm/pr-subscribers-clang Author: Daniil Kovalev (kovdan01) ChangesIn #99726, Fixes #101716 Full diff: https://github.com/llvm/llvm-project/pull/102199.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 9b5227772125b..13aeb3e75d684 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -3955,9 +3955,23 @@ void ItaniumRTTIBuilder::BuildVTablePointer(const Type *Ty) {
VTable, Two);
}
- if (auto &Schema = CGM.getCodeGenOpts().PointerAuth.CXXTypeInfoVTablePointer)
- VTable = CGM.getConstantSignedPointer(VTable, Schema, nullptr, GlobalDecl(),
- QualType(Ty, 0));
+ if (const auto &Schema =
+ CGM.getCodeGenOpts().PointerAuth.CXXTypeInfoVTablePointer) {
+ llvm::PointerType *PtrTy = llvm::PointerType::get(
+ CGM.getLLVMContext(),
+ CGM.getModule().getDataLayout().getProgramAddressSpace());
+ llvm::Constant *StorageAddress =
+ (Schema.isAddressDiscriminated()
+ ? llvm::ConstantExpr::getIntToPtr(
+ llvm::ConstantInt::get(
+ CGM.IntPtrTy,
+ llvm::ConstantPtrAuth::
+ AddrDiscriminator_CXXTypeInfoVTablePointer),
+ PtrTy)
+ : nullptr);
+ VTable = CGM.getConstantSignedPointer(VTable, Schema, StorageAddress,
+ GlobalDecl(), QualType(Ty, 0));
+ }
Fields.push_back(VTable);
}
diff --git a/clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp b/clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp
index 174aeda89d175..61eef73b5be2a 100644
--- a/clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp
+++ b/clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp
@@ -65,7 +65,7 @@ extern "C" int disc_std_type_info = __builtin_ptrauth_string_discriminator("_ZTV
// NODISC: @_ZTI10TestStruct = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2), ptr @_ZTS10TestStruct }, align 8
-// DISC: @_ZTI10TestStruct = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2, i64 [[STDTYPEINFO_DISC]]), ptr @_ZTS10TestStruct }, align 8
+// DISC: @_ZTI10TestStruct = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2, i64 [[STDTYPEINFO_DISC]], ptr inttoptr (i64 1 to ptr)), ptr @_ZTS10TestStruct }, align 8
struct TestStruct {
virtual ~TestStruct();
diff --git a/llvm/include/llvm/IR/Constants.h b/llvm/include/llvm/IR/Constants.h
index 2788751e8b62a..aaa1c197651a6 100644
--- a/llvm/include/llvm/IR/Constants.h
+++ b/llvm/include/llvm/IR/Constants.h
@@ -1056,12 +1056,18 @@ class ConstantPtrAuth final : public Constant {
return !getAddrDiscriminator()->isNullValue();
}
- /// A constant value for the address discriminator which has special
- /// significance to ctors/dtors lowering. Regular address discrimination can't
- /// be applied for them since uses of llvm.global_{c|d}tors are disallowed
- /// (see Verifier::visitGlobalVariable) and we can't emit getelementptr
- /// expressions referencing these special arrays.
- enum { AddrDiscriminator_CtorsDtors = 1 };
+ /// Constant values for the address discriminator which have special
+ /// significance to lowering in some contexts.
+ /// - For ctors/dtors, regular address discrimination can't
+ /// be applied for them since uses of llvm.global_{c|d}tors are disallowed
+ /// (see Verifier::visitGlobalVariable) and we can't emit getelementptr
+ /// expressions referencing these special arrays.
+ /// - For vtable pointers of std::type_info and classes derived from it,
+ /// we do not know the storage address when emitting ptrauth constant.
+ enum {
+ AddrDiscriminator_CtorsDtors = 1,
+ AddrDiscriminator_CXXTypeInfoVTablePointer = 1
+ };
/// Whether the address uses a special address discriminator.
/// These discriminators can't be used in real pointer-auth values; they
diff --git a/llvm/test/CodeGen/AArch64/ptrauth-type-info-vptr-discr.ll b/llvm/test/CodeGen/AArch64/ptrauth-type-info-vptr-discr.ll
new file mode 100644
index 0000000000000..b25a32f856b7a
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/ptrauth-type-info-vptr-discr.ll
@@ -0,0 +1,20 @@
+; RUN: llc -mtriple aarch64-linux-gnu -mattr=+pauth -filetype=asm -o - %s | FileCheck --check-prefix=ELF %s
+; RUN: llc -mtriple aarch64-apple-darwin -mattr=+pauth -filetype=asm -o - %s | FileCheck --check-prefix=MACHO %s
+
+; ELF-LABEL: _ZTI10Disc:
+; ELF-NEXT: .xword (_ZTVN10__cxxabiv117__class_type_infoE+16)@AUTH(da,45546,addr)
+; ELF-LABEL: _ZTI10NoDisc:
+; ELF-NEXT: .xword (_ZTVN10__cxxabiv117__class_type_infoE+16)@AUTH(da,45546)
+
+; MACHO-LABEL: __ZTI10Disc:
+; MACHO-NEXT: .quad (__ZTVN10__cxxabiv117__class_type_infoE+16)@AUTH(da,45546,addr)
+; MACHO-LABEL: __ZTI10NoDisc:
+; MACHO-NEXT: .quad (__ZTVN10__cxxabiv117__class_type_infoE+16)@AUTH(da,45546)
+
+@_ZTVN10__cxxabiv117__class_type_infoE = external global [0 x ptr]
+
+@_ZTS10Disc = constant [4 x i8] c"Disc", align 1
+@_ZTI10Disc = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2, i64 45546, ptr inttoptr (i64 1 to ptr)), ptr @_ZTS10Disc }, align 8
+
+@_ZTS10NoDisc = constant [6 x i8] c"NoDisc", align 1
+@_ZTI10NoDisc = constant { ptr, ptr } { ptr ptrauth (ptr getelementptr inbounds (ptr, ptr @_ZTVN10__cxxabiv117__class_type_infoE, i64 2), i32 2, i64 45546), ptr @_ZTS10NoDisc }, align 8
|
clang/lib/CodeGen/ItaniumCXXABI.cpp
Outdated
llvm::PointerType *PtrTy = llvm::PointerType::get( | ||
CGM.getLLVMContext(), | ||
CGM.getModule().getDataLayout().getProgramAddressSpace()); | ||
llvm::Constant *StorageAddress = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this be structured rather than using ?:
llvm::Constant *StorageAddress = nullptr;
if (Schema.isAddressDescriminated()) {
StorageAddress = llvm::ConstantExpr::getIntToPtr(
llvm::ConstantInt::get(
CGM.IntPtrTy,
llvm::ConstantPtrAuth::
AddrDiscriminator_CXXTypeInfoVTablePointer),
PtrTy);
}
This bug does make me wonder if getConstantSignedPointer(..)
etc should use a std::optional<&> rather than a pointer as that might have made it more obvious that the address was not being used in this code (obviously this is not a thing we should be changing in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this be structured rather than using ?:
The corresponding piece of code was deleted in the latest commit 0c20bcd, so probably the comment could be resolved.
Ok, it took me a moment to understand what was happening here - I had to talk to Ahmed because I didn't recognize the AddrDiscriminator_* enums and had assumed I'd forgotten them :D This needs to update |
llvm/include/llvm/IR/Constants.h
Outdated
/// (see Verifier::visitGlobalVariable) and we can't emit getelementptr | ||
/// expressions referencing these special arrays. | ||
/// - For vtable pointers of std::type_info and classes derived from it, | ||
/// we do not know the storage address when emitting ptrauth constant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to wire the storage address through ItaniumRTTIBuilder::BuildVTablePointer
from BuildTypeInfo
?
Beyond that, for static ctors, as I mentioned before, I still don't think this should have ptrauth at the IR level, but rather should be a backend decision made when encoding the IR representation of "a list of static ctors/dtors" to some object file format encoding thereof. That would avoid needing this address discriminator placeholder dance here. But I suppose that's a separate problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to wire the storage address through
ItaniumRTTIBuilder::BuildVTablePointer
fromBuildTypeInfo
?
@ahmedbougacha I was unable to find a straight-forward way to do that, but I might be missing smth (and it might be even smth obvious). I would be glad to see more detailed suggestions if you have some better implementation in mind - I agree that having an actual address instead of a placeholder is much better.
Beyond that, for static ctors, as I mentioned before, I still don't think this should have ptrauth at the IR level, but rather should be a backend decision made when encoding the IR representation of "a list of static ctors/dtors" to some object file format encoding thereof. That would avoid needing this address discriminator placeholder dance here. But I suppose that's a separate problem.
This sounds nice, it's an interesting suggestion. Anyway, init/fini stuff is out of scope of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only caller of BuildVTablePointer is BuildTypeInfo... and BuildTypeInfo itself creates the global variable in question. So it shouldn't be that hard. Just move the call to BuildVTablePointer after we construct the global.
Actually, that might be slightly trickier than I'm making it sound because we don't compute the type before we create the ConstantStruct, but it should be possible to separate computing the type from constructing the ConstantStruct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahmedbougacha @efriedma-quic Should be fixed in 0c20bcd
@efriedma-quic @ahmedbougacha @ojhunt Would be glad to see your comments after the latest commit 0c20bcd which introduces using actual storage address instead of 'inttoptr 1' placeholder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to avoid constructing the initializer twice if you restructure the code a bit to just construct the global before the call to BuildVTablePointer. I added GlobalVariable::replaceInitializer because I was looking at this review: it lets you construct the global before the initializer, without having to do awkward things with RAUW.
@efriedma-quic Sorry, I'm not sure if I got your point correctly. The issue with enabling address discrimination for signed vtable pointers is that it requires self-references - see, for example, llvm/test/CodeGen/AArch64/ptrauth-type-info-vptr-discr.ll:
As far as I understand, the correct way of creating such structures which contain self-references, is to use As for Do I miss smth? Can we somehow differently handle self-references and avoid using |
getAddrOfCurrentPosition() is one way to do it... but it's not the only way to do it. It was specifically designed to handle cases in CGExprConstant that would have been difficult to refactor otherwise. Here, it's simple to compute the correct address beforehand, so you can just do that. |
@efriedma-quic I've switched to |
clang/lib/CodeGen/ItaniumCXXABI.cpp
Outdated
new llvm::GlobalVariable(M, Init->getType(), | ||
/*isConstant=*/true, Linkage, Init, Name); | ||
llvm::GlobalVariable *GV = new llvm::GlobalVariable( | ||
M, llvm::ConstantStruct::getTypeForElements(Fields), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're going to replace the initializer anyway, you don't need to call ConstantStruct::getTypeForElements here; you can just use an arbitrary type, like i8. That should allow you to pass the global into BuildVTablePointer(), instead of replacing it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@efriedma-quic Thanks for suggestion! Followed this way in b08172f. This actually changes order of globals definitions in output IR and requires changing related tests correspondingly (see c9d26dd), but, as far as I understand, this should not cause any problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In #99726,
-fptrauth-type-info-vtable-pointer-discrimination
was introduced, which is intended to enable type and address discrimination for type_info vtable pointers. However, some codegen logic for actually enabling address discrimination was missing. This patch addresses the issue.Fixes #101716