[clang][TSA] Consider cleanup functions for thread safety analysis

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;

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);
   }

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";

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:

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>();
 

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

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;

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)));
+}

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;
 }