<div dir="ltr">Ping again.<div><br></div><div>-Warren</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Sep 6, 2013 at 4:42 PM, Warren Hunt <span dir="ltr"><<a href="mailto:whunt@google.com" target="_blank">whunt@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Ping.  Any movement on this?<span class="HOEnZb"><font color="#888888"><div><br></div><div>-Warren</div>
</font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 29, 2013 at 4:57 PM, Warren Hunt <span dir="ltr"><<a href="mailto:whunt@google.com" target="_blank">whunt@google.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><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><div>>> You’ve copied the comment here from HasOwnVFPtr.</div><div><br></div></div><div>fixed.</div><div><div><br>
</div><div>>> </div>
<div>>> +// significantly different than those produced by the Itanium ABI.  Here We note</div><div>>> +// the most important differences.</div><div>>> </div><div>>> Capitalization.</div><div><br>

</div>
</div><div>fixed.</div><div><div><br></div><div>>> </div><div>>> +// * The alignment of bitfields in unions is ignored when computing the</div><div>>> +//   alignment of the union</div><div>>> </div>

<div>>> Period.</div>
<div><br></div></div><div>fixed.</div><div><div><br></div><div>>> </div><div>>> +// * The Itanium equivelant </div><div>>> </div><div>>> equivalent</div><div><br></div></div><div>fixed.</div>
<div><div><br></div><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><div>>> I’d just say “, possibly different ones.”</div><div><br></div></div><div>fixed.</div><div><div><br></div><div>>> </div><div>>> +// * Virtual bases sometimes require a 'vtordisp' field that is layed out before</div>


<div>>> </div><div>>> laid</div><div><br></div></div><div>fixed.</div><div><div><br></div><div>>> </div><div>>> +// * vtordisps are allocated in a block of memory with size and alignment equal</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><div>>> Er.  Unless our existing tests are wildly wrong, vtordisps always occur</div><div>>> before a virtual base.</div>


<div><br></div></div><div>I adjusted to comment to be more clear.  If a virtual base has a vtordisp, the</div><div>compiler allocates an nvalign sized block of memory before that virtual base and</div><div>places the vtordisp at the end of the block (immediately prior to the virtual base).</div>

<div>
<div><br></div><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><div>>> This doesn’t seem to be true; the vbptr itself always seems to be aligned.</div><div><br></div></div><div>Clarified comment a bit.  The block of memory allocated for the can be vbptr can</div>

<div>
be arbitrairly aligned.  The vbptr itself is always properly aligned.  If the block</div><div>is unaligned, then the block may be enlargened to enable proper alignment of the</div><div>vbptr.  If this occurs, very bizarre placement rules go into effect for the first</div>


<div>elements after the vbptr.</div><div><div><br></div><div>>> </div><div>>> +// * The last zero sized non-virtual base is allocated after the placement of</div><div>>> +//   vbprt </div><div>
>> </div>
<div>>> the vbptr</div><div><br></div></div><div>fixed.</div><div><div><br></div><div>>> </div><div>>> +  void Layout(const RecordDecl *RD);</div><div>>> </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>done.</div><div><div><br></div><div>>> </div><div>>> +  // Look at all of our methods to determine if we need a VFPtr.  We need a</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>>> </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>No, MS-ABI adds a vf-table entry to the virtual bases vf-table instead of</div>


<div>creating a new vf-table.  This results in some collisions if multiple</div><div>derive classes try to override the same function in virtual base, see:</div><div><a href="http://llvm-reviews.chandlerc.com/D1076" target="_blank">http://llvm-reviews.chandlerc.com/D1076</a></div>

<div>
<div><br></div><div>>> </div><div>>> +    UpdateAlignment(Layout.getAlignment());</div><div>>> </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>comment added.</div><div><div><br></div><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>


<div>>> I feel like FieldOffsets and LastFieldIsNonZeroWidthBitfield should be reset</div><div>>> in the same place, probably here.</div><div><br></div></div><div>I can do that, LastFieldIsNonZeroWidthBitfield is logically associated with</div>


<div>the LayoutFields algorithm. (It's never read or written outside of that function</div><div>or its subroutines).  Therefore I set it at the beginning of LayoutFields instead</div><div>of somewhere else.  Given that it's implemented using a member, I can clear it</div>


<div>anywhere and could do it here.</div><div><div><br></div><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><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>

yes.  I added a little bit more to the comment.</div><div>
<div><br></div><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><div>>> Please grab this layout once at the top of the method.</div><div><br></div></div><div>RD can be null in this method, I grab it only after checking that RD is not null</div><div>on each branch.  As long as the handle layout is a reference I have to do it the</div>


<div>current way.  I went ahead and did this.</div><div><div><br></div><div>>> </div><div>>> +  CharUnits LazyEmptyBaseOffset = CharUnits::Zero();</div><div>>> +</div><div>>> </div><div>
>> Dead variable?</div>
<div><br></div></div><div>removed.</div><div><div><br></div><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><div>>> explanation</div><div>>> always</div><div><br></div></div><div>There is an explanation! Oh, that's two spelling edits. fixed.</div>

<div>
<div><br></div><div>>> </div><div>>> +      CharUnits x = Size + Alignment + Alignment;</div><div>>> </div><div>>> This variable can be sunk to where it’s used.</div><div><br></div></div><div>nope, Size changes on the next line.</div>

<div>
<div><br></div><div>>> </div><div>>> +      if (!RD->field_empty())</div><div>>> +        Size += CharUnits::fromQuantity(</div><div>>> +            x % getAdjustedFieldInfo(*RD->field_begin()).second);</div>


<div>>> </div><div>>> This is a really bizarre calculation, but if it’s what they do...</div><div><br></div></div><div>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.)</div>

