[cfe-dev] CGRecordLayoutBuilder for MS ABI

[John McCall]

On Oct 27, 2011, at 3:46 AM, r4start wrote:
> Thnx for review.
> Here is a new patch.

+    } else {
+      if (Layout.getVFPtrOffset() != CharUnits::fromQuantity(-1)) {
+        llvm::Type *FunctionType =
+          llvm::FunctionType::get(llvm::Type::getInt32Ty(Types.getLLVMContext()),
+          /*isVarArg=*/true);
+        llvm::Type *VTableTy = FunctionType->getPointerTo();
+
+        assert(NextFieldOffset.isZero() &&
+          "VTable pointer must come first!");
+        AppendField(Layout.getVFPtrOffset(), VTableTy->getPointerTo());
+      }
     }

Style nit:  since there's nothing in this 'else' clause except an if
statement, please write it as "else if (…) {".

-  // Pull the padding back off.
-  if (needsPadding)
-    FieldTypes.pop_back();
-
+  if (Types.getContext().getTargetInfo().getCXXABI() != CXXABI_Microsoft) {
+    // Pull the padding back off.
+    if (needsPadding)
+      FieldTypes.pop_back();
+  }

Did you mean to remove this diff, too?

+  if (Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
+    CharUnits NumPadBytes = Layout.getNonVirtualSize() - NextFieldOffset;
+    AppendBytes(NumPadBytes);
+  }

Please add a comment explaining why this is necessary.

-  // Append tail padding if necessary.
-  AppendTailPadding(Layout.getSize());
-
+  if (Types.getContext().getTargetInfo().getCXXABI() != CXXABI_Microsoft) {
+    // Append tail padding if necessary.
+    // In MS ABI it not necessary
+    AppendTailPadding(Layout.getSize());
+  }

That cannot be unconditionally true, unless you're saying that these
other places where you're adding padding automatically take care
of this?

     AppendBytes(padding);
+  } else if (
+    Types.getContext().getTargetInfo().getCXXABI() == CXXABI_Microsoft) {
+    // In MS mode fieldOffset always equal alignedNextFieldOffset
+    CharUnits padding = fieldOffset - NextFieldOffset;
+    
+    AppendBytes(padding);

This comment doesn't really mean anything to me.  Why does that
suggest that padding is required?

John.