[cfe-dev] Bugfix - Clang handles __builtin_object_size in wrong way

George Burgess gbiv at google.com
Tue Aug 11 16:32:27 PDT 2015


Attached is a fix for https://llvm.org/bugs/show_bug.cgi?id=15212 .

Summary: Tighten up some of the results for __builtin_object_size(ptr,
type) when given type == 1 or type == 3 + fixed a bug where we would report
incorrect results for type == 3.

> 90% of the patch is courtesy of Richard Smith. :)

George
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150811/fc2e3ed1/attachment.html>
-------------- next part --------------
diff --git a/lib/AST/ExprConstant.cpp b/lib/AST/ExprConstant.cpp
index ed749cc..637a299 100644
--- a/lib/AST/ExprConstant.cpp
+++ b/lib/AST/ExprConstant.cpp
@@ -967,10 +967,6 @@ namespace {
     // Check this LValue refers to an object. If not, set the designator to be
     // invalid and emit a diagnostic.
     bool checkSubobject(EvalInfo &Info, const Expr *E, CheckSubobjectKind CSK) {
-      // Outside C++11, do not build a designator referring to a subobject of
-      // any object: we won't use such a designator for anything.
-      if (!Info.getLangOpts().CPlusPlus11)
-        Designator.setInvalid();
       return (CSK == CSK_ArrayToPointer || checkNullPointer(Info, E, CSK)) &&
              Designator.checkSubobject(Info, E, CSK);
     }
@@ -2713,8 +2709,7 @@ static bool handleLValueToRValueConversion(EvalInfo &Info, const Expr *Conv,
 
   // Check for special cases where there is no existing APValue to look at.
   const Expr *Base = LVal.Base.dyn_cast<const Expr*>();
-  if (!LVal.Designator.Invalid && Base && !LVal.CallIndex &&
-      !Type.isVolatileQualified()) {
+  if (Base && !LVal.CallIndex && !Type.isVolatileQualified()) {
     if (const CompoundLiteralExpr *CLE = dyn_cast<CompoundLiteralExpr>(Base)) {
       // In C99, a CompoundLiteralExpr is an lvalue, and we defer evaluating the
       // initializer until now for such expressions. Such an expression can't be
@@ -5998,8 +5993,7 @@ public:
   bool VisitSizeOfPackExpr(const SizeOfPackExpr *E);
 
 private:
-  static QualType GetObjectType(APValue::LValueBase B);
-  bool TryEvaluateBuiltinObjectSize(const CallExpr *E);
+  bool TryEvaluateBuiltinObjectSize(const CallExpr *E, unsigned Type);
   // FIXME: Missing: array subscript of vector, member of vector
 };
 } // end anonymous namespace
@@ -6171,7 +6165,7 @@ static bool EvaluateBuiltinConstantP(ASTContext &Ctx, const Expr *Arg) {
 
 /// Retrieves the "underlying object type" of the given expression,
 /// as used by __builtin_object_size.
-QualType IntExprEvaluator::GetObjectType(APValue::LValueBase B) {
+static QualType getObjectType(APValue::LValueBase B) {
   if (const ValueDecl *D = B.dyn_cast<const ValueDecl*>()) {
     if (const VarDecl *VD = dyn_cast<VarDecl>(D))
       return VD->getType();
@@ -6183,49 +6177,86 @@ QualType IntExprEvaluator::GetObjectType(APValue::LValueBase B) {
   return QualType();
 }
 
-bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E) {
+bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
+                                                    unsigned Type) {
+  // Determine the denoted object.
   LValue Base;
-
   {
     // The operand of __builtin_object_size is never evaluated for side-effects.
     // 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))
       return false;
   }
 
-  if (!Base.getLValueBase()) {
-    // 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.
-    llvm::APSInt TypeIntVaue;
-    const Expr *ExprType = E->getArg(1);
-    if (!ExprType->EvaluateAsInt(TypeIntVaue, Info.Ctx))
-      return false;
-    if (TypeIntVaue == 0 || TypeIntVaue == 1)
-      return Success(-1, E);
-    if (TypeIntVaue == 2 || TypeIntVaue == 3)
-      return Success(0, E);
-    return Error(E);
+  CharUnits BaseOffset = Base.getLValueOffset();
+
+  // If we point to before the start of the object, there are no
+  // accessible bytes.
+  if (BaseOffset < CharUnits::Zero())
+    return Success(0, E);
+
+  // If Type & 1 is 0, the object in question is the complete object; reset to
+  // a complete object designator in that case.
+  //
+  // If Type is 1 and we've lost track of the subobject, just find the complete
+  // object instead. (If Type is 3, that's not correct behavior and we should
+  // return 0 instead.)
+  LValue End = Base;
+  if (((Type & 1) == 0) || (End.Designator.Invalid && Type == 1)) {
+    QualType T = getObjectType(End.getLValueBase());
+    if (T.isNull())
+      End.Designator.setInvalid();
+    else {
+      End.Designator = SubobjectDesignator(T);
+      End.Offset = CharUnits::Zero();
+    }
   }
 
-  QualType T = GetObjectType(Base.getLValueBase());
-  if (T.isNull() ||
-      T->isIncompleteType() ||
-      T->isFunctionType() ||
-      T->isVariablyModifiedType() ||
-      T->isDependentType())
+  // Note, we'll 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 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.
+  if (End.Designator.Invalid)
+    return false;
+
+  // According to the GCC documentation, we want the size of the subobject
+  // denoted by the pointer. But that's not quite right -- what we actually
+  // want is the size of the immediately-enclosing array, if there is one.
+  int64_t AmountToAdd = 1;
+  if (End.Designator.MostDerivedArraySize &&
+      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) {
+    // We're already pointing at the end of the object.
+    AmountToAdd = 0;
+  }
+
+  if (End.Designator.MostDerivedType->isIncompleteType() ||
+      End.Designator.MostDerivedType->isFunctionType())
     return Error(E);
 
-  CharUnits Size = Info.Ctx.getTypeSizeInChars(T);
-  CharUnits Offset = Base.getLValueOffset();
+  if (!HandleLValueArrayAdjustment(Info, E, End, End.Designator.MostDerivedType,
+                                   AmountToAdd))
+    return false;
 
-  if (!Offset.isNegative() && Offset <= Size)
-    Size -= Offset;
-  else
-    Size = CharUnits::Zero();
-  return Success(Size, E);
+  auto EndOffset = End.getLValueOffset();
+  if (BaseOffset > EndOffset)
+    return Success(0, E);
+
+  return Success(EndOffset - BaseOffset, E);
 }
 
 bool IntExprEvaluator::VisitCallExpr(const CallExpr *E) {
@@ -6234,17 +6265,21 @@ bool IntExprEvaluator::VisitCallExpr(const CallExpr *E) {
     return ExprEvaluatorBaseTy::VisitCallExpr(E);
 
   case Builtin::BI__builtin_object_size: {
-    if (TryEvaluateBuiltinObjectSize(E))
+    // The type was checked when we built the expression.
+    unsigned Type =
+        E->getArg(1)->EvaluateKnownConstInt(Info.Ctx).getZExtValue();
+    assert(Type <= 3 && "unexpected type");
+
+    if (TryEvaluateBuiltinObjectSize(E, Type))
       return true;
 
     // If evaluating the argument has side-effects, we can't determine the size
     // of the object, and so we lower it to unknown now. CodeGen relies on us to
     // handle all cases where the expression has side-effects.
-    if (E->getArg(0)->HasSideEffects(Info.Ctx)) {
-      if (E->getArg(1)->EvaluateKnownConstInt(Info.Ctx).getZExtValue() <= 1)
-        return Success(-1ULL, E);
-      return Success(0, E);
-    }
+    // Likewise, if Type is 3, we must handle this because CodeGen cannot give a
+    // conservatively correct answer in that case.
+    if (E->getArg(0)->HasSideEffects(Info.Ctx) || Type == 3)
+      return Success((Type & 2) ? 0 : -1, E);
 
     // Expression had no side effects, but we couldn't statically determine the
     // size of the referenced object.
@@ -6254,10 +6289,12 @@ bool IntExprEvaluator::VisitCallExpr(const CallExpr *E) {
     case EvalInfo::EM_ConstantFold:
     case EvalInfo::EM_EvaluateForOverflow:
     case EvalInfo::EM_IgnoreSideEffects:
+      // Leave it to IR generation.
       return Error(E);
     case EvalInfo::EM_ConstantExpressionUnevaluated:
     case EvalInfo::EM_PotentialConstantExpressionUnevaluated:
-      return Success(-1ULL, E);
+      // Reduce it to a constant now.
+      return Success((Type & 2) ? 0 : -1, E);
     }
   }
 
diff --git a/test/CodeGen/object-size.c b/test/CodeGen/object-size.c
index 3fa038a..17a4f53 100644
--- a/test/CodeGen/object-size.c
+++ b/test/CodeGen/object-size.c
@@ -146,3 +146,36 @@ unsigned test18(int cond) {
   // CHECK: call i64 @llvm.objectsize.i64
   return __builtin_object_size(cond ? a : b, 0);
 }
+
+// CHECK: @test19
+void test19() {
+  struct {
+    char a[4];
+    char b[4];
+  } foo;
+
+  // CHECK: store i32 8
+  gi = __builtin_object_size(&foo.a, 0);
+  // CHECK: store i32 4
+  gi = __builtin_object_size(&foo.a, 1);
+  // CHECK: store i32 8
+  gi = __builtin_object_size(&foo.a, 2);
+  // CHECK: store i32 4
+  gi = __builtin_object_size(&foo.a, 3);
+}
+
+// CHECK: @test20
+void test20() {
+  struct {
+    int t[10];
+  } t[10];
+
+  // CHECK: store i32 380
+  gi = __builtin_object_size(&t[0].t[5], 0);
+  // CHECK: store i32 20
+  gi = __builtin_object_size(&t[0].t[5], 1);
+  // CHECK: store i32 380
+  gi = __builtin_object_size(&t[0].t[5], 2);
+  // CHECK: store i32 20
+  gi = __builtin_object_size(&t[0].t[5], 3);
+}
diff --git a/test/Sema/const-eval.c b/test/Sema/const-eval.c
index 5f5b6f3..317173a 100644
--- a/test/Sema/const-eval.c
+++ b/test/Sema/const-eval.c
@@ -118,10 +118,6 @@ float varfloat;
 const float constfloat = 0;
 EVAL_EXPR(43, varfloat && constfloat) // expected-error {{must have a constant size}}
 
-// <rdar://problem/11205586>
-// (Make sure we continue to reject this.)
-EVAL_EXPR(44, "x"[0]); // expected-error {{variable length array}}
-
 // <rdar://problem/10962435>
 EVAL_EXPR(45, ((char*)-1) + 1 == 0 ? 1 : -1)
 EVAL_EXPR(46, ((char*)-1) + 1 < (char*) -1 ? 1 : -1)


More information about the cfe-dev mailing list