Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[PAC] Fix address discrimination for type info vtable pointers #102199

Merged
merged 8 commits into from
Oct 18, 2024
Merged

[PAC] Fix address discrimination for type info vtable pointers #102199

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0c20bcd
[PAC] Fix address discrimination for type info vtable pointers
kovdan01 Aug 6, 2024
e0beb3f
Merge branch 'main' into pauth-type-info-addr-discr-fix
kovdan01 Sep 16, 2024
130ff55
Use real addr instead of placeholder for signed type info vt ptrs
kovdan01 Sep 16, 2024
969a877
Merge branch 'main' into pauth-type-info-addr-discr-fix
kovdan01 Sep 29, 2024
55fc2c8
Address review comments
kovdan01 Sep 30, 2024
90d6355
Merge branch 'main' into pauth-type-info-addr-discr-fix
kovdan01 Oct 17, 2024
b08172f
Construct GV before BuildVTablePointer call
kovdan01 Oct 17, 2024
c9d26dd
Fix tests
kovdan01 Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions clang/lib/CodeGen/ItaniumCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Copy link
Contributor

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)

Copy link
Contributor Author

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.

(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);
}
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGenCXX/ptrauth-type-info-vtable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
18 changes: 12 additions & 6 deletions llvm/include/llvm/IR/Constants.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

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.

Copy link
Contributor Author

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 ?

@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

Copy link
Collaborator

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.

Copy link
Contributor Author

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

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
Expand Down
20 changes: 20 additions & 0 deletions llvm/test/CodeGen/AArch64/ptrauth-type-info-vptr-discr.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Loading