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@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 <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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
