r246877 - Increase accuracy of __builtin_object_size.

Mikhail Zolotukhin via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 11 12:15:36 PDT 2015


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 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150911/5a279119/attachment-0001.html>


More information about the cfe-commits mailing list