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 <smee...@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-boun...@lists.llvm.org on behalf of > cfe-commits@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@D@Test13@@$4PPPPPPPE@A@EAAAEAUB1@2@XZ"( > + // WIN64-LABEL: define weak_odr dso_local dereferenceable(8) > %"struct.Test13::D"* @"?foo1@D@Test13@@$4PPPPPPPE@A@EAAAEAUB1@2@XZ"( > // This adjustment. > // WIN64: getelementptr inbounds i8, i8* {{.*}}, i64 -12 > // Call implementation. > > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@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@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
