[ms-cxxabi] Raise aggregate memptr alignment to 8 for x86_32

With this change, we give different results for __alignof than MSVC, but
our record layout is compatible.

Some data member pointers also now have a size that is not a multiple of
their alignment.

Fixes PR18618.

Reviewers: majnemer

Differential Revision: http://llvm-reviews.chandlerc.com/D2669

llvm-svn: 200585

4 files changed, 147 insertions, 34 deletions

diff --git a/clang/lib/AST/MicrosoftCXXABI.cpp b/clang/lib/AST/MicrosoftCXXABI.cpp
index ee96e13e4b1..27b3e341c4b 100644
--- a/clang/lib/AST/MicrosoftCXXABI.cpp
+++ b/clang/lib/AST/MicrosoftCXXABI.cpp
@@ -197,8 +197,20 @@ std::pair<uint64_t, unsigned> MicrosoftCXXABI::getMemberPointerWidthAndAlign(
   unsigned PtrSize = Target.getPointerWidth(0);
   unsigned IntSize = Target.getIntWidth();
   uint64_t Width = Ptrs * PtrSize + Ints * IntSize;
-  unsigned Align = Ptrs > 0 ? Target.getPointerAlign(0) : Target.getIntAlign();
-  Width = llvm::RoundUpToAlignment(Width, Align);
+  unsigned Align;
+
+  // When MSVC does x86_32 record layout, it aligns aggregate member pointers to
+  // 8 bytes.  However, __alignof usually returns 4 for data memptrs and 8 for
+  // function memptrs.
+  if (Ptrs + Ints > 1 && Target.getTriple().getArch() == llvm::Triple::x86)
+    Align = 8 * 8;
+  else if (Ptrs)
+    Align = Target.getPointerAlign(0);
+  else
+    Align = Target.getIntAlign();
+
+  if (Target.getTriple().getArch() == llvm::Triple::x86_64)
+    Width = llvm::RoundUpToAlignment(Width, Align);
   return std::make_pair(Width, Align);
 }
 
diff --git a/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp b/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
index 1da8e1225ec..7624cee879f 100644
--- a/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ b/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -56,20 +56,20 @@ int UnspecSingle::*us_d_memptr;
 // CHECK: @"\01?p_d_memptr@@3PQPolymorphic@@HQ1@" = global i32 0, align 4
 // CHECK: @"\01?m_d_memptr@@3PQMultiple@@HQ1@" = global i32 -1, align 4
 // CHECK: @"\01?v_d_memptr@@3PQVirtual@@HQ1@" = global { i32, i32 }
-// CHECK:   { i32 0, i32 -1 }, align 4
+// CHECK:   { i32 0, i32 -1 }, align 8
 // CHECK: @"\01?n_d_memptr@@3PQNonZeroVBPtr@@HQ1@" = global { i32, i32 }
-// CHECK:   { i32 0, i32 -1 }, align 4
+// CHECK:   { i32 0, i32 -1 }, align 8
 // CHECK: @"\01?u_d_memptr@@3PQUnspecified@@HQ1@" = global { i32, i32, i32 }
-// CHECK:   { i32 0, i32 0, i32 -1 }, align 4
+// CHECK:   { i32 0, i32 0, i32 -1 }, align 8
 // CHECK: @"\01?us_d_memptr@@3PQUnspecSingle@@HQ1@" = global { i32, i32, i32 }
-// CHECK:   { i32 0, i32 0, i32 -1 }, align 4
+// CHECK:   { i32 0, i32 0, i32 -1 }, align 8
 
 void (Single  ::*s_f_memptr)();
 void (Multiple::*m_f_memptr)();
 void (Virtual ::*v_f_memptr)();
 // CHECK: @"\01?s_f_memptr@@3P8Single@@AEXXZQ1@" = global i8* null, align 4
