[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

1945 ↗

(On Diff #529849)

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.

1951–1952 ↗

(On Diff #529849)

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

1945 ↗

(On Diff #529849)

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.

1951–1952 ↗

(On Diff #529849)

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

1945 ↗

(On Diff #529849)

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.

1951–1952 ↗

(On Diff #529849)

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

2427

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

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

2427

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

75–76

132

Nitpick: one blank line is enough.

[tbaeder]

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

75–76

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

[aaronpuchert]

clang/lib/Analysis/ThreadSafety.cpp

2427

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

75–76

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

[tbaeder]

clang/lib/Analysis/ThreadSafety.cpp

2427

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

2427

@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

2427

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

1776–1777

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

2427

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–327

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

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

80–84

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.

140

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

1776–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

26

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/Analyses/ThreadSafetyCommon.h

struct CallingContext {
 
     // Implicit object argument -- e.g. 'this'
     llvm::PointerUnion<const Expr *, til::SExpr *> SelfArg = nullptr;
 
     // Number of funArgs
     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;
 
     CallingContext(CallingContext *P, const NamedDecl *D = nullptr)
         : Prev(P), AttrDecl(D) {}
   };
 

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 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) {
   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(/*Exp=*/nullptr, CF.getFunctionDecl(),
+                                    SxBuilder.createVariable(CF.getVarDecl()),
+                                    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/lib/Analysis/ThreadSafetyCommon.cpp

 
 /// Translate a clang expression in an attribute to a til::SExpr.
 /// Constructs the context from D, DeclExp, and SelfDecl.
 ///
 /// \param AttrExp The expression to translate.
 /// \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,
                                                til::SExpr *Self) {
   // If we are processing a raw attribute expression, with no substitutions.
   if (!DeclExp && !Self)
     return translateAttrExpr(AttrExp, nullptr);
 
CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
   } else if (const auto *CE = dyn_cast<CXXConstructExpr>(DeclExp)) {
     Ctx.SelfArg = nullptr;  // Will be set below
     Ctx.NumArgs = CE->getNumArgs();
     Ctx.FunArgs = CE->getArgs();
   }
 
   if (Self) {
     assert(!Ctx.SelfArg && "Ambiguous self argument");
+    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)
       return CapabilityExpr(
           Self, ClassifyDiagnostic(cast<CXXMethodDecl>(D)->getThisObjectType()),
           false);
     else  // For most attributes.
       return translateAttrExpr(AttrExp, &Ctx);
if (const auto *PV = dyn_cast<ParmVarDecl>(VD)) {
     unsigned I = PV->getFunctionScopeIndex();
     const DeclContext *D = PV->getDeclContext();
     if (Ctx && Ctx->FunArgs) {
       const Decl *Canonical = Ctx->AttrDecl->getCanonicalDecl();
       if (isa<FunctionDecl>(D)
               ? (cast<FunctionDecl>(D)->getCanonicalDecl() == Canonical)
               : (cast<ObjCMethodDecl>(D)->getCanonicalDecl() == Canonical)) {
         // Substitute call arguments for references to function parameters
+        if (const Expr *const *FunArgs =
+                Ctx->FunArgs.dyn_cast<const Expr *const *>()) {
           assert(I < Ctx->NumArgs);
-        return translate(Ctx->FunArgs[I], Ctx->Prev);
+          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
     // for consistent comparisons.
     VD = isa<FunctionDecl>(D)
              ? cast<FunctionDecl>(D)->getCanonicalDecl()->getParamDecl(I)
              : cast<ObjCMethodDecl>(D)->getCanonicalDecl()->getParamDecl(I);
   }

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

 #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 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 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'}} \
                   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_lock(&mu1);
   mutex_exclusive_lock(&mu2);
   Foo_fun2(2);
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}}
 
+  /// 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;
 }
 
 // We had a problem where we'd skip all attributes that follow a late-parsed
 // attribute in a single __attribute__.
 void run(void) __attribute__((guarded_by(mu1), guarded_by(mu1))); // expected-warning 2{{only applies to non-static data members and global variables}}