Ping again. -Warren
On Fri, Sep 6, 2013 at 4:42 PM, Warren Hunt <[email protected]> wrote: > 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
