[PATCH] Microsoft C++ Record Layout
Warren Hunt
whunt at google.com
Fri Sep 6 16:42:26 PDT 2013
Ping. Any movement on this?
-Warren
On Thu, Aug 29, 2013 at 4:57 PM, Warren Hunt <whunt at google.com> wrote:
> >> + /// 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/20130906/28bd0168/attachment.html>
More information about the cfe-commits
mailing list