[PATCH] D98747: Thread safety analysis: Don't warn about managed locks on join points

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Not replying in order, I hope that's not confusing. Let's start with the motivation.

In D98747#2648381 <https://reviews.llvm.org/D98747#2648381>, @delesley wrote:

> Is there a compelling reason for this change, other than consistency/symmetry concerns?  In other words, do you have real-world code where you are getting false positives when using MutexUnlock?  If so, then I am in favor of the change.  If not, then I tend to err on the side of "if it ain't broke don't fix it."  :-)

When I originally implemented relockable scopes in D49885 <https://reviews.llvm.org/D49885> I was (not sure if deliberately or not) making it stricter than regular scopes. But then there were a couple of cases in our code like this:

  RelockableMutexLock scope(&mu, DeferTraits{});
  scope.Lock(); // Often also something like "if (!scope.TryLock()) return;"
  if (condition) {
    scope.Unlock();
    // ... do stuff without the lock
  } // warning: not all paths joining here hold the lock

The warning is somewhat surprising, because if you acquire in the constructor it's fine. (Compare e.g. `Foo::test2` on line 2593 with `join` on line 2891. The latter has a warning on the join point, the former doesn't.)

These are usually easy to fix, you can just add an else branch that releases the lock, but then we got into cases with nested ifs and people started asking: why can't I just rely on the RAII type to do the unlocking for me? There were maybe 5 cases or so and I could fix them all, but I couldn't really explain why it's fine when you lock in the constructor, but not if you lock later. (All through the RAII object of course.)

> A MutexLock object uses the RAII design pattern to ensure that the underlying mutex is unlocked on every control flow path, and presumably includes logic to prevent double releases/unlocks, as is usually the case with RAII.

Precisely, if `MutexLock` has an `Unlock` method, the destructor can't assume the lock is still held. `Unlock` itself though might assume it, so we might have false negatives here. But we've been accepting them before: see `Foo::test5`. (See below for a potential solution.)

> The RAII pattern is sufficiently robust that we can "trust the implementation", and relax static checks.  A MutexUnlock object inverts the RAII pattern.  Because a lock is being acquired on different control-flow paths, rather than being released, it may make sense to keep the stronger checks in order to guard against potential deadlocks.

I think we can assume the same thing here. If `MutexUnlock` has a `Lock`, then the destructor can't assume the lock is not held. But `Lock` might assume it, which means we have essentially the same false negative, except the other way around.

When it comes to deadlocks, I'm afraid that's not new:

  MutexLocker rlock(&mu_);
  if (c)
    rlock.Unlock();
  rlock.Lock();

might deadlock, but we don't warn currently. The reason is that we remove scoped locks when there are discrepancies at join points, because we don't want to miss race conditions. So we miss that the second `Lock` call might deadlock.

We could probably address all false negatives by introducing a `FactEntry` for "managed locks that might be held", which we introduce on discrepancies at join points. We assume that they're fine for the destructor, but we would warn if there is an explicit lock/unlock. I could start working on this if you think that's a good idea, but it would not only address false negatives introduced in this change, but also false negatives that we've had before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98747