Fix PR26741 -- __builtin_object_size is not consistently conservative with C++ inheritance

[george.burgess.iv]

Summary

This patch fixes PR26741, and makes us handle inheritance more sanely.

Broken code:

struct Foo { char a[1]; };
struct Bar : Foo {};

int break() {
  Bar *b;
  int size = __builtin_object_size(b->a, 1);
  assert(size == -1); // Fires; size is 1.
}

Because we're now handling inheritance, this patch has a few fun things in it (see: the new loop at ExprConstant.cpp:6489) so that we aren't overly conservative when inheritance is involved. I'm not entirely thrilled with how we determine if a base class is considered to be at the end of a derived class; better approaches are appreciated.

[rsmith]

Can you explain a bit more about the problem? It seems to me that if I have:

struct Base {
  char k[1];
};
struct Derived : Base {};

... then the 'k' subobject of a Derived object is known to be exactly 1 byte long -- it doesn't seem obviously appropriate for our flexible-sized trailing array support to cover this case.

[george.burgess.iv]

I don't feel strongly about how we should handle this, to be honest. Feeding your example into GCC 4.8 like so:

#include <stdio.h>

struct Foo { char k[1]; };

struct Bar : Foo {};

int __attribute__((noinline)) bos(Bar *b) {
  return __builtin_object_size(&b->k[0], 1);
}

int main() {
  Bar b;
  printf("%d\n", bos(&b));
  return 0;
}

...The resultant executable prints "1". So, it seems that having this detection would be above and beyond what GCC offers. Given that "above and beyond," in this case implies "less likely to produce an accurate answer," I agree that this probably isn't such a great idea. :)

Thanks for the feedback!

lib/AST/ExprConstant.cpp

 ///   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 IsBaseAtEndOfDerived = [&Ctx](const CXXRecordDecl *Base,
+                                     const CXXRecordDecl *Derived) {
+    assert(Derived->isDerivedFrom(Base) && "Derived must be derived from Base");
+
+    // Cheap check: if we have members in the derived class, we know the
+    // designator can't be at the end.
+    if (!Derived->field_empty())
+      return false;
+
+    // Just give up when virtual inheritance is involved.
+    if (Derived->isVirtuallyDerivedFrom(Base))
+      return false;
+
+    // Otherwise, check the actual layout of the objects to see if Base lies at
+    // the end of Derived's layout.
+    auto &DerivedLayout = Ctx.getASTRecordLayout(Derived);
+    CharUnits BaseOffset = DerivedLayout.getBaseClassOffset(Base);
+    CharUnits BaseSize = Ctx.getASTRecordLayout(Base).getSize();
+    return BaseOffset + BaseSize == DerivedLayout.getSize();
+  };
+
   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 *>())) {
+    // The given MemberExpr's base may have implicit derived to base
+    // conversions. Make a best-effort attempt to find the most derived
+    // object, and check to see if we're at the end of that.
+    //
+    // Because we're meant to interpret this "as written", we won't traverse
+    // through explicit casts. In the worst case, we answer a bit more
+    // conservatively than we need to.
+    QualType RecentBaseTy = ME->getBase()->getType();
+    const Expr *Derived = ME->getBase();
+    while (auto *CE = dyn_cast<ImplicitCastExpr>(Derived)) {
+      if (CE->getCastKind() != CK_DerivedToBase &&
+          CE->getCastKind() != CK_UncheckedDerivedToBase)
+        break;
+
+      Derived = CE->getSubExpr();
+      QualType BaseTy = RecentBaseTy;
+      QualType DerivedTy = Derived->getType();
+      if (ME->isArrow()) {
+        assert(BaseTy->isPointerType() && DerivedTy->isPointerType() &&
+               "Derived->Base removed a pointer.");
+        BaseTy = BaseTy->castAs<PointerType>()->getPointeeType();
+        DerivedTy = DerivedTy->castAs<PointerType>()->getPointeeType();
+      }
+
+      const CXXRecordDecl *BaseDecl = BaseTy->getAsCXXRecordDecl();
+      const CXXRecordDecl *DerivedDecl = DerivedTy->getAsCXXRecordDecl();
+      assert(BaseDecl && DerivedDecl &&
+             "Derived->Base casts on non-CXXRecordDecl types");
+      assert(BaseDecl != DerivedDecl &&
+             "Classes can't be derived from themselves");
+
+      // This check can't just be done once at the end of the loop because we
+      // may be in an inheritance heierarchy where the most derived type
+      // inherits from multiple `Base`s. We can, however, skip doing it on each
+      // individual base in this cast chain.
+      if (!IsBaseAtEndOfDerived(BaseDecl, DerivedDecl))
+        return false;
+
+      RecentBaseTy = Derived->getType();
+    }
+
     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;
     }
