[cfe-dev] Different class layouts between clang and MSVC

[Don Williamson]

Sorry, my reply-all skills are lacking!

The problem with this is that the record builder doesn't know the value of Alignment until its done a complete pass on the layout - the double field might be right at the end of the struct.

Here are the modifications I've made that have allowed me to keep working - it passes all my tests, at least. I suppose we need to work on what's required to get this to patch-quality level :)

It will take me a while to get this up to quality as I'm self-employed currently and really need to spend time on my little business. Does this discussion need to be take to the commit list?

Index: RecordLayoutBuilder.cpp
===================================================================
--- RecordLayoutBuilder.cpp(revision 134404)
+++ RecordLayoutBuilder.cpp(working copy)
@@ -722,6 +722,7 @@
   void setDataSize(CharUnits NewSize) { DataSize = Context.toBits(NewSize); }
   void setDataSize(uint64_t NewSize) { DataSize = NewSize; }
 
+  CharUnits getAlignment() const { return Alignment; }
 
   RecordLayoutBuilder(const RecordLayoutBuilder&);   // DO NOT IMPLEMENT
   void operator=(const RecordLayoutBuilder&); // DO NOT IMPLEMENT
@@ -1805,10 +1806,12 @@
 namespace {
   // This class implements layout specific to the Microsoft ABI.
   class MSRecordLayoutBuilder : public RecordLayoutBuilder {
+  CharUnits Alignment;
   public:
     MSRecordLayoutBuilder(const ASTContext& Ctx,
-                          EmptySubobjectMap *EmptySubobjects) :
-      RecordLayoutBuilder(Ctx, EmptySubobjects) {}
+                          EmptySubobjectMap *EmptySubobjects,
+  CharUnits Alignment) :
+      RecordLayoutBuilder(Ctx, EmptySubobjects), Alignment(Alignment) {}
 
     virtual CharUnits GetVirtualPointersSize(const CXXRecordDecl *RD) const;
   };
@@ -1822,6 +1825,11 @@
     Context.toCharUnitsFromBits(Context.Target.getPointerWidth(0));
   if (RD->isPolymorphic() && RD->getNumVBases() > 0)
     return 2 * PointerWidth;
+
+  // Align the vtable pointer to the struct alignment
+  if (!Alignment.isZero())
+  return PointerWidth.RoundUpToAlignment(Alignment);
+
   return PointerWidth;
 }
 
@@ -1851,7 +1859,8 @@
       Builder.reset(new RecordLayoutBuilder(*this, &EmptySubobjects));
       break;
     case CXXABI_Microsoft:
-      Builder.reset(new MSRecordLayoutBuilder(*this, &EmptySubobjects));
+      Builder.reset(new MSRecordLayoutBuilder(*this, &EmptySubobjects,
+  CharUnits::Zero()));
     }
     // Recover resources if we crash before exiting this method.
     llvm::CrashRecoveryContextCleanupRegistrar<RecordLayoutBuilder>
@@ -1859,10 +1868,27 @@
     
     Builder->Layout(RD);
 
+if (Target.getCXXABI() == CXXABI_Microsoft &&
+Builder->getAlignment().getQuantity() > 4) {
+// MSVC rounds the vtable pointer to the struct alignment in what must
+// be a multi-pass operation. For now, let the builder figure out the
+// alignment and recalculate the layout once its known.
+Builder.reset(new MSRecordLayoutBuilder(*this, &EmptySubobjects,
+Builder->getAlignment()));
+
+// Recover resources if we crash before exiting this method.
+llvm::CrashRecoveryContextCleanupRegistrar<RecordLayoutBuilder>
+RecordBuilderCleanup(Builder.get());
+
+Builder->Layout(RD);
+}
+
     // FIXME: This is not always correct. See the part about bitfields at
     // http://www.codesourcery.com/public/cxx-abi/abi.html#POD for more info.
     // FIXME: IsPODForThePurposeOfLayout should be stored in the record layout.
-    bool IsPODForThePurposeOfLayout = cast<CXXRecordDecl>(D)->isPOD();
+// This does not affect the calculations of MSVC layouts
+    bool IsPODForThePurposeOfLayout = Target.getCXXABI() == CXXABI_Microsoft ||
+cast<CXXRecordDecl>(D)->isPOD();
 
     // FIXME: This should be done in FinalizeLayout.
     CharUnits DataSize =


Cheers,
- Don


________________________________
From: John McCall <rjmccall at apple.com>
To: Don Williamson <don.williamson at yahoo.com>
Sent: Tuesday, September 6, 2011 6:03 PM
Subject: Re: [cfe-dev] Different class layouts between clang and MSVC


On Sep 6, 2011, at 9:36 AM, Don Williamson wrote:
Who would I talk to about implementation details? The only way I can think of fixing the virtual issue that doesn't duplicate code or bother existing behaviour to much is to check and see if Alignment is bigger than PointerWidth and rebuild the layout with a vtable ptr size of PointerWidth*2 (MS ABI only).

My guess is that you should actually round up to Alignment after doing the vtable;  try adding something 16-byte aligned.  Basically, I'm guessing that MSVC is laying things out as if the class data were an independent struct following the vtable.

Feel free to just propose the simplest patch that achieves this;  maybe a simpler solution will present itself.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110907/c91a4207/attachment.html>