r246877 - Increase accuracy of __builtin_object_size.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 11 13:22:07 PDT 2015
On Fri, Sep 11, 2015 at 12:15 PM, Mikhail Zolotukhin via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> Hi George,
>
> After this commit we started to trap on the following case:
>
> #include <string.h>
> typedef struct {
> int n;
> char key[1];
> } obj_t;
>
> char *str = "hello";
>
> int main() {
> obj_t* p = (obj_t*)malloc(strlen(str) + 1 + sizeof(int));
> strcpy(p->key, str);
> free(p);
> return 0;
> }
>
> As far as I understand, this might be a common pattern in pre-C99
> codebase, and it fails when compiled with -D_FORTIFY_SOURCE=2. Is there a
> way we could fix it in clang, or the only fix is to use less strict
> FORTIFY_SOURCE level?
>
It might be reasonable for __builtin_object_size(..., 2) to give up if:
1) we lost track of the complete object, and
2) the designator refers to the final subobject of the currently-known
complete object, and
3) that subobject is either a flexible array member or an array of bound 0
or 1.
Then we'd leave it to IR generation to do the llvm.object.size(..., i1
false) dance, and in this case to grab the size of the malloc (minus the
offset of 'key').
Thanks,
> Michael
>
> On Sep 4, 2015, at 2:28 PM, George Burgess IV via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
> Author: gbiv
> Date: Fri Sep 4 16:28:13 2015
> New Revision: 246877
>
> URL: http://llvm.org/viewvc/llvm-project?rev=246877&view=rev
> Log:
> Increase accuracy of __builtin_object_size.
>
> Improvements:
>
> - For all types, we would give up in a case such as:
> __builtin_object_size((char*)&foo, N);
> even if we could provide an answer to
> __builtin_object_size(&foo, N);
> We now provide the same answer for both of the above examples in all
> cases.
>
> - For type=1|3, we now support subobjects with unknown bases, as long
> as the designator is valid.
>
> Thanks to Richard Smith for the review + design planning.
>
> Review: http://reviews.llvm.org/D12169
>
>
> Modified:
> cfe/trunk/lib/AST/ExprConstant.cpp
> cfe/trunk/test/CodeGen/object-size.c
>
> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=246877&r1=246876&r2=246877&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
> +++ cfe/trunk/lib/AST/ExprConstant.cpp Fri Sep 4 16:28:13 2015
> @@ -492,7 +492,11 @@ namespace {
> /// optimizer if we don't constant fold them here, but in an
> unevaluated
> /// context we try to fold them immediately since the optimizer never
> /// gets a chance to look at it.
> - EM_PotentialConstantExpressionUnevaluated
> + EM_PotentialConstantExpressionUnevaluated,
> +
> + /// Evaluate as a constant expression. Continue evaluating if we
> find a
> + /// MemberExpr with a base that can't be evaluated.
> + EM_DesignatorFold,
> } EvalMode;
>
> /// Are we checking whether the expression is a potential constant
> @@ -595,6 +599,7 @@ namespace {
> case EM_PotentialConstantExpression:
> case EM_ConstantExpressionUnevaluated:
> case EM_PotentialConstantExpressionUnevaluated:
> + case EM_DesignatorFold:
> HasActiveDiagnostic = false;
> return OptionalDiagnostic();
> }
> @@ -674,6 +679,7 @@ namespace {
> case EM_ConstantExpression:
> case EM_ConstantExpressionUnevaluated:
> case EM_ConstantFold:
> + case EM_DesignatorFold:
> return false;
> }
> llvm_unreachable("Missed EvalMode case");
> @@ -702,10 +708,15 @@ namespace {
> case EM_ConstantExpressionUnevaluated:
> case EM_ConstantFold:
> case EM_IgnoreSideEffects:
> + case EM_DesignatorFold:
> return false;
> }
> llvm_unreachable("Missed EvalMode case");
> }
> +
> + bool allowInvalidBaseExpr() const {
> + return EvalMode == EM_DesignatorFold;
> + }
> };
>
> /// Object used to treat all foldable expressions as constant
> expressions.
> @@ -736,6 +747,21 @@ namespace {
> }
> };
>
> + /// RAII object used to treat the current evaluation as the correct
> pointer
> + /// offset fold for the current EvalMode
> + struct FoldOffsetRAII {
> + EvalInfo &Info;
> + EvalInfo::EvaluationMode OldMode;
> + explicit FoldOffsetRAII(EvalInfo &Info, bool Subobject)
> + : Info(Info), OldMode(Info.EvalMode) {
> + if (!Info.checkingPotentialConstantExpression())
> + Info.EvalMode = Subobject ? EvalInfo::EM_DesignatorFold
> + : EvalInfo::EM_ConstantFold;
> + }
> +
> + ~FoldOffsetRAII() { Info.EvalMode = OldMode; }
> + };
> +
> /// RAII object used to suppress diagnostics and side-effects from a
> /// speculative evaluation.
> class SpeculativeEvaluationRAII {
> @@ -917,7 +943,8 @@ namespace {
> struct LValue {
> APValue::LValueBase Base;
> CharUnits Offset;
> - unsigned CallIndex;
> + bool InvalidBase : 1;
> + unsigned CallIndex : 31;
> SubobjectDesignator Designator;
>
> const APValue::LValueBase getLValueBase() const { return Base; }
> @@ -938,17 +965,23 @@ namespace {
> assert(V.isLValue());
> Base = V.getLValueBase();
> Offset = V.getLValueOffset();
> + InvalidBase = false;
> CallIndex = V.getLValueCallIndex();
> Designator = SubobjectDesignator(Ctx, V);
> }
>
> - void set(APValue::LValueBase B, unsigned I = 0) {
> + void set(APValue::LValueBase B, unsigned I = 0, bool BInvalid =
> false) {
> Base = B;
> Offset = CharUnits::Zero();
> + InvalidBase = BInvalid;
> CallIndex = I;
> Designator = SubobjectDesignator(getType(B));
> }
>
> + void setInvalid(APValue::LValueBase B, unsigned I = 0) {
> + set(B, I, true);
> + }
> +
> // Check that this LValue is not based on a null pointer. If it is,
> produce
> // a diagnostic and mark the designator as invalid.
> bool checkNullPointer(EvalInfo &Info, const Expr *E,
> @@ -4368,20 +4401,23 @@ public:
> bool VisitMemberExpr(const MemberExpr *E) {
> // Handle non-static data members.
> QualType BaseTy;
> + bool EvalOK;
> if (E->isArrow()) {
> - if (!EvaluatePointer(E->getBase(), Result, this->Info))
> - return false;
> + EvalOK = EvaluatePointer(E->getBase(), Result, this->Info);
> BaseTy =
> E->getBase()->getType()->castAs<PointerType>()->getPointeeType();
> } else if (E->getBase()->isRValue()) {
> assert(E->getBase()->getType()->isRecordType());
> - if (!EvaluateTemporary(E->getBase(), Result, this->Info))
> - return false;
> + EvalOK = EvaluateTemporary(E->getBase(), Result, this->Info);
> BaseTy = E->getBase()->getType();
> } else {
> - if (!this->Visit(E->getBase()))
> - return false;
> + EvalOK = this->Visit(E->getBase());
> BaseTy = E->getBase()->getType();
> }
> + if (!EvalOK) {
> + if (!this->Info.allowInvalidBaseExpr())
> + return false;
> + Result.setInvalid(E->getBase());
> + }
>
> const ValueDecl *MD = E->getMemberDecl();
> if (const FieldDecl *FD = dyn_cast<FieldDecl>(E->getMemberDecl())) {
> @@ -4793,7 +4829,7 @@ public:
> bool VisitObjCStringLiteral(const ObjCStringLiteral *E)
> { return Success(E); }
> bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
> - { return Success(E); }
> + { return Success(E); }
> bool VisitAddrLabelExpr(const AddrLabelExpr *E)
> { return Success(E); }
> bool VisitCallExpr(const CallExpr *E);
> @@ -4919,6 +4955,7 @@ bool PointerExprEvaluator::VisitCastExpr
> unsigned Size = Info.Ctx.getTypeSize(E->getType());
> uint64_t N = Value.getInt().extOrTrunc(Size).getZExtValue();
> Result.Base = (Expr*)nullptr;
> + Result.InvalidBase = false;
> Result.Offset = CharUnits::fromQuantity(N);
> Result.CallIndex = 0;
> Result.Designator.setInvalid();
> @@ -6211,6 +6248,26 @@ static QualType getObjectType(APValue::L
> return QualType();
> }
>
> +/// A more selective version of E->IgnoreParenCasts for
> +/// TryEvaluateBuiltinObjectSize. This ignores casts/parens that serve
> only to
> +/// change the type of E.
> +/// Ex. For E = `(short*)((char*)(&foo))`, returns `&foo`
> +///
> +/// Always returns an RValue with a pointer representation.
> +static const Expr *ignorePointerCastsAndParens(const Expr *E) {
> + assert(E->isRValue() && E->getType()->hasPointerRepresentation());
> +
> + auto *NoParens = E->IgnoreParens();
> + auto *Cast = dyn_cast<CastExpr>(NoParens);
> + if (Cast == nullptr || Cast->getCastKind() == CK_DerivedToBase)
> + return NoParens;
> +
> + auto *SubExpr = Cast->getSubExpr();
> + if (!SubExpr->getType()->hasPointerRepresentation() ||
> !SubExpr->isRValue())
> + return NoParens;
> + return ignorePointerCastsAndParens(SubExpr);
> +}
> +
> bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
> unsigned Type) {
> // Determine the denoted object.
> @@ -6220,23 +6277,35 @@ bool IntExprEvaluator::TryEvaluateBuilti
> // If there are any, but we can determine the pointed-to object
> anyway, then
> // ignore the side-effects.
> SpeculativeEvaluationRAII SpeculativeEval(Info);
> - FoldConstant Fold(Info, true);
> - if (!EvaluatePointer(E->getArg(0), Base, Info))
> + FoldOffsetRAII Fold(Info, Type & 1);
> + const Expr *Ptr = ignorePointerCastsAndParens(E->getArg(0));
> + if (!EvaluatePointer(Ptr, Base, Info))
> return false;
> }
>
> CharUnits BaseOffset = Base.getLValueOffset();
> -
> - // If we point to before the start of the object, there are no
> - // accessible bytes.
> - if (BaseOffset < CharUnits::Zero())
> + // If we point to before the start of the object, there are no
> accessible
> + // bytes.
> + if (BaseOffset.isNegative())
> return Success(0, E);
>
> - // MostDerivedType is null if we're dealing with a literal such as
> nullptr or
> - // (char*)0x100000. Lower it to LLVM in either case so it can figure
> out what
> - // to do with it.
> - // FIXME(gbiv): Try to do a better job with this in clang.
> - if (Base.Designator.MostDerivedType.isNull())
> + // In the case where we're not dealing with a subobject, we discard the
> + // subobject bit.
> + if (!Base.Designator.Invalid && Base.Designator.Entries.empty())
> + Type = Type & ~1U;
> +
> + // If Type & 1 is 0, we need to be able to statically guarantee that
> the bytes
> + // exist. If we can't verify the base, then we can't do that.
> + //
> + // As a special case, we produce a valid object size for an unknown
> object
> + // with a known designator if Type & 1 is 1. For instance:
> + //
> + // extern struct X { char buff[32]; int a, b, c; } *p;
> + // int a = __builtin_object_size(p->buff + 4, 3); // returns 28
> + // int b = __builtin_object_size(p->buff + 4, 2); // returns 0, not 40
> + //
> + // This matches GCC's behavior.
> + if ((Type & 1) == 0 && Base.InvalidBase)
> return Error(E);
>
> // If Type & 1 is 0, the object in question is the complete object;
> reset to
> @@ -6256,16 +6325,6 @@ bool IntExprEvaluator::TryEvaluateBuilti
> }
> }
>
> - // FIXME: We should produce a valid object size for an unknown object
> with a
> - // known designator, if Type & 1 is 1. For instance:
> - //
> - // extern struct X { char buff[32]; int a, b, c; } *p;
> - // int a = __builtin_object_size(p->buff + 4, 3); // returns 28
> - // int b = __builtin_object_size(p->buff + 4, 2); // returns 0, not 40
> - //
> - // This is GCC's behavior. We currently don't do this, but (hopefully)
> will in
> - // the near future.
> -
> // If it is not possible to determine which objects ptr points to at
> compile
> // time, __builtin_object_size should return (size_t) -1 for type 0 or 1
> // and (size_t) 0 for type 2 or 3.
> @@ -6280,14 +6339,15 @@ bool IntExprEvaluator::TryEvaluateBuilti
> End.Designator.Entries.size() ==
> End.Designator.MostDerivedPathLength) {
> // We got a pointer to an array. Step to its end.
> AmountToAdd = End.Designator.MostDerivedArraySize -
> - End.Designator.Entries.back().ArrayIndex;
> - } else if (End.Designator.IsOnePastTheEnd) {
> + End.Designator.Entries.back().ArrayIndex;
> + } else if (End.Designator.isOnePastTheEnd()) {
> // We're already pointing at the end of the object.
> AmountToAdd = 0;
> }
>
> - if (End.Designator.MostDerivedType->isIncompleteType() ||
> - End.Designator.MostDerivedType->isFunctionType())
> + QualType PointeeType = End.Designator.MostDerivedType;
> + assert(!PointeeType.isNull());
> + if (PointeeType->isIncompleteType() || PointeeType->isFunctionType())
> return Error(E);
>
> if (!HandleLValueArrayAdjustment(Info, E, End,
> End.Designator.MostDerivedType,
> @@ -6331,6 +6391,7 @@ bool IntExprEvaluator::VisitCallExpr(con
> case EvalInfo::EM_ConstantFold:
> case EvalInfo::EM_EvaluateForOverflow:
> case EvalInfo::EM_IgnoreSideEffects:
> + case EvalInfo::EM_DesignatorFold:
> // Leave it to IR generation.
> return Error(E);
> case EvalInfo::EM_ConstantExpressionUnevaluated:
>
> Modified: cfe/trunk/test/CodeGen/object-size.c
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/object-size.c?rev=246877&r1=246876&r2=246877&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/CodeGen/object-size.c (original)
> +++ cfe/trunk/test/CodeGen/object-size.c Fri Sep 4 16:28:13 2015
> @@ -161,6 +161,15 @@ void test19() {
> gi = __builtin_object_size(&foo.a, 2);
> // CHECK: store i32 4
> gi = __builtin_object_size(&foo.a, 3);
> +
> + // CHECK: store i32 4
> + gi = __builtin_object_size(&foo.b, 0);
> + // CHECK: store i32 4
> + gi = __builtin_object_size(&foo.b, 1);
> + // CHECK: store i32 4
> + gi = __builtin_object_size(&foo.b, 2);
> + // CHECK: store i32 4
> + gi = __builtin_object_size(&foo.b, 3);
> }
>
> // CHECK: @test20
> @@ -221,25 +230,59 @@ void test22() {
> gi = __builtin_object_size(&t[9].t[10], 2);
> // CHECK: store i32 0
> gi = __builtin_object_size(&t[9].t[10], 3);
> +
> + // CHECK: store i32 0
> + gi = __builtin_object_size((char*)&t[0] + sizeof(t), 0);
> + // CHECK: store i32 0
> + gi = __builtin_object_size((char*)&t[0] + sizeof(t), 1);
> + // CHECK: store i32 0
> + gi = __builtin_object_size((char*)&t[0] + sizeof(t), 2);
> + // CHECK: store i32 0
> + gi = __builtin_object_size((char*)&t[0] + sizeof(t), 3);
> +
> + // CHECK: store i32 0
> + gi = __builtin_object_size((char*)&t[9].t[0] + 10*sizeof(t[0].t), 0);
> + // CHECK: store i32 0
> + gi = __builtin_object_size((char*)&t[9].t[0] + 10*sizeof(t[0].t), 1);
> + // CHECK: store i32 0
> + gi = __builtin_object_size((char*)&t[9].t[0] + 10*sizeof(t[0].t), 2);
> + // CHECK: store i32 0
> + gi = __builtin_object_size((char*)&t[9].t[0] + 10*sizeof(t[0].t), 3);
> }
>
> -struct Test23Ty { int t[10]; };
> +struct Test23Ty { int a; int t[10]; };
>
> // CHECK: @test23
> -void test23(struct Test22Ty *p) {
> +void test23(struct Test23Ty *p) {
> // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
> gi = __builtin_object_size(p, 0);
> // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
> gi = __builtin_object_size(p, 1);
> // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
> gi = __builtin_object_size(p, 2);
> -
> // Note: this is currently fixed at 0 because LLVM doesn't have
> sufficient
> // data to correctly handle type=3
> // CHECK: store i32 0
> gi = __builtin_object_size(p, 3);
> -}
>
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
> + gi = __builtin_object_size(&p->a, 0);
> + // CHECK: store i32 4
> + gi = __builtin_object_size(&p->a, 1);
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
> + gi = __builtin_object_size(&p->a, 2);
> + // CHECK: store i32 4
> + gi = __builtin_object_size(&p->a, 3);
> +
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
> + gi = __builtin_object_size(&p->t[5], 0);
> + // CHECK: store i32 20
> + gi = __builtin_object_size(&p->t[5], 1);
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
> + gi = __builtin_object_size(&p->t[5], 2);
> + // CHECK: store i32 20
> + gi = __builtin_object_size(&p->t[5], 3);
> +}
>
> // PR24493 -- ICE if __builtin_object_size called with NULL and (Type & 1)
> != 0
> // CHECK: @test24
> @@ -280,3 +323,71 @@ void test25() {
> // CHECK: store i32 0
> gi = __builtin_object_size((void*)0 + 0x1000, 3);
> }
> +
> +// CHECK: @test26
> +void test26() {
> + struct { int v[10]; } t[10];
> +
> + // CHECK: store i32 316
> + gi = __builtin_object_size(&t[1].v[11], 0);
> + // CHECK: store i32 312
> + gi = __builtin_object_size(&t[1].v[12], 1);
> + // CHECK: store i32 308
> + gi = __builtin_object_size(&t[1].v[13], 2);
> + // CHECK: store i32 0
> + gi = __builtin_object_size(&t[1].v[14], 3);
> +}
> +
> +struct Test27IncompleteTy;
> +
> +// CHECK: @test27
> +void test27(struct Test27IncompleteTy *t) {
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
> + gi = __builtin_object_size(t, 0);
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
> + gi = __builtin_object_size(t, 1);
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
> + gi = __builtin_object_size(t, 2);
> + // Note: this is currently fixed at 0 because LLVM doesn't have
> sufficient
> + // data to correctly handle type=3
> + // CHECK: store i32 0
> + gi = __builtin_object_size(t, 3);
> +
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false)
> + gi = __builtin_object_size(&test27, 0);
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false)
> + gi = __builtin_object_size(&test27, 1);
> + // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true)
> + gi = __builtin_object_size(&test27, 2);
> + // Note: this is currently fixed at 0 because LLVM doesn't have
> sufficient
> + // data to correctly handle type=3
> + // CHECK: store i32 0
> + gi = __builtin_object_size(&test27, 3);
> +}
> +
> +// The intent of this test is to ensure that __builtin_object_size treats
> `&foo`
> +// and `(T*)&foo` identically, when used as the pointer argument.
> +// CHECK: @test28
> +void test28() {
> + struct { int v[10]; } t[10];
> +
> +#define addCasts(s) ((char*)((short*)(s)))
> + // CHECK: store i32 360
> + gi = __builtin_object_size(addCasts(&t[1]), 0);
> + // CHECK: store i32 360
> + gi = __builtin_object_size(addCasts(&t[1]), 1);
> + // CHECK: store i32 360
> + gi = __builtin_object_size(addCasts(&t[1]), 2);
> + // CHECK: store i32 360
> + gi = __builtin_object_size(addCasts(&t[1]), 3);
> +
> + // CHECK: store i32 356
> + gi = __builtin_object_size(addCasts(&t[1].v[1]), 0);
> + // CHECK: store i32 36
> + gi = __builtin_object_size(addCasts(&t[1].v[1]), 1);
> + // CHECK: store i32 356
> + gi = __builtin_object_size(addCasts(&t[1].v[1]), 2);
> + // CHECK: store i32 36
> + gi = __builtin_object_size(addCasts(&t[1].v[1]), 3);
> +#undef addCasts
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150911/840331fb/attachment-0001.html>
More information about the cfe-commits
mailing list