if (BaseType->isArrayType()) {
       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");
+      const CXXRecordDecl *Base = getAsBaseClass(LVal.Designator.Entries[I]);
+      const CXXRecordDecl *Derived = BaseType->getAsCXXRecordDecl();
+      assert(Base && Derived &&
+             "Expected two CXXRecordDecls for base->derived conversion");
+
+      if (!IsBaseAtEndOfDerived(Base, Derived))
         return false;
+      BaseType = QualType(Base->getTypeForDecl(), 0);
     }
   }
   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())

test/CodeGen/object-size.cpp

-// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
 
 // C++-specific tests for __builtin_object_size
 
 int gi;
 
 // CHECK-LABEL: define void @_Z5test1v()
 void test1() {
   // Guaranteeing that our cast removal logic doesn't break more interesting
void test2() {
   gi = __builtin_object_size(&c->bs[0].buf[0], 0);
   // CHECK: store i32 16
   gi = __builtin_object_size(&c->bs[0].buf[0], 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
   gi = __builtin_object_size(&c->bs[0].buf[0], 2);
   // CHECK: store i32 16
   gi = __builtin_object_size(&c->bs[0].buf[0], 3);
 }
+
+// CHECK-LABEL: define void @_Z5test3v()
+void test3() {
+  struct A { int i; char buf[1]; };
+  struct B : A {};
+  struct C : B {};
+  struct D { int i; B bs[1]; } *d;
+
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(&d->bs[0].buf[0], 0);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(&d->bs[0].buf[0], 1);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
+  gi = __builtin_object_size(&d->bs[0].buf[0], 2);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&d->bs[0].buf[0], 3);
+}
+
+// CHECK-LABEL: define void @_Z5test4v()
+void test4() {
+  struct A { int i; char buf[1]; };
+  struct B : A { char c; };
+  struct C { int i; B bs[1]; } *c;
+
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&c->bs[0].buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&c->bs[0].buf[0], 3);
+}
+
+// CHECK-LABEL: define void @_Z5test5v()
+void test5() {
+  struct A { int i; char buf[1]; };
+  struct B : A { char c; };
+  struct C : B {};
+  struct D { int i; B bs[1]; } *d;
+
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&d->bs[0].buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&d->bs[0].buf[0], 3);
+}
+
+// CHECK-LABEL: define void @_Z5test6v()
+void test6() {
+  // A   A
+  // |   |
+  // B   C
+  //  \ /
+  //   D
+  struct A { int i; char buf[1]; };
+  struct B : A {};
+  struct C : A {};
+  struct D : B, C {} *d;
+
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&d->B::buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&d->B::buf[0], 3);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(&d->C::buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&d->C::buf[0], 3);
+
+  D &dr = *d;
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&dr.B::buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&dr.B::buf[0], 3);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(&dr.C::buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&dr.C::buf[0], 3);
+
+  D &&drval = static_cast<D&&>(*d);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&drval.B::buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&drval.B::buf[0], 3);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(&drval.C::buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&drval.C::buf[0], 3);
+
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&static_cast<D&&>(drval).B::buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&static_cast<D&&>(drval).B::buf[0], 3);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(&static_cast<D&&>(drval).C::buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&static_cast<D&&>(drval).C::buf[0], 3);
+}
+
+// CHECK-LABEL: define void @_Z5test7v()
+void test7() {
+  // Always give up when virtual inheritance is involved.
+  struct A { int i; char buf[1]; };
+  struct B : virtual A {};
+  struct C : virtual A {};
+  struct D : B, C {} *d;
+
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&d->buf[0], 1);
+  // CHECK: store i32 1
+  gi = __builtin_object_size(&d->buf[0], 3);
+}