[clang] 0130b6c - Don't assume a reference refers to at least sizeof(T) bytes.
Hans Wennborg via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 13 01:46:34 PST 2020
Unless there's demand for it, I'd be inclined to pass on merging since
it doesn't seem entirely trivial.
On Wed, Feb 12, 2020 at 1:56 PM Richard Smith via cfe-commits
<cfe-commits at lists.llvm.org> wrote:
>
> It's a wrong-code bugfix, but we've had the bug since Clang 3.5, so it doesn't seem urgent.
>
> Hans, what do you think? I don't think we've had any field reports of miscompiles (though someone did notice the ubsan false-positive).
>
> On Sat, 1 Feb 2020 at 05:04, Shoaib Meenai <smeenai at fb.com> wrote:
>>
>> Should this be cherry-picked to 10.0?
>>
>> On 1/31/20, 7:09 PM, "cfe-commits on behalf of Richard Smith via cfe-commits" <cfe-commits-bounces at lists.llvm.org on behalf of cfe-commits at lists.llvm.org> wrote:
>>
>>
>> Author: Richard Smith
>> Date: 2020-01-31T19:08:17-08:00
>> New Revision: 0130b6cb5a8d94511e2bb09ac2f5a613a59f70b4
>>
>> URL: https://github.com/llvm/llvm-project/commit/0130b6cb5a8d94511e2bb09ac2f5a613a59f70b4
>> DIFF: https://github.com/llvm/llvm-project/commit/0130b6cb5a8d94511e2bb09ac2f5a613a59f70b4.diff
>>
>> LOG: Don't assume a reference refers to at least sizeof(T) bytes.
>>
>> When T is a class type, only nvsize(T) bytes need be accessible through
>> the reference. We had matching bugs in the application of the
>> dereferenceable attribute and in -fsanitize=undefined.
>>
>> Added:
>>
>>
>> Modified:
>> clang/include/clang/AST/DeclCXX.h
>> clang/lib/AST/DeclCXX.cpp
>> clang/lib/CodeGen/CGCall.cpp
>> clang/lib/CodeGen/CGClass.cpp
>> clang/lib/CodeGen/CGExpr.cpp
>> clang/lib/CodeGen/CodeGenModule.h
>> clang/test/CodeGenCXX/catch-undef-behavior.cpp
>> clang/test/CodeGenCXX/thunks.cpp
>>
>> Removed:
>>
>>
>>
>> ################################################################################
>> diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
>> index 2e8e31dbf4c7..6d3a833b5037 100644
>> --- a/clang/include/clang/AST/DeclCXX.h
>> +++ b/clang/include/clang/AST/DeclCXX.h
>> @@ -1696,6 +1696,10 @@ class CXXRecordDecl : public RecordDecl {
>> /// actually abstract.
>> bool mayBeAbstract() const;
>>
>> + /// Determine whether it's impossible for a class to be derived from this
>> + /// class. This is best-effort, and may conservatively return false.
>> + bool isEffectivelyFinal() const;
>> +
>> /// If this is the closure type of a lambda expression, retrieve the
>> /// number to be used for name mangling in the Itanium C++ ABI.
>> ///
>>
>> diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
>> index 227fe80ccab4..931a1141b1b4 100644
>> --- a/clang/lib/AST/DeclCXX.cpp
>> +++ b/clang/lib/AST/DeclCXX.cpp
>> @@ -1923,6 +1923,18 @@ bool CXXRecordDecl::mayBeAbstract() const {
>> return false;
>> }
>>
>> +bool CXXRecordDecl::isEffectivelyFinal() const {
>> + auto *Def = getDefinition();
>> + if (!Def)
>> + return false;
>> + if (Def->hasAttr<FinalAttr>())
>> + return true;
>> + if (const auto *Dtor = Def->getDestructor())
>> + if (Dtor->hasAttr<FinalAttr>())
>> + return true;
>> + return false;
>> +}
>> +
>> void CXXDeductionGuideDecl::anchor() {}
>>
>> bool ExplicitSpecifier::isEquivalent(const ExplicitSpecifier Other) const {
>> @@ -2142,12 +2154,8 @@ CXXMethodDecl *CXXMethodDecl::getDevirtualizedMethod(const Expr *Base,
>> // Similarly, if the class itself or its destructor is marked 'final',
>> // the class can't be derived from and we can therefore devirtualize the
>> // member function call.
>> - if (BestDynamicDecl->hasAttr<FinalAttr>())
>> + if (BestDynamicDecl->isEffectivelyFinal())
>> return DevirtualizedMethod;
>> - if (const auto *dtor = BestDynamicDecl->getDestructor()) {
>> - if (dtor->hasAttr<FinalAttr>())
>> - return DevirtualizedMethod;
>> - }
>>
>> if (const auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
>> if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
>>
>> diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
>> index 3f132a0a62aa..9ed2ccd54487 100644
>> --- a/clang/lib/CodeGen/CGCall.cpp
>> +++ b/clang/lib/CodeGen/CGCall.cpp
>> @@ -2054,8 +2054,8 @@ void CodeGenModule::ConstructAttributeList(
>> if (const auto *RefTy = RetTy->getAs<ReferenceType>()) {
>> QualType PTy = RefTy->getPointeeType();
>> if (!PTy->isIncompleteType() && PTy->isConstantSizeType())
>> - RetAttrs.addDereferenceableAttr(getContext().getTypeSizeInChars(PTy)
>> - .getQuantity());
>> + RetAttrs.addDereferenceableAttr(
>> + getMinimumObjectSize(PTy).getQuantity());
>> else if (getContext().getTargetAddressSpace(PTy) == 0 &&
>> !CodeGenOpts.NullPointerIsValid)
>> RetAttrs.addAttribute(llvm::Attribute::NonNull);
>> @@ -2164,8 +2164,8 @@ void CodeGenModule::ConstructAttributeList(
>> if (const auto *RefTy = ParamType->getAs<ReferenceType>()) {
>> QualType PTy = RefTy->getPointeeType();
>> if (!PTy->isIncompleteType() && PTy->isConstantSizeType())
>> - Attrs.addDereferenceableAttr(getContext().getTypeSizeInChars(PTy)
>> - .getQuantity());
>> + Attrs.addDereferenceableAttr(
>> + getMinimumObjectSize(PTy).getQuantity());
>> else if (getContext().getTargetAddressSpace(PTy) == 0 &&
>> !CodeGenOpts.NullPointerIsValid)
>> Attrs.addAttribute(llvm::Attribute::NonNull);
>>
>> diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
>> index 7389207bc8ad..acc9a9ec4f4a 100644
>> --- a/clang/lib/CodeGen/CGClass.cpp
>> +++ b/clang/lib/CodeGen/CGClass.cpp
>> @@ -35,20 +35,37 @@ using namespace CodeGen;
>> /// Return the best known alignment for an unknown pointer to a
>> /// particular class.
>> CharUnits CodeGenModule::getClassPointerAlignment(const CXXRecordDecl *RD) {
>> - if (!RD->isCompleteDefinition())
>> + if (!RD->hasDefinition())
>> return CharUnits::One(); // Hopefully won't be used anywhere.
>>
>> auto &layout = getContext().getASTRecordLayout(RD);
>>
>> // If the class is final, then we know that the pointer points to an
>> // object of that type and can use the full alignment.
>> - if (RD->hasAttr<FinalAttr>()) {
>> + if (RD->isEffectivelyFinal())
>> return layout.getAlignment();
>>
>> // Otherwise, we have to assume it could be a subclass.
>> - } else {
>> - return layout.getNonVirtualAlignment();
>> - }
>> + return layout.getNonVirtualAlignment();
>> +}
>> +
>> +/// Return the smallest possible amount of storage that might be allocated
>> +/// starting from the beginning of an object of a particular class.
>> +///
>> +/// This may be smaller than sizeof(RD) if RD has virtual base classes.
>> +CharUnits CodeGenModule::getMinimumClassObjectSize(const CXXRecordDecl *RD) {
>> + if (!RD->hasDefinition())
>> + return CharUnits::One();
>> +
>> + auto &layout = getContext().getASTRecordLayout(RD);
>> +
>> + // If the class is final, then we know that the pointer points to an
>> + // object of that type and can use the full alignment.
>> + if (RD->isEffectivelyFinal())
>> + return layout.getSize();
>> +
>> + // Otherwise, we have to assume it could be a subclass.
>> + return std::max(layout.getNonVirtualSize(), CharUnits::One());
>> }
>>
>> /// Return the best known alignment for a pointer to a virtual base,
>>
>> diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
>> index f1a5e3dcb33c..4b8a31c180c8 100644
>> --- a/clang/lib/CodeGen/CGExpr.cpp
>> +++ b/clang/lib/CodeGen/CGExpr.cpp
>> @@ -711,7 +711,7 @@ void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,
>> if (SanOpts.has(SanitizerKind::ObjectSize) &&
>> !SkippedChecks.has(SanitizerKind::ObjectSize) &&
>> !Ty->isIncompleteType()) {
>> - uint64_t TySize = getContext().getTypeSizeInChars(Ty).getQuantity();
>> + uint64_t TySize = CGM.getMinimumObjectSize(Ty).getQuantity();
>> llvm::Value *Size = llvm::ConstantInt::get(IntPtrTy, TySize);
>> if (ArraySize)
>> Size = Builder.CreateMul(Size, ArraySize);
>> @@ -742,7 +742,9 @@ void CodeGenFunction::EmitTypeCheck(TypeCheckKind TCK, SourceLocation Loc,
>> !SkippedChecks.has(SanitizerKind::Alignment)) {
>> AlignVal = Alignment.getQuantity();
>> if (!Ty->isIncompleteType() && !AlignVal)
>> - AlignVal = getContext().getTypeAlignInChars(Ty).getQuantity();
>> + AlignVal =
>> + getNaturalTypeAlignment(Ty, nullptr, nullptr, /*ForPointeeType=*/true)
>> + .getQuantity();
>>
>> // The glvalue must be suitably aligned.
>> if (AlignVal > 1 &&
>>
>> diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
>> index a711d5ccba0e..e8162fcfc0cd 100644
>> --- a/clang/lib/CodeGen/CodeGenModule.h
>> +++ b/clang/lib/CodeGen/CodeGenModule.h
>> @@ -868,6 +868,17 @@ class CodeGenModule : public CodeGenTypeCache {
>> /// Returns the assumed alignment of an opaque pointer to the given class.
>> CharUnits getClassPointerAlignment(const CXXRecordDecl *CD);
>>
>> + /// Returns the minimum object size for an object of the given class type
>> + /// (or a class derived from it).
>> + CharUnits getMinimumClassObjectSize(const CXXRecordDecl *CD);
>> +
>> + /// Returns the minimum object size for an object of the given type.
>> + CharUnits getMinimumObjectSize(QualType Ty) {
>> + if (CXXRecordDecl *RD = Ty->getAsCXXRecordDecl())
>> + return getMinimumClassObjectSize(RD);
>> + return getContext().getTypeSizeInChars(Ty);
>> + }
>> +
>> /// Returns the assumed alignment of a virtual base of a class.
>> CharUnits getVBaseAlignment(CharUnits DerivedAlign,
>> const CXXRecordDecl *Derived,
>>
>> diff --git a/clang/test/CodeGenCXX/catch-undef-behavior.cpp b/clang/test/CodeGenCXX/catch-undef-behavior.cpp
>> index ba72af038aba..c5aec53ad724 100644
>> --- a/clang/test/CodeGenCXX/catch-undef-behavior.cpp
>> +++ b/clang/test/CodeGenCXX/catch-undef-behavior.cpp
>> @@ -426,6 +426,25 @@ void indirect_function_call(void (*p)(int)) {
>> p(42);
>> }
>>
>> +namespace VBaseObjectSize {
>> + // Note: C is laid out such that offsetof(C, B) + sizeof(B) extends outside
>> + // the C object.
>> + struct alignas(16) A { void *a1, *a2; };
>> + struct B : virtual A { void *b; };
>> + struct C : virtual A, virtual B {};
>> + // CHECK-LABEL: define {{.*}} @_ZN15VBaseObjectSize1fERNS_1BE(
>> + B &f(B &b) {
>> + // Size check: check for nvsize(B) == 16 (do not require size(B) == 32)
>> + // CHECK: [[SIZE:%.+]] = call i{{32|64}} @llvm.objectsize.i64.p0i8(
>> + // CHECK: icmp uge i{{32|64}} [[SIZE]], 16,
>> +
>> + // Alignment check: check for nvalign(B) == 8 (do not require align(B) == 16)
>> + // CHECK: [[PTRTOINT:%.+]] = ptrtoint {{.*}} to i64,
>> + // CHECK: and i64 [[PTRTOINT]], 7,
>> + return b;
>> + }
>> +}
>> +
>> namespace FunctionSanitizerVirtualCalls {
>> struct A {
>> virtual void f() {}
>>
>> diff --git a/clang/test/CodeGenCXX/thunks.cpp b/clang/test/CodeGenCXX/thunks.cpp
>> index 4a0610ed488d..fe7d656eb7e5 100644
>> --- a/clang/test/CodeGenCXX/thunks.cpp
>> +++ b/clang/test/CodeGenCXX/thunks.cpp
>> @@ -412,7 +412,7 @@ namespace Test13 {
>> // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 8
>> // CHECK: ret %"struct.Test13::D"*
>>
>> - // WIN64-LABEL: define weak_odr dso_local dereferenceable(32) %"struct.Test13::D"* @"?foo1 at D@Test13@@$4PPPPPPPE at A@EAAAEAUB1 at 2@XZ"(
>> + // WIN64-LABEL: define weak_odr dso_local dereferenceable(8) %"struct.Test13::D"* @"?foo1 at D@Test13@@$4PPPPPPPE at A@EAAAEAUB1 at 2@XZ"(
>> // This adjustment.
>> // WIN64: getelementptr inbounds i8, i8* {{.*}}, i64 -12
>> // Call implementation.
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Dcommits&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=CfI0YzHTWSKGGaKoCCW_TcBbgs9qek2vZSS_cdHZDIw&s=sCuW5vldFWSuM_t_J3Hbv_-sZKRs7NYXs2ihx__jJ9A&e=
>>
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list