Perform zero-initialization of virtual base classes when emitting

a zero constant for a complete class.  rdar://problem/8424975

To make this happen, track the field indexes for virtual bases
in the complete object.  I'm curious whether we might be better
off making CGRecordLayoutBuilder *much* more reliant on
ASTRecordLayout;  we're currently duplicating an awful lot of the ABI
layout logic.



git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@125555 91177308-0d34-0410-b5e6-96231b3b80d8

4 files changed, 275 insertions, 210 deletions

diff --git a/lib/CodeGen/CGExprConstant.cpp b/lib/CodeGen/CGExprConstant.cpp
index d4d9c92838..1f222a2bd1 100644
--- a/lib/CodeGen/CGExprConstant.cpp
+++ b/lib/CodeGen/CGExprConstant.cpp
@@ -1036,87 +1036,122 @@ FillInNullDataMemberPointers(CodeGenModule &CGM, QualType T,
   }
 }
 
-static llvm::Constant *EmitNullConstant(CodeGenModule &CGM,
-                                        const CXXRecordDecl *RD) {
-  QualType T = CGM.getContext().getTagDeclType(RD);
-
-  const llvm::StructType *STy =
-    cast<llvm::StructType>(CGM.getTypes().ConvertTypeForMem(T));
-  unsigned NumElements = STy->getNumElements();
-  std::vector<llvm::Constant *> Elements(NumElements);
+static llvm::Constant *EmitNullConstantForBase(CodeGenModule &CGM,
+                                               const llvm::Type *baseType,
+                                               const CXXRecordDecl *base);
 
-  const CGRecordLayout &Layout = CGM.getTypes().getCGRecordLayout(RD);
-
-  for (CXXRecordDecl::base_class_const_iterator I = RD->bases_begin(),
-       E = RD->bases_end(); I != E; ++I) {
+static llvm::Constant *EmitNullConstant(CodeGenModule &CGM,
+                                        const CXXRecordDecl *record,
+                                        bool asCompleteObject) {
+  const CGRecordLayout &layout = CGM.getTypes().getCGRecordLayout(record);
+  const llvm::StructType *structure =
+    (asCompleteObject ? layout.getLLVMType()
+                      : layout.getBaseSubobjectLLVMType());
+
+  unsigned numElements = structure->getNumElements();
+  std::vector<llvm::Constant *> elements(numElements);
+
+  // Fill in all the bases.
+  for (CXXRecordDecl::base_class_const_iterator
+         I = record->bases_begin(), E = record->bases_end(); I != E; ++I) {
     if (I->isVirtual()) {
-      // Ignore virtual bases.
+      // Ignore virtual bases; if we're laying out for a complete
+      // object, we'll lay these out later.
       continue;
     }
 
-    const CXXRecordDecl *BaseDecl = 
-      cast<CXXRecordDecl>(I->getType()->getAs<RecordType>()->getDecl());
+    const CXXRecordDecl *base = 
+      cast<CXXRecordDecl>(I->getType()->castAs<RecordType>()->getDecl());
 
     // Ignore empty bases.
-    if (BaseDecl->isEmpty())
+    if (base->isEmpty())
       continue;
     
-    // Ignore bases that don't have any pointer to data members.
-    if (CGM.getTypes().isZeroInitializable(BaseDecl))
-      continue;
-
-    unsigned BaseFieldNo = Layout.getNonVirtualBaseLLVMFieldNo(BaseDecl);
-    const llvm::Type *BaseTy = STy->getElementType(BaseFieldNo);
-
-    if (isa<llvm::StructType>(BaseTy)) {
-      // We can just emit the base as a null constant.
-      Elements[BaseFieldNo] = EmitNullConstant(CGM, BaseDecl);
-      continue;
-    }
-
-    // Some bases are represented as arrays of i8 if the size of the
-    // base is smaller than its corresponding LLVM type.
-    // Figure out how many elements this base array has.
-    const llvm::ArrayType *BaseArrayTy =  cast<llvm::ArrayType>(BaseTy);
-    unsigned NumBaseElements = BaseArrayTy->getNumElements();
-
-    // Fill in null data member pointers.
-    std::vector<llvm::Constant *> BaseElements(NumBaseElements);
-    FillInNullDataMemberPointers(CGM, I->getType(), BaseElements, 0);
-
-    // Now go through all other elements and zero them out.
-    if (NumBaseElements) {
-      const llvm::Type* Int8Ty = llvm::Type::getInt8Ty(CGM.getLLVMContext());
-      llvm::Constant *Zero = llvm::Constant::getNullValue(Int8Ty);
-      for (unsigned I = 0; I != NumBaseElements; ++I) {
-        if (!BaseElements[I])
-          BaseElements[I] = Zero;
-      }
-    }
-      
-    Elements[BaseFieldNo] = llvm::ConstantArray::get(BaseArrayTy, BaseElements);
+    unsigned fieldIndex = layout.getNonVirtualBaseLLVMFieldNo(base);
+    const llvm::Type *baseType = structure->getElementType(fieldIndex);
+    elements[fieldIndex] = EmitNullConstantForBase(CGM, baseType, base);
   }
 
-  // Visit all fields.
-  for (RecordDecl::field_iterator I = RD->field_begin(), E = RD->field_end();
-       I != E; ++I) {
-    const FieldDecl *FD = *I;
+  // Fill in all the fields.
+  for (RecordDecl::field_iterator I = record->field_begin(),
+         E = record->field_end(); I != E; ++I) {
+    const FieldDecl *field = *I;
     
     // Ignore bit fields.
-    if (FD->isBitField())
+    if (field->isBitField())
       continue;
     
-    unsigned FieldNo = Layout.getLLVMFieldNo(FD);
-    Elements[FieldNo] = CGM.EmitNullConstant(FD->getType());
+    unsigned fieldIndex = layout.getLLVMFieldNo(field);
+    elements[fieldIndex] = CGM.EmitNullConstant(field->getType());
+  }
+
+  // Fill in the virtual bases, if we're working with the complete object.
+  if (asCompleteObject) {
+    for (CXXRecordDecl::base_class_const_iterator
+           I = record->vbases_begin(), E = record->vbases_end(); I != E; ++I) {
+      const CXXRecordDecl *base = 
+        cast<CXXRecordDecl>(I->getType()->castAs<RecordType>()->getDecl());
+
+      // Ignore empty bases.
+      if (base->isEmpty())
+        continue;
+
+      unsigned fieldIndex = layout.getVirtualBaseIndex(base);
+
+      // We might have already laid this field out.
+      if (elements[fieldIndex]) continue;
+
+      const llvm::Type *baseType = structure->getElementType(fieldIndex);
+      elements[fieldIndex] = EmitNullConstantForBase(CGM, baseType, base);
+    }
   }
 
   // Now go through all other fields and zero them out.
-  for (unsigned i = 0; i != NumElements; ++i) {
-    if (!Elements[i])
-      Elements[i] = llvm::Constant::getNullValue(STy->getElementType(i));
+  for (unsigned i = 0; i != numElements; ++i) {
+    if (!elements[i])
+      elements[i] = llvm::Constant::getNullValue(structure->getElementType(i));
   }
   
-  return llvm::ConstantStruct::get(STy, Elements);
+  return llvm::ConstantStruct::get(structure, elements);
+}
+
+/// Emit the null constant for a base subobject.
+static llvm::Constant *EmitNullConstantForBase(CodeGenModule &CGM,
+                                               const llvm::Type *baseType,
+                                               const CXXRecordDecl *base) {
+  const CGRecordLayout &baseLayout = CGM.getTypes().getCGRecordLayout(base);
+
+  // Just zero out bases that don't have any pointer to data members.
+  if (baseLayout.isZeroInitializableAsBase())
+    return llvm::Constant::getNullValue(baseType);
+
+  // If the base type is a struct, we can just use its null constant.
+  if (isa<llvm::StructType>(baseType)) {
+    return EmitNullConstant(CGM, base, /*complete*/ false);
+  }
+
+  // Otherwise, some bases are represented as arrays of i8 if the size
+  // of the base is smaller than its corresponding LLVM type.  Figure
+  // out how many elements this base array has.
+  const llvm::ArrayType *baseArrayType = cast<llvm::ArrayType>(baseType);
+  unsigned numBaseElements = baseArrayType->getNumElements();
+
+  // Fill in null data member pointers.
+  std::vector<llvm::Constant *> baseElements(numBaseElements);
+  FillInNullDataMemberPointers(CGM, CGM.getContext().getTypeDeclType(base),
+                               baseElements, 0);
+
+  // Now go through all other elements and zero them out.
+  if (numBaseElements) {
+    const llvm::Type *i8 = llvm::Type::getInt8Ty(CGM.getLLVMContext());
+    llvm::Constant *i8_zero = llvm::Constant::getNullValue(i8);
+    for (unsigned i = 0; i != numBaseElements; ++i) {
+      if (!baseElements[i])
+        baseElements[i] = i8_zero;
+    }
+  }
+      
+  return llvm::ConstantArray::get(baseArrayType, baseElements);
 }
 
 llvm::Constant *CodeGenModule::EmitNullConstant(QualType T) {
@@ -1140,7 +1175,7 @@ llvm::Constant *CodeGenModule::EmitNullConstant(QualType T) {
 
   if (const RecordType *RT = T->getAs<RecordType>()) {
     const CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
-    return ::EmitNullConstant(*this, RD);
+    return ::EmitNullConstant(*this, RD, /*complete object*/ true);
   }
 
   assert(T->isMemberPointerType() && "Should only see member pointers here!");
diff --git a/lib/CodeGen/CGRecordLayout.h b/lib/CodeGen/CGRecordLayout.h
index 2a7aa35965..30da05f421 100644
--- a/lib/CodeGen/CGRecordLayout.h
+++ b/lib/CodeGen/CGRecordLayout.h
@@ -173,12 +173,13 @@ class CGRecordLayout {
   void operator=(const CGRecordLayout&); // DO NOT IMPLEMENT
 
 private:
-  /// The LLVM type corresponding to this record layout.
-  llvm::PATypeHolder LLVMType;
+  /// The LLVM type corresponding to this record layout; used when
+  /// laying it out as a complete object.
+  llvm::PATypeHolder CompleteObjectType;
 
-  /// The LLVM type for the non-virtual part of this record layout, used for
-  /// laying out the record as a base.
-  llvm::PATypeHolder NonVirtualBaseLLVMType;
+  /// The LLVM type for the non-virtual part of this record layout;
+  /// used when laying it out as a base subobject.
+  llvm::PATypeHolder BaseSubobjectType;
 
   /// Map from (non-bit-field) struct field to the corresponding llvm struct
   /// type field no. This info is populated by record builder.
@@ -190,26 +191,41 @@ private:
 
   // FIXME: Maybe we could use a CXXBaseSpecifier as the key and use a single
   // map for both virtual and non virtual bases.
-  llvm::DenseMap<const CXXRecordDecl *, unsigned> NonVirtualBaseFields;
+  llvm::DenseMap<const CXXRecordDecl *, unsigned> NonVirtualBases;
 
-  /// Whether one of the fields in this record layout is a pointer to data
-  /// member, or a struct that contains pointer to data member.
+  /// Map from virtual bases to their field index in the complete object.
+  llvm::DenseMap<const CXXRecordDecl *, unsigned> CompleteObjectVirtualBases;
+
+  /// False if any direct or indirect subobject of this class, when
+  /// considered as a complete object, requires a non-zero bitpattern
+  /// when zero-initialized.
   bool IsZeroInitializable : 1;
 
-public:
-  CGRecordLayout(const llvm::StructType *LLVMType,
-                 const llvm::StructType *NonVirtualBaseLLVMType,
-                 bool IsZeroInitializable)
-    : LLVMType(LLVMType), NonVirtualBaseLLVMType(NonVirtualBaseLLVMType), 
-    IsZeroInitializable(IsZeroInitializable) {}
+  /// False if any direct or indirect subobject of this class, when
+  /// considered as a base subobject, requires a non-zero bitpattern
+  /// when zero-initialized.
+  bool IsZeroInitializableAsBase : 1;
 
-  /// \brief Return the LLVM type associated with this record.
+public:
+  CGRecordLayout(const llvm::StructType *CompleteObjectType,
+                 const llvm::StructType *BaseSubobjectType,
+                 bool IsZeroInitializable,
+                 bool IsZeroInitializableAsBase)
+    : CompleteObjectType(CompleteObjectType),
+      BaseSubobjectType(BaseSubobjectType),
+      IsZeroInitializable(IsZeroInitializable),
+      IsZeroInitializableAsBase(IsZeroInitializableAsBase) {}
+
+  /// \brief Return the "complete object" LLVM type associated with
+  /// this record.
   const llvm::StructType *getLLVMType() const {
-    return cast<llvm::StructType>(LLVMType.get());
+    return cast<llvm::StructType>(CompleteObjectType.get());
   }
 
-  const llvm::StructType *getNonVirtualBaseLLVMType() const {
-    return cast<llvm::StructType>(NonVirtualBaseLLVMType.get());
+  /// \brief Return the "base subobject" LLVM type associated with
+  /// this record.
+  const llvm::StructType *getBaseSubobjectLLVMType() const {
+    return cast<llvm::StructType>(BaseSubobjectType.get());
   }
 
   /// \brief Check whether this struct can be C++ zero-initialized
@@ -218,6 +234,12 @@ public:
     return IsZeroInitializable;
   }
 
+  /// \brief Check whether this struct can be C++ zero-initialized
+  /// with a zeroinitializer when considered as a base subobject.
+  bool isZeroInitializableAsBase() const {
+    return IsZeroInitializableAsBase;
+  }
+
   /// \brief Return llvm::StructType element number that corresponds to the
   /// field FD.
   unsigned getLLVMFieldNo(const FieldDecl *FD) const {
@@ -227,8 +249,15 @@ public:
   }
 
   unsigned getNonVirtualBaseLLVMFieldNo(const CXXRecordDecl *RD) const {
-    assert(NonVirtualBaseFields.count(RD) && "Invalid non-virtual base!");
-    return NonVirtualBaseFields.lookup(RD);
+    assert(NonVirtualBases.count(RD) && "Invalid non-virtual base!");
+    return NonVirtualBases.lookup(RD);
+  }
+
+  /// \brief Return the LLVM field index corresponding to the given
+  /// virtual base.  Only valid when operating on the complete object.
+  unsigned getVirtualBaseIndex(const CXXRecordDecl *base) const {
+    assert(CompleteObjectVirtualBases.count(base) && "Invalid virtual base!");
+    return CompleteObjectVirtualBases.lookup(base);
   }
 
   /// \brief Return the BitFieldInfo that corresponds to the field FD.
@@ -236,7 +265,7 @@ public:
     assert(FD->isBitField() && "Invalid call for non bit-field decl!");
     llvm::DenseMap<const FieldDecl *, CGBitFieldInfo>::const_iterator
       it = BitFields.find(FD);
-    assert(it != BitFields.end()  && "Unable to find bitfield info");
+    assert(it != BitFields.end() && "Unable to find bitfield info");
     return it->second;
   }
 
diff --git a/lib/CodeGen/CGRecordLayoutBuilder.cpp b/lib/CodeGen/CGRecordLayoutBuilder.cpp
index aaff7fc9f7..f2864944f9 100644
--- a/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -33,9 +33,10 @@ namespace {
 class CGRecordLayoutBuilder {
 public:
   /// FieldTypes - Holds the LLVM types that the struct is created from.
+  /// 
   std::vector<const llvm::Type *> FieldTypes;
 
-  /// NonVirtualBaseFieldTypes - Holds the LLVM types for the non-virtual part
+  /// BaseSubobjectType - Holds the LLVM type for the non-virtual part
   /// of the struct. For example, consider:
   ///
   /// struct A { int i; };
@@ -47,22 +48,19 @@ public:
   ///
   /// And the LLVM type of the non-virtual base struct will be
   /// %struct.C.base = type { i32 (...)**, %struct.A, i32 }
-  std::vector<const llvm::Type *> NonVirtualBaseFieldTypes;
+  ///
+  /// This only gets initialized if the base subobject type is
+  /// different from the complete-object type.
+  const llvm::StructType *BaseSubobjectType;
 
-  /// NonVirtualBaseTypeIsSameAsCompleteType - Whether the non-virtual part of
-  /// the struct is equivalent to the complete struct.
-  bool NonVirtualBaseTypeIsSameAsCompleteType;
-  
-  /// LLVMFieldInfo - Holds a field and its corresponding LLVM field number.
-  typedef std::pair<const FieldDecl *, unsigned> LLVMFieldInfo;
-  llvm::SmallVector<LLVMFieldInfo, 16> LLVMFields;
+  /// FieldInfo - Holds a field and its corresponding LLVM field number.
+  llvm::DenseMap<const FieldDecl *, unsigned> Fields;
 
-  /// LLVMBitFieldInfo - Holds location and size information about a bit field.
-  typedef std::pair<const FieldDecl *, CGBitFieldInfo> LLVMBitFieldInfo;
-  llvm::SmallVector<LLVMBitFieldInfo, 16> LLVMBitFields;
+  /// BitFieldInfo - Holds location and size information about a bit field.
+  llvm::DenseMap<const FieldDecl *, CGBitFieldInfo> BitFields;
 
-  typedef std::pair<const CXXRecordDecl *, unsigned> LLVMBaseInfo;
-  llvm::SmallVector<LLVMBaseInfo, 16> LLVMNonVirtualBases;
+  llvm::DenseMap<const CXXRecordDecl *, unsigned> NonVirtualBases;
+  llvm::DenseMap<const CXXRecordDecl *, unsigned> VirtualBases;
 
   /// IndirectPrimaryBases - Virtual base classes, direct or indirect, that are
   /// primary base classes for some other direct or indirect base class.
@@ -75,6 +73,7 @@ public:
   /// IsZeroInitializable - Whether this struct can be C++
   /// zero-initialized with an LLVM zeroinitializer.
   bool IsZeroInitializable;
+  bool IsZeroInitializableAsBase;
 
   /// Packed - Whether the resulting LLVM struct will be packed or not.
   bool Packed;
@@ -148,6 +147,7 @@ private:
   
   /// AppendBytes - Append a given number of bytes to the record.
   void AppendBytes(uint64_t NumBytes);
+  void AppendBytes(CharUnits numBytes) { AppendBytes(numBytes.getQuantity()); }
 
   /// AppendTailPadding - Append enough tail padding so that the type will have
   /// the passed size.
@@ -162,13 +162,13 @@ private:
   /// CheckZeroInitializable - Check if the given type contains a pointer
   /// to data member.
   void CheckZeroInitializable(QualType T);
-  void CheckZeroInitializable(const CXXRecordDecl *RD);
 
 public:
   CGRecordLayoutBuilder(CodeGenTypes &Types)
-    : NonVirtualBaseTypeIsSameAsCompleteType(false), IsZeroInitializable(true),
-    Packed(false), Types(Types), Alignment(0), BitsAvailableInLastField(0),
-    NextFieldOffsetInBytes(0) { }
+    : BaseSubobjectType(0),
+      IsZeroInitializable(true), IsZeroInitializableAsBase(true),
+      Packed(false), Types(Types), Alignment(0),
+      BitsAvailableInLastField(0), NextFieldOffsetInBytes(0) { }
 
   /// Layout - Will layout a RecordDecl.
   void Layout(const RecordDecl *D);
@@ -193,9 +193,10 @@ void CGRecordLayoutBuilder::Layout(const RecordDecl *D) {
   Packed = true;
   NextFieldOffsetInBytes = 0;
   FieldTypes.clear();
-  LLVMFields.clear();
-  LLVMBitFields.clear();
-  LLVMNonVirtualBases.clear();
+  Fields.clear();
+  BitFields.clear();
+  NonVirtualBases.clear();
+  VirtualBases.clear();
 
   LayoutFields(D);
 }
@@ -339,9 +340,8 @@ void CGRecordLayoutBuilder::LayoutBitField(const FieldDecl *D,
   }
 
   // Add the bit field info.
-  LLVMBitFields.push_back(
-    LLVMBitFieldInfo(D, CGBitFieldInfo::MakeInfo(Types, D, FieldOffset,
-                                                 FieldSize)));
+  BitFields.insert(std::make_pair(D,
+                   CGBitFieldInfo::MakeInfo(Types, D, FieldOffset, FieldSize)));
 
   AppendBytes(NumBytesToAppend);
 
@@ -401,7 +401,7 @@ bool CGRecordLayoutBuilder::LayoutField(const FieldDecl *D,
   AppendPadding(FieldOffsetInBytes, TypeAlignment);
 
   // Now append the field.
-  LLVMFields.push_back(LLVMFieldInfo(D, FieldTypes.size()));
+  Fields[D] = FieldTypes.size();
   AppendField(FieldOffsetInBytes, Ty);
 
   return true;
@@ -426,14 +426,13 @@ CGRecordLayoutBuilder::LayoutUnionField(const FieldDecl *Field,
       FieldTy = llvm::ArrayType::get(FieldTy, NumBytesToAppend);
 
     // Add the bit field info.
-    LLVMBitFields.push_back(
-      LLVMBitFieldInfo(Field, CGBitFieldInfo::MakeInfo(Types, Field,
-                                                       0, FieldSize)));
+    BitFields.insert(std::make_pair(Field,
+                         CGBitFieldInfo::MakeInfo(Types, Field, 0, FieldSize)));
     return FieldTy;
   }
 
   // This is a regular union field.
-  LLVMFields.push_back(LLVMFieldInfo(Field, 0));
+  Fields[Field] = 0;
   return Types.ConvertTypeForMemRecursive(Field->getType());
 }
 
@@ -495,41 +494,51 @@ void CGRecordLayoutBuilder::LayoutUnion(const RecordDecl *D) {
     AppendPadding(RecordSize, Align);
 }
 
-void CGRecordLayoutBuilder::LayoutBase(const CXXRecordDecl *BaseDecl,
-                                       uint64_t BaseOffset) {
-  CheckZeroInitializable(BaseDecl);
+void CGRecordLayoutBuilder::LayoutBase(const CXXRecordDecl *base,
+                                       uint64_t baseOffsetInBits) {
+  uint64_t baseOffsetInBytes = baseOffsetInBits / 8;
+  AppendPadding(baseOffsetInBytes, 1);
 
-  const ASTRecordLayout &Layout = 
-    Types.getContext().getASTRecordLayout(BaseDecl);
-  
-  CharUnits NonVirtualSize = Layout.getNonVirtualSize();
-  
-  AppendPadding(BaseOffset / 8, 1);
-  
-  // FIXME: Actually use a better type than [sizeof(BaseDecl) x i8] when we can.
-  AppendBytes(NonVirtualSize.getQuantity());
+  const ASTRecordLayout &baseASTLayout
+    = Types.getContext().getASTRecordLayout(base);
+
+  // FIXME: use a better type than [sizeof(base) x i8].
+  // We could use the base layout's subobject type as the actualy
+  // subobject type in the layout if its size is the nvsize of the
+  // base, or if we'd need padding out to the enclosing object anyhow.
+  AppendBytes(baseASTLayout.getNonVirtualSize());
 }
 
-void
-CGRecordLayoutBuilder::LayoutVirtualBase(const CXXRecordDecl *BaseDecl,
-                                         uint64_t BaseOffset) {
+void CGRecordLayoutBuilder::LayoutNonVirtualBase(const CXXRecordDecl *base,
+                                                 uint64_t baseOffsetInBits) {
   // Ignore empty bases.
-  if (BaseDecl->isEmpty())
-    return;
+  if (base->isEmpty()) return;
 
-  CheckZeroInitializable(BaseDecl);
+  const CGRecordLayout &baseLayout = Types.getCGRecordLayout(base);
+  if (IsZeroInitializableAsBase) {
+    assert(IsZeroInitializable &&
+           "class zero-initializable as base but not as complete object");
 
-  const ASTRecordLayout &Layout = 
-    Types.getContext().getASTRecordLayout(BaseDecl);
+    IsZeroInitializable = IsZeroInitializableAsBase =
+      baseLayout.isZeroInitializableAsBase();
+  }
 
-  CharUnits NonVirtualSize = Layout.getNonVirtualSize();
+  LayoutBase(base, baseOffsetInBits);
+  NonVirtualBases[base] = (FieldTypes.size() - 1);
+}
 
-  AppendPadding(BaseOffset / 8, 1);
-  
-  // FIXME: Actually use a better type than [sizeof(BaseDecl) x i8] when we can.
-  AppendBytes(NonVirtualSize.getQuantity());
+void
+CGRecordLayoutBuilder::LayoutVirtualBase(const CXXRecordDecl *base,
+                                         uint64_t baseOffsetInBits) {
+  // Ignore empty bases.
+  if (base->isEmpty()) return;
 
-  // FIXME: Add the vbase field info.
+  const CGRecordLayout &baseLayout = Types.getCGRecordLayout(base);
+  if (IsZeroInitializable)
+    IsZeroInitializable = baseLayout.isZeroInitializableAsBase();
+
+  LayoutBase(base, baseOffsetInBits);
+  VirtualBases[base] = (FieldTypes.size() - 1);
 }
 
 /// LayoutVirtualBases - layout the non-virtual bases of a record decl.
@@ -561,18 +570,6 @@ CGRecordLayoutBuilder::LayoutVirtualBases(const CXXRecordDecl *RD,
   }
 }
 
-void CGRecordLayoutBuilder::LayoutNonVirtualBase(const CXXRecordDecl *BaseDecl,
-                                                 uint64_t BaseOffset) {
-  // Ignore empty bases.
-  if (BaseDecl->isEmpty())
-    return;
-
-  LayoutBase(BaseDecl, BaseOffset);
-
-  // Append the base field.
-  LLVMNonVirtualBases.push_back(LLVMBaseInfo(BaseDecl, FieldTypes.size() - 1));
-}
-
 void
 CGRecordLayoutBuilder::LayoutNonVirtualBases(const CXXRecordDecl *RD,
                                              const ASTRecordLayout &Layout) {
@@ -618,37 +615,40 @@ bool
 CGRecordLayoutBuilder::ComputeNonVirtualBaseType(const CXXRecordDecl *RD) {
   const ASTRecordLayout &Layout = Types.getContext().getASTRecordLayout(RD);
 
-
   CharUnits NonVirtualSize  = Layout.getNonVirtualSize();
   CharUnits NonVirtualAlign = Layout.getNonVirtualAlign();
   uint64_t AlignedNonVirtualTypeSize =
     NonVirtualSize.RoundUpToAlignment(NonVirtualAlign).getQuantity();
   
-  
   // First check if we can use the same fields as for the complete class.
   uint64_t RecordSize = Layout.getSize().getQuantity();
-  if (AlignedNonVirtualTypeSize == RecordSize) {
-    NonVirtualBaseTypeIsSameAsCompleteType = true;
+  if (AlignedNonVirtualTypeSize == RecordSize)
     return true;
-  }
 
   // Check if we need padding.
   uint64_t AlignedNextFieldOffset =
     llvm::RoundUpToAlignment(NextFieldOffsetInBytes, 
                              getAlignmentAsLLVMStruct());
 
-  if (AlignedNextFieldOffset > AlignedNonVirtualTypeSize)
+  if (AlignedNextFieldOffset > AlignedNonVirtualTypeSize) {
+    assert(!Packed && "cannot layout even as packed struct");
     return false; // Needs packing.
+  }
+
+  bool needsPadding = (AlignedNonVirtualTypeSize != AlignedNextFieldOffset);
+  if (needsPadding) {
+    uint64_t NumBytes = AlignedNonVirtualTypeSize - AlignedNextFieldOffset;
+    FieldTypes.push_back(getByteArrayType(NumBytes));
+  }
 
-  NonVirtualBaseFieldTypes = FieldTypes;
+  BaseSubobjectType = llvm::StructType::get(Types.getLLVMContext(),
+                                            FieldTypes, Packed);
 
-  if (AlignedNonVirtualTypeSize == AlignedNextFieldOffset) {
-    // We don't need any padding.
-    return true;
+  if (needsPadding) {
+    // Pull the padding back off.
+    FieldTypes.pop_back();
   }
 
-  uint64_t NumBytes = AlignedNonVirtualTypeSize - AlignedNextFieldOffset;
-  NonVirtualBaseFieldTypes.push_back(getByteArrayType(NumBytes));
   return true;
 }
 
@@ -777,36 +777,29 @@ unsigned CGRecordLayoutBuilder::getAlignmentAsLLVMStruct() const {
   return MaxAlignment;
 }
 
+/// Merge in whether a field of the given type is zero-initializable.
 void CGRecordLayoutBuilder::CheckZeroInitializable(QualType T) {
   // This record already contains a member pointer.
-  if (!IsZeroInitializable)
+  if (!IsZeroInitializableAsBase)
     return;
 
   // Can only have member pointers if we're compiling C++.
   if (!Types.getContext().getLangOptions().CPlusPlus)
     return;
 
-  T = Types.getContext().getBaseElementType(T);
+  const Type *elementType = T->getBaseElementTypeUnsafe();
 
-  if (const MemberPointerType *MPT = T->getAs<MemberPointerType>()) {
+  if (const MemberPointerType *MPT = elementType->getAs<MemberPointerType>()) {
     if (!Types.getCXXABI().isZeroInitializable(MPT))
-      IsZeroInitializable = false;
-  } else if (const RecordType *RT = T->getAs<RecordType>()) {
+      IsZeroInitializable = IsZeroInitializableAsBase = false;
+  } else if (const RecordType *RT = elementType->getAs<RecordType>()) {
     const CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
-    CheckZeroInitializable(RD);
+    const CGRecordLayout &Layout = Types.getCGRecordLayout(RD);
+    if (!Layout.isZeroInitializable())
+      IsZeroInitializable = IsZeroInitializableAsBase = false;
   }
 }
 
-void CGRecordLayoutBuilder::CheckZeroInitializable(const CXXRecordDecl *RD) {
-  // This record already contains a member pointer.
-  if (!IsZeroInitializable)
-    return;
-
-  const CGRecordLayout &Layout = Types.getCGRecordLayout(RD);
-  if (!Layout.isZeroInitializable())
-    IsZeroInitializable = false;
-}
-
 CGRecordLayout *CodeGenTypes::ComputeRecordLayout(const RecordDecl *D) {
   CGRecordLayoutBuilder Builder(*this);
 
@@ -816,30 +809,25 @@ CGRecordLayout *CodeGenTypes::ComputeRecordLayout(const RecordDecl *D) {
                                                      Builder.FieldTypes,
                                                      Builder.Packed);
 
+  // If we're in C++, compute the base subobject type.
   const llvm::StructType *BaseTy = 0;
   if (isa<CXXRecordDecl>(D)) {
-    if (Builder.NonVirtualBaseTypeIsSameAsCompleteType)
-      BaseTy = Ty;
-    else if (!Builder.NonVirtualBaseFieldTypes.empty())
-      BaseTy = llvm::StructType::get(getLLVMContext(), 
-                                     Builder.NonVirtualBaseFieldTypes, 
-                                     Builder.Packed);
+    BaseTy = Builder.BaseSubobjectType;
+    if (!BaseTy) BaseTy = Ty;
   }
 
   CGRecordLayout *RL =
-    new CGRecordLayout(Ty, BaseTy, Builder.IsZeroInitializable);
+    new CGRecordLayout(Ty, BaseTy, Builder.IsZeroInitializable,
+                       Builder.IsZeroInitializableAsBase);
 
-  // Add all the non-virtual base field numbers.
-  RL->NonVirtualBaseFields.insert(Builder.LLVMNonVirtualBases.begin(),
-                                  Builder.LLVMNonVirtualBases.end());
+  RL->NonVirtualBases.swap(Builder.NonVirtualBases);
+  RL->CompleteObjectVirtualBases.swap(Builder.VirtualBases);
 
   // Add all the field numbers.
-  RL->FieldInfo.insert(Builder.LLVMFields.begin(),
-                       Builder.LLVMFields.end());
+  RL->FieldInfo.swap(Builder.Fields);
 
   // Add bitfield info.
-  RL->BitFields.insert(Builder.LLVMBitFields.begin(),
-                       Builder.LLVMBitFields.end());
+  RL->BitFields.swap(Builder.BitFields);
 
   // Dump the layout, if requested.
   if (getContext().getLangOptions().DumpRecordLayouts) {
@@ -913,9 +901,9 @@ CGRecordLayout *CodeGenTypes::ComputeRecordLayout(const RecordDecl *D) {
 
 void CGRecordLayout::print(llvm::raw_ostream &OS) const {
   OS << "<CGRecordLayout\n";
-  OS << "  LLVMType:" << *LLVMType << "\n";
-  if (NonVirtualBaseLLVMType)
-    OS << "  NonVirtualBaseLLVMType:" << *NonVirtualBaseLLVMType << "\n"; 
+  OS << "  LLVMType:" << *CompleteObjectType << "\n";
+  if (BaseSubobjectType)
+    OS << "  NonVirtualBaseLLVMType:" << *BaseSubobjectType << "\n"; 
   OS << "  IsZeroInitializable:" << IsZeroInitializable << "\n";
   OS << "  BitFields:[\n";
 
diff --git a/test/CodeGenCXX/pointers-to-data-members.cpp b/test/CodeGenCXX/pointers-to-data-members.cpp
index 6acccbaab9..40723a856c 100644
--- a/test/CodeGenCXX/pointers-to-data-members.cpp
+++ b/test/CodeGenCXX/pointers-to-data-members.cpp
@@ -1,37 +1,41 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin10 | FileCheck %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-apple-darwin10 -O3 | FileCheck --check-prefix=CHECK-O3 %s
+// RUN: %clang_cc1 %s -emit-llvm -o %t.ll -triple=x86_64-apple-darwin10
+// RUN: FileCheck %s < %t.ll
+// RUN: FileCheck -check-prefix=CHECK-GLOBAL %s < %t.ll
+// RUN: %clang_cc1 %s -emit-llvm -o %t-opt.ll -triple=x86_64-apple-darwin10 -O3
+// RUN: FileCheck --check-prefix=CHECK-O3 %s < %t-opt.ll
+
 struct A { int a; int b; };
 struct B { int b; };
 struct C : B, A { };
 
 // Zero init.
 namespace ZeroInit {
-  // CHECK: @_ZN8ZeroInit1aE = global i64 -1
+  // CHECK-GLOBAL: @_ZN8ZeroInit1aE = global i64 -1
   int A::* a;
   
-  // CHECK: @_ZN8ZeroInit2aaE = global [2 x i64] [i64 -1, i64 -1]
+  // CHECK-GLOBAL: @_ZN8ZeroInit2aaE = global [2 x i64] [i64 -1, i64 -1]
   int A::* aa[2];
   
-  // CHECK: @_ZN8ZeroInit3aaaE = global [2 x [2 x i64]] {{\[}}[2 x i64] [i64 -1, i64 -1], [2 x i64] [i64 -1, i64 -1]]
+  // CHECK-GLOBAL: @_ZN8ZeroInit3aaaE = global [2 x [2 x i64]] {{\[}}[2 x i64] [i64 -1, i64 -1], [2 x i64] [i64 -1, i64 -1]]
   int A::* aaa[2][2];
   
-  // CHECK: @_ZN8ZeroInit1bE = global i64 -1,
+  // CHECK-GLOBAL: @_ZN8ZeroInit1bE = global i64 -1,
   int A::* b = 0;
 
-  // CHECK: @_ZN8ZeroInit2saE = internal global %struct.anon { i64 -1 }
+  // CHECK-GLOBAL: @_ZN8ZeroInit2saE = internal global %struct.anon { i64 -1 }
   struct {
     int A::*a;
   } sa;
   void test_sa() { (void) sa; } // force emission
   
-  // CHECK: @_ZN8ZeroInit3ssaE = internal
-  // CHECK: [2 x i64] [i64 -1, i64 -1]
+  // CHECK-GLOBAL: @_ZN8ZeroInit3ssaE = internal
+  // CHECK-GLOBAL: [2 x i64] [i64 -1, i64 -1]
   struct {
     int A::*aa[2];
   } ssa[2];
   void test_ssa() { (void) ssa; }
   
-  // CHECK: @_ZN8ZeroInit2ssE = internal global %1 { %struct.anon { i64 -1 } }
+  // CHECK-GLOBAL: @_ZN8ZeroInit2ssE = internal global %1 { %struct.anon { i64 -1 } }
   struct {
     struct {
       int A::*pa;
@@ -51,13 +55,13 @@ namespace ZeroInit {
   };
 
   struct C : A, B { int j; };
-  // CHECK: @_ZN8ZeroInit1cE = global %"struct.ZeroInit::C" { [16 x i8] c"\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00", [176 x i8] c"\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF", i32 0, [4 x i8] zeroinitializer }
+  // CHECK-GLOBAL: @_ZN8ZeroInit1cE = global {{%.*}} { [16 x i8] c"\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00", [176 x i8] c"\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF", i32 0, [4 x i8] zeroinitializer }
   C c;
 }
 
 // PR5674
 namespace PR5674 {
-  // CHECK: @_ZN6PR56742pbE = global i64 4
+  // CHECK-GLOBAL: @_ZN6PR56742pbE = global i64 4
   int A::*pb = &A::b;
 }
 
@@ -168,15 +172,15 @@ struct A {
   int A::*i;
 };
 
-// FIXME: A::i should be initialized to -1 here.
+// CHECK-GLOBAL: @_ZN12VirtualBases1bE = global {{%.*}} { i32 (...)** null, [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF" }
 struct B : virtual A { };
 B b;
 
-// FIXME: A::i should be initialized to -1 here.
+// CHECK-GLOBAL: @_ZN12VirtualBases1cE = global {{%.*}} { i32 (...)** null, i64 -1, [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF" }
 struct C : virtual A { int A::*i; };
 C c;
 
-// FIXME: C::A::i should be initialized to -1 here.
+  // CHECK-GLOBAL: @_ZN12VirtualBases1dE = global {{%.*}} { [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF", i64 -1, [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF" }
 struct D : C { int A::*i; };
 D d;
 
@@ -217,3 +221,12 @@ void f(S* p, double S::*pm) {
 
 }
 
+namespace test4 {
+  struct A             { int A_i; };
+  struct B : virtual A { int A::*B_p; };
+  struct C : virtual B { int    *C_p; };
+  struct D :         C { int    *D_p; };
+
+  // CHECK-GLOBAL: @_ZN5test41dE = global {{%.*}} { [16 x i8] zeroinitializer, i32* null, [16 x i8] c"\00\00\00\00\00\00\00\00\FF\FF\FF\FF\FF\FF\FF\FF", [4 x i8] zeroinitializer }
+  D d;
+}