[PATCH] [ms-cxxabi] Correctly compute the size of member pointers

[Reid Kleckner]

Hi rjmccall,

This also relaxes the requirement on Windows that the member pointer
class type be a complete type (http://llvm.org/PR12070).  We still ask
for a complete type to instantiate any templates (MSVC does this), but
if that fails we continue as normal, relying on any inheritance
attributes on the declaration.

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

Files:
  lib/AST/MicrosoftCXXABI.cpp
  lib/Sema/SemaType.cpp
  test/SemaCXX/member-pointer-ms.cpp

Index: lib/AST/MicrosoftCXXABI.cpp
===================================================================
--- lib/AST/MicrosoftCXXABI.cpp
+++ lib/AST/MicrosoftCXXABI.cpp
@@ -13,6 +13,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CXXABI.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/RecordLayout.h"
@@ -52,17 +53,57 @@
 };
 }
 
+// getNumBases() seems to only give us the number of direct bases, and not the
+// total.  This function tells us if we inherit from anybody that uses MI.
+static bool recordHasMultipleInheritance(const CXXRecordDecl *RD) {
+  while (RD->getNumBases() > 0) {
+    if (RD->getNumBases() > 1)
+      return true;
+    assert(RD->getNumBases() == 1);
+    RD = RD->bases_begin()->getType()->getAsCXXRecordDecl();
+  }
+  return false;
+}
+
 unsigned MicrosoftCXXABI::getMemberPointerSize(const MemberPointerType *MPT) const {
   QualType Pointee = MPT->getPointeeType();
   CXXRecordDecl *RD = MPT->getClass()->getAsCXXRecordDecl();
-  if (RD->getNumVBases() > 0) {
-    if (Pointee->isFunctionType())
-      return 3;
+  attr::Kind Inheritance;
+
+  if (RD->getTypeForDecl()->isIncompleteType()) {
+    if (Attr *IA = RD->getAttr<SingleInheritanceAttr>())
+      Inheritance = IA->getKind();
+    else if (Attr *IA = RD->getAttr<MultipleInheritanceAttr>())
+      Inheritance = IA->getKind();
+    else if (Attr *IA = RD->getAttr<VirtualInheritanceAttr>())
+      Inheritance = IA->getKind();
+    else {
+      // This last case seems like an error, but MSVC allocates one more slot
+      // than is used for virtual inheritance.  We follow suit for
+      // compatibility.
+      // FIXME: Issue a warning.
+      return 3 + int(Pointee->isFunctionType());
+    }
+  } else {
+    if (RD->getNumVBases() > 0)
+      Inheritance = attr::VirtualInheritance;
+    else if (recordHasMultipleInheritance(RD))
+      Inheritance = attr::MultipleInheritance;
     else
-      return 2;
-  } else if (RD->getNumBases() > 1 && Pointee->isFunctionType())
-    return 2;
-  return 1;
+      Inheritance = attr::SingleInheritance;
+  }
+
+  switch (Inheritance) {
+  case attr::SingleInheritance:
+    return 1;
+  case attr::MultipleInheritance:
+    return 1 + int(Pointee->isFunctionType());
+  case attr::VirtualInheritance:
+    return 2 + int(Pointee->isFunctionType());
+  default:
+    llvm_unreachable("unknown inheritance model");
+    return 0;
+  }
 }
 
 CXXABI *clang::CreateMicrosoftCXXABI(ASTContext &Ctx) {
Index: lib/Sema/SemaType.cpp
===================================================================
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -1719,13 +1719,11 @@
   // according to the class type, which means that we really need a
   // complete type if possible, which means we need to instantiate templates.
   //
-  // For now, just require a complete type, which will instantiate
-  // templates.  This will also error if the type is just forward-declared,
-  // which is a bug, but it's a bug that saves us from dealing with some
-  // complexities at the moment.
-  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
-      RequireCompleteType(Loc, Class, diag::err_incomplete_type))
-    return QualType();
+  // If template instantiation fails or the type is just incomplete, the ABI has
+  // a poorly understood fallback mechanism in place, so we continue without any
+  // diagnostics.
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft())
+    RequireCompleteType(Loc, Class, 0);
 
   return Context.getMemberPointerType(T, Class.getTypePtr());
 }
Index: test/SemaCXX/member-pointer-ms.cpp
===================================================================
--- test/SemaCXX/member-pointer-ms.cpp
+++ test/SemaCXX/member-pointer-ms.cpp
@@ -1,14 +1,74 @@
-// RUN: %clang_cc1 -cxx-abi microsoft -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++11 -cxx-abi microsoft -fms-compatibility -fsyntax-only -verify %s
+//
+// This file should also give no diagnostics when run through cl.exe from MSVS
+// 2012, which supports C++11 and static_assert.
+// expected-no-diagnostics
 
