[clang] 0130b6c - Don't assume a reference refers to at least sizeof(T) bytes.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 12 04:55:58 PST 2020
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=
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200212/1ea4b156/attachment-0001.html>
More information about the cfe-commits
mailing list