diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 0866b09bab299..8211f55b08890 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -230,6 +230,9 @@ class ThreadSafetyHandler { /// Warn that there is a cycle in acquired_before/after dependencies. virtual void handleBeforeAfterCycle(Name L1Name, SourceLocation Loc) {} + virtual void handleAttributeMismatch(const NamedDecl *D1, + const NamedDecl *D2) {} + /// Called by the analysis when starting analysis of a function. /// Used to issue suggestions for changes to annotations. virtual void enterFunction(const FunctionDecl *FD) {} diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 157d77b38b354..b6e45bb9655ac 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -4004,6 +4004,9 @@ def err_attribute_argument_out_of_bounds_extra_info : Error< "%plural{0:no parameters to index into|" "1:can only be 1, since there is one parameter|" ":must be between 1 and %2}2">; +def warn_attribute_mismatch : Warning< + "attribute mismatch between function declarations of %0">, + InGroup, DefaultIgnore; // Thread Safety Analysis def warn_unlock_but_no_lock : Warning<"releasing %0 '%1' that was not held">, @@ -4055,7 +4058,6 @@ def warn_acquired_before_after_cycle : Warning< "cycle in acquired_before/after dependencies, starting with '%0'">, InGroup, DefaultIgnore; - // Thread safety warnings negative capabilities def warn_acquire_requires_negative_cap : Warning< "acquiring %0 '%1' requires negative capability '%2'">, diff --git a/clang/lib/AST/ByteCode/Function.cpp b/clang/lib/AST/ByteCode/Function.cpp index 896a4fb3f9469..3b3a488bbcfcb 100644 --- a/clang/lib/AST/ByteCode/Function.cpp +++ b/clang/lib/AST/ByteCode/Function.cpp @@ -34,6 +34,7 @@ Function::ParamDescriptor Function::getParamDescriptor(unsigned Offset) const { } SourceInfo Function::getSource(CodePtr PC) const { + llvm::errs() << __PRETTY_FUNCTION__ << '\n'; assert(PC >= getCodeBegin() && "PC does not belong to this function"); assert(PC <= getCodeEnd() && "PC Does not belong to this function"); assert(hasBody() && "Function has no body"); diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 5577f45aa5217..5784c1c470eb4 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1073,6 +1073,8 @@ class ThreadSafetyAnalyzer { ProtectedOperationKind POK); void checkPtAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK, ProtectedOperationKind POK); + + void checkMismatchedFunctionAttrs(const NamedDecl *ND); }; } // namespace @@ -2263,6 +2265,37 @@ static bool neverReturns(const CFGBlock *B) { return false; } +template +static CapExprSet collectAttrArgs(SExprBuilder &SxBuilder, const Decl *D) { + CapExprSet Caps; + for (const auto *A : D->specific_attrs()) { + for (const Expr *E : A->args()) + Caps.push_back_nodup(SxBuilder.translateAttrExpr(E, nullptr)); + } + return Caps; +} + +template +static void maybeDiagnoseFunctionAttrs(const NamedDecl *ND, + SExprBuilder &SxBuilder, + ThreadSafetyHandler &Handler) { + + // FIXME: The diagnostic here is suboptimal. It would be better to print + // what attributes are missing in the first declaration. + CapExprSet NDArgs = collectAttrArgs(SxBuilder, ND); + for (const Decl *D = ND->getPreviousDecl(); D; D = D->getPreviousDecl()) { + CapExprSet DArgs = collectAttrArgs(SxBuilder, D); + + if (NDArgs.size() != DArgs.size()) + Handler.handleAttributeMismatch(ND, cast(D)); + } +} + +void ThreadSafetyAnalyzer::checkMismatchedFunctionAttrs(const NamedDecl *ND) { + maybeDiagnoseFunctionAttrs(ND, SxBuilder, Handler); + maybeDiagnoseFunctionAttrs(ND, SxBuilder, Handler); +} + /// Check a function's CFG for thread-safety violations. /// /// We traverse the blocks in the CFG, compute the set of mutexes that are held @@ -2282,6 +2315,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { const NamedDecl *D = walker.getDecl(); CurrentFunction = dyn_cast(D); + checkMismatchedFunctionAttrs(D); + if (D->hasAttr()) return; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 37d966a5a0463..b0deb766ec496 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2065,6 +2065,16 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { Warnings.emplace_back(std::move(Warning), getNotes()); } + void handleAttributeMismatch(const NamedDecl *ThisDecl, + const NamedDecl *PrevDecl) override { + PartialDiagnosticAt Warning(ThisDecl->getLocation(), + S.PDiag(diag::warn_attribute_mismatch) + << ThisDecl); + PartialDiagnosticAt Note(PrevDecl->getLocation(), + S.PDiag(diag::note_previous_decl) << PrevDecl); + Warnings.emplace_back(std::move(Warning), getNotes(Note)); + } + void enterFunction(const FunctionDecl* FD) override { CurrentFunction = FD; } diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 8477200456d98..d6c5e03647fb8 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2198,8 +2198,8 @@ namespace FunctionDefinitionTest { class Foo { public: void foo1(); - void foo2(); - void foo3(Foo *other); + void foo2() EXCLUSIVE_LOCKS_REQUIRED(mu_); + void foo3(Foo *other) EXCLUSIVE_LOCKS_REQUIRED(other->mu_); template void fooT1(const T& dummy1); @@ -2249,8 +2249,8 @@ void fooF1(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { f->a = 1; } -void fooF2(Foo *f); -void fooF2(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { +void fooF2(Foo *f); // expected-note {{declared here}} +void fooF2(Foo *f) EXCLUSIVE_LOCKS_REQUIRED(f->mu_) { // expected-warning {{attribute mismatch between function declarations of 'fooF2'}} f->a = 2; } @@ -2269,9 +2269,9 @@ void test() { Foo myFoo; myFoo.foo2(); // \ - // expected-warning {{calling function 'foo2' requires holding mutex 'myFoo.mu_' exclusively}} + // expected-warning 2{{calling function 'foo2' requires holding mutex 'myFoo.mu_' exclusively}} myFoo.foo3(&myFoo); // \ - // expected-warning {{calling function 'foo3' requires holding mutex 'myFoo.mu_' exclusively}} + // expected-warning 2{{calling function 'foo3' requires holding mutex 'myFoo.mu_' exclusively}} myFoo.fooT1(dummy); // \ // expected-warning {{calling function 'fooT1' requires holding mutex 'myFoo.mu_' exclusively}}