CodeGen: Don't crash when initializing pointer-to-member fields in bases

Clang uses two types to talk about a C++ class, the
NonVirtualBaseLLVMType and the LLVMType.  Previously, we would allow one
of these to be packed and the other not.

This is problematic.  If both don't agree on a common subset of fields,
then routines like getLLVMFieldNo will point to the wrong field.  Solve
this by copying the 'packed'-ness of the complete type to the
non-virtual subobject.  For this to work, we need to take into account
the non-virtual subobject's size and alignment when we are computing the
layout of the complete object.

This fixes PR21089.

llvm-svn: 218577

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

@@ -93,7 +93,7 @@ struct CGRecordLowering {
     bool operator <(const MemberInfo& a) const { return Offset < a.Offset; }
   };
   // The constructor.
-  CGRecordLowering(CodeGenTypes &Types, const RecordDecl *D);
+  CGRecordLowering(CodeGenTypes &Types, const RecordDecl *D, bool Packed);
   // Short helper routines.
   /// \brief Constructs a MemberInfo instance from an offset and llvm::Type *.
   MemberInfo StorageInfo(CharUnits Offset, llvm::Type *Data) {
@@ -174,7 +174,7 @@ struct CGRecordLowering {
   /// padding that is or can potentially be used.
   void clipTailPadding();
   /// \brief Determines if we need a packed llvm struct.
-  void determinePacked();
+  void determinePacked(bool NVBaseType);
   /// \brief Inserts padding everwhere it's needed.
   void insertPadding();
   /// \brief Fills out the structures that are ultimately consumed.
@@ -203,12 +203,12 @@ private:
 };
 } // namespace {
 
-CGRecordLowering::CGRecordLowering(CodeGenTypes &Types, const RecordDecl *D)
+CGRecordLowering::CGRecordLowering(CodeGenTypes &Types, const RecordDecl *D,                                 bool Packed)
   : Types(Types), Context(Types.getContext()), D(D),
     RD(dyn_cast<CXXRecordDecl>(D)),
     Layout(Types.getContext().getASTRecordLayout(D)),
     DataLayout(Types.getDataLayout()), IsZeroInitializable(true),
