r246877 - Increase accuracy of __builtin_object_size.
George Burgess IV via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 11 13:44:52 PDT 2015
> It might be reasonable for __builtin_object_size(..., 2) to give up if
[...]
That sounds like the best overall solution to me, as well. Working on a fix
now.
On Fri, Sep 11, 2015 at 1:22 PM, Richard Smith <richard at metafoo.co.uk>
wrote:
> 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/35af5cd6/attachment-0001.html>
More information about the cfe-commits
mailing list