<div>
<div><br></div><div>>> </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>done.  (However I think the using both begin and empty is defensible because</div>


<div>this code is so far off the critical path it's unlikely to even be called once</div><div>in most cases and I think it's slightly clearer the current way.)</div><div><div><br></div><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>


<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>done.</div><div><div><br></div><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><div>>> I’d expect Sema to have already errored, actually.</div><div><br></div></div><div>You may be correct, I am pretty new to this project and am not certain about the</div>


<div>order in which these things run, I changed the comment to "Sema will throw an error."</div><div><div><br></div><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><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>Added more detailed comment.</div><div><div><br></div><div>>> </div><div>>> +    // TODO (whunt): Add a Sema warning that MS ignores bitfield alignment</div>


<div>>> +    // in unions.</div><div>>> </div><div>>> We kindof frown on personalized TODOs.</div><div><br></div></div><div>Okay, it was normal in Chrome.  I removed the (whunt).</div><div><div>
<br></div><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><div>>> This should be checking the target C++ ABI, not the target triple directly.</div><div>


<br></div></div><div>changed, this should be discussed in detail.</div><div><div><br></div><div>>> </div><div>>> Also, I don’t understand why it’s *filtering out* things with ms_struct?</div><div><br>
</div></div><div>used during debugging, fixed.</div><div>
<div><br></div><div>>> </div><div>>> Also, you’ve left the rest of this block mis-indented.</div><div><br></div></div><div>fixed.</div><div><div><br></div><div>>> </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>>> -    // 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><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>This is a complicated issue.  I don't want to deprecate it until I'm sure</div><div>that it's not being used.  #pragma ms_struct has the behavior that it mixes the</div><div>win32 and itanium ABIs.</div>

<div>
<div><br></div><div>>> </div><div>>> Index: test/Layout/ms-x86-aligned-tail-padding.cpp</div><div>>> ===================================================================</div><div>>> --- /dev/null</div>


<div>>> +++ test/Layout/ms-x86-aligned-tail-padding.cpp</div><div>>> </div><div>>> All of your new tests have carriage returns.</div><div><br></div></div><div>fixed.</div><div><div><br></div>
<div>>> </div><div>
>> -// CHECK-NEXT:  sizeof=20, dsize=20, align=4</div><div>>> +// CHECK-NEXT:  sizeof=20, dsize=4, align=4</div><div>>> </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><div>done.</div><div><br></div></div><div>

<div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Thu, Aug 29, 2013 at 4:57 PM, Warren Hunt <span dir="ltr"><<a href="mailto:whunt@google.com" target="_blank">whunt@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


  Addresses John's last round of feedback, a more detailed replay will be sent via email.  Also passes all lit tests.<br>
<div><br>
Hi rnk, rsmith,<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1026" target="_blank">http://llvm-reviews.chandlerc.com/D1026</a><br>
<br>
CHANGE SINCE LAST DIFF<br>
</div>  <a href="http://llvm-reviews.chandlerc.com/D1026?vs=2896&id=3909#toc" target="_blank">http://llvm-reviews.chandlerc.com/D1026?vs=2896&id=3909#toc</a><br>
<br>
Files:<br>
  include/clang/AST/ASTContext.h<br>
  include/clang/AST/RecordLayout.h<br>
  include/clang/Basic/DiagnosticSemaKinds.td<br>
<div>  lib/AST/CMakeLists.txt<br>
  lib/AST/MicrosoftRecordLayoutBuilder.cpp<br>
  lib/AST/RecordLayout.cpp<br>
  lib/AST/RecordLayoutBuilder.cpp<br>
  lib/CodeGen/CGRecordLayoutBuilder.cpp<br>
  lib/CodeGen/MicrosoftVBTables.cpp<br>
</div>  lib/Sema/SemaDecl.cpp<br>
  lib/Sema/SemaDeclCXX.cpp<br>
  test/CodeGen/pr2394.c<br>
  test/CodeGenCXX/microsoft-abi-structors.cpp<br>
  test/CodeGenCXX/microsoft-abi-virtual-inheritance.cpp<br>
<div>  test/Layout/ms-x86-aligned-tail-padding.cpp<br>
  test/Layout/ms-x86-basic-layout.cpp<br>
  test/Layout/ms-x86-empty-nonvirtual-bases.cpp<br>
  test/Layout/ms-x86-empty-virtual-base.cpp<br>
  test/Layout/ms-x86-lazy-empty-nonvirtual-base.cpp<br>
  test/Layout/ms-x86-primary-bases.cpp<br>
  test/Layout/ms-x86-size-alignment-fail.cpp<br>
  test/Layout/ms-x86-vfvb-alignment.cpp<br>
  test/Layout/ms-x86-vfvb-sharing.cpp<br>
  test/Layout/ms-x86-vtordisp.cpp<br>
</div>  test/PCH/rdar10830559.cpp<br>
  test/Sema/ms_bitfield_layout.c<br>
  test/Sema/ms_class_layout.cpp<br>
  test/SemaCXX/ms_struct.cpp<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>