Sema: Diagnose improper application of inheritance keywords

We would previously allow inappropriate inheritance keywords to appear
on class declarations.  We would also allow inheritance keywords on
templates which were not fully specialized; this was divergent from
MSVC.

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

llvm-svn: 200423

8 files changed, 113 insertions, 29 deletions

diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index 8f5fa31d880..b15a46cbe5d 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -1602,6 +1602,8 @@ public:
   MSInheritanceAttr::Spelling getMSInheritanceModel() const;
   /// \brief Locks-in the inheritance model for this class.
   void setMSInheritanceModel();
+  /// \brief Calculate what the inheritance model would be for this class.
+  MSInheritanceAttr::Spelling calculateInheritanceModel() const;
 
   /// \brief Determine whether this lambda expression was known to be dependent
   /// at the time it was created, even if its context does not appear to be
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index a704f3bea31..d9de72ddf9d 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1401,10 +1401,6 @@ def MSInheritance : InheritableAttr {
                    Keyword<"__multiple_inheritance">,
                    Keyword<"__virtual_inheritance">,
                    Keyword<"__unspecified_inheritance">];
-  let Accessors = [Accessor<"IsSingle", [Keyword<"__single_inheritance">]>,
-                   Accessor<"IsMultiple", [Keyword<"__multiple_inheritance">]>,
-                   Accessor<"IsVirtual", [Keyword<"__virtual_inheritance">]>,
-                   Accessor<"IsUnspecified", [Keyword<"__unspecified_inheritance">]>];
 }
 
 def Unaligned : IgnoredAttr {
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 67f5837246f..406ad8691d2 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2351,6 +2351,13 @@ def warn_attribute_protected_visibility :
   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 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 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">;
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 780937da928..57f6c891a09 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -1863,6 +1863,10 @@ public:
                                     unsigned AttrSpellingListIndex);
   DLLExportAttr *mergeDLLExportAttr(Decl *D, SourceRange Range,
                                     unsigned AttrSpellingListIndex);
+  MSInheritanceAttr *
+  mergeMSInheritanceAttr(Decl *D, SourceRange Range,
+                         unsigned AttrSpellingListIndex,
+                         MSInheritanceAttr::Spelling SemanticSpelling);
   FormatAttr *mergeFormatAttr(Decl *D, SourceRange Range,
                               IdentifierInfo *Format, int FormatIdx,
                               int FirstArg, unsigned AttrSpellingListIndex);
@@ -2572,6 +2576,9 @@ public:
   bool checkStringLiteralArgumentAttr(const AttributeList &Attr,
                                       unsigned ArgNum, StringRef &Str,
                                       SourceLocation *ArgLocation = 0);
+  bool checkMSInheritanceAttrOnDefinition(
+      CXXRecordDecl *RD, SourceRange Range,
+      MSInheritanceAttr::Spelling SemanticSpelling);
 
   void CheckAlignasUnderalignment(Decl *D);
 
diff --git a/clang/lib/AST/MicrosoftCXXABI.cpp b/clang/lib/AST/MicrosoftCXXABI.cpp
index c3478747a69..ee96e13e4b1 100644
--- a/clang/lib/AST/MicrosoftCXXABI.cpp
+++ b/clang/lib/AST/MicrosoftCXXABI.cpp
@@ -92,26 +92,12 @@ static bool usesMultipleInheritanceModel(const CXXRecordDecl *RD) {
   return false;
 }
 
