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?
Thanks,
Michael
> On Sep 4, 2015, at 2:28 PM, George Burgess IV via cfe-commits
> <cfe-commits@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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
