[cfe-dev] CGRecordLayoutBuilder for MS ABI

[John McCall]

On Oct 27, 2011, at 4:27 AM, r4start wrote:
> Okay, I've added comments.

+  if (Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
+    // MSVC can add padding before virtual bases.
+    // If we don`t have vbases then it will be tail padding.
+    CharUnits NumPadBytes = Layout.getNonVirtualSize() - NextFieldOffset;
+    AppendBytes(NumPadBytes);
+  }

I don't see why this needs to happen right here.  The Itanium ABI
can also add padding before virtual bases, but that just naturally
happens as part of laying them out;  and the tail padding case
should just work correctly if you weren't hacking
ComputeNonVirtualBaseType.

+  if (Types.getContext().getTargetInfo().getCXXABI() != CXXABI_Microsoft) {
+    // Append tail padding if necessary.
+    // In MS ABI it not necessary, because
+    // if we don`t have vbases then we have already add padding.
+    // And MSVC doesn`t add padding after vbases.
+    AppendTailPadding(Layout.getSize());
+  }

Not adding padding is not good enough, because LLVM's struct
layout for non-packed struct types uses the C layout algorithm, which
means that structs get automatically tail-padded to make their size
a multiple of their (IR) alignment.  If we're laying out a class where the
required size is not a multiple of the IR alignment (as determined by
getAlignmentAsLLVMStruct), we need to implicitly go back and build
a packed struct layout, which you can do here just by returning false.

+  } else if (
+    Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
+    // In MS mode this code helps insert padding bytes before fields,
+    // like MSVC does.
+    CharUnits padding = fieldOffset - NextFieldOffset;
+    
+    AppendBytes(padding);

This still shouldn't be necessary.  This routine just adds padding bytes
to get us up to fieldOffset, which is determined by the ASTRecordLayout.
If the alignment of the next IR type is high enough, we don't need explicit
padding bytes, which is what the 'if' condition above this is testing.  This
isn't any different in the MS ABI.

That said, it does look like there's a bug here, because we *always* need
to add padding bytes if we're building a packed layout, and that can
happen in MSVC;  see above.  So I think the condition should be:

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

There may be a similar thing required in AddTailPadding.

John.