[cfe-dev] CGRecordLayoutBuilder for MS ABI

[John McCall]

On Oct 28, 2011, at 1:13 AM, r4start wrote:
> On 27/10/2011 23:31, John McCall wrote:
>> +  } 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.

Neither of these types is correct;  both incorrectly place the
vbtbl at offset 4.  It's just that the first type happens to create
some extra padding in the wrong place, and that makes the
total struct size right.

The type should be this:
  type { i32 (...)**, [4 x i8], i32*, double, %class.D }

If you place the vbtbl at the right offset, you should find that
everything else falls out correctly.

John.