[clang] [UBSAN] [NFC] add coverage for null and alignment checks (PR #176210)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 15 10:17:48 PST 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: VASU SHARMA (vasu-the-sharma)
<details>
<summary>Changes</summary>
---
Patch is 166.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/176210.diff
4 Files Affected:
- (modified) clang/lib/CodeGen/CGClass.cpp (+784-818)
- (modified) clang/lib/CodeGen/CGExprAgg.cpp (+82-97)
- (modified) clang/lib/CodeGen/CGExprCXX.cpp (+234-259)
- (added) clang/test/CodeGen/ubsan-aggregate-null-align.c (+507)
``````````diff
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index 96fde10f24f32..688f70c2b4643 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -190,10 +190,9 @@ CharUnits CodeGenModule::computeNonVirtualBaseClassOffset(
return Offset;
}
-llvm::Constant *
-CodeGenModule::GetNonVirtualBaseClassOffset(const CXXRecordDecl *ClassDecl,
- CastExpr::path_const_iterator PathBegin,
- CastExpr::path_const_iterator PathEnd) {
+llvm::Constant *CodeGenModule::GetNonVirtualBaseClassOffset(
+ const CXXRecordDecl *ClassDecl, CastExpr::path_const_iterator PathBegin,
+ CastExpr::path_const_iterator PathEnd) {
assert(PathBegin != PathEnd && "Base path should not be empty!");
CharUnits Offset =
@@ -212,11 +211,9 @@ CodeGenModule::GetNonVirtualBaseClassOffset(const CXXRecordDecl *ClassDecl,
/// when the type is known to be complete (e.g. in complete destructors).
///
/// The object pointed to by 'This' is assumed to be non-null.
-Address
-CodeGenFunction::GetAddressOfDirectBaseInCompleteClass(Address This,
- const CXXRecordDecl *Derived,
- const CXXRecordDecl *Base,
- bool BaseIsVirtual) {
+Address CodeGenFunction::GetAddressOfDirectBaseInCompleteClass(
+ Address This, const CXXRecordDecl *Derived, const CXXRecordDecl *Base,
+ bool BaseIsVirtual) {
// 'this' must be a pointer (in some address space) to Derived.
assert(This.getElementType() == ConvertType(Derived));
@@ -238,12 +235,10 @@ CodeGenFunction::GetAddressOfDirectBaseInCompleteClass(Address This,
return V.withElementType(ConvertType(Base));
}
-static Address
-ApplyNonVirtualAndVirtualOffset(CodeGenFunction &CGF, Address addr,
- CharUnits nonVirtualOffset,
- llvm::Value *virtualOffset,
- const CXXRecordDecl *derivedClass,
- const CXXRecordDecl *nearestVBase) {
+static Address ApplyNonVirtualAndVirtualOffset(
+ CodeGenFunction &CGF, Address addr, CharUnits nonVirtualOffset,
+ llvm::Value *virtualOffset, const CXXRecordDecl *derivedClass,
+ const CXXRecordDecl *nearestVBase) {
// Assert that we have something to do.
assert(!nonVirtualOffset.isZero() || virtualOffset != nullptr);
@@ -273,8 +268,8 @@ ApplyNonVirtualAndVirtualOffset(CodeGenFunction &CGF, Address addr,
CharUnits alignment;
if (virtualOffset) {
assert(nearestVBase && "virtual offset without vbase?");
- alignment = CGF.CGM.getVBaseAlignment(addr.getAlignment(),
- derivedClass, nearestVBase);
+ alignment = CGF.CGM.getVBaseAlignment(addr.getAlignment(), derivedClass,
+ nearestVBase);
} else {
alignment = addr.getAlignment();
}
@@ -390,19 +385,17 @@ Address CodeGenFunction::GetAddressOfBaseClass(
return Value;
}
-Address
-CodeGenFunction::GetAddressOfDerivedClass(Address BaseAddr,
- const CXXRecordDecl *Derived,
- CastExpr::path_const_iterator PathBegin,
- CastExpr::path_const_iterator PathEnd,
- bool NullCheckValue) {
+Address CodeGenFunction::GetAddressOfDerivedClass(
+ Address BaseAddr, const CXXRecordDecl *Derived,
+ CastExpr::path_const_iterator PathBegin,
+ CastExpr::path_const_iterator PathEnd, bool NullCheckValue) {
assert(PathBegin != PathEnd && "Base path should not be empty!");
CanQualType DerivedTy = getContext().getCanonicalTagType(Derived);
llvm::Type *DerivedValueTy = ConvertType(DerivedTy);
llvm::Value *NonVirtualOffset =
- CGM.GetNonVirtualBaseClassOffset(Derived, PathBegin, PathEnd);
+ CGM.GetNonVirtualBaseClassOffset(Derived, PathBegin, PathEnd);
if (!NonVirtualOffset) {
// No offset, we can just cast back.
@@ -475,12 +468,11 @@ llvm::Value *CodeGenFunction::GetVTTParameter(GlobalDecl GD,
SubVTTIndex = 0;
} else {
const ASTRecordLayout &Layout = getContext().getASTRecordLayout(RD);
- CharUnits BaseOffset = ForVirtualBase ?
- Layout.getVBaseClassOffset(Base) :
- Layout.getBaseClassOffset(Base);
+ CharUnits BaseOffset = ForVirtualBase ? Layout.getVBaseClassOffset(Base)
+ : Layout.getBaseClassOffset(Base);
SubVTTIndex =
- CGM.getVTables().getSubVTTIndex(RD, BaseSubobject(Base, BaseOffset));
+ CGM.getVTables().getSubVTTIndex(RD, BaseSubobject(Base, BaseOffset));
assert(SubVTTIndex != 0 && "Sub-VTT index must be greater than zero!");
}
@@ -491,52 +483,51 @@ llvm::Value *CodeGenFunction::GetVTTParameter(GlobalDecl GD,
} else {
// We're the complete constructor, so get the VTT by name.
llvm::GlobalValue *VTT = CGM.getVTables().GetAddrOfVTT(RD);
- return Builder.CreateConstInBoundsGEP2_64(
- VTT->getValueType(), VTT, 0, SubVTTIndex);
+ return Builder.CreateConstInBoundsGEP2_64(VTT->getValueType(), VTT, 0,
+ SubVTTIndex);
}
}
namespace {
- /// Call the destructor for a direct base class.
- struct CallBaseDtor final : EHScopeStack::Cleanup {
- const CXXRecordDecl *BaseClass;
- bool BaseIsVirtual;
- CallBaseDtor(const CXXRecordDecl *Base, bool BaseIsVirtual)
+/// Call the destructor for a direct base class.
+struct CallBaseDtor final : EHScopeStack::Cleanup {
+ const CXXRecordDecl *BaseClass;
+ bool BaseIsVirtual;
+ CallBaseDtor(const CXXRecordDecl *Base, bool BaseIsVirtual)
: BaseClass(Base), BaseIsVirtual(BaseIsVirtual) {}
- void Emit(CodeGenFunction &CGF, Flags flags) override {
- const CXXRecordDecl *DerivedClass =
+ void Emit(CodeGenFunction &CGF, Flags flags) override {
+ const CXXRecordDecl *DerivedClass =
cast<CXXMethodDecl>(CGF.CurCodeDecl)->getParent();
- const CXXDestructorDecl *D = BaseClass->getDestructor();
- // We are already inside a destructor, so presumably the object being
- // destroyed should have the expected type.
- QualType ThisTy = D->getFunctionObjectParameterType();
- Address Addr =
- CGF.GetAddressOfDirectBaseInCompleteClass(CGF.LoadCXXThisAddress(),
- DerivedClass, BaseClass,
- BaseIsVirtual);
- CGF.EmitCXXDestructorCall(D, Dtor_Base, BaseIsVirtual,
- /*Delegating=*/false, Addr, ThisTy);
- }
- };
+ const CXXDestructorDecl *D = BaseClass->getDestructor();
+ // We are already inside a destructor, so presumably the object being
+ // destroyed should have the expected type.
+ QualType ThisTy = D->getFunctionObjectParameterType();
+ Address Addr = CGF.GetAddressOfDirectBaseInCompleteClass(
+ CGF.LoadCXXThisAddress(), DerivedClass, BaseClass, BaseIsVirtual);
+ CGF.EmitCXXDestructorCall(D, Dtor_Base, BaseIsVirtual,
+ /*Delegating=*/false, Addr, ThisTy);
+ }
+};
- /// A visitor which checks whether an initializer uses 'this' in a
- /// way which requires the vtable to be properly set.
- struct DynamicThisUseChecker : ConstEvaluatedExprVisitor<DynamicThisUseChecker> {
- typedef ConstEvaluatedExprVisitor<DynamicThisUseChecker> super;
+/// A visitor which checks whether an initializer uses 'this' in a
+/// way which requires the vtable to be properly set.
+struct DynamicThisUseChecker
+ : ConstEvaluatedExprVisitor<DynamicThisUseChecker> {
+ typedef ConstEvaluatedExprVisitor<DynamicThisUseChecker> super;
- bool UsesThis;
+ bool UsesThis;
- DynamicThisUseChecker(const ASTContext &C) : super(C), UsesThis(false) {}
+ DynamicThisUseChecker(const ASTContext &C) : super(C), UsesThis(false) {}
- // Black-list all explicit and implicit references to 'this'.
- //
- // Do we need to worry about external references to 'this' derived
- // from arbitrary code? If so, then anything which runs arbitrary
- // external code might potentially access the vtable.
- void VisitCXXThisExpr(const CXXThisExpr *E) { UsesThis = true; }
- };
+ // Black-list all explicit and implicit references to 'this'.
+ //
+ // Do we need to worry about external references to 'this' derived
+ // from arbitrary code? If so, then anything which runs arbitrary
+ // external code might potentially access the vtable.
+ void VisitCXXThisExpr(const CXXThisExpr *E) { UsesThis = true; }
+};
} // end anonymous namespace
static bool BaseInitializerUsesThis(ASTContext &C, const Expr *Init) {
@@ -548,8 +539,7 @@ static bool BaseInitializerUsesThis(ASTContext &C, const Expr *Init) {
static void EmitBaseInitializer(CodeGenFunction &CGF,
const CXXRecordDecl *ClassDecl,
CXXCtorInitializer *BaseInit) {
- assert(BaseInit->isBaseInitializer() &&
- "Must have base initializer!");
+ assert(BaseInit->isBaseInitializer() && "Must have base initializer!");
Address ThisPtr = CGF.LoadCXXThisAddress();
@@ -565,17 +555,12 @@ static void EmitBaseInitializer(CodeGenFunction &CGF,
// We can pretend to be a complete class because it only matters for
// virtual bases, and we only do virtual bases for complete ctors.
- Address V =
- CGF.GetAddressOfDirectBaseInCompleteClass(ThisPtr, ClassDecl,
- BaseClassDecl,
- isBaseVirtual);
- AggValueSlot AggSlot =
- AggValueSlot::forAddr(
- V, Qualifiers(),
- AggValueSlot::IsDestructed,
- AggValueSlot::DoesNotNeedGCBarriers,
- AggValueSlot::IsNotAliased,
- CGF.getOverlapForBaseInit(ClassDecl, BaseClassDecl, isBaseVirtual));
+ Address V = CGF.GetAddressOfDirectBaseInCompleteClass(
+ ThisPtr, ClassDecl, BaseClassDecl, isBaseVirtual);
+ AggValueSlot AggSlot = AggValueSlot::forAddr(
+ V, Qualifiers(), AggValueSlot::IsDestructed,
+ AggValueSlot::DoesNotNeedGCBarriers, AggValueSlot::IsNotAliased,
+ CGF.getOverlapForBaseInit(ClassDecl, BaseClassDecl, isBaseVirtual));
CGF.EmitAggExpr(BaseInit->getInit(), AggSlot);
@@ -649,8 +634,8 @@ static void EmitMemberInitializer(CodeGenFunction &CGF,
// AST and perform the copy we know is equivalent.
// FIXME: This is hacky at best... if we had a bit more explicit information
// in the AST, we could generalize it more easily.
- const ConstantArrayType *Array
- = CGF.getContext().getAsConstantArrayType(FieldType);
+ const ConstantArrayType *Array =
+ CGF.getContext().getAsConstantArrayType(FieldType);
if (Array && Constructor->isDefaulted() &&
Constructor->isCopyOrMoveConstructor()) {
QualType BaseElementTy = CGF.getContext().getBaseElementType(Array);
@@ -659,13 +644,14 @@ static void EmitMemberInitializer(CodeGenFunction &CGF,
(CE && isMemcpyEquivalentSpecialMember(CE->getConstructor()))) {
unsigned SrcArgIndex =
CGF.CGM.getCXXABI().getSrcArgforCopyCtor(Constructor, Args);
- llvm::Value *SrcPtr
- = CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(Args[SrcArgIndex]));
+ llvm::Value *SrcPtr =
+ CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(Args[SrcArgIndex]));
LValue ThisRHSLV = CGF.MakeNaturalAlignAddrLValue(SrcPtr, RecordTy);
LValue Src = CGF.EmitLValueForFieldInitialization(ThisRHSLV, Field);
// Copy the aggregate.
- CGF.EmitAggregateCopy(LHS, Src, FieldType, CGF.getOverlapForFieldInit(Field),
+ CGF.EmitAggregateCopy(LHS, Src, FieldType,
+ CGF.getOverlapForFieldInit(Field),
LHS.isVolatileQualified());
// Ensure that we destroy the objects if an exception is thrown later in
// the constructor.
@@ -770,7 +756,8 @@ void CodeGenFunction::EmitAsanPrologueOrEpilogue(bool Prologue) {
const CXXRecordDecl *ClassDecl =
Prologue ? cast<CXXConstructorDecl>(CurGD.getDecl())->getParent()
: cast<CXXDestructorDecl>(CurGD.getDecl())->getParent();
- if (!ClassDecl->mayInsertExtraPadding()) return;
+ if (!ClassDecl->mayInsertExtraPadding())
+ return;
struct SizeAndOffset {
uint64_t Size;
@@ -796,13 +783,13 @@ void CodeGenFunction::EmitAsanPrologueOrEpilogue(bool Prologue) {
NumFields++;
}
assert(NumFields == SSV.size());
- if (SSV.size() <= 1) return;
+ if (SSV.size() <= 1)
+ return;
// We will insert calls to __asan_* run-time functions.
// LLVM AddressSanitizer pass may decide to inline them later.
llvm::Type *Args[2] = {IntPtrTy, IntPtrTy};
- llvm::FunctionType *FTy =
- llvm::FunctionType::get(CGM.VoidTy, Args, false);
+ llvm::FunctionType *FTy = llvm::FunctionType::get(CGM.VoidTy, Args, false);
llvm::FunctionCallee F = CGM.CreateRuntimeFunction(
FTy, Prologue ? "__asan_poison_intra_object_redzone"
: "__asan_unpoison_intra_object_redzone");
@@ -883,377 +870,370 @@ void CodeGenFunction::EmitConstructorBody(FunctionArgList &Args) {
}
namespace {
- /// RAII object to indicate that codegen is copying the value representation
- /// instead of the object representation. Useful when copying a struct or
- /// class which has uninitialized members and we're only performing
- /// lvalue-to-rvalue conversion on the object but not its members.
- class CopyingValueRepresentation {
- public:
- explicit CopyingValueRepresentation(CodeGenFunction &CGF)
- : CGF(CGF), OldSanOpts(CGF.SanOpts) {
- CGF.SanOpts.set(SanitizerKind::Bool, false);
- CGF.SanOpts.set(SanitizerKind::Enum, false);
- }
- ~CopyingValueRepresentation() {
- CGF.SanOpts = OldSanOpts;
- }
- private:
- CodeGenFunction &CGF;
- SanitizerSet OldSanOpts;
- };
+/// RAII object to indicate that codegen is copying the value representation
+/// instead of the object representation. Useful when copying a struct or
+/// class which has uninitialized members and we're only performing
+/// lvalue-to-rvalue conversion on the object but not its members.
+class CopyingValueRepresentation {
+public:
+ explicit CopyingValueRepresentation(CodeGenFunction &CGF)
+ : CGF(CGF), OldSanOpts(CGF.SanOpts) {
+ CGF.SanOpts.set(SanitizerKind::Bool, false);
+ CGF.SanOpts.set(SanitizerKind::Enum, false);
+ }
+ ~CopyingValueRepresentation() { CGF.SanOpts = OldSanOpts; }
+
+private:
+ CodeGenFunction &CGF;
+ SanitizerSet OldSanOpts;
+};
} // end anonymous namespace
namespace {
- class FieldMemcpyizer {
- public:
- FieldMemcpyizer(CodeGenFunction &CGF, const CXXRecordDecl *ClassDecl,
- const VarDecl *SrcRec)
+class FieldMemcpyizer {
+public:
+ FieldMemcpyizer(CodeGenFunction &CGF, const CXXRecordDecl *ClassDecl,
+ const VarDecl *SrcRec)
: CGF(CGF), ClassDecl(ClassDecl), SrcRec(SrcRec),
RecLayout(CGF.getContext().getASTRecordLayout(ClassDecl)),
FirstField(nullptr), LastField(nullptr), FirstFieldOffset(0),
LastFieldOffset(0), LastAddedFieldIndex(0) {}
- bool isMemcpyableField(FieldDecl *F) const {
- // Never memcpy fields when we are adding poisoned paddings.
- if (CGF.getContext().getLangOpts().SanitizeAddressFieldPadding)
- return false;
- Qualifiers Qual = F->getType().getQualifiers();
- if (Qual.hasVolatile() || Qual.hasObjCLifetime())
- return false;
- if (PointerAuthQualifier Q = F->getType().getPointerAuth();
- Q && Q.isAddressDiscriminated())
- return false;
- return true;
- }
-
- void addMemcpyableField(FieldDecl *F) {
- if (isEmptyFieldForLayout(CGF.getContext(), F))
- return;
- if (!FirstField)
- addInitialField(F);
- else
- addNextField(F);
- }
+ bool isMemcpyableField(FieldDecl *F) const {
+ // Never memcpy fields when we are adding poisoned paddings.
+ if (CGF.getContext().getLangOpts().SanitizeAddressFieldPadding)
+ return false;
+ Qualifiers Qual = F->getType().getQualifiers();
+ if (Qual.hasVolatile() || Qual.hasObjCLifetime())
+ return false;
+ if (PointerAuthQualifier Q = F->getType().getPointerAuth();
+ Q && Q.isAddressDiscriminated())
+ return false;
+ return true;
+ }
- CharUnits getMemcpySize(uint64_t FirstByteOffset) const {
- ASTContext &Ctx = CGF.getContext();
- unsigned LastFieldSize =
- LastField->isBitField()
- ? LastField->getBitWidthValue()
- : Ctx.toBits(
- Ctx.getTypeInfoDataSizeInChars(LastField->getType()).Width);
- uint64_t MemcpySizeBits = LastFieldOffset + LastFieldSize -
- FirstByteOffset + Ctx.getCharWidth() - 1;
- CharUnits MemcpySize = Ctx.toCharUnitsFromBits(MemcpySizeBits);
- return MemcpySize;
+ void addMemcpyableField(FieldDecl *F) {
+ if (isEmptyFieldForLayout(CGF.getContext(), F))
+ return;
+ if (!FirstField)
+ addInitialField(F);
+ else
+ addNextField(F);
+ }
+
+ CharUnits getMemcpySize(uint64_t FirstByteOffset) const {
+ ASTContext &Ctx = CGF.getContext();
+ unsigned LastFieldSize =
+ LastField->isBitField()
+ ? LastField->getBitWidthValue()
+ : Ctx.toBits(
+ Ctx.getTypeInfoDataSizeInChars(LastField->getType()).Width);
+ uint64_t MemcpySizeBits = LastFieldOffset + LastFieldSize -
+ FirstByteOffset + Ctx.getCharWidth() - 1;
+ CharUnits MemcpySize = Ctx.toCharUnitsFromBits(MemcpySizeBits);
+ return MemcpySize;
+ }
+
+ void emitMemcpy() {
+ // Give the subclass a chance to bail out if it feels the memcpy isn't
+ // worth it (e.g. Hasn't aggregated enough data).
+ if (!FirstField) {
+ return;
}
- void emitMemcpy() {
- // Give the subclass a chance to bail out if it feels the memcpy isn't
- // worth it (e.g. Hasn't aggregated enough data).
- if (!FirstField) {
- return;
- }
-
- uint64_t FirstByteOffset;
- if (FirstField->isBitField()) {
- const CGRecordLayout &RL =
+ uint64_t FirstByteOffset;
+ if (FirstField->isBitField()) {
+ const CGRecordLayout &RL =
CGF.getTypes().getCGRecordLayout(FirstField->getParent());
- const CGBitFieldInfo &BFInfo = RL.getBitFieldInfo(FirstField);
- // FirstFieldOffset is not appropriate for bitfields,
- // we need to use the storage offset instead.
- FirstByteOffset = CGF.getContext().toBits(BFInfo.StorageOffset);
- } else {
- FirstByteOffset = FirstFieldOffset;
- }
-
- CharUnits MemcpySize = getMemcpySize(FirstByteOffset);
- CanQualType RecordTy = CGF.getContext().getCanonicalTagType(ClassDecl);
- Address ThisPtr = CGF.LoadCXXThisAddress();
- LValue DestLV = CGF.MakeAddrLValue(ThisPtr, RecordTy);
- LValue Dest = CGF.EmitLValueForFieldInitialization(DestLV, FirstField);
- llvm::Value *SrcPtr = CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(SrcRec));
- LValue SrcLV = CGF.MakeNaturalAlignAddrLValue(SrcPtr, RecordTy);
- LValue Src = CGF.EmitLValueForFieldInitialization(SrcLV, FirstField);
-
- emitMemcpyIR(
- Dest.isBitField() ? Dest.getBitFieldAddress() : Dest.getAddress(),
- Src.isBitField() ? Src.getBitFieldAddress() : Src.getAddress(),
- MemcpySize);
- reset();
- }
-
- void reset() {
- FirstField = nullptr;
- }
-
- protected:
- CodeGenFunction &CGF;
- const CXXRecordDecl *ClassDecl;
-
- private:
- void emitMemcpyIR(Address DestPtr, Address SrcPtr, CharUnits Size) {
- DestPtr = DestPtr.withElementType(CGF.Int8Ty);
- SrcPtr = SrcPtr.withElementType(CGF.Int8Ty);
- auto *I = CGF.Builder.CreateMemCpy(DestPtr, SrcPtr, Size.getQuantity());
- CGF.addInstToCurrentSourceAtom(I, nullptr);
+ const CGBitFieldInfo &BFInfo = RL.getBitFieldInfo(FirstField);
+ // FirstFieldOffset is not appropriate for bitfields,
+ // we need to use the storage offset instead.
+ FirstByteOffset = CGF.getContext().toBits(BFInfo.StorageOffset);
+ ...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/176210
More information about the cfe-commits
mailing list