[PATCH] Sema: Diagnose improper application of inheritance keywords

Aaron Ballman aaron.ballman at gmail.com
Mon Jan 20 16:14:04 PST 2014


Thanks for working on this! Changes look mostly reasonable, but I do
have some concerns listed below:

> Index: include/clang/AST/DeclCXX.h
> ===================================================================
> --- include/clang/AST/DeclCXX.h
> +++ include/clang/AST/DeclCXX.h
> @@ -1602,6 +1602,8 @@
>    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
> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td
> +++ include/clang/Basic/Attr.td
> @@ -1379,10 +1379,6 @@
>                     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 {
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -2341,6 +2341,18 @@
>    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 previous declaration">;

It'd be good to highlight what the inheritance models are as part of
the diagnostic.

> +def warn_ignored_ms_inheritance_on_partial_spec : Warning<
> +  "inheritance model ignored on partial specialization">,
> +  InGroup<IgnoredAttributes>;
> +def warn_ignored_ms_inheritance_on_primary_template : Warning<
> +  "inheritance model ignored on primary template">,
> +  InGroup<IgnoredAttributes>;
> +def err_inconsistent_ms_inheritance : Error<
> +  "inheritance model does not match definition">;

Same here.

> +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">;
> Index: include/clang/Sema/Sema.h
> ===================================================================
> --- include/clang/Sema/Sema.h
> +++ include/clang/Sema/Sema.h
> @@ -1861,6 +1861,8 @@
>                                      unsigned AttrSpellingListIndex);
>    DLLExportAttr *mergeDLLExportAttr(Decl *D, SourceRange Range,
>                                      unsigned AttrSpellingListIndex);
> +  MSInheritanceAttr *mergeMSInheritanceAttr(Decl *D, SourceRange Range,
> +                                            unsigned AttrSpellingListIndex);
>    FormatAttr *mergeFormatAttr(Decl *D, SourceRange Range,
>                                IdentifierInfo *Format, int FormatIdx,
>                                int FirstArg, unsigned AttrSpellingListIndex);
> @@ -2570,6 +2572,8 @@
>    bool checkStringLiteralArgumentAttr(const AttributeList &Attr,
>                                        unsigned ArgNum, StringRef &Str,
>                                        SourceLocation *ArgLocation = 0);
> +  bool checkMSInheritanceAttrOnDefinition(CXXRecordDecl *RD, SourceRange Range,
> +                                          unsigned AttrSpellingListIndex);
>
>    void CheckAlignasUnderalignment(Decl *D);
>
> Index: lib/AST/MicrosoftCXXABI.cpp
> ===================================================================
> --- lib/AST/MicrosoftCXXABI.cpp
> +++ lib/AST/MicrosoftCXXABI.cpp
> @@ -92,43 +92,29 @@
>    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;
>  }
>
>  MSInheritanceAttr::Spelling
>  CXXRecordDecl::getMSInheritanceModel() const {
>    MSInheritanceAttr *IA = getAttr<MSInheritanceAttr>();
>    assert(IA && "Expected MSInheritanceAttr on the CXXRecordDecl!");
> -  return MSInheritanceAttrToModel(IA);
> +  return (MSInheritanceAttr::Spelling)IA->getSpellingListIndex();

This is unsafe; there's no guarantee that the Spelling enumeration
matches the spelling list index. It happens to be that way today, but
I don't want to be tied to that. I would feel much more comfortable
using MSInheritanceAttrToModel and have it use accessors to do the
mapping. It's less performant, but alleviates my worries.

>  }
>
>  void CXXRecordDecl::setMSInheritanceModel() {
>    if (hasAttr<MSInheritanceAttr>())
>      return;
>
>    addAttr(MSInheritanceAttr::CreateImplicit(
> -      getASTContext(), calculateInheritanceModel(this), getSourceRange()));
> +      getASTContext(), calculateInheritanceModel(), getSourceRange()));
>  }
>
>  // Returns the number of pointer and integer slots used to represent a member
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -1993,6 +1993,9 @@
>    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);
>    else if (isa<AlignedAttr>(Attr))
>      // AlignedAttrs are handled separately, because we need to handle all
>      // such attributes on a declaration at the same time.
> @@ -12144,9 +12147,15 @@
>      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->getSpellingListIndex());
> +    }
> +
>      // 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.
> Index: lib/Sema/SemaDeclAttr.cpp
> ===================================================================
> --- lib/Sema/SemaDeclAttr.cpp
> +++ lib/Sema/SemaDeclAttr.cpp
> @@ -2862,6 +2862,23 @@
>    }
>  }
>
> +bool Sema::checkMSInheritanceAttrOnDefinition(CXXRecordDecl *RD,
> +                                               SourceRange Range,
> +                                               unsigned AttrSpellingListIndex) {
> +  assert(RD->hasDefinition() && "RD has no definition!");
> +
> +  if (AttrSpellingListIndex !=
> +          MSInheritanceAttr::Keyword_unspecified_inheritance &&
> +      RD->calculateInheritanceModel() != AttrSpellingListIndex) {

Again, this relies too heavily on the Spelling enum exactly matching
the spelling list index value.

> +    Diag(Range.getBegin(), diag::err_inconsistent_ms_inheritance);
> +    Diag(RD->getDefinition()->getLocation(), diag::note_defined_here)
> +        << RD->getNameAsString();

No need to call getNameAsString -- the diagnostics engine understands
named decls directly.

> +    return true;
> +  }
> +
> +  return false;
> +}
> +
>  /// handleModeAttr - This attribute modifies the width of a decl with primitive
>  /// type.
>  ///
> @@ -3664,6 +3681,18 @@
>                                          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());
> +  if (IA)
> +    D->addAttr(IA);
> +}
> +
>  static void handleARMInterruptAttr(Sema &S, Decl *D,
>                                     const AttributeList &Attr) {
>    // Check the attribute arguments.
> @@ -3840,6 +3869,37 @@
>      D->addAttr(NewAttr);
>  }
>
> +MSInheritanceAttr *
> +Sema::mergeMSInheritanceAttr(Decl *D, SourceRange Range,
> +                             unsigned AttrSpellingListIndex) {
> +  if (MSInheritanceAttr *IA = D->getAttr<MSInheritanceAttr>()) {
> +    if (IA->getSpellingListIndex() == AttrSpellingListIndex)
> +      return 0;
> +    Diag(IA->getLocation(), diag::err_mismatched_ms_inheritance);
> +    Diag(Range.getBegin(), diag::note_previous_ms_inheritance);
> +    D->dropAttr<MSInheritanceAttr>();
> +  }
> +
> +  CXXRecordDecl *RD = cast<CXXRecordDecl>(D);
> +  if (RD->hasDefinition()) {
> +    if (checkMSInheritanceAttrOnDefinition(RD, Range, AttrSpellingListIndex)) {
> +      return 0;
> +    }
> +  } else {
> +    if (isa<ClassTemplatePartialSpecializationDecl>(RD)) {
> +      Diag(Range.getBegin(), diag::warn_ignored_ms_inheritance_on_partial_spec);
> +      return 0;
> +    }
> +    if (RD->getDescribedClassTemplate()) {
> +      Diag(Range.getBegin(), diag::warn_ignored_ms_inheritance_on_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.
> @@ -4121,7 +4181,7 @@
>      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:
> Index: test/SemaCXX/member-pointer-ms.cpp
> ===================================================================
> --- test/SemaCXX/member-pointer-ms.cpp
> +++ test/SemaCXX/member-pointer-ms.cpp
> @@ -117,9 +117,7 @@
>
>  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 @@
>
>  int Virtual::*CastTest = reinterpret_cast<int Virtual::*>(&AA::x);
>    // expected-error at -1 {{cannot reinterpret_cast from member pointer type}}
> +
> +namespace ErrorTest {
> +template <typename T, typename U> struct __single_inheritance A;
> +  // expected-warning at -1 {{inheritance model ignored on primary template}}
> +template <typename T> struct __multiple_inheritance A<T, T>;
> +  // expected-warning at -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 at -1 {{C defined here}}
> +}
>

~Aaron

On Mon, Jan 20, 2014 at 6:18 PM, David Majnemer
<david.majnemer at gmail.com> wrote:
> Hi rnk, rsmith, aaron.ballman,
>
> 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.
>
> http://llvm-reviews.chandlerc.com/D2585
>
> Files:
>   include/clang/AST/DeclCXX.h
>   include/clang/Basic/Attr.td
>   include/clang/Basic/DiagnosticSemaKinds.td
>   include/clang/Sema/Sema.h
>   lib/AST/MicrosoftCXXABI.cpp
>   lib/Sema/SemaDecl.cpp
>   lib/Sema/SemaDeclAttr.cpp
>   test/SemaCXX/member-pointer-ms.cpp



More information about the cfe-commits mailing list