>> + /// 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 <[email protected]> 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
