MS ABI: Diagnose unspecified inheritance model ptrs in the best case

[majnemer]

Summary

If the compiler is choosing pointer members using a best-case scheme,
diagnose when we select the unspecified representation. It is unlikely
that this is what the user intended.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

2514–2515

Maybe this should also mention that (a) this happened because the class type was incomplete at the point where the inheritance model was first required, and (b) a different inheritance model might be picked in a different TU?

[majnemer]

• Address review comments.

[majnemer]

Ping.

[rsmith]

lib/Sema/SemaType.cpp

5177–5179

I think I'd prefer to switch over these diagnostics, so the primary diagnostic says "I needed the inheritance model here, but the class was incomplete, so I picked one", and the note says "here's the forward declaration; you can add an explicit inheritance model here to suppress the diagnostic" along with a fixit.

include/clang/Basic/DiagnosticGroups.td

 def HeaderHygiene : DiagGroup<"header-hygiene">;
 def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
 def CompareDistinctPointerType : DiagGroup<"compare-distinct-pointer-types">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;
 def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
 def Varargs : DiagGroup<"varargs">;
 
 def Unsequenced : DiagGroup<"unsequenced">;
+def UnspecifiedInheritanceModel : DiagGroup<"ms-unspecified-inheritance-model">;
 // GCC name for -Wunsequenced
 def : DiagGroup<"sequence-point", [Unsequenced]>;
 
 // Preprocessor warnings.
 def AmbiguousMacro : DiagGroup<"ambiguous-macro">;
 
 // Just silence warnings about -Wstrict-aliasing for now.
 def : DiagGroup<"strict-aliasing=0">;

include/clang/Basic/DiagnosticSemaKinds.td

 def note_attribute : Note<"attribute is here">;
 def err_mismatched_ms_inheritance : Error<
   "inheritance model does not match %select{definition|previous declaration}0">;
 def warn_ignored_ms_inheritance : Warning<
   "inheritance model ignored on %select{primary template|partial specialization}0">,
   InGroup<IgnoredAttributes>;
 def note_previous_ms_inheritance : Note<
   "previous inheritance model specified here">;
+def warn_unspecified_ms_inheritance : Warning<
+  "incomplete classes use the 'full_generality' pointer-to-member "
+  "representation; a different translation unit may have chosen a different "
+  "representation">,
+  InGroup<UnspecifiedInheritanceModel>;
+def note_ms_inheritance_selected : Note<"inheritance model selected here">;
 def err_machine_mode : Error<"%select{unknown|unsupported}0 machine mode %1">;
 def err_mode_not_primitive : Error<
   "mode attribute only supported for integer and floating-point types">;
 def err_mode_wrong_type : Error<
   "type of machine mode does not match type of base type">;
 def err_attr_wrong_decl : Error<
   "%0 attribute invalid on this declaration, requires typedef or value">;
 def warn_attribute_nonnull_no_pointers : Warning<

lib/Sema/SemaType.cpp

static bool hasVisibleDefinition(Sema &S, NamedDecl *D, NamedDecl **Suggested) {
 
   // FIXME: If we merged any other decl into D, and that declaration is visible,
   // then we should consider a definition to be visible.
   *Suggested = D;
   return LookupResult::isVisible(S, D);
 }
 
 /// Locks in the inheritance model for the given class and all of its bases.
