commit cf8e189a99f988398a48148b9ea7901948665ab0 Author: Timm Bäder Date: Wed Sep 6 12:19:20 2023 +0200 [clang][TSA] Thread safety cleanup functions Consider cleanup functions in thread safety analysis. Differential Revision: https://reviews.llvm.org/D152504 diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h index 9d28325c1ea6..13e37ac2b56b 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h @@ -361,7 +361,7 @@ public: unsigned NumArgs = 0; // Function arguments - const Expr *const *FunArgs = nullptr; + llvm::PointerUnion FunArgs = nullptr; // is Self referred to with -> or .? bool SelfArrow = false; diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 3107d035254d..3e6ceb7d54c4 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1773,7 +1773,8 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK, /// /// \param Exp The call expression. /// \param D The callee declaration. -/// \param Self If \p Exp = nullptr, the implicit this argument. +/// \param Self If \p Exp = nullptr, the implicit this argument or the argument +/// of an implicitly called cleanup function. /// \param Loc If \p Exp = nullptr, the location. void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D, til::LiteralPtr *Self, SourceLocation Loc) { @@ -2417,6 +2418,15 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { AD.getTriggerStmt()->getEndLoc()); break; } + + case CFGElement::CleanupFunction: { + const CFGCleanupFunction &CF = BI.castAs(); + LocksetBuilder.handleCall(/*Exp=*/nullptr, CF.getFunctionDecl(), + SxBuilder.createVariable(CF.getVarDecl()), + CF.getVarDecl()->getLocation()); + break; + } + case CFGElement::TemporaryDtor: { auto TD = BI.castAs(); diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp index b8286cef396c..63cc66852a9e 100644 --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp @@ -110,7 +110,8 @@ static StringRef ClassifyDiagnostic(QualType VDT) { /// \param D The declaration to which the attribute is attached. /// \param DeclExp An expression involving the Decl to which the attribute /// is attached. E.g. the call to a function. -/// \param Self S-expression to substitute for a \ref CXXThisExpr. +/// \param Self S-expression to substitute for a \ref CXXThisExpr in a call, +/// or argument to a cleanup function. CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp, const NamedDecl *D, const Expr *DeclExp, @@ -144,7 +145,11 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp, if (Self) { assert(!Ctx.SelfArg && "Ambiguous self argument"); - Ctx.SelfArg = Self; + assert(isa(D) && "Self argument requires function"); + if (isa(D)) + Ctx.SelfArg = Self; + else + Ctx.FunArgs = Self; // If the attribute has no arguments, then assume the argument is "this". if (!AttrExp) @@ -312,8 +317,14 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE, ? (cast(D)->getCanonicalDecl() == Canonical) : (cast(D)->getCanonicalDecl() == Canonical)) { // Substitute call arguments for references to function parameters - assert(I < Ctx->NumArgs); - return translate(Ctx->FunArgs[I], Ctx->Prev); + if (const Expr *const *FunArgs = + Ctx->FunArgs.dyn_cast()) { + assert(I < Ctx->NumArgs); + return translate(FunArgs[I], Ctx->Prev); + } + + assert(I == 0); + return Ctx->FunArgs.get(); } } // Map the param back to the param of the original function declaration diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index 355616b73d96..642ea88ec3c9 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -72,6 +72,8 @@ int get_value(int *p) SHARED_LOCKS_REQUIRED(foo_.mu_){ return *p; } +void unlock_scope(struct Mutex *const *mu) __attribute__((release_capability(**mu))); + int main(void) { Foo_fun1(1); // expected-warning{{calling function 'Foo_fun1' requires holding mutex 'mu2'}} \ @@ -127,6 +129,13 @@ int main(void) { // expected-note@-1{{mutex released here}} mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' that was not held}} + /// Cleanup functions + { + struct Mutex* const __attribute__((cleanup(unlock_scope))) scope = &mu1; + mutex_exclusive_lock(scope); // Note that we have to lock through scope, because no alias analysis! + // Cleanup happens automatically -> no warning. + } + return 0; }