-static MSInheritanceAttr::Spelling
-MSInheritanceAttrToModel(const MSInheritanceAttr *Attr) {
-  if (Attr->IsSingle())
-    return MSInheritanceAttr::Keyword_single_inheritance;
-  else if (Attr->IsMultiple())
-    return MSInheritanceAttr::Keyword_multiple_inheritance;
-  else if (Attr->IsVirtual())
-    return MSInheritanceAttr::Keyword_virtual_inheritance;
-  
-  assert(Attr->IsUnspecified() && "Expected unspecified inheritance attr");
-  return MSInheritanceAttr::Keyword_unspecified_inheritance;
-}
-
-static MSInheritanceAttr::Spelling
-calculateInheritanceModel(const CXXRecordDecl *RD) {
-  if (!RD->hasDefinition())
+MSInheritanceAttr::Spelling CXXRecordDecl::calculateInheritanceModel() const {
+  if (!hasDefinition())
     return MSInheritanceAttr::Keyword_unspecified_inheritance;
-  if (RD->getNumVBases() > 0)
+  if (getNumVBases() > 0)
     return MSInheritanceAttr::Keyword_virtual_inheritance;
-  if (usesMultipleInheritanceModel(RD))
+  if (usesMultipleInheritanceModel(this))
     return MSInheritanceAttr::Keyword_multiple_inheritance;
   return MSInheritanceAttr::Keyword_single_inheritance;
 }
@@ -120,7 +106,7 @@ MSInheritanceAttr::Spelling
 CXXRecordDecl::getMSInheritanceModel() const {
   MSInheritanceAttr *IA = getAttr<MSInheritanceAttr>();
   assert(IA && "Expected MSInheritanceAttr on the CXXRecordDecl!");
-  return MSInheritanceAttrToModel(IA);
+  return IA->getSemanticSpelling();
 }
 
 void CXXRecordDecl::setMSInheritanceModel() {
@@ -128,7 +114,7 @@ void CXXRecordDecl::setMSInheritanceModel() {
     return;
 
   addAttr(MSInheritanceAttr::CreateImplicit(
-      getASTContext(), calculateInheritanceModel(this), getSourceRange()));
+      getASTContext(), calculateInheritanceModel(), getSourceRange()));
 }
 
 // Returns the number of pointer and integer slots used to represent a member
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index a054adfa731..59539566e7d 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -1962,6 +1962,9 @@ static bool mergeDeclAttribute(Sema &S, NamedDecl *D, InheritableAttr *Attr,
   else if (SectionAttr *SA = dyn_cast<SectionAttr>(Attr))
     NewAttr = S.mergeSectionAttr(D, SA->getRange(), SA->getName(),
                                  AttrSpellingListIndex);
+  else if (MSInheritanceAttr *IA = dyn_cast<MSInheritanceAttr>(Attr))
+    NewAttr = S.mergeMSInheritanceAttr(D, IA->getRange(), AttrSpellingListIndex,
+                                       IA->getSemanticSpelling());
   else if (isa<AlignedAttr>(Attr))
     // AlignedAttrs are handled separately, because we need to handle all
     // such attributes on a declaration at the same time.
@@ -12104,9 +12107,15 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
     if (!Completed)
       Record->completeDefinition();
 
-    if (Record->hasAttrs())
+    if (Record->hasAttrs()) {
       CheckAlignasUnderalignment(Record);
 
+      if (MSInheritanceAttr *IA = Record->getAttr<MSInheritanceAttr>())
+        checkMSInheritanceAttrOnDefinition(cast<CXXRecordDecl>(Record),
+                                           IA->getRange(),
+                                           IA->getSemanticSpelling());
+    }
+
     // Check if the structure/union declaration is a type that can have zero
     // size in C. For C this is a language extension, for C++ it may cause
     // compatibility problems.
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index dd7aa477b17..50ec236fb79 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -2872,6 +2872,23 @@ void Sema::CheckAlignasUnderalignment(Decl *D) {
   }
 }
 
+bool Sema::checkMSInheritanceAttrOnDefinition(
+    CXXRecordDecl *RD, SourceRange Range,
+    MSInheritanceAttr::Spelling SemanticSpelling) {
+  assert(RD->hasDefinition() && "RD has no definition!");
+
+  if (SemanticSpelling != MSInheritanceAttr::Keyword_unspecified_inheritance &&
+      RD->calculateInheritanceModel() != SemanticSpelling) {
+    Diag(Range.getBegin(), diag::err_mismatched_ms_inheritance)
+        << 0 /*definition*/;
+    Diag(RD->getDefinition()->getLocation(), diag::note_defined_here)
+        << RD->getNameAsString();
+    return true;
+  }
+
+  return false;
+}
+
 /// handleModeAttr - This attribute modifies the width of a decl with primitive
 /// type.
 ///
@@ -3674,6 +3691,19 @@ static void handleUuidAttr(Sema &S, Decl *D, const AttributeList &Attr) {
                                         Attr.getAttributeSpellingListIndex()));
 }
 
