[clang][ThreadSafety] Analyze cleanup functions

[tbaeder]

Summary

When a declaration has a cleanup function attached, add it to the CFG. This is basically a destructor on a per-declaration basis.

[NoQ]

I think this is the right solution and I'm happy to see it!

We have a few "direct" CFG tests in test/Analysis/cfg.c etc.; your patch doesn't seem to be specific to thread safety analysis so it's probably a good idea to add a few direct tests.

clang/lib/Analysis/CFG.cpp

1949

Is there a way to attach some valid source locations here, like the source location of the attribute's identifier argument?

I'm worried that the static analyzer may be unable to consume the CFG without them, it typically requires source locations for almost arbitrary statements. I'm still not sure how it'll react to synthetic statements, maybe turning them into a custom CFGElement subclass (like destructors) would be safer as it'll let all clients of the CFG to treat these in a custom manner. Otherwise it makes it look like a completely normal call-expression to the clients, which is not necessarily a good thing.

1955–1956

Shouldn't there be a unary operator & here somewhere? In your example the variable is int but the function accepts int *.

[tbaeder]

clang/lib/Analysis/CFG.cpp

1949

Look like I can't query the attribute for any locations except its entire range; I can also just pass VD->getLocation() to all locations here.

I thought doing this without creating fake AST nodes is better as well but I wasn't sure how to do that properly, or if I have to update all consumers of the CFG.

1955–1956

Hm, I think you are right, but it doesn't make difference for the CFG (at least not for thread anysis), does it? Is there a way for me to test that I pass the right parameter value?

[tbaeder]

Ping

[NoQ]

clang/lib/Analysis/CFG.cpp

1949

Based on the other comment thread, yeah I think it's better to have a new CFG element class than to build synthetic AST. The new element would replace like 5-6 elements that the synthetic AST would otherwise imply. And it'll have the benefit of explicitly indicating that this isn't a "normal" call-expression, which is good because it's likely that many clients actually want *at least a little bit* of custom logic, that they'll otherwise be unable to implement.

If you don't want to update all clients, the usual thing to do is to add yet another flag in CFG::BuildOptions, and leave it off by default so that different clients could opt-in gradually as they implement support for the new element.

Given that thread-safety analysis is an analysis-based warning, and all analysis-based warnings share the same CFG, you'll probably still have to update the other analysis-based warnings. But there are like 4 of them and all their entry points are in the same function so you can easily see what they are.

1955–1956

it doesn't make difference for the CFG

Hmm wait no that's not how it's supposed to be. The CFG is supposed to have every sub-expression flattened. So if you're adding

