<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">On Jul 18, 2013, at 1:32 PM, Warren Hunt <<a href="mailto:whunt@google.com">whunt@google.com</a>> wrote:<br><div><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"> Fixed code-gen such that all lit tests pass! The patch should be pretty much good to go.<br></div></blockquote></div><br><div><div>+ /// HasOwnVBPtr - Does this class provide a virtual function table</div><div>+ /// (vtable in Itanium, VBtbl in Microsoft) that is independent from</div><div>+ /// its base classes?</div><div>+ bool HasOwnVBPtr : 1;</div><div>+ </div></div><div><br></div><div>You’ve copied the comment here from HasOwnVFPtr.</div><div><br></div><div><div>+// significantly different than those produced by the Itanium ABI. Here We note</div><div>+// the most important differences.</div></div><div><br></div><div>Capitalization.</div><div><br></div><div><div>+// * The alignment of bitfields in unions is ignored when computing the</div><div>+// alignment of the union</div></div><div><br></div><div>Period.</div><div><br></div><div>+// * The Itanium equivelant </div><div><br></div><div>equivalent</div><div><br></div><div><div>+// function pointer) and a vbptr (virtual base pointer). They can each be</div><div>+// shared with a non-virtual base and each can be shared with a distinct</div><div>+// non-virtual base.</div></div><div><br></div><div>I’d just say “, possibly different ones.”</div><div><br></div><div><div>+// * Virtual bases sometimes require a 'vtordisp' field that is layed out before</div></div><div><br></div><div>laid</div><div><br></div><div><div>+// * vtordisps are allocated in a block of memory with size and alignment equal</div></div><div><div>+// to the alignment of the completed structure (before applying __declspec(</div><div>+// align())). They always occur at the end of the allocation.</div></div><div><br></div><div>Er. Unless our existing tests are wildly wrong, vtordisps always occur</div><div>before a virtual base.</div><div><br></div><div><div>+// * vbptrs are allocated in a block of memory equal to the alignment of the</div><div>+// fields and non-virtual bases at a potentially unaligned offset.</div></div><div><br></div><div>This doesn’t seem to be true; the vbptr itself always seems to be aligned.</div><div><br></div><div><div>+// * The last zero sized non-virtual base is allocated after the placement of</div><div>+// vbprt </div></div><div><br></div><div>the vbptr</div><div><br></div><div>+ void Layout(const RecordDecl *RD);</div><div><br></div><div>Since this is all new code, please lowercase method names in accordance</div><div>with LLVM style conventions.</div><div><br></div><div><div>+ // Look at all of our methods to determine if we need a VFPtr. We need a</div></div><div><div><div>+ // vfptr if we define a new virtual function.</div><div>+ if (!HasVFPtr && RD->isDynamicClass())</div><div>+ for (clang::CXXRecordDecl::method_iterator i = RD->method_begin(),</div><div>+ e = RD->method_end();</div><div>+ !HasVFPtr && i != e; ++i)</div><div>+ HasVFPtr = i->isVirtual() && i->size_overridden_methods() == 0;</div><div>+ if (!HasVFPtr)</div><div>+ return;</div><div><br></div></div><div>I believe we also need a new vf-table if we have a non-trivial covariant</div><div>override of a virtual function.</div><div><br></div><div><div>+ UpdateAlignment(Layout.getAlignment());</div></div><div><br></div><div>If this really isn’t affected by whether the class is packed, that’s worth</div><div>a comment.</div><div><br></div><div><div>+ // Use LayoutFields to compute the alignment of the fields. The layout</div><div>+ // is discarded. This is the simplest way to get all of the bit-field</div><div>+ // behavior correct and is not actually very expensive.</div><div>+ LayoutFields(RD);</div><div>+ Size = CharUnits::Zero();</div><div>+ FieldOffsets.clear();</div></div><div><br></div><div>I feel like FieldOffsets and LastFieldIsNonZeroWidthBitfield should be reset</div><div>in the same place, probably here.</div><div><br></div><div><div>+ // MSVC potentially over-aligns the vb-table pointer by giving it</div><div>+ // the max alignment of all the non-virtual objects in the class.</div><div>+ // This is completely unnecessary, but we're not here to pass</div><div>+ // judgment.</div></div><div><br></div><div>Er, what follows is all the nvdata, not just the vbptr. What you’re saying</div><div>is that MSVC over-aligns following a new vfptr, basically as if the layout</div><div>used struct layout rules for this type:</div><div> { vfptr, { nvdata } }</div><div><br></div><div><div>+ // Empty bases only consume space when followed by another empty base.</div><div>+ if (RD && Context.getASTRecordLayout(RD).getNonVirtualSize().isZero())</div></div><div><br></div><div>Please grab this layout once at the top of the method.</div><div><br></div><div><div>+ CharUnits LazyEmptyBaseOffset = CharUnits::Zero();</div><div>+</div></div><div><br></div><div>Dead variable?</div><div><br></div><div><div>+ // Handle strange padding rules. I have no explenation for why the</div><div>+ // virtual base is padded in such an odd way. My guess is that they</div><div>+ // allways Add 2 * Alignment and incorrectly round down to the appropriate</div></div><div><br></div><div>explanation</div><div>always</div><div><br></div><div><div>+ CharUnits x = Size + Alignment + Alignment;</div></div><div><br></div><div>This variable can be sunk to where it’s used.</div><div><div><br></div><div>+ if (!RD->field_empty())</div><div>+ Size += CharUnits::fromQuantity(</div><div>+ x % getAdjustedFieldInfo(*RD->field_begin()).second);</div></div><div><br></div><div>This is a really bizarre calculation, but if it’s what they do...</div><div><br></div><div>field_begin() and field_empty() do some non-trivial redundant</div><div>work; please re-use the result of field_begin().</div><div><br></div><div><div>+ if (FD->isBitField()) {</div><div>+ LayoutBitField(FD);</div><div>+ return;</div><div>+ }</div><div>+</div><div>+ std::pair<CharUnits, CharUnits> FieldInfo = getAdjustedFieldInfo(FD);</div><div>+ CharUnits FieldSize = FieldInfo.first;</div><div>+ CharUnits FieldAlign = FieldInfo.second;</div><div>+</div><div>+ LastFieldIsNonZeroWidthBitfield = false;</div></div><div><br></div><div>I feel that the invariants would be clearer if you put this line immediately</div><div>after the isBitField() block.</div><div><br></div><div><div>+ // Clamp the bitfield to a containable size for the sake of being able</div><div>+ // to lay them out. Sema will error later.</div></div><div><br></div><div>I’d expect Sema to have already errored, actually.</div><div><br></div><div><div>+ if (!IsUnion && LastFieldIsNonZeroWidthBitfield &&</div><div>+ CurrentBitfieldSize == FieldSize && Width <= RemainingBitsInField) {</div><div>+ PlaceFieldAtBitOffset(Context.toBits(Size) - RemainingBitsInField);</div><div>+ RemainingBitsInField -= Width;</div><div>+ return;</div></div><div><br></div><div>Most of these checks are obvious, but please add a comment pointing out</div><div>the unusual thing here: that MSVC refuses to pack bit-fields together if their</div><div>formal type has a different size.</div><div><br></div><div><div>+ // TODO (whunt): Add a Sema warning that MS ignores bitfield alignment</div><div>+ // in unions.</div></div><div><br></div><div>We kindof frown on personalized TODOs.</div><div><br></div><div><div>+ if (getTargetInfo().getTriple().getArch() == llvm::Triple::x86 &&</div><div>+ getTargetInfo().getTriple().getOS() == llvm::Triple::Win32 &&</div><div>+ !D->isMsStruct(*this)) {</div><div>+ NewEntry = BuildMicrosoftASTRecordLayout(D);</div><div>+ } else {</div></div><div><br></div><div>This should be checking the target C++ ABI, not the target triple directly.</div><div><br></div><div>Also, I don’t understand why it’s *filtering out* things with ms_struct?</div><div><br></div><div>Also, you’ve left the rest of this block mis-indented.</div><div><br></div><div>You should really just do this at the top level:</div><div> if (getCXXABI().isMicrosoft()) {</div><div> NewEntry = BuildMicrosoftASTRecordLayout(D);</div><div> } else if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D)) {</div><div> ...</div><div> </div><div><div>- // MSVC gives the vb-table pointer an alignment equal to that of</div><div>- // the non-virtual part of the structure. That's an inherently</div><div>- // multi-pass operation. If our first pass doesn't give us</div><div>- // adequate alignment, try again with the specified minimum</div><div>- // alignment. This is *much* more maintainable than computing the</div><div>- // alignment in advance in a separately-coded pass; it's also</div><div>- // significantly more efficient in the common case where the</div><div>- // vb-table doesn't need extra padding.</div><div>- if (Builder.VBPtrOffset != CharUnits::fromQuantity(-1) &&</div><div>- (Builder.VBPtrOffset % Builder.NonVirtualAlignment) != 0) {</div><div>- Builder.resetWithTargetAlignment(Builder.NonVirtualAlignment);</div><div>- Builder.Layout(RD);</div><div>- }</div><div>-</div></div><div><br></div><div>This is great to remove, but you should also remove the *rest* of the</div><div>code in this file which is now completely dead.</div><div><br></div><div><div>Index: test/Layout/ms-x86-aligned-tail-padding.cpp</div></div><div><div>===================================================================</div><div>--- /dev/null</div><div>+++ test/Layout/ms-x86-aligned-tail-padding.cpp</div></div><div><br></div><div>All of your new tests have carriage returns.</div><div><br></div><div><div>-// CHECK-NEXT: sizeof=20, dsize=20, align=4</div><div>+// CHECK-NEXT: sizeof=20, dsize=4, align=4</div></div><div><br></div><div>You have a bunch of changes like this. dsize doesn’t really apply to the</div><div>MS ABI, which never tail-allocates anything; if it’s at all convenient to do so,</div><div>maybe we just shouldn’t be printing it.</div><div><br></div><div>John.</div><div></div></div></body></html>