[cfe-dev] CGRecordLayoutBuilder for MS ABI

[John McCall]

On Oct 26, 2011, at 2:09 AM, r4start wrote:
> New patch with minor changes.

+    if (HasNewVirtualFunction(RD)) {
+      CharUnits PtrWidth = 
+       Context.toCharUnitsFromBits(Context.getTargetInfo().getPointerWidth(0));
+
+      VFPtrOffset = getSize();
+      setSize(getSize() + PtrWidth);
+      setDataSize(getSize());
+
+      if (!HasNonVirtBases) {
+        setSize(getSize().RoundUpToAlignment(Alignment));
+        setDataSize(getSize());
+      }
+    }

'Alignment' isn't really meaningful here yet — we haven't laid out
any of the subobjects, so it's just what we've gotten from
attributes, etc.  Also, I believe the alignment you want is the
alignment just from the fields;  my working theory is that MSVC
basically lays out the struct like this:
  struct Derived {
    void *_vfptr; // only if no primary bases

    // Non-virtual bases, starting with primary base.
    struct NVBase1 _NVBase1;
    ...
    struct NVBaseN _NVBaseN;

    struct DerivedFields _Fields;
  };
That's inevitably going to require a custom computation.

Anyway, why do this in DeterminePrimaryBase?  Do it in
LayoutNonVirtualBases at the same place where we add a
vtable in the Itanium ABI.  It's even pretty much the same code,
except that we need to add the crazy extra alignment (which
there should be a comment explaining).

Also, HasNewVirtualFunction is pretty expensive, and you
only actually need to call it here if you have any virtual bases.
I think you can make the full condition:

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

The other changes here look okay, though.

On to CodeGen.

+  if (Types.getContext().getTargetInfo().getCXXABI() != CXXABI_Microsoft) {
+    AppendPadding(baseOffset, CharUnits::One());
+  } else {
+    AppendPadding(baseOffset, Alignment);
+  }

The FieldAlignment argument to AppendPadding is the natural
ABI alignment of the IR type which is about to be appended;  the
idea is to avoid adding useless "fields" when LLVM IR is clever
enough to round up for us already.  I don't think that's what you
want here at all.

-    // And lay out the virtual bases.
-    RD->getIndirectPrimaryBases(IndirectPrimaryBases);
-    if (Layout.isPrimaryBaseVirtual())
-      IndirectPrimaryBases.insert(Layout.getPrimaryBase());
+    if (Types.getContext().getTargetInfo().getCXXABI() != CXXABI_Microsoft) {
+      // And lay out the virtual bases.
+      RD->getIndirectPrimaryBases(IndirectPrimaryBases);
+      if (Layout.isPrimaryBaseVirtual())
+        IndirectPrimaryBases.insert(Layout.getPrimaryBase());
+    }

Please leave at least a cursory comment explaining why this
is unnecessary in the MS ABI (i.e. because there are no
virtual primary bases).

+  if (Types.getContext().getTargetInfo().getCXXABI() != CXXABI_Microsoft) {
+    needsPadding = (AlignedNonVirtualTypeSize != AlignedNextFieldOffset);
+    if (needsPadding) {
+      CharUnits NumBytes = AlignedNonVirtualTypeSize - AlignedNextFieldOffset;
+      FieldTypes.push_back(getByteArrayType(NumBytes));
+    }

I don't understand why the tail-padding logic needs to be
completely different in the MS ABI — if nothing else, it means
you get unnecessary padding fields.  AFAICT, the only ABI
difference with tail padding is that we have to deal with vbases
sometimes getting bizarro alignments, forcing us to use
packed struct types.

John.