-    IsZeroInitializableAsBase(true), Packed(false) {}
+    IsZeroInitializableAsBase(true), Packed(Packed) {}
 
 void CGRecordLowering::setBitFieldInfo(
     const FieldDecl *FD, CharUnits StartOffset, llvm::Type *StorageType) {
@@ -269,7 +269,7 @@ void CGRecordLowering::lower(bool NVBaseType) {
   std::stable_sort(Members.begin(), Members.end());
   Members.push_back(StorageInfo(Size, getIntNType(8)));
   clipTailPadding();
-  determinePacked();
+  determinePacked(NVBaseType);
   insertPadding();
   Members.pop_back();
   calculateZeroInit();
@@ -533,8 +533,13 @@ void CGRecordLowering::clipTailPadding() {
   }
 }
 
-void CGRecordLowering::determinePacked() {
+void CGRecordLowering::determinePacked(bool NVBaseType) {
+  if (Packed)
+    return;
   CharUnits Alignment = CharUnits::One();
+  CharUnits NVAlignment = CharUnits::One();
+  CharUnits NVSize =
+      !NVBaseType && RD ? Layout.getNonVirtualSize() : CharUnits::Zero();
   for (std::vector<MemberInfo>::const_iterator Member = Members.begin(),
                                                MemberEnd = Members.end();
        Member != MemberEnd; ++Member) {
@@ -544,12 +549,19 @@ void CGRecordLowering::determinePacked() {
     // then the entire record must be packed.
     if (Member->Offset % getAlignment(Member->Data))
       Packed = true;
+    if (Member->Offset < NVSize)
+      NVAlignment = std::max(NVAlignment, getAlignment(Member->Data));
     Alignment = std::max(Alignment, getAlignment(Member->Data));
   }
   // If the size of the record (the capstone's offset) is not a multiple of the
   // record's alignment, it must be packed.
   if (Members.back().Offset % Alignment)
     Packed = true;
+  // If the non-virtual sub-object is not a multiple of the non-virtual
+  // sub-object's alignment, it must be packed.  We cannot have a packed
+  // non-virtual sub-object and an unpacked complete object or vise versa.
+  if (NVSize % NVAlignment)
+    Packed = true;
   // Update the alignment of the sentinal.
   if (!Packed)
     Members.back().Data = getIntNType(Context.toBits(Alignment));
@@ -641,20 +653,24 @@ CGBitFieldInfo CGBitFieldInfo::MakeInfo(CodeGenTypes &Types,
 
 CGRecordLayout *CodeGenTypes::ComputeRecordLayout(const RecordDecl *D,
                                                   llvm::StructType *Ty) {
-  CGRecordLowering Builder(*this, D);
+  CGRecordLowering Builder(*this, D, /*Packed=*/false);
 
-  Builder.lower(false);
+  Builder.lower(/*NonVirtualBaseType=*/false);
 
   // If we're in C++, compute the base subobject type.
   llvm::StructType *BaseTy = nullptr;
   if (isa<CXXRecordDecl>(D) && !D->isUnion() && !D->hasAttr<FinalAttr>()) {
     BaseTy = Ty;
     if (Builder.Layout.getNonVirtualSize() != Builder.Layout.getSize()) {
-      CGRecordLowering BaseBuilder(*this, D);
+      CGRecordLowering BaseBuilder(*this, D, /*Packed=*/Builder.Packed);
-      BaseBuilder.lower(true);
+      BaseBuilder.lower(/*NonVirtualBaseType=*/true);
       BaseTy = llvm::StructType::create(
           getLLVMContext(), BaseBuilder.FieldTypes, "", BaseBuilder.Packed);
       addRecordTypeName(D, BaseTy, ".base");
+      // BaseTy and Ty must agree on their packedness for getLLVMFieldNo to work
+      // on both of them with the same index.
+      assert(Builder.Packed == BaseBuilder.Packed &&
+             "Non-virtual and complete types must agree on packedness");
     }
   }
 

clang/test/CodeGenCXX/class-layout.cpp

@@ -14,7 +14,7 @@ namespace Test2 {
 
 namespace Test3 {
   // C should have a vtable pointer.
-  // CHECK: %"struct.Test3::A" = type { i32 (...)**, i32 }
+  // CHECK: %"struct.Test3::A" = type <{ i32 (...)**, i32, [4 x i8] }>
   struct A { virtual void f(); int a; } *a;
 }
 

clang/test/CodeGenCXX/pointers-to-data-members.cpp

@@ -55,7 +55,7 @@ namespace ZeroInit {
   };
 
   struct C : A, B { int j; };
-  // CHECK-GLOBAL: @_ZN8ZeroInit1cE = global {{%.*}} { %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::B" { [10 x %"struct.ZeroInit::A"] [%"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }], i8 0, i64 -1 }, i32 0 }, align 8
+  // CHECK-GLOBAL: @_ZN8ZeroInit1cE = global {{%.*}} <{ %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::B" { [10 x %"struct.ZeroInit::A"] [%"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }, %"struct.ZeroInit::A" { i64 -1, i32 0 }], i8 0, i64 -1 }, i32 0, [4 x i8] zeroinitializer }>, align 8
   C c;
 }
 
@@ -255,4 +255,17 @@ namespace PR13097 {
   // CHECK: call void @_ZN7PR130971XC1ERKS0_
 }
 
+namespace PR21089 {
+struct A {
+  bool : 1;
+  int A::*x;
+  bool y;
+  A();
+};
+struct B : A {
+};
+B b;
+// CHECK-GLOBAL: @_ZN7PR210891bE = global %"struct.PR21089::B" { %"struct.PR21089::A.base" <{ i8 0, [7 x i8] zeroinitializer, i64 -1, i8 0 }>, [7 x i8] zeroinitializer }, align 8
+}
+
 // CHECK-O3: attributes [[NUW]] = { nounwind readnone{{.*}} }

clang/test/CodeGenCXX/pr18962.cpp

@@ -26,7 +26,7 @@ fn2(C *) {
 
 // We end up using an opaque type for 'append' to avoid circular references.
 // CHECK: %class.A = type { {}* }
-// CHECK: %class.C = type { %class.D*, %class.B }
+// CHECK: %class.C = type <{ %class.D*, %class.B, [3 x i8] }>
 // CHECK: %class.D = type { %class.C.base, [3 x i8] }
 // CHECK: %class.C.base = type <{ %class.D*, %class.B }>
 // CHECK: %class.B = type { i8 }