+static void handleMSInheritanceAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  if (!S.LangOpts.CPlusPlus) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_not_supported_in_lang)
+      << Attr.getName() << AttributeLangSupport::C;
+    return;
+  }
+  MSInheritanceAttr *IA = S.mergeMSInheritanceAttr(
+      D, Attr.getRange(), Attr.getAttributeSpellingListIndex(),
+      (MSInheritanceAttr::Spelling)Attr.getSemanticSpelling());
+  if (IA)
+    D->addAttr(IA);
+}
+
 static void handleARMInterruptAttr(Sema &S, Decl *D,
                                    const AttributeList &Attr) {
   // Check the attribute arguments.
@@ -3848,6 +3878,41 @@ static void handleDLLExportAttr(Sema &S, Decl *D, const AttributeList &Attr) {
     D->addAttr(NewAttr);
 }
 
+MSInheritanceAttr *
+Sema::mergeMSInheritanceAttr(Decl *D, SourceRange Range,
+                             unsigned AttrSpellingListIndex,
+                             MSInheritanceAttr::Spelling SemanticSpelling) {
+  if (MSInheritanceAttr *IA = D->getAttr<MSInheritanceAttr>()) {
+    if (IA->getSemanticSpelling() == SemanticSpelling)
+      return 0;
+    Diag(IA->getLocation(), diag::err_mismatched_ms_inheritance)
+        << 1 /*previous declaration*/;
+    Diag(Range.getBegin(), diag::note_previous_ms_inheritance);
+    D->dropAttr<MSInheritanceAttr>();
+  }
+
+  CXXRecordDecl *RD = cast<CXXRecordDecl>(D);
+  if (RD->hasDefinition()) {
+    if (checkMSInheritanceAttrOnDefinition(RD, Range, SemanticSpelling)) {
+      return 0;
+    }
+  } else {
+    if (isa<ClassTemplatePartialSpecializationDecl>(RD)) {
+      Diag(Range.getBegin(), diag::warn_ignored_ms_inheritance)
+          << 1 /*partial specialization*/;
+      return 0;
+    }
+    if (RD->getDescribedClassTemplate()) {
+      Diag(Range.getBegin(), diag::warn_ignored_ms_inheritance)
+          << 0 /*primary template*/;
+      return 0;
+    }
+  }
+
+  return ::new (Context)
+      MSInheritanceAttr(Range, Context, AttrSpellingListIndex);
+}
+
 /// Handles semantic checking for features that are common to all attributes,
 /// such as checking whether a parameter was properly specified, or the correct
 /// number of arguments were passed, etc.
@@ -4130,7 +4195,7 @@ static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
     handleUuidAttr(S, D, Attr);
     break;
   case AttributeList::AT_MSInheritance:
-    handleSimpleAttribute<MSInheritanceAttr>(S, D, Attr); break;
+    handleMSInheritanceAttr(S, D, Attr); break;
   case AttributeList::AT_ForceInline:
     handleSimpleAttribute<ForceInlineAttr>(S, D, Attr); break;
   case AttributeList::AT_SelectAny:
diff --git a/clang/test/SemaCXX/member-pointer-ms.cpp b/clang/test/SemaCXX/member-pointer-ms.cpp
index 11260edac4e..ca37e07496a 100644
--- a/clang/test/SemaCXX/member-pointer-ms.cpp
+++ b/clang/test/SemaCXX/member-pointer-ms.cpp
@@ -117,9 +117,7 @@ struct ForwardDecl2 : B {
 
 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, "");
+static_assert(sizeof(MemPtr2) == kSingleDataSize, "");
 
 struct MemPtrInBody {
   typedef int MemPtrInBody::*MemPtr;
@@ -166,3 +164,17 @@ struct MemPtrInTemplate {
 
 int Virtual::*CastTest = reinterpret_cast<int Virtual::*>(&AA::x);
   // expected-error@-1 {{cannot reinterpret_cast from member pointer type}}
+
+namespace ErrorTest {
+template <typename T, typename U> struct __single_inheritance A;
+  // expected-warning@-1 {{inheritance model ignored on primary template}}
+template <typename T> struct __multiple_inheritance A<T, T>;
+  // expected-warning@-1 {{inheritance model ignored on partial specialization}}
+template <> struct __single_inheritance A<int, float>;
+
+struct B {}; // expected-note {{B defined here}}
+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}}
+}