-// Test that we reject pointers to members of incomplete classes (for now)
-struct A; //expected-note{{forward declaration of 'A'}}
-int A::*pai1; //expected-error{{incomplete type 'A'}}
+// Test the size of various member pointer combinations:
+// - complete and incomplete
+// - single, multiple, and virtual inheritance (and unspecified for incomplete)
+// - data and function pointers
+// - templated with declared specializations with annotations
+// - template that can be instantiated
 
-// Test that we don't allow reinterpret_casts from pointers of one size to
-// pointers of a different size.
-struct A {};
-struct B {};
-struct C: A, B {};
+// Use std=c++11 for static_assert.
 
-void (A::*paf)();
-void (C::*pcf)() = reinterpret_cast<void (C::*)()>(paf); //expected-error{{cannot reinterpret_cast from member pointer type}}
+// http://llvm.org/PR12070
+struct Foo {
+  typedef int Foo::*FooInt;
+  int f;
+};
+
+// incomplete types
+class __single_inheritance IncSingle;
+class __multiple_inheritance IncMultiple;
+class __virtual_inheritance IncVirtual;
+static_assert(sizeof(int IncSingle::*)   == 1 * sizeof(void *), "");
+static_assert(sizeof(int IncMultiple::*) == 1 * sizeof(void *), "");
+static_assert(sizeof(int IncVirtual::*)  == 2 * sizeof(void *), "");
+static_assert(sizeof(void (IncSingle::*)())   == 1 * sizeof(void *), "");
+static_assert(sizeof(void (IncMultiple::*)()) == 2 * sizeof(void *), "");
+static_assert(sizeof(void (IncVirtual::*)())  == 3 * sizeof(void *), "");
+
+// 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;
+static_assert(sizeof(int IncUnspecified::*) == 3 * sizeof(void *), "");
+static_assert(sizeof(void (IncUnspecified::*)()) == 4 * sizeof(void *), "");
+
+// complete types
+struct B1 { };
+struct B2 { };
+struct Single { };
+struct Multiple : B1, B2 { };
+struct Virtual : virtual B1 { };
+static_assert(sizeof(int Single::*)   == 1 * sizeof(void *), "");
+static_assert(sizeof(int Multiple::*) == 1 * sizeof(void *), "");
+static_assert(sizeof(int Virtual::*)  == 2 * sizeof(void *), "");
+static_assert(sizeof(void (Single::*)())   == 1 * sizeof(void *), "");
+static_assert(sizeof(void (Multiple::*)()) == 2 * sizeof(void *), "");
+static_assert(sizeof(void (Virtual::*)())  == 3 * sizeof(void *), "");
+
+// Test both declared and defined templates.
+template <typename T> class X;
+template <> class __single_inheritance   X<IncSingle>;
+template <> class __multiple_inheritance X<IncMultiple>;
+template <> class __virtual_inheritance  X<IncVirtual>;
+// Don't declare X<IncUnspecified>.
+static_assert(sizeof(int X<IncSingle>::*)           == 1 * sizeof(void *), "");
+static_assert(sizeof(int X<IncMultiple>::*)         == 1 * sizeof(void *), "");
+static_assert(sizeof(int X<IncVirtual>::*)          == 2 * sizeof(void *), "");
+static_assert(sizeof(int X<IncUnspecified>::*)      == 3 * sizeof(void *), "");
+static_assert(sizeof(void (X<IncSingle>::*)())      == 1 * sizeof(void *), "");
+static_assert(sizeof(void (X<IncMultiple>::*)())    == 2 * sizeof(void *), "");
+static_assert(sizeof(void (X<IncVirtual>::*)())     == 3 * sizeof(void *), "");
+static_assert(sizeof(void (X<IncUnspecified>::*)()) == 4 * sizeof(void *), "");
+
+template <typename T>
+struct Y : T { };
+static_assert(sizeof(int Y<Single>::*)        == 1 * sizeof(void *), "");
+static_assert(sizeof(int Y<Multiple>::*)      == 1 * sizeof(void *), "");
+static_assert(sizeof(int Y<Virtual>::*)       == 2 * sizeof(void *), "");
+static_assert(sizeof(void (Y<Single>::*)())   == 1 * sizeof(void *), "");
+static_assert(sizeof(void (Y<Multiple>::*)()) == 2 * sizeof(void *), "");
+static_assert(sizeof(void (Y<Virtual>::*)())  == 3 * sizeof(void *), "");
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D568.1.patch
Type: text/x-patch
Size: 7987 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130321/e8a8264f/attachment.bin>