[PATCH] Microsoft C++ Record Layout

[Warren Hunt]

>> +    /// HasOwnVBPtr - Does this class provide a virtual function table
>> +    /// (vtable in Itanium, VBtbl in Microsoft) that is independent from
>> +    /// its base classes?
>> +    bool HasOwnVBPtr : 1;
>> +
>>
>> You’ve copied the comment here from HasOwnVFPtr.

fixed.

>>
>> +// significantly different than those produced by the Itanium ABI.
 Here We note
>> +// the most important differences.
>>
>> Capitalization.

fixed.

>>
>> +// * The alignment of bitfields in unions is ignored when computing the
>> +//   alignment of the union
>>
>> Period.

fixed.

>>
>> +// * The Itanium equivelant
>>
>> equivalent

fixed.

>>
>> +//   function pointer) and a vbptr (virtual base pointer).  They can
each be
>> +//   shared with a non-virtual base and each can be shared with a
distinct
>> +//   non-virtual base.
>>
>> I’d just say “, possibly different ones.”

fixed.

>>
>> +// * Virtual bases sometimes require a 'vtordisp' field that is layed
out before
>>
>> laid

fixed.

>>
>> +// * vtordisps are allocated in a block of memory with size and
alignment equal
>> +//   to the alignment of the completed structure (before applying
__declspec(
>> +//   align())).  They always occur at the end of the allocation.
>>
>> Er.  Unless our existing tests are wildly wrong, vtordisps always occur
>> before a virtual base.

I adjusted to comment to be more clear.  If a virtual base has a vtordisp,
the
compiler allocates an nvalign sized block of memory before that virtual
base and
places the vtordisp at the end of the block (immediately prior to the
virtual base).

>>
>> +// * vbptrs are allocated in a block of memory equal to the alignment
of the
>> +//   fields and non-virtual bases at a potentially unaligned offset.
>>
>> This doesn’t seem to be true; the vbptr itself always seems to be
aligned.

Clarified comment a bit.  The block of memory allocated for the can be
vbptr can
be arbitrairly aligned.  The vbptr itself is always properly aligned.  If
the block
is unaligned, then the block may be enlargened to enable proper alignment
of the
vbptr.  If this occurs, very bizarre placement rules go into effect for the
first
elements after the vbptr.

>>
>> +// * The last zero sized non-virtual base is allocated after the
placement of
>> +//   vbprt
>>
>> the vbptr

fixed.

>>
>> +  void Layout(const RecordDecl *RD);
>>
>> Since this is all new code, please lowercase method names in accordance
>> with LLVM style conventions.

done.

>>
>> +  // Look at all of our methods to determine if we need a VFPtr.  We
need a
>> +  // vfptr if we define a new virtual function.
>> +  if (!HasVFPtr && RD->isDynamicClass())
>> +    for (clang::CXXRecordDecl::method_iterator i = RD->method_begin(),
>> +                                               e = RD->method_end();
>> +         !HasVFPtr && i != e; ++i)
>> +      HasVFPtr = i->isVirtual() && i->size_overridden_methods() == 0;
>> +  if (!HasVFPtr)
>> +    return;
>>
>> I believe we also need a new vf-table if we have a non-trivial covariant
>> override of a virtual function.

No, MS-ABI adds a vf-table entry to the virtual bases vf-table instead of
creating a new vf-table.  This results in some collisions if multiple
derive classes try to override the same function in virtual base, see:
http://llvm-reviews.chandlerc.com/D1076

>>
>> +    UpdateAlignment(Layout.getAlignment());
>>
>> If this really isn’t affected by whether the class is packed, that’s
worth
>> a comment.

comment added.

>>
>> +  // Use LayoutFields to compute the alignment of the fields.  The
layout
>> +  // is discarded.  This is the simplest way to get all of the bit-field
>> +  // behavior correct and is not actually very expensive.
>> +  LayoutFields(RD);
>> +  Size = CharUnits::Zero();
>> +  FieldOffsets.clear();
>>
>> I feel like FieldOffsets and LastFieldIsNonZeroWidthBitfield should be
reset
>> in the same place, probably here.

I can do that, LastFieldIsNonZeroWidthBitfield is logically associated with
the LayoutFields algorithm. (It's never read or written outside of that
function
or its subroutines).  Therefore I set it at the beginning of LayoutFields
instead
of somewhere else.  Given that it's implemented using a member, I can clear
it
anywhere and could do it here.

>>
>> +  // MSVC potentially over-aligns the vb-table pointer by giving it
>> +  // the max alignment of all the non-virtual objects in the class.
>> +  // This is completely unnecessary, but we're not here to pass
>> +  // judgment.
>>
>> Er, what follows is all the nvdata, not just the vbptr.  What you’re
saying
>> is that MSVC over-aligns following a new vfptr, basically as if the
layout
>> used struct layout rules for this type:
>>   { vfptr, { nvdata } }

yes.  I added a little bit more to the comment.

>>
>> +    // Empty bases only consume space when followed by another empty
base.
>> +    if (RD &&
Context.getASTRecordLayout(RD).getNonVirtualSize().isZero())
>>
>> Please grab this layout once at the top of the method.

RD can be null in this method, I grab it only after checking that RD is not
null
on each branch.  As long as the handle layout is a reference I have to do
it the
current way.  I went ahead and did this.

>>
>> +  CharUnits LazyEmptyBaseOffset = CharUnits::Zero();
>> +
>>
>> Dead variable?

removed.

>>
>> +      // Handle strange padding rules.  I have no explenation for why
the
>> +      // virtual base is padded in such an odd way.  My guess is that
they
>> +      // allways Add 2 * Alignment and incorrectly round down to the
appropriate
>>
>> explanation
>> always

There is an explanation! Oh, that's two spelling edits. fixed.

>>
>> +      CharUnits x = Size + Alignment + Alignment;
>>
>> This variable can be sunk to where it’s used.

nope, Size changes on the next line.

>>
>> +      if (!RD->field_empty())
>> +        Size += CharUnits::fromQuantity(
>> +            x % getAdjustedFieldInfo(*RD->field_begin()).second);
>>
>> This is a really bizarre calculation, but if it’s what they do...

I agree!!  All I know is that it works for all my test cases (And I have
over a dozen tests to cover this case.)

>>
>> field_begin() and field_empty() do some non-trivial redundant
>> work; please re-use the result of field_begin().

done.  (However I think the using both begin and empty is defensible because
this code is so far off the critical path it's unlikely to even be called
once
in most cases and I think it's slightly clearer the current way.)

>>
>> +  if (FD->isBitField()) {
>> +    LayoutBitField(FD);
>> +    return;
>> +  }
>> +
>> +  std::pair<CharUnits, CharUnits> FieldInfo = getAdjustedFieldInfo(FD);
>> +  CharUnits FieldSize = FieldInfo.first;
>> +  CharUnits FieldAlign = FieldInfo.second;
>> +
>> +  LastFieldIsNonZeroWidthBitfield = false;
>>
>> I feel that the invariants would be clearer if you put this line
immediately
>> after the isBitField() block.

done.

>>
>> +  // Clamp the bitfield to a containable size for the sake of being able
>> +  // to lay them out.  Sema will error later.
>>
>> I’d expect Sema to have already errored, actually.

You may be correct, I am pretty new to this project and am not certain
about the
order in which these things run, I changed the comment to "Sema will throw
an error."

>>
>> +  if (!IsUnion && LastFieldIsNonZeroWidthBitfield &&
>> +      CurrentBitfieldSize == FieldSize && Width <=
RemainingBitsInField) {
>> +    PlaceFieldAtBitOffset(Context.toBits(Size) - RemainingBitsInField);
>> +    RemainingBitsInField -= Width;
>> +    return;
>>
>> Most of these checks are obvious, but please add a comment pointing out
>> the unusual thing here: that MSVC refuses to pack bit-fields together if
their
>> formal type has a different size.

Added more detailed comment.

>>
>> +    // TODO (whunt): Add a Sema warning that MS ignores bitfield
alignment
>> +    // in unions.
>>
>> We kindof frown on personalized TODOs.

Okay, it was normal in Chrome.  I removed the (whunt).

>>
>> +    if (getTargetInfo().getTriple().getArch() == llvm::Triple::x86 &&
>> +        getTargetInfo().getTriple().getOS() == llvm::Triple::Win32 &&
>> +        !D->isMsStruct(*this)) {
>> +      NewEntry = BuildMicrosoftASTRecordLayout(D);
>> +    } else {
>>
>> This should be checking the target C++ ABI, not the target triple
directly.

changed, this should be discussed in detail.

>>
>> Also, I don’t understand why it’s *filtering out* things with ms_struct?

used during debugging, fixed.

>>
>> Also, you’ve left the rest of this block mis-indented.

fixed.

>>
>> You should really just do this at the top level:
>>   if (getCXXABI().isMicrosoft()) {
>>     NewEntry = BuildMicrosoftASTRecordLayout(D);
>>   } else if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D)) {
>>     ...
>>
>> -    // MSVC gives the vb-table pointer an alignment equal to that of
>> -    // the non-virtual part of the structure.  That's an inherently
>> -    // multi-pass operation.  If our first pass doesn't give us
>> -    // adequate alignment, try again with the specified minimum
>> -    // alignment.  This is *much* more maintainable than computing the
>> -    // alignment in advance in a separately-coded pass; it's also
>> -    // significantly more efficient in the common case where the
>> -    // vb-table doesn't need extra padding.
>> -    if (Builder.VBPtrOffset != CharUnits::fromQuantity(-1) &&
>> -        (Builder.VBPtrOffset % Builder.NonVirtualAlignment) != 0) {
>> -      Builder.resetWithTargetAlignment(Builder.NonVirtualAlignment);
>> -      Builder.Layout(RD);
>> -    }
>> -
>>
>> This is great to remove, but you should also remove the *rest* of the
>> code in this file which is now completely dead.

This is a complicated issue.  I don't want to deprecate it until I'm sure
that it's not being used.  #pragma ms_struct has the behavior that it mixes
the
win32 and itanium ABIs.

>>
>> Index: test/Layout/ms-x86-aligned-tail-padding.cpp
>> ===================================================================
>> --- /dev/null
>> +++ test/Layout/ms-x86-aligned-tail-padding.cpp
>>
>> All of your new tests have carriage returns.

fixed.

>>
>> -// CHECK-NEXT:  sizeof=20, dsize=20, align=4
>> +// CHECK-NEXT:  sizeof=20, dsize=4, align=4
>>
>> You have a bunch of changes like this.  dsize doesn’t really apply to the
>> MS ABI, which never tail-allocates anything; if it’s at all convenient
to do so,
>> maybe we just shouldn’t be printing it.

done.



On Thu, Aug 29, 2013 at 4:57 PM, Warren Hunt <whunt at google.com> wrote:

>   Addresses John's last round of feedback, a more detailed replay will be
> sent via email.  Also passes all lit tests.
>
> Hi rnk, rsmith,
>
> http://llvm-reviews.chandlerc.com/D1026
>
> CHANGE SINCE LAST DIFF
>   http://llvm-reviews.chandlerc.com/D1026?vs=2896&id=3909#toc
>
> Files:
>   include/clang/AST/ASTContext.h
>   include/clang/AST/RecordLayout.h
>   include/clang/Basic/DiagnosticSemaKinds.td
>   lib/AST/CMakeLists.txt
>   lib/AST/MicrosoftRecordLayoutBuilder.cpp
>   lib/AST/RecordLayout.cpp
>   lib/AST/RecordLayoutBuilder.cpp
>   lib/CodeGen/CGRecordLayoutBuilder.cpp
>   lib/CodeGen/MicrosoftVBTables.cpp
>   lib/Sema/SemaDecl.cpp
>   lib/Sema/SemaDeclCXX.cpp
>   test/CodeGen/pr2394.c
>   test/CodeGenCXX/microsoft-abi-structors.cpp
>   test/CodeGenCXX/microsoft-abi-virtual-inheritance.cpp
>   test/Layout/ms-x86-aligned-tail-padding.cpp
>   test/Layout/ms-x86-basic-layout.cpp
>   test/Layout/ms-x86-empty-nonvirtual-bases.cpp
>   test/Layout/ms-x86-empty-virtual-base.cpp
>   test/Layout/ms-x86-lazy-empty-nonvirtual-base.cpp
>   test/Layout/ms-x86-primary-bases.cpp
>   test/Layout/ms-x86-size-alignment-fail.cpp
>   test/Layout/ms-x86-vfvb-alignment.cpp
>   test/Layout/ms-x86-vfvb-sharing.cpp
>   test/Layout/ms-x86-vtordisp.cpp
>   test/PCH/rdar10830559.cpp
>   test/Sema/ms_bitfield_layout.c
>   test/Sema/ms_class_layout.cpp
>   test/SemaCXX/ms_struct.cpp
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130829/77f99196/attachment.html>