[PATCH] D104261: Thread safety analysis: Always warn when dropping locks on back edges

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think the CI failure (libarcher.races::lock-unrelated.c) is not related to this patch but is a tsan thing, but you may want to double-check that just in case.

I think this looks reasonable, but I'd like to hear from @delesley before landing.



================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:643
     sls_mu.Lock();  // \
-      // expected-note {{the other acquisition of mutex 'sls_mu' is here}}
+      // expected-warning {{mutex 'sls_mu' is acquired exclusively and shared in the same scope}}
   } while (getBool());
----------------
aaronpuchert wrote:
> These are just swapped because I'm merging the branches in a different order now. I think that's Ok.
I think the new order is actually an improvement because it diagnoses the second acquisition (diagnosing the first is a bit weirdly predictive for my tastes).


================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:2806-2816
+void loopReleaseContinue() {
+  RelockableMutexLock scope(&mu, ExclusiveTraits{}); // expected-note {{mutex acquired here}}
+  // We have to warn on this join point despite the lock being managed ...
+  for (unsigned i = 1; i < 10; ++i) {
+    x = 1; // ... because we might miss that this doesn't always happen under lock.
+    if (i == 5) {
+      scope.Unlock();
----------------
How about a test like:
```
void foo() {
  RelockableMutexLock scope(&mu, ExclusiveTraits{});
  for (unsigned i = 1; i < 10; ++i) {
    if (i > 0) // Always true
      scope.Lock(); // So we always lock
    x = 1; // So I'd assume we don't warn
  }
}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104261