[PATCH] Microsoft C++ Record Layout
Warren Hunt
whunt at google.com
Fri Sep 13 11:49:24 PDT 2013
Ping again.
-Warren
On Fri, Sep 6, 2013 at 4:42 PM, Warren Hunt <whunt at google.com> wrote:
> 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/20130913/c9cdf188/attachment.html>
More information about the cfe-commits
mailing list