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

Reid Kleckner reid at kleckner.net
Thu Mar 28 13:02:56 PDT 2013


Author: rnk
Date: Thu Mar 28 15:02:56 2013
New Revision: 178283

URL: http://llvm.org/viewvc/llvm-project?rev=178283&view=rev
Log:
[ms-cxxabi] Correctly compute the size of member pointers

Summary:
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.

Reviewers: rjmccall

CC: triton, timurrrr, cfe-commits

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

Modified:
    cfe/trunk/lib/AST/ASTContext.cpp
    cfe/trunk/lib/AST/CXXABI.h
    cfe/trunk/lib/AST/ItaniumCXXABI.cpp
    cfe/trunk/lib/AST/MicrosoftCXXABI.cpp
    cfe/trunk/lib/Sema/SemaType.cpp
    cfe/trunk/test/SemaCXX/member-pointer-ms.cpp

Modified: cfe/trunk/lib/AST/ASTContext.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=178283&r1=178282&r2=178283&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ASTContext.cpp (original)
+++ cfe/trunk/lib/AST/ASTContext.cpp Thu Mar 28 15:02:56 2013
@@ -1496,10 +1496,7 @@ ASTContext::getTypeInfoImpl(const Type *
   }
   case Type::MemberPointer: {
     const MemberPointerType *MPT = cast<MemberPointerType>(T);
-    std::pair<uint64_t, unsigned> PtrDiffInfo =
-      getTypeInfo(getPointerDiffType());
-    Width = PtrDiffInfo.first * ABI->getMemberPointerSize(MPT);
-    Align = PtrDiffInfo.second;
+    llvm::tie(Width, Align) = ABI->getMemberPointerWidthAndAlign(MPT);
     break;
   }
   case Type::Complex: {

Modified: cfe/trunk/lib/AST/CXXABI.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/CXXABI.h?rev=178283&r1=178282&r2=178283&view=diff
==============================================================================
--- cfe/trunk/lib/AST/CXXABI.h (original)
+++ cfe/trunk/lib/AST/CXXABI.h Thu Mar 28 15:02:56 2013
@@ -27,9 +27,9 @@ class CXXABI {
 public:
   virtual ~CXXABI();
 
-  /// Returns the size of a member pointer in multiples of the target
-  /// pointer size.
-  virtual unsigned getMemberPointerSize(const MemberPointerType *MPT) const = 0;
+  /// Returns the width and alignment of a member pointer in bits.
+  virtual std::pair<uint64_t, unsigned>
+  getMemberPointerWidthAndAlign(const MemberPointerType *MPT) const = 0;
 
   /// Returns the default calling convention for C++ methods.
   virtual CallingConv getDefaultMethodCallConv(bool isVariadic) const = 0;

Modified: cfe/trunk/lib/AST/ItaniumCXXABI.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ItaniumCXXABI.cpp?rev=178283&r1=178282&r2=178283&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ItaniumCXXABI.cpp (original)
+++ cfe/trunk/lib/AST/ItaniumCXXABI.cpp Thu Mar 28 15:02:56 2013
@@ -33,10 +33,15 @@ protected:
 public:
   ItaniumCXXABI(ASTContext &Ctx) : Context(Ctx) { }
 
-  unsigned getMemberPointerSize(const MemberPointerType *MPT) const {
-    QualType Pointee = MPT->getPointeeType();
-    if (Pointee->isFunctionType()) return 2;
-    return 1;
+  std::pair<uint64_t, unsigned>
+  getMemberPointerWidthAndAlign(const MemberPointerType *MPT) const {
+    const TargetInfo &Target = Context.getTargetInfo();
+    TargetInfo::IntType PtrDiff = Target.getPtrDiffType(0);
+    uint64_t Width = Target.getTypeWidth(PtrDiff);
+    unsigned Align = Target.getTypeAlign(PtrDiff);
+    if (MPT->getPointeeType()->isFunctionType())
+      Width = 2 * Width;
+    return std::make_pair(Width, Align);
   }
 
   CallingConv getDefaultMethodCallConv(bool isVariadic) const {

Modified: cfe/trunk/lib/AST/MicrosoftCXXABI.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/MicrosoftCXXABI.cpp?rev=178283&r1=178282&r2=178283&view=diff
==============================================================================
--- cfe/trunk/lib/AST/MicrosoftCXXABI.cpp (original)
+++ cfe/trunk/lib/AST/MicrosoftCXXABI.cpp Thu Mar 28 15:02:56 2013
@@ -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"
@@ -27,7 +28,8 @@ class MicrosoftCXXABI : public CXXABI {
 public:
   MicrosoftCXXABI(ASTContext &Ctx) : Context(Ctx) { }
 
-  unsigned getMemberPointerSize(const MemberPointerType *MPT) const;
+  std::pair<uint64_t, unsigned>
+  getMemberPointerWidthAndAlign(const MemberPointerType *MPT) const;
 
   CallingConv getDefaultMethodCallConv(bool isVariadic) const {
     if (!isVariadic && Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
@@ -52,17 +54,77 @@ public:
 };
 }
 
-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;
-    else
-      return 2;
-  } else if (RD->getNumBases() > 1 && Pointee->isFunctionType())
-    return 2;
-  return 1;
+// 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, or if
+// we have a non-primary base class, which uses the multiple inheritance model.
+static bool usesMultipleInheritanceModel(const CXXRecordDecl *RD) {
+  while (RD->getNumBases() > 0) {
+    if (RD->getNumBases() > 1)
+      return true;
+    assert(RD->getNumBases() == 1);
+    const CXXRecordDecl *Base = RD->bases_begin()->getType()->getAsCXXRecordDecl();
+    if (RD->isPolymorphic() && !Base->isPolymorphic())
+      return true;
+    RD = Base;
+  }
+  return false;
+}
+
+std::pair<uint64_t, unsigned>
+MicrosoftCXXABI::getMemberPointerWidthAndAlign(const MemberPointerType *MPT) const {
+  const CXXRecordDecl *RD = MPT->getClass()->getAsCXXRecordDecl();
+  const TargetInfo &Target = Context.getTargetInfo();
+
+  assert(Target.getTriple().getArch() == llvm::Triple::x86 ||
+         Target.getTriple().getArch() == llvm::Triple::x86_64);
+
+  Attr *IA = RD->getAttr<MSInheritanceAttr>();
+  attr::Kind Inheritance;
+  if (IA) {
+    Inheritance = IA->getKind();
+  } else if (RD->getNumVBases() > 0) {
+    Inheritance = attr::VirtualInheritance;
+  } else if (MPT->getPointeeType()->isFunctionType() &&
+             usesMultipleInheritanceModel(RD)) {
+    Inheritance = attr::MultipleInheritance;
+  } else {
+    Inheritance = attr::SingleInheritance;
+  }
+
+  unsigned PtrSize = Target.getPointerWidth(0);
+  unsigned IntSize = Target.getIntWidth();
+  uint64_t Width;
+  unsigned Align;
+  if (MPT->getPointeeType()->isFunctionType()) {
+    // Member function pointers are a struct of a function pointer followed by a
+    // variable number of ints depending on the inheritance model used.  The
+    // function pointer is a real function if it is non-virtual and a vftable
+    // slot thunk if it is virtual.  The ints select the object base passed for
+    // the 'this' pointer.
+    Align = Target.getPointerAlign(0);
+    switch (Inheritance) {
+    case attr::SingleInheritance:      Width = PtrSize;               break;
+    case attr::MultipleInheritance:    Width = PtrSize + 1 * IntSize; break;
+    case attr::VirtualInheritance:     Width = PtrSize + 2 * IntSize; break;
+    case attr::UnspecifiedInheritance: Width = PtrSize + 3 * IntSize; break;
+    default: llvm_unreachable("unknown inheritance model");
+    }
+  } else {
+    // Data pointers are an aggregate of ints.  The first int is an offset
+    // followed by vbtable-related offsets.
+    Align = Target.getIntAlign();
+    switch (Inheritance) {
+    case attr::SingleInheritance:       // Same as multiple inheritance.
+    case attr::MultipleInheritance:     Width = 1 * IntSize; break;
+    case attr::VirtualInheritance:      Width = 2 * IntSize; break;
+    case attr::UnspecifiedInheritance:  Width = 3 * IntSize; break;
+    default: llvm_unreachable("unknown inheritance model");
+    }
+  }
+  Width = llvm::RoundUpToAlignment(Width, Align);
+
+  // FIXME: Verify that our alignment matches MSVC.
+  return std::make_pair(Width, Align);
 }
 
 CXXABI *clang::CreateMicrosoftCXXABI(ASTContext &Ctx) {

Modified: cfe/trunk/lib/Sema/SemaType.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaType.cpp?rev=178283&r1=178282&r2=178283&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaType.cpp (original)
+++ cfe/trunk/lib/Sema/SemaType.cpp Thu Mar 28 15:02:56 2013
@@ -1728,13 +1728,29 @@ QualType Sema::BuildMemberPointerType(Qu
   // 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, we have to
+  // add an extra slot to the member pointer.  Yes, this does cause problems
+  // when passing pointers between TUs that disagree about the size.
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+    CXXRecordDecl *RD = Class->getAsCXXRecordDecl();
+    if (!RD->hasAttr<MSInheritanceAttr>()) {
+      // Lock in the inheritance model on the first use of a member pointer.
+      // Otherwise we may disagree about the size at different points in the TU.
+      // FIXME: MSVC picks a model on the first use that needs to know the size,
+      // rather than on the first mention of the type, e.g. typedefs.
+      SourceRange DeclRange = RD->getSourceRange();
+      if (RequireCompleteType(Loc, Class, 0) && !RD->isBeingDefined()) {
+        // We know it doesn't have an attribute and it's incomplete, so use the
+        // unspecified inheritance model.  If we're in the record body, we can
+        // figure out the inheritance model.
+        for (CXXRecordDecl::redecl_iterator I = RD->redecls_begin(),
+             E = RD->redecls_end(); I != E; ++I) {
+          I->addAttr(::new (Context) UnspecifiedInheritanceAttr(
+              RD->getSourceRange(), Context));
+        }
+      }
+    }
+  }
 
   return Context.getMemberPointerType(T, Class.getTypePtr());
 }

Modified: cfe/trunk/test/SemaCXX/member-pointer-ms.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/member-pointer-ms.cpp?rev=178283&r1=178282&r2=178283&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/member-pointer-ms.cpp (original)
+++ cfe/trunk/test/SemaCXX/member-pointer-ms.cpp Thu Mar 28 15:02:56 2013
@@ -1,14 +1,134 @@
-// RUN: %clang_cc1 -cxx-abi microsoft -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++11 -cxx-abi microsoft -fms-compatibility -fsyntax-only -triple=i386-pc-win32 -verify %s
+// RUN: %clang_cc1 -std=c++11 -cxx-abi microsoft -fms-compatibility -fsyntax-only -triple=x86_64-pc-win32 -verify %s
+//
+// This file should also give no diagnostics when run through cl.exe from MSVS
+// 2012, which supports C++11 and static_assert.  It should pass for both 64-bit
+// and 32-bit x86.
+//
+// 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 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 {};
+// 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
 
-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;
+};
+
+enum {
+  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 *),
+};
+
+// incomplete types
+class __single_inheritance IncSingle;
+class __multiple_inheritance IncMultiple;
+class __virtual_inheritance IncVirtual;
+static_assert(sizeof(int IncSingle::*)        == kSingleDataSize, "");
+static_assert(sizeof(int IncMultiple::*)      == kMultipleDataSize, "");
+static_assert(sizeof(int IncVirtual::*)       == kVirtualDataSize, "");
+static_assert(sizeof(void (IncSingle::*)())   == kSingleFunctionSize, "");
+static_assert(sizeof(void (IncMultiple::*)()) == kMultipleFunctionSize, "");
+static_assert(sizeof(void (IncVirtual::*)())  == kVirtualFunctionSize, "");
+
+// 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::*) == kUnspecifiedDataSize, "");
+static_assert(sizeof(void (IncUnspecified::*)()) == kUnspecifiedFunctionSize, "");
+
+// complete types
+struct B1 { };
+struct B2 { };
+struct Single { };
+struct Multiple : B1, B2 { };
+struct Virtual : virtual B1 { };
+static_assert(sizeof(int Single::*)        == kSingleDataSize, "");
+static_assert(sizeof(int Multiple::*)      == kMultipleDataSize, "");
+static_assert(sizeof(int Virtual::*)       == kVirtualDataSize, "");
+static_assert(sizeof(void (Single::*)())   == kSingleFunctionSize, "");
+static_assert(sizeof(void (Multiple::*)()) == kMultipleFunctionSize, "");
+static_assert(sizeof(void (Virtual::*)())  == kVirtualFunctionSize, "");
+
+// 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>::*)           == kSingleDataSize, "");
+static_assert(sizeof(int X<IncMultiple>::*)         == kMultipleDataSize, "");
+static_assert(sizeof(int X<IncVirtual>::*)          == kVirtualDataSize, "");
+static_assert(sizeof(int X<IncUnspecified>::*)      == kUnspecifiedDataSize, "");
+static_assert(sizeof(void (X<IncSingle>::*)())      == kSingleFunctionSize, "");
+static_assert(sizeof(void (X<IncMultiple>::*)())    == kMultipleFunctionSize, "");
+static_assert(sizeof(void (X<IncVirtual>::*)())     == kVirtualFunctionSize, "");
+static_assert(sizeof(void (X<IncUnspecified>::*)()) == kUnspecifiedFunctionSize, "");
+
+template <typename T>
+struct Y : T { };
+static_assert(sizeof(int Y<Single>::*)        == kSingleDataSize, "");
+static_assert(sizeof(int Y<Multiple>::*)      == kMultipleDataSize, "");
+static_assert(sizeof(int Y<Virtual>::*)       == kVirtualDataSize, "");
+static_assert(sizeof(void (Y<Single>::*)())   == kSingleFunctionSize, "");
+static_assert(sizeof(void (Y<Multiple>::*)()) == kMultipleFunctionSize, "");
+static_assert(sizeof(void (Y<Virtual>::*)())  == kVirtualFunctionSize, "");
+
+struct A { int x; void bar(); };
+struct B : A { virtual void foo(); };
+static_assert(sizeof(int B::*) == kSingleDataSize, "");
+// A non-primary base class uses the multiple inheritance model for member
+// pointers.
+static_assert(sizeof(void (B::*)()) == kMultipleFunctionSize, "");
+
+struct AA { int x; virtual void foo(); };
+struct BB : AA { void bar(); };
+struct CC : BB { virtual void baz(); };
+static_assert(sizeof(void (CC::*)()) == kSingleFunctionSize, "");
+
+// We start out unspecified.
+struct ForwardDecl1;
+struct ForwardDecl2;
+
+// Re-declare to force us to iterate decls when adding attributes.
+struct ForwardDecl1;
+struct ForwardDecl2;
+
+typedef int ForwardDecl1::*MemPtr1;
+typedef int ForwardDecl2::*MemPtr2;
+MemPtr1 variable_forces_sizing;
+
+struct ForwardDecl1 : B {
+  virtual void foo();
+};
+struct ForwardDecl2 : B {
+  virtual void foo();
+};
+
+static_assert(sizeof(variable_forces_sizing) == kUnspecifiedDataSize, "");
+static_assert(sizeof(MemPtr1) == kUnspecifiedDataSize, "");
+// FIXME: Clang fails this assert because it locks in the inheritance model at
+// the point of the typedef instead of the first usage, while MSVC does not.
+//static_assert(sizeof(MemPtr2) == kSingleDataSize, "");
+
+struct MemPtrInBody {
+  typedef int MemPtrInBody::*MemPtr;
+  int a;
+  operator MemPtr() const {
+    return a ? &MemPtrInBody::a : 0;
+  }
+};
+
+static_assert(sizeof(MemPtrInBody::MemPtr) == kSingleDataSize, "");





More information about the cfe-commits mailing list