[PATCH] Microsoft C++ Record Layout

[John McCall]

On Jul 18, 2013, at 1:32 PM, Warren Hunt <whunt at google.com> wrote:
>  Fixed code-gen such that all lit tests pass!  The patch should be pretty much good to go.

+    /// 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.

+// significantly different than those produced by the Itanium ABI.  Here We note
+// the most important differences.

Capitalization.

+// * The alignment of bitfields in unions is ignored when computing the
+//   alignment of the union

Period.

+// * The Itanium equivelant 

equivalent

+//   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.”

+// * Virtual bases sometimes require a 'vtordisp' field that is layed out before

laid

+// * 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.

+// * 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.

+// * The last zero sized non-virtual base is allocated after the placement of
+//   vbprt 

the vbptr

+  void Layout(const RecordDecl *RD);

Since this is all new code, please lowercase method names in accordance
with LLVM style conventions.

+  // 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.

+    UpdateAlignment(Layout.getAlignment());

If this really isn’t affected by whether the class is packed, that’s worth
a comment.

+  // 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.

+  // 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 } }

+    // 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.

+  CharUnits LazyEmptyBaseOffset = CharUnits::Zero();
+

Dead variable?

+      // 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

+      CharUnits x = Size + Alignment + Alignment;

This variable can be sunk to where it’s used.

+      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...

field_begin() and field_empty() do some non-trivial redundant
work; please re-use the result of field_begin().

+  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.

+  // 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.

+  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.

+    // TODO (whunt): Add a Sema warning that MS ignores bitfield alignment
+    // in unions.

We kindof frown on personalized TODOs.

+    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.

Also, I don’t understand why it’s *filtering out* things with ms_struct?

Also, you’ve left the rest of this block mis-indented.

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.

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.

-// 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.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130725/b7ac37f3/attachment.html>