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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D104261#2832892 <https://reviews.llvm.org/D104261#2832892>, @aaron.ballman wrote:

> 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.

Seems that an expected race didn't materialize, perhaps it's a bit flaky. I'd be surprised if it was related.

> I'd like to hear from @delesley before landing.

Me too. Generally about treating back edges differently, as we can't modify the lockset anymore. (I have a similar change that's going to take back some of relaxations in D102026 <https://reviews.llvm.org/D102026>, namely for an exclusive lock coming back as a shared lock on the back edge.)



================
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();
----------------
aaron.ballman wrote:
> 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
>   }
> }
The CFG isn't that clever, it would only consider `i > 0` always true if `i` was a compile-time constant. It doesn't even consider type limits, so for `i >= 0` the else-branch would still be considered reachable:

```
 [B2]
   1: x
   2: [B2.1] (ImplicitCastExpr, LValueToRValue, unsigned int)
   3: 0
   4: [B2.3] (ImplicitCastExpr, IntegralCast, unsigned int)
   5: [B2.2] >= [B2.4]
   T: if [B2.5]
   Preds (1): B3
   Succs (2): B1 B0
```

versus the same with `true`:

```
 [B2]
   1: true
   T: if [B2.1]
   Preds (1): B3
   Succs (2): B1 B0(Unreachable)
```

But let's say we use a compile-time constant, then it's equivalent to not having the `Lock` call conditional at all, and there is no warning. Actually I might just change `loopAcquire` into this, because as that test is written right now it doesn't affect the back edge at all. (The lock will be removed from the lockset when the if joins with the else, before we loop back.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104261