MS ABI: Downgrade inheritance model mismatches to a warning sometimes

[majnemer]

Summary

A polymorphic class with a single base class uses a multiple inheritance
model, not a single inheritance model. However, a pointer-to-member
field may appear before we know that there are any virtual methods.
We will, wrongly, assign the class a single-inheritance
pointer-to-member representation. Later on, we will check to see what
representation it actually got and issue a diagnostic.

Downgrade this diagnostic to a warning *if* the selection of inheritance
model is implicit.

This fixes PR20464.

tl;dr
A derived class, a base class, a virtual method and a pointer-to-member
walk into a bar. The pointer-to-member ruins the experience for
everyone.

[majnemer]

I implemented this for the sake of completeness, I don't think that we should actually fix this. This can be easily fixed in user code by making sure their virtual methods occur first in the class definition.

[rnk]

I think this is a good change if this is kept as an error instead of a warning.

[majnemer]

• Address review comments

[rnk]

lgtm

test/SemaCXX/member-pointer-ms.cpp

253

Maybe, namespace PR20464 since this isn't a warning any more?

[Eugene.Zelenko]

Looks like patch was not committed.

include/clang/Basic/DiagnosticSemaKinds.td

def warn_attribute_unknown_visibility : Warning<"unknown visibility %0">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_protected_visibility :
   Warning<"target does not support 'protected' visibility; using 'default'">,
   InGroup<DiagGroup<"unsupported-visibility">>;
 def err_mismatched_visibility: Error<"visibility does not match previous declaration">;
 def note_previous_attribute : Note<"previous attribute is here">;
 def note_attribute : Note<"attribute is here">;
 def err_mismatched_ms_inheritance : Error<
-  "inheritance model does not match %select{definition|previous declaration}0">;
+  "inheritance model does not match %select{definition|previous declaration}0 (%select{single|multiple|virtual|most general}1 vs %select{single|multiple|virtual|most general}2)">;
 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 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">;

lib/Sema/SemaDeclAttr.cpp

bool Sema::checkMSInheritanceAttrOnDefinition(
   // have to wait until the record is defined to catch any mismatches.
   if (!RD->getDefinition()->isCompleteDefinition())
     return false;
 
   // The unspecified model never matches what a definition could need.
   if (SemanticSpelling == MSInheritanceAttr::Keyword_unspecified_inheritance)
     return false;
 
+  MSInheritanceAttr::Spelling CalculatedInheritanceModel =
+      RD->calculateInheritanceModel();
   if (BestCase) {
-    if (RD->calculateInheritanceModel() == SemanticSpelling)
+    if (CalculatedInheritanceModel == SemanticSpelling)
       return false;
   } else {
-    if (RD->calculateInheritanceModel() <= SemanticSpelling)
+    if (CalculatedInheritanceModel <= SemanticSpelling)
       return false;
   }
 
   Diag(Range.getBegin(), diag::err_mismatched_ms_inheritance)
-      << 0 /*definition*/;
+      << 0 /*definition*/ << SemanticSpelling << CalculatedInheritanceModel;
+
   Diag(RD->getDefinition()->getLocation(), diag::note_defined_here)
       << RD->getNameAsString();
   return true;
 }
 
 /// handleModeAttr - This attribute modifies the width of a decl with primitive
 /// type.
 ///

 MSInheritanceAttr *
 Sema::mergeMSInheritanceAttr(Decl *D, SourceRange Range, bool BestCase,
                              unsigned AttrSpellingListIndex,
                              MSInheritanceAttr::Spelling SemanticSpelling) {
   if (MSInheritanceAttr *IA = D->getAttr<MSInheritanceAttr>()) {
     if (IA->getSemanticSpelling() == SemanticSpelling)
       return nullptr;
     Diag(IA->getLocation(), diag::err_mismatched_ms_inheritance)
-        << 1 /*previous declaration*/;
+        << 1 /*previous declaration*/ << SemanticSpelling
+        << IA->getSemanticSpelling();
     Diag(Range.getBegin(), diag::note_previous_ms_inheritance);
     D->dropAttr<MSInheritanceAttr>();
   }
 
   CXXRecordDecl *RD = cast<CXXRecordDecl>(D);
   if (RD->hasDefinition()) {
     if (checkMSInheritanceAttrOnDefinition(RD, Range, BestCase,
                                            SemanticSpelling)) {

test/SemaCXX/member-pointer-ms.cpp

 struct __multiple_inheritance B; // expected-error{{inheritance model does not match definition}}
 
 struct __multiple_inheritance C {}; // expected-error{{inheritance model does not match definition}}
  // expected-note@-1 {{C defined here}}
 
 struct __virtual_inheritance D;
 struct D : virtual B {};
 }
+
+#ifdef VMB
+namespace WarnTest {
+struct A {};
+struct B : A { // expected-error {{inheritance model does not match definition (single vs multiple)}}
+  // expected-note@-1 {{B defined here}}
+  int B::*x;
+  virtual void f();
+};
+}
+#endif
+
 #ifdef VMB
 
 namespace PR20017 {
 template <typename T>
 struct A {
   int T::*f();
 };