Timm =?utf-8?q?Bäder?= <tbae...@redhat.com>
https://github.com/tbaederr created https://github.com/llvm/llvm-project/pull/65462: None >From 20aa3e3225f6ea48df00ea05bcbfcc5cc2278af5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Tue, 8 Aug 2023 14:11:33 +0200 Subject: [PATCH 1/2] [clang][CFG] Cleanup functions --- clang/include/clang/Analysis/CFG.h | 38 +++++++++++- clang/lib/Analysis/CFG.cpp | 40 ++++++++---- clang/lib/Analysis/PathDiagnostic.cpp | 1 + clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 1 + clang/test/Analysis/scopes-cfg-output.cpp | 65 ++++++++++++++++++++ 5 files changed, 131 insertions(+), 14 deletions(-) diff --git a/clang/include/clang/Analysis/CFG.h b/clang/include/clang/Analysis/CFG.h index cf4fa2da2a358e..67383bb316d318 100644 --- a/clang/include/clang/Analysis/CFG.h +++ b/clang/include/clang/Analysis/CFG.h @@ -14,10 +14,11 @@ #ifndef LLVM_CLANG_ANALYSIS_CFG_H #define LLVM_CLANG_ANALYSIS_CFG_H -#include "clang/Analysis/Support/BumpVector.h" -#include "clang/Analysis/ConstructionContext.h" +#include "clang/AST/Attr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/ExprObjC.h" +#include "clang/Analysis/ConstructionContext.h" +#include "clang/Analysis/Support/BumpVector.h" #include "clang/Basic/LLVM.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/GraphTraits.h" @@ -74,7 +75,8 @@ class CFGElement { MemberDtor, TemporaryDtor, DTOR_BEGIN = AutomaticObjectDtor, - DTOR_END = TemporaryDtor + DTOR_END = TemporaryDtor, + CleanupFunction, }; protected: @@ -383,6 +385,32 @@ class CFGImplicitDtor : public CFGElement { } }; +class CFGCleanupFunction final : public CFGElement { +public: + CFGCleanupFunction() = default; + CFGCleanupFunction(const VarDecl *VD) + : CFGElement(Kind::CleanupFunction, VD) { + assert(VD->hasAttr<CleanupAttr>()); + } + + const VarDecl *getVarDecl() const { + return static_cast<VarDecl *>(Data1.getPointer()); + } + + /// Returns the function to be called when cleaning up the var decl. + const FunctionDecl *getFunctionDecl() const { + const CleanupAttr *A = getVarDecl()->getAttr<CleanupAttr>(); + return A->getFunctionDecl(); + } + +private: + friend class CFGElement; + + static bool isKind(const CFGElement E) { + return E.getKind() == Kind::CleanupFunction; + } +}; + /// Represents C++ object destructor implicitly generated for automatic object /// or temporary bound to const reference at the point of leaving its local /// scope. @@ -1142,6 +1170,10 @@ class CFGBlock { Elements.push_back(CFGAutomaticObjDtor(VD, S), C); } + void appendCleanupFunction(const VarDecl *VD, BumpVectorContext &C) { + Elements.push_back(CFGCleanupFunction(VD), C); + } + void appendLifetimeEnds(VarDecl *VD, Stmt *S, BumpVectorContext &C) { Elements.push_back(CFGLifetimeEnds(VD, S), C); } diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index b82f9010a83f77..03ab4c6fdf29ca 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -881,6 +881,10 @@ class CFGBuilder { B->appendAutomaticObjDtor(VD, S, cfg->getBumpVectorContext()); } + void appendCleanupFunction(CFGBlock *B, VarDecl *VD) { + B->appendCleanupFunction(VD, cfg->getBumpVectorContext()); + } + void appendLifetimeEnds(CFGBlock *B, VarDecl *VD, Stmt *S) { B->appendLifetimeEnds(VD, S, cfg->getBumpVectorContext()); } @@ -1346,7 +1350,8 @@ class CFGBuilder { return {}; } - bool hasTrivialDestructor(VarDecl *VD); + bool hasTrivialDestructor(const VarDecl *VD) const; + bool needsAutomaticDestruction(const VarDecl *VD) const; }; } // namespace @@ -1861,14 +1866,14 @@ void CFGBuilder::addAutomaticObjDestruction(LocalScope::const_iterator B, if (B == E) return; - SmallVector<VarDecl *, 10> DeclsNonTrivial; - DeclsNonTrivial.reserve(B.distance(E)); + SmallVector<VarDecl *, 10> DeclsNeedDestruction; + DeclsNeedDestruction.reserve(B.distance(E)); for (VarDecl* D : llvm::make_range(B, E)) - if (!hasTrivialDestructor(D)) - DeclsNonTrivial.push_back(D); + if (needsAutomaticDestruction(D)) + DeclsNeedDestruction.push_back(D); - for (VarDecl *VD : llvm::reverse(DeclsNonTrivial)) { + for (VarDecl *VD : llvm::reverse(DeclsNeedDestruction)) { if (BuildOpts.AddImplicitDtors) { // If this destructor is marked as a no-return destructor, we need to // create a new block for the destructor which does not have as a @@ -1879,7 +1884,8 @@ void CFGBuilder::addAutomaticObjDestruction(LocalScope::const_iterator B, Ty = getReferenceInitTemporaryType(VD->getInit()); Ty = Context->getBaseElementType(Ty); - if (Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn()) + const CXXRecordDecl *CRD = Ty->getAsCXXRecordDecl(); + if (CRD && CRD->isAnyDestructorNoReturn()) Block = createNoReturnBlock(); } @@ -1890,8 +1896,10 @@ void CFGBuilder::addAutomaticObjDestruction(LocalScope::const_iterator B, // objects, we end lifetime with scope end. if (BuildOpts.AddLifetime) appendLifetimeEnds(Block, VD, S); - if (BuildOpts.AddImplicitDtors) + if (BuildOpts.AddImplicitDtors && !hasTrivialDestructor(VD)) appendAutomaticObjDtor(Block, VD, S); + if (VD->hasAttr<CleanupAttr>()) + appendCleanupFunction(Block, VD); } } @@ -1922,7 +1930,7 @@ void CFGBuilder::addScopeExitHandling(LocalScope::const_iterator B, // is destroyed, for automatic variables, this happens when the end of the // scope is added. for (VarDecl* D : llvm::make_range(B, E)) - if (hasTrivialDestructor(D)) + if (!needsAutomaticDestruction(D)) DeclsTrivial.push_back(D); if (DeclsTrivial.empty()) @@ -2095,7 +2103,11 @@ LocalScope* CFGBuilder::addLocalScopeForDeclStmt(DeclStmt *DS, return Scope; } -bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) { +bool CFGBuilder::needsAutomaticDestruction(const VarDecl *VD) const { + return !hasTrivialDestructor(VD) || VD->hasAttr<CleanupAttr>(); +} + +bool CFGBuilder::hasTrivialDestructor(const VarDecl *VD) const { // Check for const references bound to temporary. Set type to pointee. QualType QT = VD->getType(); if (QT->isReferenceType()) { @@ -2149,7 +2161,7 @@ LocalScope* CFGBuilder::addLocalScopeForVarDecl(VarDecl *VD, return Scope; if (!BuildOpts.AddLifetime && !BuildOpts.AddScopes && - hasTrivialDestructor(VD)) { + !needsAutomaticDestruction(VD)) { assert(BuildOpts.AddImplicitDtors); return Scope; } @@ -5287,6 +5299,7 @@ CFGImplicitDtor::getDestructorDecl(ASTContext &astContext) const { case CFGElement::CXXRecordTypedCall: case CFGElement::ScopeBegin: case CFGElement::ScopeEnd: + case CFGElement::CleanupFunction: llvm_unreachable("getDestructorDecl should only be used with " "ImplicitDtors"); case CFGElement::AutomaticObjectDtor: { @@ -5830,6 +5843,11 @@ static void print_elem(raw_ostream &OS, StmtPrinterHelper &Helper, break; } + case CFGElement::Kind::CleanupFunction: + OS << "CleanupFunction (" + << E.castAs<CFGCleanupFunction>().getFunctionDecl()->getName() << ")\n"; + break; + case CFGElement::Kind::LifetimeEnds: Helper.handleDecl(E.castAs<CFGLifetimeEnds>().getVarDecl(), OS); OS << " (Lifetime ends)\n"; diff --git a/clang/lib/Analysis/PathDiagnostic.cpp b/clang/lib/Analysis/PathDiagnostic.cpp index 93e6d98492ddef..4be88f6c6f332b 100644 --- a/clang/lib/Analysis/PathDiagnostic.cpp +++ b/clang/lib/Analysis/PathDiagnostic.cpp @@ -565,6 +565,7 @@ getLocationForCaller(const StackFrameContext *SFC, } case CFGElement::ScopeBegin: case CFGElement::ScopeEnd: + case CFGElement::CleanupFunction: llvm_unreachable("not yet implemented!"); case CFGElement::LifetimeEnds: case CFGElement::LoopExit: diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 0e2ac78f7089c5..d7c5bd1d404235 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -993,6 +993,7 @@ void ExprEngine::processCFGElement(const CFGElement E, ExplodedNode *Pred, ProcessLoopExit(E.castAs<CFGLoopExit>().getLoopStmt(), Pred); return; case CFGElement::LifetimeEnds: + case CFGElement::CleanupFunction: case CFGElement::ScopeBegin: case CFGElement::ScopeEnd: return; diff --git a/clang/test/Analysis/scopes-cfg-output.cpp b/clang/test/Analysis/scopes-cfg-output.cpp index 6877d124e67a4f..5e6706602d4564 100644 --- a/clang/test/Analysis/scopes-cfg-output.cpp +++ b/clang/test/Analysis/scopes-cfg-output.cpp @@ -1419,3 +1419,68 @@ void test_multiple_goto_entering_scopes() { } } } + +// CHECK: [B1] +// CHECK-NEXT: 1: CFGScopeBegin(i) +// CHECK-NEXT: 2: int i __attribute__((cleanup(cleanup_int))); +// CHECK-NEXT: 3: CleanupFunction (cleanup_int) +// CHECK-NEXT: 4: CFGScopeEnd(i) +void cleanup_int(int *i); +void test_cleanup_functions() { + int i __attribute__((cleanup(cleanup_int))); +} + +// CHECK: [B1] +// CHECK-NEXT: 1: 10 +// CHECK-NEXT: 2: i +// CHECK-NEXT: 3: [B1.2] = [B1.1] +// CHECK-NEXT: 4: return; +// CHECK-NEXT: 5: CleanupFunction (cleanup_int) +// CHECK-NEXT: 6: CFGScopeEnd(i) +// CHECK-NEXT: Preds (1): B3 +// CHECK-NEXT: Succs (1): B0 +// CHECK: [B2] +// CHECK-NEXT: 1: return; +// CHECK-NEXT: 2: CleanupFunction (cleanup_int) +// CHECK-NEXT: 3: CFGScopeEnd(i) +// CHECK-NEXT: Preds (1): B3 +// CHECK-NEXT: Succs (1): B0 +// CHECK: [B3] +// CHECK-NEXT: 1: CFGScopeBegin(i) +// CHECK-NEXT: 2: int i __attribute__((cleanup(cleanup_int))); +// CHECK-NEXT: 3: m +// CHECK-NEXT: 4: [B3.3] (ImplicitCastExpr, LValueToRValue, int) +// CHECK-NEXT: 5: 1 +// CHECK-NEXT: 6: [B3.4] == [B3.5] +// CHECK-NEXT: T: if [B3.6] +// CHECK-NEXT: Preds (1): B4 +// CHECK-NEXT: Succs (2): B2 B1 +void test_cleanup_functions2(int m) { + int i __attribute__((cleanup(cleanup_int))); + + if (m == 1) { + return; + } + + i = 10; + return; +} + +// CHECK: [B1] +// CHECK-NEXT: 1: CFGScopeBegin(f) +// CHECK-NEXT: 2: (CXXConstructExpr, [B1.3], F) +// CHECK-NEXT: 3: F f __attribute__((cleanup(cleanup_F))); +// CHECK-NEXT: 4: CleanupFunction (cleanup_F) +// CHECK-NEXT: 5: [B1.3].~F() (Implicit destructor) +// CHECK-NEXT: 6: CFGScopeEnd(f) +// CHECK-NEXT: Preds (1): B2 +// CHECK-NEXT: Succs (1): B0 +class F { +public: + ~F(); +}; +void cleanup_F(F *f); + +void test() { + F f __attribute((cleanup(cleanup_F))); +} >From 0ddb7754eca5919108d7e1987106ef29ba7ba58c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbae...@redhat.com> Date: Wed, 6 Sep 2023 12:19:20 +0200 Subject: [PATCH 2/2] [clang][TSA] Thread safety cleanup functions --- .../Analysis/Analyses/ThreadSafetyCommon.h | 2 +- clang/lib/Analysis/ThreadSafety.cpp | 9 ++++++ clang/lib/Analysis/ThreadSafetyCommon.cpp | 19 ++++++++++--- clang/test/Sema/warn-thread-safety-analysis.c | 28 +++++++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h index 9d28325c1ea67a..13e37ac2b56b64 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h @@ -361,7 +361,7 @@ class SExprBuilder { unsigned NumArgs = 0; // Function arguments - const Expr *const *FunArgs = nullptr; + llvm::PointerUnion<const Expr *const *, til::SExpr *> 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 34260ac8f4e7d6..4a771f0636bf9f 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2414,6 +2414,15 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { AD.getTriggerStmt()->getEndLoc()); break; } + + case CFGElement::CleanupFunction: { + const CFGCleanupFunction &CF = BI.castAs<CFGCleanupFunction>(); + LocksetBuilder.handleCall(/*Exp=*/nullptr, CF.getFunctionDecl(), + SxBuilder.createVariable(CF.getVarDecl()), + CF.getVarDecl()->getLocation()); + break; + } + case CFGElement::TemporaryDtor: { auto TD = BI.castAs<CFGTemporaryDtor>(); diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp index b8286cef396c06..9ab17786704489 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<FunctionDecl>(D) && "Self argument requires function"); + if (isa<CXXMethodDecl>(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<FunctionDecl>(D)->getCanonicalDecl() == Canonical) : (cast<ObjCMethodDecl>(D)->getCanonicalDecl() == Canonical)) { // Substitute call arguments for references to function parameters - assert(I < Ctx->NumArgs); - return translate(Ctx->FunArgs[I], Ctx->Prev); + if (const auto *const *FunArgs = + Ctx->FunArgs.dyn_cast<const Expr *const *>()) { + assert(I < Ctx->NumArgs); + return translate(FunArgs[I], Ctx->Prev); + } + + assert(I == 0); + return Ctx->FunArgs.get<til::SExpr *>(); } } // 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 355616b73d967e..0e216507e49270 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -22,6 +22,8 @@ #define SHARED_LOCKS_REQUIRED(...) \ __attribute__ ((shared_locks_required(__VA_ARGS__))) #define NO_THREAD_SAFETY_ANALYSIS __attribute__ ((no_thread_safety_analysis)) +#define CLEANUP(A) __attribute__ ((cleanup(A))) + // Define the mutex struct. // Simplified only for test purpose. @@ -72,6 +74,17 @@ int get_value(int *p) SHARED_LOCKS_REQUIRED(foo_.mu_){ return *p; } +void cleanup_int(int *unused) __attribute__((release_capability(mu1))) { + (void)unused; + mutex_exclusive_unlock(&mu1); +} + +void broken_cleanup_int(int *unused) __attribute__((release_capability(mu1))) { + (void)unused; + mutex_exclusive_unlock(&mu1); + Bar_fun1(6); // expected-warning {{calling function 'Bar_fun1' requires holding mutex 'mu1' exclusively}} +} + int main(void) { Foo_fun1(1); // expected-warning{{calling function 'Foo_fun1' requires holding mutex 'mu2'}} \ @@ -127,6 +140,21 @@ int main(void) { // expected-note@-1{{mutex released here}} mutex_shared_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' that was not held}} + /// Cleanup functions + { + mutex_exclusive_lock(&mu1); + int CLEANUP(cleanup_int) i; + + Bar_fun1(3); + } + Bar_fun1(4); // expected-warning {{calling function 'Bar_fun1' requires holding mutex 'mu1' exclusively}} + + + { + mutex_exclusive_lock(&mu1); + int CLEANUP(broken_cleanup_int) i2; + } + return 0; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
