[cfe-dev] CGRecordLayoutBuilder for MS ABI

[John McCall]

On Oct 30, 2011, at 11:56 PM, r4start wrote:
> Please review new patch.
> I added the generation of padding bytes between vfptr & vbptr.

+  } else if (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft &&
+             !PrimaryBase && RD->isPolymorphic() && 
+             (RD->getNumVBases() == 0 || HasNewVirtualFunction(RD))) {
+

Please add the following comment:
  // 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 (HasNewVirtualFunction(RD) &&
-        (!PrimaryBase || !BaseHasVFPtr(PrimaryBase))) {
-      EnsureVTablePointerAlignment(PtrAlign);
-      VFPtrOffset = getSize();
-      setSize(getSize() + PtrWidth);
-      setDataSize(getSize());
-    }
-    if (RD->getNumVBases() &&
+    if (HasDirectVirtualBases &&
         (!PrimaryBase || !PrimaryBase->getNumVBases())) {

I'm sorry if I've asked this before, but have you verified that this is the
correct condition?  Specifically, does the ABI not re-use vbptrs from
non-primary non-virtual bases?

For example, consider this hierarchy:
  class A { int a; };
  class B { int b; virtual void foo(); };
  class C : public virtual A { int c; };
  class D { int d; };
  class E : public B, public virtual D { int e; };

Does class E ultimately have two vbptrs or one?

+    } else if (Layout.getVFPtrOffset() != CharUnits::fromQuantity(-1)) {
+        llvm::Type *FunctionType =
+          llvm::FunctionType::get(llvm::Type::getInt32Ty(Types.getLLVMContext()),
+          /*isVarArg=*/true);
+        llvm::Type *VTableTy = FunctionType->getPointerTo();
+
+        assert(NextFieldOffset.isZero() &&
+          "VTable pointer must come first!");
+        AppendField(Layout.getVFPtrOffset(), VTableTy->getPointerTo());

Indentation.

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

I'm still not sure why this is necessary.  AppendField should already
be adding padding to bring NextFieldOffset up to VBPtrOffset.  The
only time it doesn't add that padding explicitly is when the ABI
alignment of the field type being added would implicitly introduce it.

+  if (alignedNextFieldOffset < fieldOffset ||
+      (!Packed && (fieldOffset != NextFieldOffset))) {

As I covered in my last review, this condition should be Packed,
not !Packed.  It's only when we're adding a packed structure that
we need to unconditionally insert padding.

And now I'm concerned, because you clearly didn't run the test
suite on anything but your new testcase before asking me to
review this patch.

John.