[cfe-dev] CGRecordLayoutBuilder for MS ABI

[John McCall]

On Nov 4, 2011, at 9:45 AM, r4start wrote:
> Sorry, at first time i didn't include MSLayoutVirtBases for codegen.

-    if (Base->isDynamicClass()) {
+    // In the MS ABI, we need a vfptr if this class defines new virtual
+    // functions but has no primary base.  The AST doesn't tell us
+    // directly that a class has new virtual functions, and checking for it
+    // is a bit expensive, but we can avoid the full check in some important
+    // cases:  non-polymorphic classes have no virtual functions by
+    // definition, whereas polymorphic classes must either declare new
+    // virtual functions or inherit them from some base, and if that base
+    // were non-virtual then there would be a primary base.
+    if ((Base->isDynamicClass() &&
+        Context.getTargetInfo().getCXXABI() != CXXABI_Microsoft) ||
+        (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft &&
+         BaseHasVFPtr(Base))) {

I actually meant this comment to go right before this line:

+  } else if (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft &&
+             !PrimaryBase && RD->isPolymorphic() && 
+             (RD->getNumVBases() == 0 || HasNewVirtualFunction(RD))) {
+
+    CharUnits PtrWidth = 
+       Context.toCharUnitsFromBits(Context.getTargetInfo().getPointerWidth(0));
+
+    VFPtrOffset = getSize();
+    setSize(getSize() + PtrWidth);
+    setDataSize(getSize());
   }

Also, this condition:

+    if ((Base->isDynamicClass() &&
+        Context.getTargetInfo().getCXXABI() != CXXABI_Microsoft) ||
+        (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft &&
+         BaseHasVFPtr(Base))) {

You've actually lost the isPolymorphic() test.  I suggest pulling this out as
a separate function:
  bool isPossiblePrimaryBase(const CXXRecordDecl *Base) {
    if (Context.getTargetInfo().getCXXABI() != CXXABI_Microsoft)
      return Base->isDynamicClass();
    return Base->isPolymorphic() && BaseHasVFPtr(Base);
  }

Also, I know this isn't new to your patch, but BaseHasVFPtr can be
significantly optimized.  Here's a rewrite. :)

/// BaseHasVFPtr - Returns true if the given class has a vfptr we can
/// re-use when laid out as a base-class subobject.
bool 
RecordLayoutBuilder::BaseHasVFPtr(const CXXRecordDecl *Base) const {
  assert(Base->isPolymorphic() && "asking about a non-polymorphic class?");

  // Polymorphic classes without virtual bases must have a vfptr we can use.
  if (!Base->getNumVBases()) return true;

  // In the MS ABI, primary bases always have vfptrs by definition.
  if (Context.getASTRecordLayout(Base).getPrimaryBase()) return true;

  // Okay, none of its non-virtual base classes has a vfptr.  The class itself
  // only gets a vfptr if it defines a new virtual function.
  return HasNewVirtualFunction(Base);
}

+    // In MS ABI we must reuse vbtable from base class.
+    const ASTRecordLayout &BaseLayout = Context.getASTRecordLayout(BaseDecl);
+    if (BaseLayout.getVBPtrOffset() != CharUnits::fromQuantity(-1))
+      UseBaseVBTable = true;

This doesn't handle the case of a base class with indirect virtual bases, e.g.:
  struct A { int a; };
  struct B : virtual A { int b; };
  struct C : B { int c; }; // C's layout has no VBPtrOffset, but it does have a vbptr from B
  struct D : C { int d; };
So I think we should use this condition, which is incidentally much faster:
  if (BaseDecl->getNumVBases())

 bool 
 RecordLayoutBuilder::HasNewVirtualFunction(const CXXRecordDecl *RD) const {
+  if (!RD->getNumBases() && RD->isPolymorphic())
+    return true;

I would just assert on isPolymorphic() here, but if you think it's
convenient to call this on non-polymorphic classes, I would suggest:
  if (!RD->isPolymorphic()) return false;
  if (!RD->getNumBases()) return true;

Over to the CG world:

+    CharUnits VBPtrOffset = Layout.getVBPtrOffset();
+    if (NextFieldOffset < VBPtrOffset) {
+      AppendBytes(VBPtrOffset - NextFieldOffset);
+    }
+    llvm::Type *Vbptr = llvm::Type::getInt32PtrTy(Types.getLLVMContext());
+    AppendField(VBPtrOffset, Vbptr);

This can just be:
    llvm::Type *Vbptr = llvm::Type::getInt32PtrTy(Types.getLLVMContext());
    AppendPadding(VBPtrOffset, getTypeAlignment(Vbptr));
    AppendField(VBPtrOffset, Vbptr);

+/// MSLayoutVirtualBases - layout the virtual bases of a record decl, like MSVC.
+void
+CGRecordLayoutBuilder::MSLayoutVirtualBases(const CXXRecordDecl *RD,
+                                          const ASTRecordLayout &Layout) {
+  if (!RD->getNumVBases())
+    return;
+
+  for (CXXRecordDecl::base_class_const_iterator I = RD->vbases_begin(),
+        E = RD->vbases_end(); I != E; ++I) {
+
+    const CXXRecordDecl *BaseDecl = 
+      cast<CXXRecordDecl>(I->getType()->getAs<RecordType>()->getDecl());
+
+    CharUnits vbaseOffset = Layout.getVBaseClassOffset(BaseDecl);
+    LayoutVirtualBase(BaseDecl, vbaseOffset);
+  }
+}

Please put a comment in here like:
  // The vbases list is ordered using a depth-first traversal, which is precisely
  // what we need.

Otherwise looks good!

John.