[Clang] Fix Microsoft ABI inheritance model when member pointer is used in a base specifier

clang/docs/ReleaseNotes.rst

@@ -95,6 +95,10 @@ ABI Changes in This Version
 - Fixed Microsoft calling convention when returning classes that have a deleted
   copy assignment operator. Such a class should be returned indirectly.
 
+- Fixed Microsoft layout of pointer-to-members of classes when the layout is needed
+  directly or indirectly by the base classes of a class. These should use the most
+  general unspecified inheritance layout. Also affects -fcomplete-member-pointers.
+
 AST Dumping Potentially Breaking Changes
 ----------------------------------------
 

clang/include/clang/AST/DeclCXX.h

@@ -297,7 +297,8 @@ class CXXRecordDecl : public RecordDecl {
     LLVM_PREFERRED_TYPE(bool)
     unsigned IsLambda : 1;
 
-    /// Whether we are currently parsing base specifiers.
+    /// Whether we are currently parsing base specifiers; the
+    /// colon has been consumed but the beginning left brace hasn't.
     LLVM_PREFERRED_TYPE(bool)
     unsigned IsParsingBaseSpecifiers : 1;
 
@@ -598,7 +599,9 @@ class CXXRecordDecl : public RecordDecl {
     return !hasDefinition() || !isDynamicClass() || hasAnyDependentBases();
   }
 
-  void setIsParsingBaseSpecifiers() { data().IsParsingBaseSpecifiers = true; }
+  void setIsParsingBaseSpecifiers(bool to = true) {
+    data().IsParsingBaseSpecifiers = to;
+  }
 
   bool isParsingBaseSpecifiers() const {
     return data().IsParsingBaseSpecifiers;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -8052,6 +8052,8 @@ def err_bad_memptr_rhs : Error<
 def err_bad_memptr_lhs : Error<
   "left hand operand to %0 must be a %select{|pointer to }1class "
   "compatible with the right hand operand, but is %2">;
+def note_memptr_incomplete_until_bases : Note<
+  "this will affect the ABI of the member pointer until the bases have been specified">;
 def err_memptr_incomplete : Error<
   "member pointer has incomplete base type %0">;
 def warn_exception_caught_by_earlier_handler : Warning<

clang/lib/AST/DeclCXX.cpp

@@ -474,10 +474,8 @@ CXXRecordDecl::setBases(CXXBaseSpecifier const * const *Bases,
   if (data().IsStandardLayout && NumBases > 1 && hasRepeatedBaseClass(this))
     data().IsStandardLayout = false;
 
-  if (VBases.empty()) {
-    data().IsParsingBaseSpecifiers = false;
+  if (VBases.empty())
     return;
-  }
 
   // Create base specifier for any direct or indirect virtual bases.
   data().VBases = new (C) CXXBaseSpecifier[VBases.size()];
@@ -488,8 +486,6 @@ CXXRecordDecl::setBases(CXXBaseSpecifier const * const *Bases,
       addedClassSubobject(Type->getAsCXXRecordDecl());
     data().getVBases()[I] = *VBases[I];
   }
-
-  data().IsParsingBaseSpecifiers = false;
 }
 
 unsigned CXXRecordDecl::getODRHash() const {

clang/lib/Parse/ParseDeclCXX.cpp

@@ -2332,6 +2332,28 @@ void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,
   }
 }
 
+namespace {
+struct ParsingBaseSpecifiersGuard {
+  CXXRecordDecl *RD = nullptr;
+  ParsingBaseSpecifiersGuard(Sema &S, Decl *D) {
+    if (D) {
+      S.AdjustDeclIfTemplate(D);
+      RD = cast<CXXRecordDecl>(D);
+      assert(!RD->isParsingBaseSpecifiers() &&
+             "Recursively parsing base specifiers of the same class?");
+      RD->setIsParsingBaseSpecifiers(true);
+    }
+  }
+  ~ParsingBaseSpecifiersGuard() {
+    if (RD) {
+      assert(RD->isParsingBaseSpecifiers() &&
+             "Stopped parsing base specifiers before exiting ParseBaseClause?");
+      RD->setIsParsingBaseSpecifiers(false);
+    }
+  }
+};
+} // namespace
+
 /// ParseBaseClause - Parse the base-clause of a C++ class [C++ class.derived].
 ///
 ///       base-clause : [C++ class.derived]
@@ -2343,6 +2365,8 @@ void Parser::ParseBaseClause(Decl *ClassDecl) {
   assert(Tok.is(tok::colon) && "Not a base clause");
   ConsumeToken();
 
+  ParsingBaseSpecifiersGuard Guard(Actions, ClassDecl);
+
   // Build up an array of parsed base specifiers.
   SmallVector<CXXBaseSpecifier *, 8> BaseInfo;
 

clang/lib/Sema/SemaCast.cpp

@@ -1795,7 +1795,8 @@ TryStaticMemberPointerUpcast(Sema &Self, ExprResult &SrcExpr, QualType SrcType,
 
   // Lock down the inheritance model right now in MS ABI, whether or not the
   // pointee types are the same.
-  if (Self.Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+  if (Self.Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+      Self.getLangOpts().CompleteMemberPointers) {
     (void)Self.isCompleteType(OpRange.getBegin(), SrcType);
     (void)Self.isCompleteType(OpRange.getBegin(), DestType);
   }
@@ -2337,7 +2338,8 @@ static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr,
         SrcMemPtr->isMemberFunctionPointer())
       return TC_NotApplicable;
 
-    if (Self.Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+    if (Self.Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+        Self.getLangOpts().CompleteMemberPointers) {
       // We need to determine the inheritance model that the class will use if
       // haven't yet.
       (void)Self.isCompleteType(OpRange.getBegin(), SrcType);

clang/lib/Sema/SemaDeclCXX.cpp

@@ -2816,9 +2816,6 @@ BaseResult Sema::ActOnBaseSpecifier(Decl *classdecl, SourceRange SpecifierRange,
   if (!Class)
     return true;
 
-  // We haven't yet attached the base specifiers.
-  Class->setIsParsingBaseSpecifiers();
-
   // We do not support any C++11 attributes on base-specifiers yet.
   // Diagnose any attributes we see.
   for (const ParsedAttr &AL : Attributes) {

clang/lib/Sema/SemaExpr.cpp

@@ -728,7 +728,8 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E) {
 
   // Under the MS ABI, lock down the inheritance model now.
   if (T->isMemberPointerType() &&
-      Context.getTargetInfo().getCXXABI().isMicrosoft())
+      (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+       getLangOpts().CompleteMemberPointers))
     (void)isCompleteType(E->getExprLoc(), T);
 
   ExprResult Res = CheckLValueToRValueConversionOperand(E);
@@ -14208,7 +14209,8 @@ QualType Sema::CheckAddressOfOperand(ExprResult &OrigOp, SourceLocation OpLoc) {
     QualType MPTy = Context.getMemberPointerType(
         op->getType(), Context.getTypeDeclType(MD->getParent()).getTypePtr());
     // Under the MS ABI, lock down the inheritance model now.
-    if (Context.getTargetInfo().getCXXABI().isMicrosoft())
+    if (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+        getLangOpts().CompleteMemberPointers)
       (void)isCompleteType(OpLoc, MPTy);
     return MPTy;
   } else if (lval != Expr::LV_Valid && lval != Expr::LV_IncompleteVoidType) {
@@ -14288,7 +14290,8 @@ QualType Sema::CheckAddressOfOperand(ExprResult &OrigOp, SourceLocation OpLoc) {
               op->getType(),
               Context.getTypeDeclType(cast<RecordDecl>(Ctx)).getTypePtr());
           // Under the MS ABI, lock down the inheritance model now.
-          if (Context.getTargetInfo().getCXXABI().isMicrosoft())
+          if (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+              getLangOpts().CompleteMemberPointers)
             (void)isCompleteType(OpLoc, MPTy);
           return MPTy;
         }

clang/lib/Sema/SemaExprCXX.cpp

@@ -4679,7 +4679,8 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType,
 
     // We may not have been able to figure out what this member pointer resolved
     // to up until this exact point.  Attempt to lock-in it's inheritance model.
-    if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+    if (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+        getLangOpts().CompleteMemberPointers) {
       (void)isCompleteType(From->getExprLoc(), From->getType());
       (void)isCompleteType(From->getExprLoc(), ToType);
     }

clang/lib/Sema/SemaOverload.cpp

@@ -16442,7 +16442,8 @@ ExprResult Sema::FixOverloadedFunctionReference(Expr *E, DeclAccessPair Found,
         QualType MemPtrType
           = Context.getMemberPointerType(Fn->getType(), ClassType.getTypePtr());
         // Under the MS ABI, lock down the inheritance model now.
-        if (Context.getTargetInfo().getCXXABI().isMicrosoft())
+        if (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+            getLangOpts().CompleteMemberPointers)
           (void)isCompleteType(UnOp->getOperatorLoc(), MemPtrType);
 
         return UnaryOperator::Create(Context, SubExpr.get(), UO_AddrOf,

clang/lib/Sema/SemaType.cpp

@@ -2117,7 +2117,8 @@ QualType Sema::BuildArrayType(QualType T, ArraySizeModifier ASM,
 
     // Mentioning a member pointer type for an array type causes us to lock in
     // an inheritance model, even if it's inside an unused typedef.
-    if (Context.getTargetInfo().getCXXABI().isMicrosoft())
+    if (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+        getLangOpts().CompleteMemberPointers)
       if (const MemberPointerType *MPTy = T->getAs<MemberPointerType>())
         if (!MPTy->getClass()->isDependentType())
           (void)isCompleteType(Loc, T);
@@ -9022,17 +9023,26 @@ bool Sema::RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
 
   if (const MemberPointerType *MPTy = T->getAs<MemberPointerType>()) {
     if (!MPTy->getClass()->isDependentType()) {
-      if (getLangOpts().CompleteMemberPointers &&
-          !MPTy->getClass()->getAsCXXRecordDecl()->isBeingDefined() &&
-          RequireCompleteType(Loc, QualType(MPTy->getClass(), 0), Kind,
-                              diag::err_memptr_incomplete))
-        return true;
-
       // We lock in the inheritance model once somebody has asked us to ensure
       // that a pointer-to-member type is complete.
-      if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
-        (void)isCompleteType(Loc, QualType(MPTy->getClass(), 0));
-        assignInheritanceModel(*this, MPTy->getMostRecentCXXRecordDecl());
+      if (Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+          getLangOpts().CompleteMemberPointers) {
+        QualType Class = QualType(MPTy->getClass(), 0);
+        (void)isCompleteType(Loc, Class);
+        CXXRecordDecl *RD = MPTy->getMostRecentCXXRecordDecl();
+        assignInheritanceModel(*this, RD);
+
+        if (getLangOpts().CompleteMemberPointers &&
+            RD->getMSInheritanceModel() == MSInheritanceModel::Unspecified) {
+          if (RD->hasDefinition() && RD->isParsingBaseSpecifiers()) {
+            Diag(Loc, diag::err_memptr_incomplete) << Class;
+            Diag(RD->getDefinition()->getLocation(),
+                 diag::note_memptr_incomplete_until_bases);
+          } else if (!RequireCompleteType(Loc, Class,
+                                          diag::err_memptr_incomplete))
+            Diag(Loc, diag::err_memptr_incomplete) << Class;
+          return true;
+        }
       }
     }
   }

clang/test/SemaCXX/complete-member-pointers.cpp

@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -verify -fsyntax-only -fcomplete-member-pointers %s
+// RUN: %clang_cc1 -verify -fsyntax-only -fc++-abi=itanium -fms-extensions -fcomplete-member-pointers %s
+// RUN: %clang_cc1 -verify -fsyntax-only -triple=x86_64-unknown-win32 -fc++-abi=microsoft -fms-compatibility -fcomplete-member-pointers %s
+// RUN: %clang_cc1 -verify -fsyntax-only -triple=x86_64-unknown-win32 -fc++-abi=itanium -fms-compatibility -fcomplete-member-pointers %s
 
 struct S; // expected-note {{forward declaration of 'S'}}
 typedef int S::*t;
@@ -13,3 +15,23 @@ template <typename T>
 struct S3 {
   int T::*foo;
 };
+
+struct __single_inheritance S4;
+int S4::* baz;
+
+template<int I> struct Base {};
+struct __single_inheritance S5 : Base<sizeof(int S5::*)> {};
+struct
+S6  // #S6
+:
+Base<sizeof(int S6::*)>
+// expected-error@-1 {{member pointer has incomplete base type 'S6'}}
+//   expected-note@#S6 {{this will affect the ABI of the member pointer until the bases have been specified}}
+{
+};
+
+template<typename T> struct S7 {};
+int S7<void>::* qux;  // #qux
+template<> struct S7<void> {};
+// expected-error@-1 {{explicit specialization of 'S7<void>' after instantiation}}
+//   expected-note@#qux {{implicit instantiation first required here}}

clang/test/SemaCXX/member-pointer-ms.cpp

@@ -215,6 +215,14 @@ struct NewUnspecified;
 SingleTemplate<void (IncSingle::*)()> tmpl_single;
 UnspecTemplate<void (NewUnspecified::*)()> tmpl_unspec;
 
+// Member pointers used in base specifiers force an unspecified inheritance model
+struct MemPtrInBase : UnspecTemplate<void (MemPtrInBase::*)()> {};
+#ifdef VMB
+struct __single_inheritance SpecifiedMemPtrInBase;
+struct SpecifiedMemPtrInBase : SingleTemplate<void (SpecifiedMemPtrInBase::*)()> {};
+struct __single_inheritance SpecifiedMemPtrInBase2 : SingleTemplate<void (SpecifiedMemPtrInBase2::*)()> {};
+#endif
+
 struct NewUnspecified { };
 
 static_assert(sizeof(void (NewUnspecified::*)()) == kUnspecifiedFunctionSize, "");