[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