[PATCH] D152504: [clang][ThreadSafety] Analyze cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: clang/lib/Analysis/ThreadSafetyCommon.cpp:320
         // 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 *>()) {
----------------
I'd suggest to go either with `const auto *` or `const Expr *const *`.


================
Comment at: clang/test/Sema/warn-thread-safety-analysis.c:82-86
+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}}
+}
----------------
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.


================
Comment at: clang/test/Sema/warn-thread-safety-analysis.c:159
   return 0;
 }
 
----------------
Would you mind adding a test case that uses substitution? Something like this:

```lang=c
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152504/new/

https://reviews.llvm.org/D152504