Ping. Any movement on this? -Warren
On Thu, Aug 29, 2013 at 4:57 PM, Warren Hunt <[email protected]> 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 <[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
