[cfe-dev] CGRecordLayoutBuilder for MS ABI

[r4start]

On 27/10/2011 23:31, John McCall wrote:
> 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);
> +  }
Ok, without that we just won't have padding bytes at the end of 
struct/classes.
I mean that we simply won't see they in layout info.
> 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());
> +  }
I do some tests and you are right. I removed previous code and this "if".
All tests completed well.
> 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)) {
This condition doesn't work for class I from test.
We have such layout
  struct I
   0 |   (I vftable pointer)
   8 |   (I vbtable pointer)
16 |   double q
24 |   class D (virtual base)
24 |     (D vftable pointer)
32 |     double a
and layout must be %struct.I = type { i32 (...)**, i32*, [4 x i8], 
double, %class.D }
But we will have %struct.I = type { i32 (...)**, i32*, double, %class.D 
} and Packed = false.
May be you mean (!Packed && (fieldOffset != NextFieldOffset))) ?
With this condition all works fine. Also, I compiled with Itanium ABI 
and everything seems ok.

  - Dmitry.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ms-codegen.patch
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20111028/aca8072c/attachment.ksh>