Bugfix - Clang handles __builtin_object_size in wrong way

[george.burgess.iv]

Summary

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. :)

[rsmith]

lib/AST/ExprConstant.cpp

6221–6223

Please add a testcase like this (where the base object is unknown but the designator is known, and thus we can compute the Type == 1 and Type == 3 forms but not the Type == 0 and Type == 2 forms).

6243–6245

Please add testcases for the pointer-to-the-end case:

int n;
static_assert(__builtin_object_size(&n + 1, 1) == 0);

struct X { int a, b, c; } x;
static_assert(__builtin_object_size(&x.a + 1, 1) == 0);

[george.burgess.iv]

Addressed feedback.

lib/AST/ExprConstant.cpp

6221–6223

EvaluatePointer is stricter than I thought -- this is actually not possible without adding a decent amount of complexity to the current patch. Will replace the comment with a TODO, and add the test case in the next patch that makes us support things like this. :)

[rsmith]

LGTM with one more testcase :)

lib/AST/ExprConstant.cpp

6243–6245

I think you now have a testcase for the end-of-array case, but not the end-of-nonarray case.

[george.burgess.iv]

r245323

lib/AST/ExprConstant.cpp

bool checkNullPointer(EvalInfo &Info, const Expr *E,
         return false;
       }
       return true;
     }
 
     // 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);
     }
 
     void addDecl(EvalInfo &Info, const Expr *E,
                  const Decl *D, bool Virtual = false) {
       if (checkSubobject(Info, E, isa<FieldDecl>(D) ? CSK_Field : CSK_Base))
         Designator.addDeclUnchecked(D, Virtual);

 static bool handleLValueToRValueConversion(EvalInfo &Info, const Expr *Conv,
                                            QualType Type,
                                            const LValue &LVal, APValue &RVal) {
   if (LVal.Designator.Invalid)
     return false;
 
   // 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
       // an ICE in C, so this only matters for fold.
       assert(!Info.getLangOpts().CPlusPlus && "lvalue compound literal in c++?");
       if (Type.isVolatileQualified()) {
         Info.Diag(Conv);
         return false;
public:
 
   bool VisitUnaryReal(const UnaryOperator *E);
   bool VisitUnaryImag(const UnaryOperator *E);
 
   bool VisitCXXNoexceptExpr(const CXXNoexceptExpr *E);
   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
 
 /// EvaluateIntegerOrLValue - Evaluate an rvalue integral-typed expression, and
 /// produce either the integer value or a pointer.
 ///
 /// GCC has a heinous extension which folds casts between pointer types and
static bool EvaluateBuiltinConstantP(ASTContext &Ctx, const Expr *Arg) {
   }
 
   // Anything else isn't considered to be sufficiently constant.
   return false;
 }
 
 /// 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();
   } else if (const Expr *E = B.get<const Expr*>()) {
     if (isa<CompoundLiteralExpr>(E))
       return E->getType();
   }
 
   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())
+
+  // 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())
+  // 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.
+  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;
+
+  auto EndOffset = End.getLValueOffset();
+  if (BaseOffset > EndOffset)
+    return Success(0, E);
 
-  if (!Offset.isNegative() && Offset <= Size)
-    Size -= Offset;
-  else
-    Size = CharUnits::Zero();
-  return Success(Size, E);
+  return Success(EndOffset - BaseOffset, E);
 }
 
 bool IntExprEvaluator::VisitCallExpr(const CallExpr *E) {
   switch (unsigned BuiltinOp = E->getBuiltinCallee()) {
   default:
     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.
     switch (Info.EvalMode) {
     case EvalInfo::EM_ConstantExpression:
     case EvalInfo::EM_PotentialConstantExpression:
     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);
     }
   }
 
   case Builtin::BI__builtin_bswap16:
   case Builtin::BI__builtin_bswap32:
   case Builtin::BI__builtin_bswap64: {
     APSInt Val;
     if (!EvaluateInteger(E->getArg(0), Val, Info))

test/CodeGen/object-size.c

 
 // CHECK: @test18
 unsigned test18(int cond) {
   int a[4], b[4];
   // CHECK: phi i32*
   // CHECK: call i64 @llvm.objectsize.i64
   return __builtin_object_size(cond ? a : b, 0);
 }
+
+// CHECK: @test19
+void test19() {
+  struct {
+    int a, b;
+  } 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);
+}
+
+// CHECK: @test21
+void test21() {
+  struct { int t[10]; } t[10];
+
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[10], 0);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[10], 1);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[10], 2);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[10], 3);
+
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[9].t[10], 0);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[9].t[10], 1);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[9].t[10], 2);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[9].t[10], 3);
+}
+
+struct Test22Ty { int t[10]; };
+
+// CHECK: @test22
+void test22(struct Test22Ty *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);
+}

test/Sema/const-eval.c

 struct PR11391 { _Complex float f; } pr11391;
 EVAL_EXPR(42, __builtin_constant_p(pr11391.f = 1))
 
 // PR12043
 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)
 EVAL_EXPR(47, &x < &x + 1 ? 1 : -1)
 EVAL_EXPR(48, &x != &x - 1 ? 1 : -1)
 EVAL_EXPR(49, &x < &x - 100 ? 1 : -1) // expected-error {{must have a constant size}}
 
 extern struct Test50S Test50;
 EVAL_EXPR(50, &Test50 < (struct Test50S*)((unsigned)&Test50 + 10)) // expected-error {{must have a constant size}}
 
 // <rdar://problem/11874571>
 EVAL_EXPR(51, 0 != (float)1e99)
 
 // PR21945
 void PR21945() { int i = (({}), 0l); }