> 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 <rich...@metafoo.co.uk> wrote: > On Fri, Sep 11, 2015 at 12:15 PM, Mikhail Zolotukhin via cfe-commits < > cfe-commits@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@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 >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits