Author: Aaron Ballman Date: 2021-04-13T15:20:30-04:00 New Revision: c058a7122787c8e503cf6306b8da03c0e56a41a5
URL: https://github.com/llvm/llvm-project/commit/c058a7122787c8e503cf6306b8da03c0e56a41a5 DIFF: https://github.com/llvm/llvm-project/commit/c058a7122787c8e503cf6306b8da03c0e56a41a5.diff LOG: Correct the tablegen for checking mutually exclusive stmt attrs The previous implementation was insufficient for checking statement attribute mutual exclusion because attributed statements do not collect their attributes one-at-a-time in the same way that declarations do. So the design that was attempting to check for mutual exclusion as each attribute was processed would not ever catch a mutual exclusion in a statement. This was missed due to insufficient test coverage, which has now been added for the [[likely]] and [[unlikely]] attributes. The new approach is to check all of attributes that are to be applied to the attributed statement in a group. This required generating another DiagnoseMutualExclusions() function into AttrParsedAttrImpl.inc. Added: Modified: clang/include/clang/Sema/ParsedAttr.h clang/lib/Sema/ParsedAttr.cpp clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaStmtAttr.cpp clang/test/SemaCXX/attr-likelihood.cpp clang/utils/TableGen/ClangAttrEmitter.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Sema/ParsedAttr.h b/clang/include/clang/Sema/ParsedAttr.h index 471a1ecfac9b2..e508b4651be1c 100644 --- a/clang/include/clang/Sema/ParsedAttr.h +++ b/clang/include/clang/Sema/ParsedAttr.h @@ -92,12 +92,6 @@ struct ParsedAttrInfo { const Decl *D) const { return true; } - /// Check if the given attribute is mutually exclusive with other attributes - /// already applied to the given statement. - virtual bool diagMutualExclusion(Sema &S, const ParsedAttr &A, - const Stmt *St) const { - return true; - } /// Check if this attribute is allowed by the language we are compiling, and /// issue a diagnostic if not. virtual bool diagLangOpts(Sema &S, const ParsedAttr &Attr) const { @@ -612,7 +606,12 @@ class ParsedAttr final bool diagnoseAppertainsTo(class Sema &S, const Decl *D) const; bool diagnoseAppertainsTo(class Sema &S, const Stmt *St) const; bool diagnoseMutualExclusion(class Sema &S, const Decl *D) const; - bool diagnoseMutualExclusion(class Sema &S, const Stmt *St) const; + // This function stub exists for parity with the declaration checking code so + // that checkCommonAttributeFeatures() can work generically on declarations + // or statements. + bool diagnoseMutualExclusion(class Sema &S, const Stmt *St) const { + return true; + } bool appliesToDecl(const Decl *D, attr::SubjectMatchRule MatchRule) const; void getMatchRules(const LangOptions &LangOpts, SmallVectorImpl<std::pair<attr::SubjectMatchRule, bool>> diff --git a/clang/lib/Sema/ParsedAttr.cpp b/clang/lib/Sema/ParsedAttr.cpp index 989db3c2a4064..ed03b0c7f688b 100644 --- a/clang/lib/Sema/ParsedAttr.cpp +++ b/clang/lib/Sema/ParsedAttr.cpp @@ -167,10 +167,6 @@ bool ParsedAttr::diagnoseMutualExclusion(Sema &S, const Decl *D) const { return getInfo().diagMutualExclusion(S, *this, D); } -bool ParsedAttr::diagnoseMutualExclusion(Sema &S, const Stmt *St) const { - return getInfo().diagMutualExclusion(S, *this, St); -} - bool ParsedAttr::appliesToDecl(const Decl *D, attr::SubjectMatchRule MatchRule) const { return checkAttributeMatchRuleAppliesTo(D, MatchRule); diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index ea9f0949eac25..1d57d8e848497 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -2540,9 +2540,9 @@ static bool mergeAlignedAttrs(Sema &S, NamedDecl *New, Decl *Old) { return AnyAdded; } -#define WANT_MERGE_LOGIC +#define WANT_DECL_MERGE_LOGIC #include "clang/Sema/AttrParsedAttrImpl.inc" -#undef WANT_MERGE_LOGIC +#undef WANT_DECL_MERGE_LOGIC static bool mergeDeclAttribute(Sema &S, NamedDecl *D, const InheritableAttr *Attr, diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp index f914f1273f719..763c6b61fbd90 100644 --- a/clang/lib/Sema/SemaStmtAttr.cpp +++ b/clang/lib/Sema/SemaStmtAttr.cpp @@ -227,9 +227,22 @@ static Attr *handleUnlikely(Sema &S, Stmt *St, const ParsedAttr &A, return ::new (S.Context) UnlikelyAttr(S.Context, A); } +#define WANT_STMT_MERGE_LOGIC +#include "clang/Sema/AttrParsedAttrImpl.inc" +#undef WANT_STMT_MERGE_LOGIC + static void CheckForIncompatibleAttributes(Sema &S, const SmallVectorImpl<const Attr *> &Attrs) { + // The vast majority of attributed statements will only have one attribute + // on them, so skip all of the checking in the common case. + if (Attrs.size() < 2) + return; + + // First, check for the easy cases that are table-generated for us. + if (!DiagnoseMutualExclusions(S, Attrs)) + return; + // There are 6 categories of loop hints attributes: vectorize, interleave, // unroll, unroll_and_jam, pipeline and distribute. Except for distribute they // come in two variants: a state form and a numeric form. The state form diff --git a/clang/test/SemaCXX/attr-likelihood.cpp b/clang/test/SemaCXX/attr-likelihood.cpp index d2f95d17069ca..03ba301251d78 100644 --- a/clang/test/SemaCXX/attr-likelihood.cpp +++ b/clang/test/SemaCXX/attr-likelihood.cpp @@ -147,5 +147,11 @@ void o() if constexpr (true) [[likely]] { // expected-warning@+1 {{attribute 'likely' has no effect when annotating an 'if constexpr' statement}} } else [[likely]]; + + if (1) [[likely, unlikely]] { // expected-error {{'unlikely' and 'likely' attributes are not compatible}} \ + // expected-note {{conflicting attribute is here}} + } else [[unlikely]][[likely]] { // expected-error {{'likely' and 'unlikely' attributes are not compatible}} \ + // expected-note {{conflicting attribute is here}} + } } #endif diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp index ececceb5d3fd4..d679d58aaef17 100644 --- a/clang/utils/TableGen/ClangAttrEmitter.cpp +++ b/clang/utils/TableGen/ClangAttrEmitter.cpp @@ -3633,7 +3633,8 @@ static void GenerateAppertainsTo(const Record &Attr, raw_ostream &OS) { static void GenerateMutualExclusionsChecks(const Record &Attr, const RecordKeeper &Records, raw_ostream &OS, - raw_ostream &MergeOS) { + raw_ostream &MergeDeclOS, + raw_ostream &MergeStmtOS) { // Find all of the definitions that inherit from MutualExclusions and include // the given attribute in the list of exclusions to generate the // diagMutualExclusion() check. @@ -3702,43 +3703,59 @@ static void GenerateMutualExclusionsChecks(const Record &Attr, // Sema &S, Decl *D, Attr *A and that returns a bool (false on diagnostic, // true on success). if (Attr.isSubClassOf("InheritableAttr")) { - MergeOS << " if (const auto *Second = dyn_cast<" - << (Attr.getName() + "Attr").str() << ">(A)) {\n"; + MergeDeclOS << " if (const auto *Second = dyn_cast<" + << (Attr.getName() + "Attr").str() << ">(A)) {\n"; for (const std::string &A : DeclAttrs) { - MergeOS << " if (const auto *First = D->getAttr<" << A << ">()) {\n"; - MergeOS << " S.Diag(First->getLocation(), " - << "diag::err_attributes_are_not_compatible) << First << " - << "Second;\n"; - MergeOS << " S.Diag(Second->getLocation(), " - << "diag::note_conflicting_attribute);\n"; - MergeOS << " return false;\n"; - MergeOS << " }\n"; + MergeDeclOS << " if (const auto *First = D->getAttr<" << A + << ">()) {\n"; + MergeDeclOS << " S.Diag(First->getLocation(), " + << "diag::err_attributes_are_not_compatible) << First << " + << "Second;\n"; + MergeDeclOS << " S.Diag(Second->getLocation(), " + << "diag::note_conflicting_attribute);\n"; + MergeDeclOS << " return false;\n"; + MergeDeclOS << " }\n"; } - MergeOS << " return true;\n"; - MergeOS << " }\n"; + MergeDeclOS << " return true;\n"; + MergeDeclOS << " }\n"; } } + + // Statement attributes are a bit diff erent from declarations. With + // declarations, each attribute is added to the declaration as it is + // processed, and so you can look on the Decl * itself to see if there is a + // conflicting attribute. Statement attributes are processed as a group + // because AttributedStmt needs to tail-allocate all of the attribute nodes + // at once. This means we cannot check whether the statement already contains + // an attribute to check for the conflict. Instead, we need to check whether + // the given list of semantic attributes contain any conflicts. It is assumed + // this code will be executed in the context of a function with parameters: + // Sema &S, const SmallVectorImpl<const Attr *> &C. The code will be within a + // loop which loops over the container C with a loop variable named A to + // represent the current attribute to check for conflicts. + // + // FIXME: it would be nice not to walk over the list of potential attributes + // to apply to the statement more than once, but statements typically don't + // have long lists of attributes on them, so re-walking the list should not + // be an expensive operation. if (!StmtAttrs.empty()) { - // Generate the ParsedAttrInfo subclass logic for statements. - OS << " bool diagMutualExclusion(Sema &S, const ParsedAttr &AL, " - << "const Stmt *St) const override {\n"; - OS << " if (const auto *AS = dyn_cast<AttributedStmt>(St)) {\n"; - OS << " const ArrayRef<const Attr *> &Attrs = AS->getAttrs();\n"; - for (const std::string &A : StmtAttrs) { - OS << " auto Iter" << A << " = llvm::find_if(Attrs, [](const Attr " - << "*A) { return isa<" << A << ">(A); });\n"; - OS << " if (Iter" << A << " != Attrs.end()) {\n"; - OS << " S.Diag(AL.getLoc(), " - << "diag::err_attributes_are_not_compatible) << AL << *Iter" << A - << ";\n"; - OS << " S.Diag((*Iter" << A << ")->getLocation(), " - << "diag::note_conflicting_attribute);\n"; - OS << " return false;\n"; - OS << " }\n"; - } - OS << " }\n"; - OS << " return true;\n"; - OS << " }\n\n"; + MergeStmtOS << " if (const auto *Second = dyn_cast<" + << (Attr.getName() + "Attr").str() << ">(A)) {\n"; + MergeStmtOS << " auto Iter = llvm::find_if(C, [](const Attr *Check) " + << "{ return isa<"; + interleave( + StmtAttrs, [&](const std::string &Name) { MergeStmtOS << Name; }, + [&] { MergeStmtOS << ", "; }); + MergeStmtOS << ">(Check); });\n"; + MergeStmtOS << " if (Iter != C.end()) {\n"; + MergeStmtOS << " S.Diag((*Iter)->getLocation(), " + << "diag::err_attributes_are_not_compatible) << *Iter << " + << "Second;\n"; + MergeStmtOS << " S.Diag(Second->getLocation(), " + << "diag::note_conflicting_attribute);\n"; + MergeStmtOS << " return false;\n"; + MergeStmtOS << " }\n"; + MergeStmtOS << " }\n"; } } @@ -3890,7 +3907,8 @@ static bool IsKnownToGCC(const Record &Attr) { void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) { emitSourceFileHeader("Parsed attribute helpers", OS); - OS << "#if !defined(WANT_MERGE_LOGIC)\n"; + OS << "#if !defined(WANT_DECL_MERGE_LOGIC) && " + << "!defined(WANT_STMT_MERGE_LOGIC)\n"; PragmaClangAttributeSupport &PragmaAttributeSupport = getPragmaAttributeSupport(Records); @@ -3914,8 +3932,8 @@ void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) { // This stream is used to collect all of the declaration attribute merging // logic for performing mutual exclusion checks. This gets emitted at the // end of the file in a helper function of its own. - std::string DeclMergeChecks; - raw_string_ostream MergeOS(DeclMergeChecks); + std::string DeclMergeChecks, StmtMergeChecks; + raw_string_ostream MergeDeclOS(DeclMergeChecks), MergeStmtOS(StmtMergeChecks); // Generate a ParsedAttrInfo struct for each of the attributes. for (auto I = Attrs.begin(), E = Attrs.end(); I != E; ++I) { @@ -3970,7 +3988,7 @@ void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) { OS << " Spellings = " << I->first << "Spellings;\n"; OS << " }\n"; GenerateAppertainsTo(Attr, OS); - GenerateMutualExclusionsChecks(Attr, Records, OS, MergeOS); + GenerateMutualExclusionsChecks(Attr, Records, OS, MergeDeclOS, MergeStmtOS); GenerateLangOptRequirements(Attr, OS); GenerateTargetRequirements(Attr, Dupes, OS); GenerateSpellingIndexToSemanticSpelling(Attr, OS); @@ -3991,16 +4009,27 @@ void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) { // Generate the attribute match rules. emitAttributeMatchRules(PragmaAttributeSupport, OS); - OS << "#else // WANT_MERGE_LOGIC\n\n"; + OS << "#elif defined(WANT_DECL_MERGE_LOGIC)\n\n"; // Write out the declaration merging check logic. OS << "static bool DiagnoseMutualExclusions(Sema &S, const NamedDecl *D, " << "const Attr *A) {\n"; - OS << MergeOS.str(); + OS << MergeDeclOS.str(); OS << " return true;\n"; OS << "}\n\n"; - OS << "#endif // WANT_MERGE_LOGIC\n"; + OS << "#elif defined(WANT_STMT_MERGE_LOGIC)\n\n"; + + // Write out the statement merging check logic. + OS << "static bool DiagnoseMutualExclusions(Sema &S, " + << "const SmallVectorImpl<const Attr *> &C) {\n"; + OS << " for (const Attr *A : C) {\n"; + OS << MergeStmtOS.str(); + OS << " }\n"; + OS << " return true;\n"; + OS << "}\n\n"; + + OS << "#endif\n"; } // Emits the kind list of parsed attributes _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