-static void assignInheritanceModel(Sema &S, CXXRecordDecl *RD) {
+static void assignInheritanceModel(Sema &S, CXXRecordDecl *RD,
+                                   SourceLocation Loc) {
   RD = RD->getMostRecentDecl();
   if (!RD->hasAttr<MSInheritanceAttr>()) {
     MSInheritanceAttr::Spelling IM;
 
     switch (S.MSPointerToMemberRepresentationMethod) {
     case LangOptions::PPTMK_BestCase:
       IM = RD->calculateInheritanceModel();
       break;
     case LangOptions::PPTMK_FullGeneralitySingleInheritance:
       IM = MSInheritanceAttr::Keyword_single_inheritance;
       break;
     case LangOptions::PPTMK_FullGeneralityMultipleInheritance:
       IM = MSInheritanceAttr::Keyword_multiple_inheritance;
       break;
     case LangOptions::PPTMK_FullGeneralityVirtualInheritance:
       IM = MSInheritanceAttr::Keyword_unspecified_inheritance;
       break;
     }
 
+    // The user probably did not want the unspecified inheritance model in the
+    // best case.  Diagnose this case so that the code could be reworked by
+    // providing a definition prior to the use of a pointer-to-member or via the
+    // use of inheritance keywords.
+    if (S.MSPointerToMemberRepresentationMethod ==
+            LangOptions::PPTMK_BestCase &&
+        IM == MSInheritanceAttr::Keyword_unspecified_inheritance) {
+      S.Diag(RD->getSourceRange().getBegin(),
+             diag::warn_unspecified_ms_inheritance);
+      S.Diag(Loc, diag::note_ms_inheritance_selected);
+    }
+
     RD->addAttr(MSInheritanceAttr::CreateImplicit(
         S.getASTContext(), IM,
         /*BestCase=*/S.MSPointerToMemberRepresentationMethod ==
             LangOptions::PPTMK_BestCase,
         S.ImplicitMSInheritanceAttrLoc.isValid()
             ? S.ImplicitMSInheritanceAttrLoc
             : RD->getSourceRange()));
   }
 
   if (RD->hasDefinition()) {
     // Assign inheritance models to all of the base classes, because now we can
     // form pointers to members of base classes without calling
     // RequireCompleteType on the pointer to member of the base class type.
     for (const CXXBaseSpecifier &BS : RD->bases())
-      assignInheritanceModel(S, BS.getType()->getAsCXXRecordDecl());
+      assignInheritanceModel(S, BS.getType()->getAsCXXRecordDecl(), Loc);
   }
 }
 
 /// \brief The implementation of RequireCompleteType
 bool Sema::RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
                                    TypeDiagnoser &Diagnoser) {
   // FIXME: Add this assertion to make sure we always get instantiation points.
   //  assert(!Loc.isInvalid() && "Invalid location in RequireCompleteType");
if (!T->isIncompleteType(&Def)) {
     }
 
     // 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()) {
       if (const MemberPointerType *MPTy = T->getAs<MemberPointerType>()) {
         if (!MPTy->getClass()->isDependentType()) {
           RequireCompleteType(Loc, QualType(MPTy->getClass(), 0), 0);
-          assignInheritanceModel(*this, MPTy->getMostRecentCXXRecordDecl());
+          assignInheritanceModel(*this, MPTy->getMostRecentCXXRecordDecl(),
+                                 Loc);
         }
       }
     }
 
     return false;
   }
 
   const TagType *Tag = T->getAs<TagType>();

test/SemaCXX/member-pointer-ms.cpp

 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;
+#ifdef VMB
+// expected-warning@-2 {{incomplete classes use the 'full_generality' pointer-to-member representation}}
+#endif
 static_assert(sizeof(int IncUnspecified::*) == kUnspecifiedDataSize, "");
+#ifdef VMB
+// expected-note@-2 {{inheritance model selected here}}
+#endif
 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;
 #ifdef VMB
+// expected-warning@-2 {{incomplete classes use the 'full_generality' pointer-to-member representation}}
+#endif
+#ifdef VMB
 template <> class __single_inheritance   X<IncSingle>;
 template <> class __multiple_inheritance X<IncMultiple>;
 template <> class __virtual_inheritance  X<IncVirtual>;
 #else
 template <> class X<IncSingle>;
 template <> class X<IncMultiple>;
 template <> class X<IncVirtual>;
 #endif
 // 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, "");
+#ifdef VMB
+// expected-note@-2 {{inheritance model selected here}}
+#endif
 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(void (CC::*)()) == kSingleFunctionSize, "");
 
 // We start out unspecified.
 struct ForwardDecl1;
 struct ForwardDecl2;
 
 // Re-declare to force us to iterate decls when adding attributes.
 struct ForwardDecl1;
+#ifdef VMB
+// expected-warning@-2 {{incomplete classes use the 'full_generality' pointer-to-member representation}}
+#endif
 struct ForwardDecl2;
 
 typedef int ForwardDecl1::*MemPtr1;
 typedef int ForwardDecl2::*MemPtr2;
 MemPtr1 variable_forces_sizing;
+#ifdef VMB
+// expected-note@-2 {{inheritance model selected here}}
+#endif
 
 struct ForwardDecl1 : B {
   virtual void foo();
 };
 struct ForwardDecl2 : B {
   virtual void foo();
 };
 
struct SingleTemplate<void (T::*)(void)> {
   static_assert(sizeof(void (T::*)()) == kSingleFunctionSize, "");
 };
 
 template<typename T>
 struct UnspecTemplate;
 template<typename T>
 struct UnspecTemplate<void (T::*)(void)> {
   static_assert(sizeof(int T::*) == kUnspecifiedDataSize, "");
+#ifdef VMB
+// expected-note@-2 {{inheritance model selected here}}
+#endif
   static_assert(sizeof(void (T::*)()) == kUnspecifiedFunctionSize, "");
 };
 
 struct NewUnspecified;
+#ifdef VMB
+// expected-warning@-2 {{incomplete classes use the 'full_generality' pointer-to-member representation}}
+#endif
 SingleTemplate<void (IncSingle::*)()> tmpl_single;
 UnspecTemplate<void (NewUnspecified::*)()> tmpl_unspec;
+#ifdef VMB
+// expected-note@-2 {{in instantiation of template class}}
+#endif
 
 struct NewUnspecified { };
 
 static_assert(sizeof(void (NewUnspecified::*)()) == kUnspecifiedFunctionSize, "");
 
 template <typename T>
 struct MemPtrInTemplate {
   // We can't require that the template arg be complete until we're