-// CHECK: @"\01?m_f_memptr@@3P8Multiple@@AEXXZQ1@" = global { i8*, i32 } zeroinitializer, align 4
-// CHECK: @"\01?v_f_memptr@@3P8Virtual@@AEXXZQ1@" = global { i8*, i32, i32 } zeroinitializer, align 4
+// CHECK: @"\01?m_f_memptr@@3P8Multiple@@AEXXZQ1@" = global { i8*, i32 } zeroinitializer, align 8
+// CHECK: @"\01?v_f_memptr@@3P8Virtual@@AEXXZQ1@" = global { i8*, i32, i32 } zeroinitializer, align 8
 
 // We can define Unspecified after locking in the inheritance model.
 struct Unspecified : Multiple, Virtual {
@@ -91,13 +91,13 @@ void (UnspecSingle::*us_f_mp)() = &UnspecSingle::foo;
 // CHECK: @"\01?s_f_mp@Const@@3P8Single@@AEXXZQ2@" =
 // CHECK:   global i8* bitcast ({{.*}} @"\01?foo@Single@@QAEXXZ" to i8*), align 4
 // CHECK: @"\01?m_f_mp@Const@@3P8Multiple@@AEXXZQ2@" =
-// CHECK:   global { i8*, i32 } { i8* bitcast ({{.*}} @"\01?foo@B2@@QAEXXZ" to i8*), i32 4 }, align 4
+// CHECK:   global { i8*, i32 } { i8* bitcast ({{.*}} @"\01?foo@B2@@QAEXXZ" to i8*), i32 4 }, align 8
 // CHECK: @"\01?v_f_mp@Const@@3P8Virtual@@AEXXZQ2@" =
-// CHECK:   global { i8*, i32, i32 } { i8* bitcast ({{.*}} @"\01?foo@Virtual@@QAEXXZ" to i8*), i32 0, i32 0 }, align 4
+// CHECK:   global { i8*, i32, i32 } { i8* bitcast ({{.*}} @"\01?foo@Virtual@@QAEXXZ" to i8*), i32 0, i32 0 }, align 8
 // CHECK: @"\01?u_f_mp@Const@@3P8Unspecified@@AEXXZQ2@" =
-// CHECK:   global { i8*, i32, i32, i32 } { i8* bitcast ({{.*}} @"\01?foo@Unspecified@@QAEXXZ" to i8*), i32 0, i32 12, i32 0 }, align 4
+// CHECK:   global { i8*, i32, i32, i32 } { i8* bitcast ({{.*}} @"\01?foo@Unspecified@@QAEXXZ" to i8*), i32 0, i32 12, i32 0 }, align 8
 // CHECK: @"\01?us_f_mp@Const@@3P8UnspecSingle@@AEXXZQ2@" =
-// CHECK:   global { i8*, i32, i32, i32 } { i8* bitcast ({{.*}} @"\01?foo@UnspecSingle@@QAEXXZ" to i8*), i32 0, i32 0, i32 0 }, align 4
+// CHECK:   global { i8*, i32, i32, i32 } { i8* bitcast ({{.*}} @"\01?foo@UnspecSingle@@QAEXXZ" to i8*), i32 0, i32 0, i32 0 }, align 8
 }
 
 namespace CastParam {
@@ -119,11 +119,11 @@ void (A::*ptr1)(void *) = (void (A::*)(void *)) &A::foo;
 // Try a reinterpret_cast followed by a memptr conversion.
 void (C::*ptr2)(void *) = (void (C::*)(void *)) (void (A::*)(void *)) &A::foo;
 // CHECK: @"\01?ptr2@CastParam@@3P8C@1@AEXPAX@ZQ21@" =
-// CHECK:   global { i8*, i32 } { i8* bitcast (void ({{.*}})* @"\01?foo@A@CastParam@@QAEXPAU12@@Z" to i8*), i32 4 }, align 4
+// CHECK:   global { i8*, i32 } { i8* bitcast (void ({{.*}})* @"\01?foo@A@CastParam@@QAEXPAU12@@Z" to i8*), i32 4 }, align 8
 
 void (C::*ptr3)(void *) = (void (C::*)(void *)) (void (A::*)(void *)) (void (A::*)(A *)) 0;
 // CHECK: @"\01?ptr3@CastParam@@3P8C@1@AEXPAX@ZQ21@" =
-// CHECK:   global { i8*, i32 } zeroinitializer, align 4
+// CHECK:   global { i8*, i32 } zeroinitializer, align 8
 
 struct D : C {
   virtual void isPolymorphic();
@@ -156,23 +156,23 @@ void EmitNonVirtualMemberPointers() {
   void (UnspecWithVBPtr::*u2_f_memptr)() = &UnspecWithVBPtr::foo;
 // CHECK: define void @"\01?EmitNonVirtualMemberPointers@@YAXXZ"() {{.*}} {
 // CHECK:   alloca i8*, align 4
-// CHECK:   alloca { i8*, i32 }, align 4
-// CHECK:   alloca { i8*, i32, i32 }, align 4
-// CHECK:   alloca { i8*, i32, i32, i32 }, align 4
+// CHECK:   alloca { i8*, i32 }, align 8
+// CHECK:   alloca { i8*, i32, i32 }, align 8
+// CHECK:   alloca { i8*, i32, i32, i32 }, align 8
 // CHECK:   store i8* bitcast (void (%{{.*}}*)* @"\01?foo@Single@@QAEXXZ" to i8*), i8** %{{.*}}, align 4
 // CHECK:   store { i8*, i32 }
 // CHECK:     { i8* bitcast (void (%{{.*}}*)* @"\01?foo@Multiple@@QAEXXZ" to i8*), i32 0 },
-// CHECK:     { i8*, i32 }* %{{.*}}, align 4
+// CHECK:     { i8*, i32 }* %{{.*}}, align 8
 // CHECK:   store { i8*, i32, i32 }
 // CHECK:     { i8* bitcast (void (%{{.*}}*)* @"\01?foo@Virtual@@QAEXXZ" to i8*), i32 0, i32 0 },
-// CHECK:     { i8*, i32, i32 }* %{{.*}}, align 4
+// CHECK:     { i8*, i32, i32 }* %{{.*}}, align 8
 // CHECK:   store { i8*, i32, i32, i32 }
 // CHECK:     { i8* bitcast (void (%{{.*}}*)* @"\01?foo@Unspecified@@QAEXXZ" to i8*), i32 0, i32 12, i32 0 },
-// CHECK:     { i8*, i32, i32, i32 }* %{{.*}}, align 4
+// CHECK:     { i8*, i32, i32, i32 }* %{{.*}}, align 8
 // CHECK:   store { i8*, i32, i32, i32 }
 // CHECK:     { i8* bitcast (void (%{{.*}}*)* @"\01?foo@UnspecWithVBPtr@@QAEXXZ" to i8*),
 // CHECK:       i32 0, i32 4, i32 0 },
-// CHECK:     { i8*, i32, i32, i32 }* %{{.*}}, align 4
+// CHECK:     { i8*, i32, i32, i32 }* %{{.*}}, align 8
 // CHECK:   ret void
 // CHECK: }
 }
@@ -219,9 +219,9 @@ void polymorphicMemPtrs() {
 bool nullTestDataUnspecified(int Unspecified::*mp) {
   return mp;
 // CHECK: define zeroext i1 @"\01?nullTestDataUnspecified@@YA_NPQUnspecified@@H@Z"{{.*}} {
-// CHECK:   %{{.*}} = load { i32, i32, i32 }* %{{.*}}, align 4
-// CHECK:   store { i32, i32, i32 } {{.*}} align 4
-// CHECK:   %[[mp:.*]] = load { i32, i32, i32 }* %{{.*}}, align 4
+// CHECK:   %{{.*}} = load { i32, i32, i32 }* %{{.*}}, align 8
+// CHECK:   store { i32, i32, i32 } {{.*}} align 8
+// CHECK:   %[[mp:.*]] = load { i32, i32, i32 }* %{{.*}}, align 8
 // CHECK:   %[[mp0:.*]] = extractvalue { i32, i32, i32 } %[[mp]], 0
 // CHECK:   %[[cmp0:.*]] = icmp ne i32 %[[mp0]], 0
 // CHECK:   %[[mp1:.*]] = extractvalue { i32, i32, i32 } %[[mp]], 1
@@ -237,9 +237,9 @@ bool nullTestDataUnspecified(int Unspecified::*mp) {
 bool nullTestFunctionUnspecified(void (Unspecified::*mp)()) {
   return mp;
 // CHECK: define zeroext i1 @"\01?nullTestFunctionUnspecified@@YA_NP8Unspecified@@AEXXZ@Z"{{.*}} {
-// CHECK:   %{{.*}} = load { i8*, i32, i32, i32 }* %{{.*}}, align 4
-// CHECK:   store { i8*, i32, i32, i32 } {{.*}} align 4
-// CHECK:   %[[mp:.*]] = load { i8*, i32, i32, i32 }* %{{.*}}, align 4
+// CHECK:   %{{.*}} = load { i8*, i32, i32, i32 }* %{{.*}}, align 8
+// CHECK:   store { i8*, i32, i32, i32 } {{.*}} align 8
+// CHECK:   %[[mp:.*]] = load { i8*, i32, i32, i32 }* %{{.*}}, align 8
 // CHECK:   %[[mp0:.*]] = extractvalue { i8*, i32, i32, i32 } %[[mp]], 0
 // CHECK:   %[[cmp0:.*]] = icmp ne i8* %[[mp0]], null
 // CHECK:   ret i1 %[[cmp0]]
@@ -252,7 +252,7 @@ int loadDataMemberPointerVirtual(Virtual *o, int Virtual::*memptr) {
 // data pointer.
 // CHECK: define i32 @"\01?loadDataMemberPointerVirtual@@YAHPAUVirtual@@PQ1@H@Z"{{.*}} {
 // CHECK:   %[[o:.*]] = load %{{.*}}** %{{.*}}, align 4
-// CHECK:   %[[memptr:.*]] = load { i32, i32 }* %{{.*}}, align 4
+// CHECK:   %[[memptr:.*]] = load { i32, i32 }* %{{.*}}, align 8
 // CHECK:   %[[memptr0:.*]] = extractvalue { i32, i32 } %[[memptr:.*]], 0
 // CHECK:   %[[memptr1:.*]] = extractvalue { i32, i32 } %[[memptr:.*]], 1
 // CHECK:   %[[v6:.*]] = bitcast %{{.*}}* %[[o]] to i8*
@@ -276,7 +276,7 @@ int loadDataMemberPointerUnspecified(Unspecified *o, int Unspecified::*memptr) {
 // data pointer.
 // CHECK: define i32 @"\01?loadDataMemberPointerUnspecified@@YAHPAUUnspecified@@PQ1@H@Z"{{.*}} {
 // CHECK:   %[[o:.*]] = load %{{.*}}** %{{.*}}, align 4
-// CHECK:   %[[memptr:.*]] = load { i32, i32, i32 }* %{{.*}}, align 4
+// CHECK:   %[[memptr:.*]] = load { i32, i32, i32 }* %{{.*}}, align 8
 // CHECK:   %[[memptr0:.*]] = extractvalue { i32, i32, i32 } %[[memptr:.*]], 0
 // CHECK:   %[[memptr1:.*]] = extractvalue { i32, i32, i32 } %[[memptr:.*]], 1
 // CHECK:   %[[memptr2:.*]] = extractvalue { i32, i32, i32 } %[[memptr:.*]], 2
@@ -462,7 +462,7 @@ void (B2::*convertMultipleFuncToB2(void (Multiple::*mp)()))() {
 //
 // CHECK: define i32 @"\01?convertMultipleFuncToB2@@YAP8B2@@AEXXZP8Multiple@@AEXXZ@Z"{{.*}} {
 // CHECK:   store
-// CHECK:   %[[src:.*]] = load { i8*, i32 }* %{{.*}}, align 4
+// CHECK:   %[[src:.*]] = load { i8*, i32 }* %{{.*}}, align 8
 // CHECK:   extractvalue { i8*, i32 } %[[src]], 0
 // CHECK:   icmp ne i8* %{{.*}}, null
 // CHECK:   br i1 %{{.*}}, label %{{.*}}, label %{{.*}}
@@ -487,7 +487,7 @@ void (D::*convertCToD(void (C::*mp)()))() {
   return mp;
 // CHECK: define void @"\01?convertCToD@Test1@@YAP8D@1@AEXXZP8C@1@AEXXZ@Z"{{.*}} {
 // CHECK:   store
-// CHECK:   load { i8*, i32, i32 }* %{{.*}}, align 4
+// CHECK:   load { i8*, i32, i32 }* %{{.*}}, align 8
 // CHECK:   extractvalue { i8*, i32, i32 } %{{.*}}, 0
 // CHECK:   icmp ne i8* %{{.*}}, null
 // CHECK:   br i1 %{{.*}}, label %{{.*}}, label %{{.*}}
diff --git a/clang/test/Layout/ms-x86-member-pointers.cpp b/clang/test/Layout/ms-x86-member-pointers.cpp
new file mode 100644
index 00000000000..89dd211a351
--- /dev/null
+++ b/clang/test/Layout/ms-x86-member-pointers.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fno-rtti -emit-llvm-only -triple i686-pc-win32 -fdump-record-layouts -fms-extensions -fsyntax-only %s 2>&1 | FileCheck %s
+
+struct __single_inheritance S;
+struct __multiple_inheritance M;
+struct __virtual_inheritance V;
+struct U;
+
+struct SD { char a; int S::*mp; };
+struct MD { char a; int M::*mp; };
+struct VD { char a; int V::*mp; };
+struct UD { char a; int U::*mp; };
+struct SF { char a; int (S::*mp)(); };
+struct MF { char a; int (M::*mp)(); };
+struct VF { char a; int (V::*mp)(); };
+struct UF { char a; int (U::*mp)(); };
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct SD
+// CHECK-NEXT:    0 |   char a
+// CHECK-NEXT:    4 |   int struct S::* mp
+// CHECK-NEXT:      | [sizeof=8, align=4
+// CHECK-NEXT:      |  nvsize=8, nvalign=4]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct MD
+// CHECK-NEXT:    0 |   char a
+// CHECK-NEXT:    4 |   int struct M::* mp
+// CHECK-NEXT:      | [sizeof=8, align=4
+// CHECK-NEXT:      |  nvsize=8, nvalign=4]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct VD
+// CHECK-NEXT:    0 |   char a
+// CHECK-NEXT:    8 |   int struct V::* mp
+// CHECK-NEXT:      | [sizeof=16, align=8
+// CHECK-NEXT:      |  nvsize=16, nvalign=8]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct UD
+// CHECK-NEXT:    0 |   char a
+// CHECK-NEXT:    8 |   int struct U::* mp
+// CHECK-NEXT:      | [sizeof=24, align=8
+// CHECK-NEXT:      |  nvsize=24, nvalign=8]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct SF
+// CHECK-NEXT:    0 |   char a
+// CHECK-NEXT:    4 |   int (struct S::*)(void) __attribute__((thiscall)) mp
+// CHECK-NEXT:      | [sizeof=8, align=4
+// CHECK-NEXT:      |  nvsize=8, nvalign=4]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct MF
+// CHECK-NEXT:    0 |   char a
+// CHECK-NEXT:    8 |   int (struct M::*)(void) __attribute__((thiscall)) mp
+// CHECK-NEXT:      | [sizeof=16, align=8
+// CHECK-NEXT:      |  nvsize=16, nvalign=8]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct VF
+// CHECK-NEXT:    0 |   char a
+// CHECK-NEXT:    8 |   int (struct V::*)(void) __attribute__((thiscall)) mp
+// CHECK-NEXT:      | [sizeof=24, align=8
+// CHECK-NEXT:      |  nvsize=24, nvalign=8]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:    0 | struct UF
+// CHECK-NEXT:    0 |   char a
+// CHECK-NEXT:    8 |   int (struct U::*)(void) __attribute__((thiscall)) mp
+// CHECK-NEXT:      | [sizeof=24, align=8
+// CHECK-NEXT:      |  nvsize=24, nvalign=8]
+
+char a[sizeof(SD) +
+       sizeof(MD) +
+       sizeof(VD) +
+       sizeof(UD) +
+       sizeof(SF) +
+       sizeof(MF) +
+       sizeof(VF) +
+       sizeof(UF)];
diff --git a/clang/test/SemaCXX/member-pointer-ms.cpp b/clang/test/SemaCXX/member-pointer-ms.cpp
index ca37e07496a..aa3caa74285 100644
--- a/clang/test/SemaCXX/member-pointer-ms.cpp
+++ b/clang/test/SemaCXX/member-pointer-ms.cpp
@@ -19,15 +19,29 @@ struct Foo {
 };
 
 enum {
+  kSingleDataAlign             = 1 * sizeof(int),
+  kSingleFunctionAlign         = 1 * sizeof(void *),
+  kMultipleDataAlign           = 1 * sizeof(int),
+  // Everything with more than 1 field is 8 byte aligned, except virtual data
+  // member pointers on x64 (ugh).
+  kMultipleFunctionAlign       = 8,
+#ifdef _M_X64
+  kVirtualDataAlign            = 4,
+#else
+  kVirtualDataAlign            = 8,
+#endif
+  kVirtualFunctionAlign        = 8,
+  kUnspecifiedDataAlign        = 8,
+  kUnspecifiedFunctionAlign    = 8,
+
   kSingleDataSize             = 1 * sizeof(int),
   kSingleFunctionSize         = 1 * sizeof(void *),
   kMultipleDataSize           = 1 * sizeof(int),
   kMultipleFunctionSize       = 2 * sizeof(void *),
   kVirtualDataSize            = 2 * sizeof(int),
   kVirtualFunctionSize        = 2 * sizeof(int) + 1 * sizeof(void *),
-  // Unspecified is weird, it's 1 more slot than virtual.
-  kUnspecifiedDataSize        = kVirtualDataSize + 1 * sizeof(int),
-  kUnspecifiedFunctionSize    = kVirtualFunctionSize + 1 * sizeof(void *),
+  kUnspecifiedDataSize        = 3 * sizeof(int),
+  kUnspecifiedFunctionSize    = 2 * sizeof(int) + 2 * sizeof(void *),
 };
 
 // incomplete types
@@ -41,6 +55,13 @@ static_assert(sizeof(void (IncSingle::*)())   == kSingleFunctionSize, "");
 static_assert(sizeof(void (IncMultiple::*)()) == kMultipleFunctionSize, "");
 static_assert(sizeof(void (IncVirtual::*)())  == kVirtualFunctionSize, "");
 
+static_assert(__alignof(int IncSingle::*)        == kSingleDataAlign, "");
+static_assert(__alignof(int IncMultiple::*)      == kMultipleDataAlign, "");
+static_assert(__alignof(int IncVirtual::*)       == kVirtualDataAlign, "");
+static_assert(__alignof(void (IncSingle::*)())   == kSingleFunctionAlign, "");
+static_assert(__alignof(void (IncMultiple::*)()) == kMultipleFunctionAlign, "");
+static_assert(__alignof(void (IncVirtual::*)())  == kVirtualFunctionAlign, "");
+
 // An incomplete type with an unspecified inheritance model seems to take one
 // more slot than virtual.  It's not clear what it's used for yet.
 class IncUnspecified;