Make __builtin_object_size more conservative

r246877 made __builtin_object_size substantially more aggressive with
unknown bases if Type=1 or Type=3, which causes issues when we encounter
code like this:

struct Foo {
  int a;
  char str[1];
};

const char str[] = "Hello, World!";
struct Foo *f = (struct Foo *)malloc(sizeof(*f) + strlen(str));
strcpy(&f->str, str);

__builtin_object_size(&f->str, 1) would hand back 1, which is
technically correct given the type of Foo, but the type of Foo lies to
us about how many bytes are available in this case.

This patch adds support for this "writing off the end" idiom -- we now
answer conservatively when we're given the address of the very last
member in a struct.

Differential Revision: http://reviews.llvm.org/D12169

llvm-svn: 250488

1 files changed, 138 insertions, 22 deletions

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index df1f6ba4711..08fcd260a3b 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -114,7 +114,8 @@ namespace {
   static
   unsigned findMostDerivedSubobject(ASTContext &Ctx, QualType Base,
                                     ArrayRef<APValue::LValuePathEntry> Path,
-                                    uint64_t &ArraySize, QualType &Type) {
+                                    uint64_t &ArraySize, QualType &Type,
+                                    bool &IsArray) {
     unsigned MostDerivedLength = 0;
     Type = Base;
     for (unsigned I = 0, N = Path.size(); I != N; ++I) {
@@ -124,18 +125,22 @@ namespace {
         Type = CAT->getElementType();
         ArraySize = CAT->getSize().getZExtValue();
         MostDerivedLength = I + 1;
+        IsArray = true;
       } else if (Type->isAnyComplexType()) {
         const ComplexType *CT = Type->castAs<ComplexType>();
         Type = CT->getElementType();
         ArraySize = 2;
         MostDerivedLength = I + 1;
+        IsArray = true;
       } else if (const FieldDecl *FD = getAsField(Path[I])) {
         Type = FD->getType();
         ArraySize = 0;
         MostDerivedLength = I + 1;
+        IsArray = false;
       } else {
         // Path[I] describes a base class.
         ArraySize = 0;
+        IsArray = false;
       }
     }
     return MostDerivedLength;
@@ -157,12 +162,17 @@ namespace {
     /// Is this a pointer one past the end of an object?
     bool IsOnePastTheEnd : 1;
 
+    /// Indicator of whether the most-derived object is an array element.
+    bool MostDerivedIsArrayElement : 1;
+
     /// The length of the path to the most-derived object of which this is a
     /// subobject.
-    unsigned MostDerivedPathLength : 30;
+    unsigned MostDerivedPathLength : 29;
 
-    /// The size of the array of which the most-derived object is an element, or
-    /// 0 if the most-derived object is not an array element.
+    /// The size of the array of which the most-derived object is an element.
+    /// This will always be 0 if the most-derived object is not an array
+    /// element. 0 is not an indicator of whether or not the most-derived object
+    /// is an array, however, because 0-length arrays are allowed.
     uint64_t MostDerivedArraySize;
 
     /// The type of the most derived object referred to by this address.
@@ -176,21 +186,26 @@ namespace {
     SubobjectDesignator() : Invalid(true) {}
 
     explicit SubobjectDesignator(QualType T)
-      : Invalid(false), IsOnePastTheEnd(false), MostDerivedPathLength(0),
-        MostDerivedArraySize(0), MostDerivedType(T) {}
+        : Invalid(false), IsOnePastTheEnd(false),
+          MostDerivedIsArrayElement(false), MostDerivedPathLength(0),
+          MostDerivedArraySize(0), MostDerivedType(T) {}
 
     SubobjectDesignator(ASTContext &Ctx, const APValue &V)
-      : Invalid(!V.isLValue() || !V.hasLValuePath()), IsOnePastTheEnd(false),
-        MostDerivedPathLength(0), MostDerivedArraySize(0) {
+        : Invalid(!V.isLValue() || !V.hasLValuePath()), IsOnePastTheEnd(false),
+          MostDerivedIsArrayElement(false), MostDerivedPathLength(0),
+          MostDerivedArraySize(0) {
       if (!Invalid) {
         IsOnePastTheEnd = V.isLValueOnePastTheEnd();
         ArrayRef<PathEntry> VEntries = V.getLValuePath();
         Entries.insert(Entries.end(), VEntries.begin(), VEntries.end());
-        if (V.getLValueBase())
+        if (V.getLValueBase()) {
+          bool IsArray = false;
           MostDerivedPathLength =
               findMostDerivedSubobject(Ctx, getType(V.getLValueBase()),
                                        V.getLValuePath(), MostDerivedArraySize,
-                                       MostDerivedType);
+                                       MostDerivedType, IsArray);
+          MostDerivedIsArrayElement = IsArray;
+        }
       }
     }
 
@@ -204,7 +219,7 @@ namespace {
       assert(!Invalid);
       if (IsOnePastTheEnd)
         return true;
-      if (MostDerivedArraySize &&
+      if (MostDerivedIsArrayElement &&
           Entries[MostDerivedPathLength - 1].ArrayIndex == MostDerivedArraySize)
         return true;
       return false;
@@ -228,6 +243,7 @@ namespace {
 
       // This is a most-derived object.
       MostDerivedType = CAT->getElementType();
+      MostDerivedIsArrayElement = true;
       MostDerivedArraySize = CAT->getSize().getZExtValue();
       MostDerivedPathLength = Entries.size();
     }
@@ -242,6 +258,7 @@ namespace {
       // If this isn't a base class, it's a new most-derived object.
       if (const FieldDecl *FD = dyn_cast<FieldDecl>(D)) {
         MostDerivedType = FD->getType();
+        MostDerivedIsArrayElement = false;
         MostDerivedArraySize = 0;
         MostDerivedPathLength = Entries.size();
       }
@@ -255,6 +272,7 @@ namespace {
       // This is technically a most-derived object, though in practice this
       // is unlikely to matter.
       MostDerivedType = EltTy;
+      MostDerivedIsArrayElement = true;
       MostDerivedArraySize = 2;
       MostDerivedPathLength = Entries.size();
     }
@@ -262,7 +280,8 @@ namespace {
     /// Add N to the address of this subobject.
     void adjustIndex(EvalInfo &Info, const Expr *E, uint64_t N) {
       if (Invalid) return;
-      if (MostDerivedPathLength == Entries.size() && MostDerivedArraySize) {
+      if (MostDerivedPathLength == Entries.size() &&
+          MostDerivedIsArrayElement) {
         Entries.back().ArrayIndex += N;
         if (Entries.back().ArrayIndex > MostDerivedArraySize) {
           diagnosePointerArithmetic(Info, E, Entries.back().ArrayIndex);
@@ -834,7 +853,7 @@ bool SubobjectDesignator::checkSubobject(EvalInfo &Info, const Expr *E,
 
 void SubobjectDesignator::diagnosePointerArithmetic(EvalInfo &Info,
                                                     const Expr *E, uint64_t N) {
-  if (MostDerivedPathLength == Entries.size() && MostDerivedArraySize)
+  if (MostDerivedPathLength == Entries.size() && MostDerivedIsArrayElement)
     Info.CCEDiag(E, diag::note_constexpr_array_index)
       << static_cast<int>(N) << /*array*/ 0
       << static_cast<unsigned>(MostDerivedArraySize);
@@ -2517,7 +2536,7 @@ static bool AreElementsOfSameArray(QualType ObjType,
   if (A.Entries.size() != B.Entries.size())
     return false;
 
-  bool IsArray = A.MostDerivedArraySize != 0;
+  bool IsArray = A.MostDerivedIsArrayElement;
   if (IsArray && A.MostDerivedPathLength != A.Entries.size())
     // A is a subobject of the array element.
     return false;
@@ -4416,7 +4435,8 @@ public:
     if (!EvalOK) {
       if (!this->Info.allowInvalidBaseExpr())
         return false;
-      Result.setInvalid(E->getBase());
+      Result.setInvalid(E);
+      return true;
     }
 
     const ValueDecl *MD = E->getMemberDecl();
@@ -6276,6 +6296,85 @@ static const Expr *ignorePointerCastsAndParens(const Expr *E) {
   return ignorePointerCastsAndParens(SubExpr);
 }
 
+/// Checks to see if the given LValue's Designator is at the end of the LValue's
+/// record layout. e.g.
+///   struct { struct { int a, b; } fst, snd; } obj;
+///   obj.fst   // no
+///   obj.snd   // yes
+///   obj.fst.a // no
+///   obj.fst.b // no
+///   obj.snd.a // no
+///   obj.snd.b // yes
+///
+/// Please note: this function is specialized for how __builtin_object_size
+/// views "objects".
+static bool isDesignatorAtObjectEnd(const ASTContext &Ctx, const LValue &LVal) {
+  assert(!LVal.Designator.Invalid);
+
+  auto IsLastFieldDecl = [&Ctx](const FieldDecl *FD) {
+    if (FD->getParent()->isUnion())
+      return true;
+    const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(FD->getParent());
+    return FD->getFieldIndex() + 1 == Layout.getFieldCount();
+  };
+
+  auto &Base = LVal.getLValueBase();
+  if (auto *ME = dyn_cast_or_null<MemberExpr>(Base.dyn_cast<const Expr *>())) {
+    if (auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
+      if (!IsLastFieldDecl(FD))
+        return false;
+    } else if (auto *IFD = dyn_cast<IndirectFieldDecl>(ME->getMemberDecl())) {
+      for (auto *FD : IFD->chain())
+        if (!IsLastFieldDecl(cast<FieldDecl>(FD)))
+          return false;
+    }
+  }
+
+  QualType BaseType = getType(Base);
+  for (int I = 0, E = LVal.Designator.Entries.size(); I != E; ++I) {
+    if (BaseType->isArrayType()) {
+      // Because __builtin_object_size treats arrays as objects, we can ignore
+      // the index iff this is the last array in the Designator.
+      if (I + 1 == E)
+        return true;
+      auto *CAT = cast<ConstantArrayType>(Ctx.getAsArrayType(BaseType));
+      uint64_t Index = LVal.Designator.Entries[I].ArrayIndex;
+      if (Index + 1 != CAT->getSize())
+        return false;
+      BaseType = CAT->getElementType();
+    } else if (BaseType->isAnyComplexType()) {
+      auto *CT = BaseType->castAs<ComplexType>();
+      uint64_t Index = LVal.Designator.Entries[I].ArrayIndex;
+      if (Index != 1)
+        return false;
+      BaseType = CT->getElementType();
+    } else if (auto *FD = getAsField(LVal.Designator.Entries[I])) {
+      if (!IsLastFieldDecl(FD))
+        return false;
+      BaseType = FD->getType();
+    } else {
+      assert(getAsBaseClass(LVal.Designator.Entries[I]) != nullptr &&
+             "Expecting cast to a base class");
+      return false;
+    }
+  }
+  return true;
+}
+
+/// Tests to see if the LValue has a designator (that isn't necessarily valid).
+static bool refersToCompleteObject(const LValue &LVal) {
+  if (LVal.Designator.Invalid || !LVal.Designator.Entries.empty())
+    return false;
+
+  if (!LVal.InvalidBase)
+    return true;
+
+  auto *E = LVal.Base.dyn_cast<const Expr *>();
+  (void)E;
+  assert(E != nullptr && isa<MemberExpr>(E));
+  return false;
+}
+
 bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
                                                     unsigned Type) {
   // Determine the denoted object.
@@ -6299,8 +6398,7 @@ bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
 
   // 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;
+  bool SubobjectOnly = (Type & 1) != 0 && !refersToCompleteObject(Base);
 
   // 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.
@@ -6313,17 +6411,17 @@ bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
   //   int b = __builtin_object_size(p->buff + 4, 2); // returns 0, not 40
   //
   // This matches GCC's behavior.
-  if ((Type & 1) == 0 && Base.InvalidBase)
+  if (Base.InvalidBase && !SubobjectOnly)
     return Error(E);
 
-  // If Type & 1 is 0, the object in question is the complete object; reset to
-  // a complete object designator in that case.
+  // If we're not examining only the subobject, then we reset to a complete
+  // object designator
   //
   // 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)) {
+  if (!SubobjectOnly || (End.Designator.Invalid && Type == 1)) {
     QualType T = getObjectType(End.getLValueBase());
     if (T.isNull())
       End.Designator.setInvalid();
@@ -6343,7 +6441,7 @@ bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
   // 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 &&
+  if (End.Designator.MostDerivedIsArrayElement &&
       End.Designator.Entries.size() == End.Designator.MostDerivedPathLength) {
     // We got a pointer to an array. Step to its end.
     AmountToAdd = End.Designator.MostDerivedArraySize -
@@ -6363,6 +6461,24 @@ bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
     return false;
 
   auto EndOffset = End.getLValueOffset();
+
+  // The following is a moderately common idiom in C:
+  //
+  // struct Foo { int a; char c[1]; };
+  // struct Foo *F = (struct Foo *)malloc(sizeof(struct Foo) + strlen(Bar));
+  // strcpy(&F->c[0], Bar);
+  //
+  // So, if we see that we're examining a 1-length (or 0-length) array at the
+  // end of a struct with an unknown base, we give up instead of breaking code
+  // that behaves this way. Note that we only do this when Type=1, because
+  // Type=3 is a lower bound, so answering conservatively is fine.
+  if (End.InvalidBase && SubobjectOnly && Type == 1 &&
+      End.Designator.Entries.size() == End.Designator.MostDerivedPathLength &&
+      End.Designator.MostDerivedIsArrayElement &&
+      End.Designator.MostDerivedArraySize < 2 &&
+      isDesignatorAtObjectEnd(Info.Ctx, End))
+    return false;
+
   if (BaseOffset > EndOffset)
     return Success(0, E);