CallExpr 'foo'
`- DeclRefExpr 'x'

to the CFG, you're supposed to be adding two elements: one for DeclRefExpr, then one for CallExpr *below* it:

[B1]
1. DeclRefExpr 'x'
2. CallExpr 'foo([B1.1])'

And if you also add a UnaryOperator:

CallExpr 'foo'
`- UnaryOperator '&'
   `- DeclRefExpr 'x'

then it's gotta be three CFG elements:

[B1]
1. DeclRefExpr 'x'
2. UnaryOperator '&[B1.1]'
3. CallExpr 'foo([B1.2])'

This is what clients expect. This probably doesn't matter for thread safety analysis because this DeclRefExpr doesn't have any effect on that analysis, but other clients expect these expressions to be fully flattened this way. (This is especially important in expressions with control flow such as && or ?:.)

Typically CallExprs will also have a DeclRefExpr to the callee function as a child, probably with some function to pointer decay, which is a couple more elements.

[tbaeder]

Added a new element for cleanup functions.

clang/lib/Analysis/ThreadSafety.cpp

2436

This handles the function call, but without the instance parameter. I was wondering how to best do that.

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

2436

Should you not simply pass SxBuilder.createVariable(CF.getVarDecl()) as third parameter in analogy with the AutomaticObjectDtor case? It might also make sense to copy the attribute check.

clang/test/Sema/warn-thread-safety-analysis.c

76–77

136

Nitpick: one blank line is enough.

[tbaeder]

clang/test/Sema/warn-thread-safety-analysis.c

76–77

omitting the parameter name in a function definition is a C2x extension

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

2436

Can you write a test case that relies on passing the variable? Here is an idea:

void unlock_scope(Mutex **mu) __attribute__((release_capability(*mu))) {
  mutex_exclusive_unlock(*mu);
}

Mutex* const CLEANUP(unlock_scope) scope = &mu1;
mutex_exclusive_lock(*scope);
// Unlock should happen automatically.

I think this is mildly more interesting than the cleanup function with an unused parameter.

Unfortunately this is not quite as powerful as a scoped lock in C++, as we don't track the identity scope == &mu1. So guarded_by won't work with this. But we can at least see warnings on balanced locking/unlocking.

As for proper scoped locking, we could treat some variable initializations like construction of a C++ scoped lock. But let's discuss this separately.

clang/test/Sema/warn-thread-safety-analysis.c

76–77

Ah, I didn't know that C gained this only recently.

[tbaeder]

clang/lib/Analysis/ThreadSafety.cpp

2436

Yeah, it doesn't find the lock; I assume that's because the first parameter here is not an _actual_ this parameter; I'd have to handle the var decl as a regular parameter.

[tbaeder]

Ping

[tbaeder]

clang/lib/Analysis/ThreadSafety.cpp

2436

@aaronpuchert Can you explain how that would work? One goal from before was to avoid creating new fake AST nodes, would I'd have to insert the instance parameter as a new DeclRefExpr in the CFG, wouldn't I?

[tbaeder]

clang/lib/Analysis/ThreadSafety.cpp

2436

Ping

[tbaeder]

Ping

[tbaeder]

I split out the CFG parts as https://reviews.llvm.org/D152504

[aaronpuchert]

Sorry that it took so long, I had to debug it and think a bit. This should do it:

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<const Expr *const *, til::SExpr *> FunArgs = nullptr;
 
     // is Self referred to with -> or .?
     bool SelfArrow = false;
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index b8286cef396c..1eb48c0d5554 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 Expr *const *FunArgs =
+                Ctx->FunArgs.dyn_cast<const Expr *const *>()) {
+          assert(I < Ctx->NumArgs);
+          return translate(FunArgs[I], Ctx->Prev);
+        } else {
+          assert(I == 0);
+          return Ctx->FunArgs.get<til::SExpr *>();
+        }
       }
     }
     // Map the param back to the param of the original function declaration

The idea (which might be expressed in additional comments) is that until now we only had Self for CXXMethodDecl, but now we also have it for other FunctionDecl, in which case it is to be understood as the sole function argument.

clang/lib/Analysis/ThreadSafety.cpp

1777

We should relax this like "the implicit this argument or the argument to an implicitly called cleanup function".

2436

I assume that's because the first parameter here is not an _actual_ this parameter; I'd have to handle the var decl as a regular parameter.

Right, in SExprBuilder::translateAttrExpr we put Self into Ctx.SelfArg, but this is only used for substituting a CXXThisExpr, not a function parameter.

[aaronpuchert]

clang/lib/Analysis/ThreadSafetyCommon.cpp

320 ↗

(On Diff #556399)

I'd suggest to go either with const auto * or const Expr *const *.

clang/test/Sema/warn-thread-safety-analysis.c

81–85

FWIW, this might not be so interesting. In the function we don't know that this is being used as a cleanup function, so we just look at it like any function. So we warn. This should already by covered by other tests and has nothing to do with the cleanup attribute.

In fact, we probably don't need the definition of any cleanup function, since the function we're interested in is one where it's being used.

146

Would you mind adding a test case that uses substitution? Something like this:

void unlock_scope(struct Mutex *const *mu) __attribute__((release_capability(**mu)));

void test() {
  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.
}

This looks like something that might be used in actual C code, maybe hidden behind a macro.

In fact, the existing test case doesn't look terribly interesting to me.

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

1777

Would be nice if you could also adapt the comment as stated.

[aaronpuchert]

Side note: I hope you've seen the failing test Analysis/scopes-cfg-output.cpp, but since that belongs to the predecessor change I assume it's an issue there.

[aaronpuchert]

Thanks, looks good! I'd like to hear from @aaron.ballman if this warrants a release note, but that can be added separately.

[aaronpuchert]

clang/test/Sema/warn-thread-safety-analysis.c

27

Oh, I wouldn't mind if you were to drop this spurious added newline. But you can do this when committing, no need for a new patch set.

clang/include/clang/Analysis/CFG.h

 //===- CFG.h - Classes for representing and building CFGs -------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the CFG and CFGBuilder classes for representing and
 //  building Control-Flow Graphs (CFGs) from ASTs.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/raw_ostream.h"
 #include <bitset>
enum Kind {
     STMT_END = CXXRecordTypedCall,
     // dtor kind
     AutomaticObjectDtor,
     DeleteDtor,
     BaseDtor,
     MemberDtor,
     TemporaryDtor,
     DTOR_BEGIN = AutomaticObjectDtor,
-    DTOR_END = TemporaryDtor
+    DTOR_END = TemporaryDtor,
+    CleanupFunction,
   };
 
 protected:
   // The int bits are used to mark the kind.
   llvm::PointerIntPair<void *, 2> Data1;
   llvm::PointerIntPair<void *, 2> Data2;
 
   CFGElement(Kind kind, const void *Ptr1, const void *Ptr2 = nullptr)
private:
   friend class CFGElement;
 
   static bool isKind(const CFGElement &E) {
     Kind kind = E.getKind();
     return kind >= DTOR_BEGIN && kind <= DTOR_END;
   }
 };
 
+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.
 class CFGAutomaticObjDtor: public CFGImplicitDtor {
 public:
   CFGAutomaticObjDtor(const VarDecl *var, const Stmt *stmt)
       : CFGImplicitDtor(AutomaticObjectDtor, var, stmt) {}
 
public:
   void appendTemporaryDtor(CXXBindTemporaryExpr *E, BumpVectorContext &C) {
     Elements.push_back(CFGTemporaryDtor(E), C);
   }
 
   void appendAutomaticObjDtor(VarDecl *VD, Stmt *S, BumpVectorContext &C) {
     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);
   }
 
   void appendLoopExit(const Stmt *LoopStmt, BumpVectorContext &C) {
     Elements.push_back(CFGLoopExit(LoopStmt), C);
   }
 

clang/lib/Analysis/CFG.cpp

private:
   void appendTemporaryDtor(CFGBlock *B, CXXBindTemporaryExpr *E) {
     B->appendTemporaryDtor(E, cfg->getBumpVectorContext());
   }
 
   void appendAutomaticObjDtor(CFGBlock *B, VarDecl *VD, Stmt *S) {
     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());
   }
 
   void appendLoopExit(CFGBlock *B, const Stmt *LoopStmt) {
     B->appendLoopExit(LoopStmt, cfg->getBumpVectorContext());
   }
 
void CFGBuilder::addAutomaticObjDtors(LocalScope::const_iterator B,
   // this sequence up and replay them in reverse order when appending onto the
   // CFGBlock(s).
   SmallVector<VarDecl*, 10> Decls;
   Decls.reserve(B.distance(E));
   for (LocalScope::const_iterator I = B; I != E; ++I)
     Decls.push_back(*I);
 
   for (VarDecl *VD : llvm::reverse(Decls)) {
-    if (hasTrivialDestructor(VD)) {
+    bool HasCleanupAttr = VD->hasAttr<CleanupAttr>();
+    if (hasTrivialDestructor(VD) && !HasCleanupAttr) {
       // If AddScopes is enabled and *I is a first variable in a scope, add a
       // ScopeEnd marker in a Block.
       if (BuildOpts.AddScopes && DeclsWithEndedScope.count(VD)) {
         autoCreateBlock();
         appendScopeEnd(Block, VD, S);
       }
       continue;
     }
     // 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 successor
     // anything built thus far: control won't flow out of this block.
     QualType Ty = VD->getType();
     if (Ty->isReferenceType()) {
       Ty = getReferenceInitTemporaryType(VD->getInit());
     }
     Ty = Context->getBaseElementType(Ty);
 
-    if (Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
+    bool IsCXXRecordType = Ty->getAsCXXRecordDecl() != nullptr;
+    if (IsCXXRecordType && Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
       Block = createNoReturnBlock();
     else
       autoCreateBlock();
 
     // Add ScopeEnd just after automatic obj destructor.
     if (BuildOpts.AddScopes && DeclsWithEndedScope.count(VD))
       appendScopeEnd(Block, VD, S);
+
+    if (HasCleanupAttr)
+      appendCleanupFunction(Block, VD);
+
+    if (IsCXXRecordType)
       appendAutomaticObjDtor(Block, VD, S);
   }
 }
 
 /// addImplicitDtorsForDestructor - Add implicit destructors generated for
 /// base and member objects in destructor.
 void CFGBuilder::addImplicitDtorsForDestructor(const CXXDestructorDecl *DD) {
   assert(BuildOpts.AddImplicitDtors &&
          "Can be called only when dtors should be added");
   const CXXRecordDecl *RD = DD->getParent();
 
   // At the end destroy virtual base objects.
   for (const auto &VI : RD->vbases()) {
     // TODO: Add a VirtualBaseBranch to see if the most derived class
     // (which is different from the current class) is responsible for
     // destroying them.
     const CXXRecordDecl *CD = VI.getType()->getAsCXXRecordDecl();
     if (CD && !CD->hasTrivialDestructor()) {
if (!BuildOpts.AddImplicitDtors && !BuildOpts.AddLifetime &&
       !BuildOpts.AddScopes)
     return Scope;
 
   // Check if variable is local.
   if (!VD->hasLocalStorage())
     return Scope;
 
   if (BuildOpts.AddImplicitDtors) {
-    if (!hasTrivialDestructor(VD) || BuildOpts.AddScopes) {
+    if (!hasTrivialDestructor(VD) || VD->hasAttr<CleanupAttr>() ||
+        BuildOpts.AddScopes) {
       // Add the variable to scope
       Scope = createOrReuseLocalScope(Scope);
       Scope->addVar(VD);
       ScopePos = Scope->begin();
     }
     return Scope;
   }
 
switch (getKind()) {
     case CFGElement::NewAllocator:
     case CFGElement::LoopExit:
     case CFGElement::LifetimeEnds:
     case CFGElement::Statement:
     case CFGElement::Constructor:
     case CFGElement::CXXRecordTypedCall:
     case CFGElement::ScopeBegin:
     case CFGElement::ScopeEnd:
-      llvm_unreachable("getDestructorDecl should only be used with "
-                       "ImplicitDtors");
+    case CFGElement::CleanupFunction:
     case CFGElement::AutomaticObjectDtor: {
       const VarDecl *var = castAs<CFGAutomaticObjDtor>().getVarDecl();
       QualType ty = var->getType();
 
       // FIXME: See CFGBuilder::addLocalScopeForVarDecl.
       //
       // Lifetime-extending constructs are handled here. This works for a single
       // temporary in an initializer expression.
if (T->isReferenceType())
       T = getReferenceInitTemporaryType(VD->getInit(), nullptr);
 
     OS << ".~";
     T.getUnqualifiedType().print(OS, PrintingPolicy(Helper.getLangOpts()));
     OS << "() (Implicit destructor)\n";
     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";
     break;
 
   case CFGElement::Kind::LoopExit:
     OS << E.castAs<CFGLoopExit>().getLoopStmt()->getStmtClassName() << " (LoopExit)\n";
     break;

clang/lib/Analysis/ThreadSafety.cpp

 ///
 /// FIXME: For classes annotated with one of the guarded annotations, we need
 /// to treat const method calls as reads and non-const method calls as writes,
 /// and check that the appropriate locks are held. Non-const method calls with
 /// the same signature as const method calls can be also treated as reads.
 ///
 /// \param Exp   The call expression.
 /// \param D     The callee declaration.
 /// \param Self  If \p Exp = nullptr, the implicit this argument.
 /// \param Loc   If \p Exp = nullptr, the location.
 void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
                               til::LiteralPtr *Self, SourceLocation Loc) {
   CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd;
   CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove;
   CapExprSet ScopedReqsAndExcludes;
 
   // Figure out if we're constructing an object of scoped lockable class
for (const auto &BI : *CurrBlock) {
           if (!DD->hasAttrs())
             break;
 
           LocksetBuilder.handleCall(nullptr, DD,
                                     SxBuilder.createVariable(AD.getVarDecl()),
                                     AD.getTriggerStmt()->getEndLoc());
           break;
         }
+
+        case CFGElement::CleanupFunction: {
+          const CFGCleanupFunction &CF = BI.castAs<CFGCleanupFunction>();
+
+          LocksetBuilder.handleCall(nullptr, CF.getFunctionDecl(), nullptr,
+                                    CF.getVarDecl()->getLocation());
+          break;
+        }
+
         case CFGElement::TemporaryDtor: {
           auto TD = BI.castAs<CFGTemporaryDtor>();
 
           // Clean up constructed object even if there are no attributes to
           // keep the number of objects in limbo as small as possible.
           if (auto Object = LocksetBuilder.ConstructedObjects.find(
                   TD.getBindTemporaryExpr()->getSubExpr());
               Object != LocksetBuilder.ConstructedObjects.end()) {

clang/test/Sema/warn-thread-safety-analysis.c

 #define UNLOCK_FUNCTION(...)            __attribute__ ((unlock_function(__VA_ARGS__)))
 #define LOCK_RETURNED(x)    __attribute__ ((lock_returned(x)))
 #define LOCKS_EXCLUDED(...) __attribute__ ((locks_excluded(__VA_ARGS__)))
 #define EXCLUSIVE_LOCKS_REQUIRED(...) \
   __attribute__ ((exclusive_locks_required(__VA_ARGS__)))
 #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.
 struct LOCKABLE Mutex {};
 
 struct Foo {
   struct Mutex *mu_;
 };
 
 // Declare mutex lock/unlock functions.

 void set_value(int *a, int value) EXCLUSIVE_LOCKS_REQUIRED(foo_.mu_) {
   *a = value;
 }
 
 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);
+}
+
 int main(void) {
 
   Foo_fun1(1); // expected-warning{{calling function 'Foo_fun1' requires holding mutex 'mu2'}} \
                   expected-warning{{calling function 'Foo_fun1' requires holding mutex 'mu1' exclusively}}
 
   mutex_exclusive_lock(&mu1); // expected-note{{mutex acquired here}}
   mutex_shared_lock(&mu2);
   Foo_fun1(1);
 
   mutex_shared_lock(&mu1); // expected-warning{{acquiring mutex 'mu1' that is already held}} \
                               expected-warning{{mutex 'mu1' must be acquired before 'mu2'}}
   mutex_unlock(&mu1);
   mutex_unlock(&mu2);
mutex_shared_unlock(&mu1);     // expected-warning {{releasing mutex 'mu1' using shared access, expected exclusive access}}
                                  // expected-note@-1{{mutex released here}}
   mutex_exclusive_unlock(&mu1);  // expected-warning {{releasing mutex 'mu1' that was not held}}
 
   mutex_shared_lock(&mu1);      // expected-note {{mutex acquired here}}
   mutex_exclusive_unlock(&mu1); // expected-warning {{releasing mutex 'mu1' using exclusive access, expected shared access}}
                                 // expected-note@-1{{mutex released here}}
   mutex_shared_unlock(&mu1);    // expected-warning {{releasing mutex 'mu1' that was not held}}
 
+
+  {
+    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}}
+
   return 0;
 }
 
 struct Holder {
   struct Mutex *M;
   int counter GUARDED_BY(__builtin_instance_member(M));
 };
 
 static void lock_holder(struct Holder *H) __attribute__((acquire_capability(H->M))) NO_THREAD_SAFETY_ANALYSIS {}
 static void unlock_holder(struct Holder *H) __attribute__((release_capability(H->M))) NO_THREAD_SAFETY_ANALYSIS {}