[PATCH] Microsoft 64-bit Record Layout

[Reid Kleckner]

  w00t!  Lots of deleted code and added tests.  :)  Probably worth another round of review.


================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2009-2010
@@ -2347,3 +2008,4 @@
 static bool isMsLayout(const RecordDecl* D) {
   // FIXME: Use MS record layout for x64 code and remove MS C++ support from the
   // Itanium record layout code.
+  return D->getASTContext().getTargetInfo().getCXXABI().isMicrosoft();
----------------
This is now fixed.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2274
@@ -2603,2 +2273,3 @@
   VirtualAlignment = CharUnits::One();
+  AlignAfterVBases = PointerSize.getQuantity() != 4;
 
----------------
Check the architecture on the triple rather than the pointer size.  You could cache that in an IsX64 builder member for readability.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2336
@@ -2663,2 +2335,3 @@
   updateAlignment(PointerAlignment);
-  Size += Alignment;
+  if (PointerSize.getQuantity() != 4)
+    Size = Size.RoundUpToAlignment(PointerAlignment) + PointerSize;
----------------
Ditto

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2345
@@ -2669,2 +2344,3 @@
   LazyEmptyBase = 0;
+  LastBaseWasEmpty = 0;
 
----------------
s/0/false/

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2418
@@ +2417,3 @@
+    // second to last base was also zero sized.
+    int64_t X = (SizeX + PtrSizeX + PtrAlignX - 1) / PtrAlignX * PtrAlignX;
+    if (BasesAndFieldsAlignment <= PointerAlignment) {
----------------
X seems like a bad name.  I missed that in the other review.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2408-2418
@@ -2729,17 +2407,13 @@
     VBPtrOffset = Size.RoundUpToAlignment(PointerAlignment);
-
-    if (Alignment == PointerAlignment && Size % PointerAlignment) {
-      CharUnits x = Size + Alignment + Alignment;
-      Size = VBPtrOffset + Alignment;
-      // Handle strange padding rules.  I have no explanation for why the
-      // virtual base is padded in such an odd way.  My guess is that they
-      // always Add 2 * Alignment and incorrectly round down to the appropriate
-      // alignment.  It's important to get this case correct because it impacts
-      // the layout of the first member of the struct.
-      
-      RecordDecl::field_iterator FieldBegin = RD->field_begin();
-      if (FieldBegin != RD->field_end())
-        Size += CharUnits::fromQuantity(
-            x % getAdjustedFieldInfo(*FieldBegin).second);
-    } else
-      Size += Alignment;
+    int64_t AlignX = BasesAndFieldsAlignment.getQuantity();
+    int64_t SizeX = Size.getQuantity();
+    int64_t PtrAlignX = PointerAlignment.getQuantity();
+    int64_t PtrSizeX = PointerSize.getQuantity();
+    // Handle strange padding rules for the lazily placed base.  I have no
+    // explanation for why the last virtual base is padded in such an odd way.
+    // Two things to note about this padding are that the rules are different
+    // if the alignment of the bases+fields is <= to the alignemnt of a pointer
+    // and that the rule in 64-bit mode behaves differently depending on if the
+    // second to last base was also zero sized.
+    int64_t X = (SizeX + PtrSizeX + PtrAlignX - 1) / PtrAlignX * PtrAlignX;
+    if (BasesAndFieldsAlignment <= PointerAlignment) {
----------------
I think this can all be something like:

  CharUnits LazyBaseOffset = (Size + PointerSize).RoundUpToAlignment(PtrAlign);

And then you can nuke the int64_t *X vars.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2422
@@ +2421,3 @@
+    } else {
+      if (PtrSizeX == 4)
+        Size += BasesAndFieldsAlignment;
----------------
Should be IsX64 or something checking the triple.

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2572
@@ -2891,1 +2571,3 @@
+    // is less than 4 bytes, at which it is placed at 4 byte offset in the
+    // chunck.  We have no idea why.
     if (RD && Context.getASTRecordLayout(RD).getNonVirtualSize().isZero())
----------------
s/chunck/chunk/

================
Comment at: lib/AST/RecordLayoutBuilder.cpp:2591
@@ -2908,2 +2590,3 @@
 
+  // vtordisps are always 4 bytes (even in 64-bit mode)
   if (HasVtordisp)
----------------
Yep, this is true throughout the ABI.  It seems objects cannot be larger than 2**32.


http://llvm-reviews.chandlerc.com/D2003