[clang][PAC] Support trivially_relocating polymorphic objects #144420
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds support for trivial relocation of polymorphic objects with address discriminated vtable pointers. This is implemented as a post-memmove fixup pass over the impacted objects. We do this by traversing the object graph and finding all the vtable slots in the type being relocated, or any of the subobjects.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Oliver Hunt (ojhunt) ChangesAdds support for trivial relocation of polymorphic objects with address discriminated vtable pointers. This is implemented as a post-memmove fixup pass over the impacted objects. We do this by traversing the object graph and finding all the vtable slots in the type being relocated, or any of the subobjects. Patch is 38.76 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/144420.diff 6 Files Affected:
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 189e67e4eed0d..5760f382b59ed 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1727,6 +1727,11 @@ ASTContext::PointerAuthContent ASTContext::findPointerAuthContent(QualType T) {
T = T.getCanonicalType();
if (T.hasAddressDiscriminatedPointerAuth())
return PointerAuthContent::AddressDiscriminatedData;
+
+ T = getBaseElementType(T).getCanonicalType();
+ if (T.hasAddressDiscriminatedPointerAuth())
+ return PointerAuthContent::AddressDiscriminatedData;
+
const RecordDecl *RD = T->getAsRecordDecl();
if (!RD)
return PointerAuthContent::None;
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 1f69274351676..dea0f214f6806 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -4467,17 +4467,25 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
case Builtin::BI__builtin_trivially_relocate:
case Builtin::BImemmove:
case Builtin::BI__builtin_memmove: {
+ QualType CopiedType = E->getArg(0)->getType()->getPointeeType();
Address Dest = EmitPointerWithAlignment(E->getArg(0));
Address Src = EmitPointerWithAlignment(E->getArg(1));
Value *SizeVal = EmitScalarExpr(E->getArg(2));
- if (BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_trivially_relocate)
+ if (BuiltinIDIfNoAsmLabel == Builtin::BI__builtin_trivially_relocate) {
+ QualType BaseElementType = getContext().getBaseElementType(CopiedType);
+ if (getContext().containsAddressDiscriminatedPointerAuth(
+ BaseElementType)) {
+ llvm::Instruction *LastFixupInstruction =
+ EmitPointerAuthRelocationFixup(CopiedType, Dest, Src, SizeVal);
+ addInstToNewSourceAtom(LastFixupInstruction, nullptr);
+ return RValue::get(Dest, *this);
+ }
SizeVal = Builder.CreateMul(
SizeVal,
ConstantInt::get(
SizeVal->getType(),
- getContext()
- .getTypeSizeInChars(E->getArg(0)->getType()->getPointeeType())
- .getQuantity()));
+ getContext().getTypeSizeInChars(CopiedType).getQuantity()));
+ }
EmitArgCheck(TCK_Store, Dest, E->getArg(0), 0);
EmitArgCheck(TCK_Load, Src, E->getArg(1), 1);
auto *I = Builder.CreateMemMove(Dest, Src, SizeVal, false);
diff --git a/clang/lib/CodeGen/CGPointerAuth.cpp b/clang/lib/CodeGen/CGPointerAuth.cpp
index dcef01a5eb6d3..9ccfcb390292e 100644
--- a/clang/lib/CodeGen/CGPointerAuth.cpp
+++ b/clang/lib/CodeGen/CGPointerAuth.cpp
@@ -420,6 +420,222 @@ void CodeGenFunction::EmitPointerAuthCopy(PointerAuthQualifier Qual, QualType T,
Builder.CreateStore(Value, DestAddress);
}
+static const ConstantArrayType *tryGetTypeAsConstantArrayType(QualType T) {
+ if (!T->isConstantArrayType())
+ return nullptr;
+ return cast<ConstantArrayType>(T->castAsArrayTypeUnsafe());
+}
+
+using FixupErrorTy = std::pair<const CXXRecordDecl *, CXXBaseSpecifier>;
+class CodeGenFunction::FixupFinder {
+public:
+ using FixupVectorTy = CodeGenFunction::FixupVectorTy;
+ static FixupVectorTy findFixups(CodeGenFunction &CGF, QualType T) {
+ FixupFinder Finder(CGF);
+ FixupVectorTy Result;
+ Finder.findFixups(Result, T, CharUnits::Zero());
+ std::sort(Result.begin(), Result.end(),
+ [](const auto &L, const auto &R) { return L.Offset < R.Offset; });
+ return Result;
+ }
+
+private:
+ explicit FixupFinder(CodeGenFunction &CGF)
+ : CGF(CGF), Context(CGF.getContext()) {}
+
+ void findVTablePointerFixups(FixupVectorTy &Output, CXXRecordDecl *RD,
+ CharUnits Offset) {
+ CodeGenFunction::VPtrsVector VPtrs = CGF.getVTablePointers(RD);
+ for (auto VPtr : VPtrs) {
+ std::optional<PointerAuthQualifier> PointerAuth =
+ CGF.CGM.getVTablePointerAuthentication(VPtr.Base.getBase());
+ if (PointerAuth && PointerAuth->isAddressDiscriminated())
+ Output.push_back(
+ {Offset + VPtr.Base.getBaseOffset(), KnownNonNull, *PointerAuth});
+ }
+ }
+ void findObjectFixups(FixupVectorTy &Output, CXXRecordDecl *RD,
+ CharUnits Offset) {
+ if (RD->isPolymorphic())
+ findVTablePointerFixups(Output, RD, Offset);
+ findFixups(Output, RD, Offset, /*SubobjectIsBase=*/true);
+ }
+
+ void findFixups(FixupVectorTy &Output, CXXRecordDecl *RD,
+ CharUnits SubobjectOffset, bool SubobjectIsBase) {
+ // If we've found a union it by definition cannot contain
+ // address discriminated fields.
+ if (RD->isUnion())
+ return;
+ const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
+ if (Layout.hasOwnVFPtr() && RD == Layout.getPrimaryBase())
+ findVTablePointerFixups(Output, RD, SubobjectOffset);
+
+ for (auto Base : RD->bases()) {
+ CXXRecordDecl *BaseDecl =
+ Base.getType()->getAsCXXRecordDecl()->getDefinition();
+ assert(!Base.isVirtual());
+ CharUnits BaseOffset = Layout.getBaseClassOffset(BaseDecl);
+ findFixups(Output, BaseDecl, SubobjectOffset + BaseOffset,
+ /*SubobjectIsBase=*/true);
+ }
+
+ for (const FieldDecl *Field : RD->fields()) {
+ if (Field->isBitField())
+ continue;
+ unsigned FieldBitOffset = Layout.getFieldOffset(Field->getFieldIndex());
+ CharUnits FieldOffset = Context.toCharUnitsFromBits(FieldBitOffset);
+ findFixups(Output, Field->getType(), SubobjectOffset + FieldOffset);
+ }
+ }
+ void findFixups(FixupVectorTy &Output, QualType T, CharUnits Offset) {
+ T = T.getCanonicalType();
+ if (!Context.containsAddressDiscriminatedPointerAuth(T))
+ return;
+
+ if (const ConstantArrayType *CAT = tryGetTypeAsConstantArrayType(T)) {
+ if (CAT->getSize() == 0)
+ return;
+ Output.push_back({Offset, CAT});
+ return;
+ }
+
+ if (PointerAuthQualifier Q = T.getPointerAuth();
+ Q && Q.isAddressDiscriminated()) {
+ // FIXME: Would it be reasonable to consider nullability?
+ Output.push_back({Offset, NotKnownNonNull, Q});
+ return;
+ }
+
+ CXXRecordDecl *RD = T->getAsCXXRecordDecl();
+ if (!RD)
+ return;
+ findObjectFixups(Output, RD, Offset);
+ }
+ CodeGenFunction &CGF;
+ ASTContext &Context;
+};
+
+void CodeGenFunction::EmitSingleObjectPointerAuthRelocationFixup(
+ const FixupVectorTy &Fixups, QualType ElementType, Address Dst,
+ Address Src) {
+ auto GetFixupAddress = [&](Address BaseAddress, CharUnits Offset,
+ KnownNonNull_t IsKnownNonNull,
+ const char *Reason) {
+ llvm::Value *BasePtr = BaseAddress.emitRawPointer(*this);
+ llvm::Value *OffsetValue =
+ llvm::ConstantInt::get(PtrDiffTy, Offset.getQuantity());
+ llvm::Value *FixupAddress =
+ Builder.CreateInBoundsGEP(Int8Ty, BasePtr, OffsetValue, Reason);
+ return Address(FixupAddress, VoidPtrPtrTy,
+ BaseAddress.getAlignment().alignmentAtOffset(Offset),
+ IsKnownNonNull);
+ };
+ for (auto &Fixup : Fixups) {
+ if (const ConstantArrayType *CAT = Fixup.getAsConstantArrayType()) {
+ llvm::Value *CountValue = llvm::ConstantInt::get(SizeTy, 1);
+ EmitArrayPointerAuthRelocationFixup(QualType(CAT, 0), Dst, Src,
+ CountValue);
+ continue;
+ }
+ auto [IsKnownNonNull, Qualifier] = Fixup.getValueFixup();
+
+ // We don't use the existing copy helpers as we'll be resigning a
+ // value in place assuming the old address for the read.
+ Address FixupDst = GetFixupAddress(Dst, Fixup.Offset, IsKnownNonNull,
+ "fixup.dst.with.offset");
+ CGPointerAuthInfo DstPtrAuth = EmitPointerAuthInfo(Qualifier, FixupDst);
+
+ Address FixupSrc = GetFixupAddress(Src, Fixup.Offset, IsKnownNonNull,
+ "fixup.src.with.offset");
+ CGPointerAuthInfo SrcPtrAuth = EmitPointerAuthInfo(Qualifier, FixupSrc);
+
+ // We're loading from the destination here as we've already performed the
+ // copy from src to dst, and as relocation has memmove semantics, the src
+ // address may have been overwritten.
+ llvm::Value *Value = Builder.CreateLoad(FixupDst);
+ Value = emitPointerAuthResign(Value, QualType(), SrcPtrAuth, DstPtrAuth,
+ IsKnownNonNull);
+ Builder.CreateStore(Value, FixupDst);
+ }
+}
+
+llvm::Instruction *CodeGenFunction::EmitArrayPointerAuthRelocationFixup(
+ QualType ElementType, Address Dst, Address Src, llvm::Value *Count) {
+ // Preemptively flatten array types so we don't end up with multiple levels
+ // of loops unnecessarily
+ if (const ConstantArrayType *CAT =
+ tryGetTypeAsConstantArrayType(ElementType)) {
+ uint64_t ElementCount = getContext().getConstantArrayElementCount(CAT);
+ llvm::Value *ElementCountValue =
+ llvm::ConstantInt::get(SizeTy, ElementCount);
+ Count = Builder.CreateMul(Count, ElementCountValue);
+ ElementType = getContext().getBaseElementType(QualType(CAT, 0));
+ }
+
+ FixupVectorTy *Fixups;
+ if (const auto Existing = FixupLists.find(ElementType);
+ Existing != FixupLists.end())
+ Fixups = Existing->second.get();
+ else {
+ auto FoundFixups = FixupFinder::findFixups(*this, ElementType);
+ auto [EntryPoint, Inserted] = FixupLists.try_emplace(
+ ElementType, std::make_unique<FixupVectorTy>(std::move(FoundFixups)));
+ (void)Inserted;
+ Fixups = EntryPoint->second.get();
+ }
+
+ CharUnits ElementSize = getContext().getTypeSizeInChars(ElementType);
+ CharUnits ElementAlign =
+ Src.getAlignment().alignmentOfArrayElement(ElementSize);
+ llvm::Type *LLVMElemType = ConvertTypeForMem(ElementType);
+
+ llvm::BasicBlock *RelocationFixupEntry = Builder.GetInsertBlock();
+ llvm::BasicBlock *RelocationFixupBody =
+ createBasicBlock("relocation_ptrauth_fixup.body");
+ EmitBlock(RelocationFixupBody);
+ llvm::Value *Zero = llvm::ConstantInt::get(SizeTy, 0);
+ llvm::PHINode *Index =
+ Builder.CreatePHI(SizeTy, 2, "relocation_ptrauth_fixup.index");
+ Index->addIncoming(Zero, RelocationFixupEntry);
+ llvm::Value *DstElement =
+ Builder.CreateInBoundsGEP(LLVMElemType, Dst.emitRawPointer(*this), Index,
+ "relocation_ptrauth_fixup.dstobject");
+ Address DstElementAddress = Address(DstElement, LLVMElemType, ElementAlign);
+ llvm::Value *SrcElement =
+ Builder.CreateInBoundsGEP(LLVMElemType, Src.emitRawPointer(*this), Index,
+ "relocation_ptrauth_fixup.srcobject");
+ Address SrcElementAddress = Address(SrcElement, LLVMElemType, ElementAlign);
+
+ // Do the fixup
+ EmitSingleObjectPointerAuthRelocationFixup(
+ *Fixups, ElementType, DstElementAddress, SrcElementAddress);
+
+ llvm::Value *NextIndex =
+ Builder.CreateNUWAdd(Index, llvm::ConstantInt::get(Index->getType(), 1),
+ "relocation_ptrauth_fixup.next_index");
+ Index->addIncoming(NextIndex, Builder.GetInsertBlock());
+ llvm::Value *IsComplete = Builder.CreateICmpEQ(
+ NextIndex, Count, "relocation_ptrauth_fixup.is_complete");
+ llvm::BasicBlock *RelocationFixupFinished =
+ createBasicBlock("relocation_ptrauth_fixup.end");
+ Builder.CreateCondBr(IsComplete, RelocationFixupFinished,
+ RelocationFixupBody);
+ EmitBlock(RelocationFixupFinished);
+ return RelocationFixupFinished->getTerminator();
+}
+
+llvm::Instruction *CodeGenFunction::EmitPointerAuthRelocationFixup(
+ QualType ElementType, Address Dst, Address Src, llvm::Value *Count) {
+ size_t ElementSize =
+ getContext().getTypeSizeInChars(ElementType).getQuantity();
+ llvm::Value *ElementSizeValue =
+ llvm::ConstantInt::get(Count->getType(), ElementSize);
+ llvm::Value *Size = Builder.CreateMul(Count, ElementSizeValue);
+ Builder.CreateMemMove(Dst, Src, Size, false);
+ return EmitArrayPointerAuthRelocationFixup(ElementType, Dst, Src, Count);
+}
+
llvm::Constant *
CodeGenModule::getConstantSignedPointer(llvm::Constant *Pointer, unsigned Key,
llvm::Constant *StorageAddress,
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index a5ab9df01dba9..16098e83f0cd6 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -4610,6 +4610,10 @@ class CodeGenFunction : public CodeGenTypeCache {
void EmitPointerAuthCopy(PointerAuthQualifier Qualifier, QualType Type,
Address DestField, Address SrcField);
+ llvm::Instruction *EmitPointerAuthRelocationFixup(QualType ElementType,
+ Address Dst, Address Src,
+ llvm::Value *SrcEnd);
+
std::pair<llvm::Value *, CGPointerAuthInfo>
EmitOrigPointerRValue(const Expr *E);
@@ -4618,6 +4622,46 @@ class CodeGenFunction : public CodeGenTypeCache {
Address authPointerToPointerCast(Address Ptr, QualType SourceType,
QualType DestType);
+private:
+ llvm::Instruction *EmitArrayPointerAuthRelocationFixup(QualType ElementType,
+ Address Dst,
+ Address Src,
+ llvm::Value *Count);
+
+ struct RelocatedAddressDiscriminatedPointerAuthFixup {
+ using FieldFixup = std::pair<KnownNonNull_t, PointerAuthQualifier>;
+ CharUnits Offset;
+ RelocatedAddressDiscriminatedPointerAuthFixup(
+ CharUnits Offset, KnownNonNull_t IsKnownNonNull,
+ PointerAuthQualifier Qualifier)
+ : Offset(Offset), Info(FieldFixup{IsKnownNonNull, Qualifier}) {}
+ RelocatedAddressDiscriminatedPointerAuthFixup(
+ CharUnits Offset, const ConstantArrayType *ArrayType)
+ : Offset(Offset), Info(ArrayType) {}
+ const ConstantArrayType *getAsConstantArrayType() const {
+ if (!std::holds_alternative<const ConstantArrayType *>(Info))
+ return nullptr;
+ return std::get<const ConstantArrayType *>(Info);
+ }
+ std::pair<KnownNonNull_t, PointerAuthQualifier> getValueFixup() const {
+ assert(std::holds_alternative<FieldFixup>(Info));
+ return std::get<FieldFixup>(Info);
+ }
+
+ private:
+ std::variant<FieldFixup, const ConstantArrayType *> Info;
+ };
+
+ using FixupVectorTy =
+ llvm::SmallVector<RelocatedAddressDiscriminatedPointerAuthFixup>;
+ class FixupFinder;
+ llvm::DenseMap<QualType, std::unique_ptr<FixupVectorTy>> FixupLists;
+
+ void EmitSingleObjectPointerAuthRelocationFixup(const FixupVectorTy &Fixups,
+ QualType ElementType,
+ Address Dst, Address Src);
+
+public:
Address getAsNaturalAddressOf(Address Addr, QualType PointeeTy);
llvm::Value *getAsNaturalPointerTo(Address Addr, QualType PointeeType) {
diff --git a/clang/test/CodeGenCXX/cxx2c-trivially-relocatable-ptrauth.cpp b/clang/test/CodeGenCXX/cxx2c-trivially-relocatable-ptrauth.cpp
new file mode 100644
index 0000000000000..e83858187f33d
--- /dev/null
+++ b/clang/test/CodeGenCXX/cxx2c-trivially-relocatable-ptrauth.cpp
@@ -0,0 +1,362 @@
+// RUN: %clang_cc1 -std=c++26 -triple aarch64-linux-gnu -fptrauth-intrinsics -fptrauth-calls -emit-llvm -o - %s | FileCheck %s
+
+typedef __SIZE_TYPE__ size_t;
+
+
+#if 1
+#define vtable_ptrauth(...) [[clang::ptrauth_vtable_pointer(__VA_ARGS__)]]
+
+#else
+#define vtable_ptrauth(...)
+#define __ptrauth(...)
+#endif
+
+#define ADDR_AND_TYPE_DISC vtable_ptrauth(process_independent, address_discrimination, type_discrimination)
+#define TYPE_DISC_ONLY vtable_ptrauth(process_independent, no_address_discrimination, type_discrimination)
+
+struct TYPE_DISC_ONLY NoAddrDiscPoly trivially_relocatable_if_eligible {
+ NoAddrDiscPoly(const NoAddrDiscPoly&);
+ virtual ~NoAddrDiscPoly();
+ int *__ptrauth(1,0,1) no_addr_disc;
+ int b;
+};
+
+// A simple test to ensure that we don't do anything more than the memmove
+// if there's no actual reason to do so, despite being in a configuration
+// where in principle such work _could_ be required
+// CHECK-LABEL: define internal void @_ZL4testI14NoAddrDiscPolyEvPvS1_m(
+// CHECK: call void @llvm.memmove.p0.p0.i64(ptr align 8 %2, ptr align 8 %3, i64 %5, i1 false)
+// CHECK-NEXT: ret void
+
+struct ADDR_AND_TYPE_DISC AddrDiscPoly trivially_relocatable_if_eligible {
+ AddrDiscPoly(const AddrDiscPoly&);
+ virtual ~AddrDiscPoly();
+ int *__ptrauth(1,0,1) no_addr_disc;
+ int b;
+};
+
+// CHECK-LABEL: define internal void @_ZL4testI12AddrDiscPolyEvPvS1_m(
+// CHECK: [[DST_PTR:%.*]] = load ptr, ptr %dest, align 8
+// CHECK: [[SRC_PTR:%.*]] = load ptr, ptr %source, align 8
+// CHECK: [[COUNT:%.*]] = load i64, ptr %count.addr, align 8
+// CHECK: [[SIZE:%.*]] = mul i64 [[COUNT]], 24
+// CHECK: call void @llvm.memmove.p0.p0.i64(ptr{{.*}}[[DST_PTR]], ptr{{.*}}[[SRC_PTR]], i64 [[SIZE]], i1 false)
+// CHECK: br label %[[COPY_BODY:[a-zA-Z._]+]]
+// CHECK: [[COPY_BODY]]:
+// CHECK-NEXT: [[INDEX:%.*]] = phi i64 [ 0, %entry ], [ [[NEXT_INDEX:%.*]], %[[COPY_BODY]] ]
+// CHECK: [[DST_OBJ:%.*]] = getelementptr inbounds %struct.AddrDiscPoly, ptr [[DST_PTR]], i64 [[INDEX]]
+// CHECK: [[SRC_OBJ:%.*]] = getelementptr inbounds %struct.AddrDiscPoly, ptr [[SRC_PTR]], i64 [[INDEX]]
+// CHECK: [[FIXUP_DST_ADDR:%.*]] = getelementptr inbounds i8, ptr [[DST_OBJ]], i64 0
+// CHECK: [[FIXUP_DST_ADDR_INT:%.*]] = ptrtoint ptr [[FIXUP_DST_ADDR]] to i64
+// CHECK: [[FIXUP_DST_DISC:%.*]] = call i64 @llvm.ptrauth.blend(i64 [[FIXUP_DST_ADDR_INT]], i64 [[TYPE_DISC:49645]])
+// CHECK: [[FIXUP_SRC_ADDR:%.*]] = getelementptr inbounds i8, ptr [[SRC_OBJ]], i64 0
+// CHECK: [[FIXUP_SRC_ADDR_INT:%.*]] = ptrtoint ptr [[FIXUP_SRC_ADDR]] to i64
+// CHECK: [[FIXUP_SRC_DISC:%.*]] = call i64 @llvm.ptrauth.blend(i64 [[FIXUP_SRC_ADDR_INT]], i64 [[TYPE_DISC]])
+// CHECK: [[PREFIXUP_VALUE:%.*]] = load ptr, ptr [[FIXUP_DST_ADDR]]
+// CHECK: [[PREFIXUP_VALUE_INT:%.*]] = ptrtoint ptr [[PREFIXUP_VALUE]] to i64
+// CHECK: [[FIXEDUP_VALUE_INT:%.*]] = call i64 @llvm.ptrauth.resign(i64 [[PREFIXUP_VALUE_INT]], i32 2, i64 [[FIXUP_SRC_DISC]], i32 2, i64 [[FIXUP_DST_DISC]])
+// CHECK: [[FIXEDUP_VALUE:%.*]] = inttoptr i64 [[FIXEDUP_VALUE_INT]] to ptr
+// CHECK: store ptr [[FIXEDUP_VALUE]], ptr [[FIXUP_DST_ADDR]]
+// CHECK: [[NEXT_INDEX:%.*]] = add nuw i64 [[INDEX]], 1
+// CHECK: [[IS_COMPLETE:%.*]] = icmp eq i64 [[NEXT_INDEX]], [[COUNT]]
+// CHECK: br i1 [[IS_COMPLETE]], label %[[FIXUP_END:[A-Za-z._]*]], label %[[COPY_BODY]]
+// CHECK: [[FIXUP_END]]:
+// CHECK-NEXT: ret void
+
+struct ADDR_AND_TYPE_DISC A trivially_relocatable_if_eligible {
+ virtual ~A();
+ int i;
+};
+
+struct ADDR_AND_TYPE_DISC B trivially_relocatable_if_eligible {
+ virtual ~B();
+ int j;
+};
+
+struct ADDR_AND_TYPE_DISC C trivially_relocatable_if_eligible {
+ virtual ~C();
+ int k;
+};
+
+struct ADDR_AND_TYPE_DISC D trivially_relocatable_if_eligible {
+ virtual ~D();
+ int l;
+};
+
+// Though different types, the structure of MultipleBaseClasses1
+// and MultipleBaseClasses1 is actually identical
+struct MultipleBaseClasses1 trivially_relocatable_if_eligible : A, B {
+ C c;
+ D d;
+};
+
+// CHECK-LABEL: define internal void @_ZL4testI20MultipleBaseClasses1EvPvS1_m(
+// CHECK: [[DST_PTR:%.*]] = load ptr, ptr %dest, align 8
+// CHECK: [[SRC_PTR:%.*]] = load ptr, ptr %source, align 8
+// CHECK: [[COUNT:%.*]] = load i64, ptr %count.addr, align 8
+// CHECK: [[SIZE:%.*]] = mul i64 [[COUNT]], 64
+// CHECK: call void @llvm.memmove.p0.p0.i64(ptr{{.*}}[[DST_PTR]], ptr{{.*}}[[SRC_PTR]], i64 [[SIZE]], i1 false)
+// CHECK: br label %[[COPY_BODY:[a-zA-Z._]+]]
+// CHECK: [[COPY_BODY]]:
+// CHECK-NEXT: [[INDEX:%.*]] = phi i64 [ 0, %entry ], [ [[NEXT_INDEX:%.*]], %[[COPY_BODY]] ]
+// CHECK: [[DST_OBJ:%.*]] = getelementptr inbounds %struct.MultipleBaseClasses1, ptr [[DST_PTR]], i64 [[INDEX]]
+// CHECK: [[SRC_OBJ:%.*]] = getelementptr inbounds %struct.MultipleBaseClasses1, ptr [[SRC_PTR]], i64 [[INDEX]]
+
+// Fixup 1: MultipleBaseClasses1::A vtable pointer
+// CHECK: [[FIXUP_DST_ADDR:%.*]] = getelementptr inbounds i8, ptr...
[truncated]
|
ojhunt
commented
Jun 16, 2025
| @@ -1727,6 +1727,11 @@ ASTContext::PointerAuthContent ASTContext::findPointerAuthContent(QualType T) { | |||
| T = T.getCanonicalType(); | |||
| if (T.hasAddressDiscriminatedPointerAuth()) | |||
| return PointerAuthContent::AddressDiscriminatedData; | |||
|
|
|||
| T = getBaseElementType(T).getCanonicalType(); | |||
There was a problem hiding this comment.
Adding this simplifies the logic in codegen, and to an extent removes a footgun in the form of not requiring every caller to lower the type to the base element prior to calling.
ojhunt
commented
Jun 16, 2025
| if (PointerAuthQualifier Q = T.getPointerAuth(); | ||
| Q && Q.isAddressDiscriminated()) { | ||
| // FIXME: Would it be reasonable to consider nullability? | ||
| Output.push_back({Offset, NotKnownNonNull, Q}); |
There was a problem hiding this comment.
I think this is a reasonable question, but it would also turn an incorrectly null value become a runtime error?
rjmccall
reviewed
Jun 17, 2025
There was a problem hiding this comment.
If it's not a bitwise copy, it's by definition not a trivial relocation, so I don't think this works. I think you're arguing for some kind of intermediate "non-failing relocation" feature.
Are we really reporting polymorphic class types as trivially relocatable?
|
I've been informed that I've misunderstood the nature of "trivial relocation" — it is not a "trivial" operation in the usual sense of triviality, it's just an operation that (1) can be done primitively by the compiler and (2) cannot fail. That is exactly the non-failing relocation operation I mentioned. That's not what I would have expected to be standardized, but if it's what the committee did, it addresses my concern, because we can certainly implement it for polymorphic classes. I think we should make a point of implementing this for address-discriminated |
cor3ntin
reviewed
Jun 18, 2025
Comment on lines
+4477
to
+4480
| BaseElementType)) { | ||
| llvm::Instruction *LastFixupInstruction = | ||
| EmitPointerAuthRelocationFixup(CopiedType, Dest, Src, SizeVal); | ||
| addInstToNewSourceAtom(LastFixupInstruction, nullptr); |
There was a problem hiding this comment.
It's not really a fixup if it's doing the reallocation inside the fixup.
So either we need a better name, or we need to do it after the memmove line 4492
There was a problem hiding this comment.
I was so confused by this comment, to the extent I thought I had managed to break this and not catch it, but yet I agree now the name should be changed :D
When I first wrote this the memmove was not part of that function, so it was purely a fixup, but that is obviously no longer the case.
I had been talking to @cor3ntin and decided we could do it later as this is all intrinsics and semantics that would be tied to a single TU, but I can see that it would result in different symbols being resolved or produced. We felt that it would be sufficiently localized to TU level behavior that it wouldn't leak, but I could see that I might be possible to get it leak out from there. @rjmccall Do you have an example you're thinking of that you think is plausible - just so I can't mull it/turn it over in my head? The earlier implementation did fixup address discriminated explicitly qualified data, and as you commented (not on GH) you can see the model I built out for the fixups easily supports that so it's not an implementation difficulty problem (there are some performance questions that come up, but they're just QoI issues that can be addressed without changing behavior or semantics). |
|
It is very common for type traits to factor into the ABI of a template specialization, such that |
ah right, it's not that the instantiated symbol directly references the trait in mangling or similar, it's that the layout differs based on the query even though the symbol name does not reference the trait. @cor3ntin I think the correct call is to revert the removal of support for explicit qualifiers even though it does mean we'll need to do more work on perf design in future - do you agree with @rjmccall's rationale? |
|
Converting to draft so I don't accidentally hit the big green commit button (that is big and green despite no approvals ?!?!) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds support for trivial relocation of polymorphic objects with address discriminated vtable pointers.
This is implemented as a post-memmove fixup pass over the impacted objects. We do this by traversing the object graph and finding all the vtable slots in the type being relocated, or any of the subobjects.