[PATCH] D51187: Thread safety analysis: Warn on double (un-)lock of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Sure. As I wrote in the commit message, I'm not sure about it myself, but I think it's worth a discussion. Maybe I should have tagged it as RFC.

Releasable scopes need a way of knowing whether the lock is currently held to prevent double unlocking in the destructor. There are basically two ways to achieve that: one can ask the underlying mutex if it is locked, or keep track of the lock status. The second might be the only option available, as not all mutex implementations offer a way to find out if they are currently held.

Suppose we have something like:

  class ReleasableScope {
    Mutex *mu;
    bool Locked;
  
  public:
    ReleasableScope(Mutex *mu) : mu(mu), Locked(true) {
      mu->Lock();
    }
    void Unlock() { mu->Unlock(); Locked = false; }
    ~ReleasableScope() { if (Locked) mu->Unlock(); }
  };

Then it can be a problem if someone works directly on the underlying mutex without informing the scoped capability:

  Mutex mu;
  
  void test1() {
    ReleasableScope scope(&mu);
    mu.Unlock();
  } // ~ReleasableScope unlocks mu again, but we don't warn about double unlock.
  
  void test2() {
    ReleasableScope scope(&mu);
    scope.Unlock()
    mu.Lock();
  } // ~ReleasableScope does not unlock, but we don't warn about the lock still being held.

In the `LockableFactEntry` we call `mu` the "managed" mutex of `scope` and I think that implies that for its lifetime only `scope` should touch `mu`. What we do in `test1` is akin to manually `delete`ing the underlying pointer of a `std::unique_ptr`. I would actually consider releasing `mu` again in `ScopedLockableFactEntry::handleUnlock` in the epilogue of `test1` and not releasing it in the epilogue of `test2`, because we think it is still locked/already unlocked. Then we would emit warnings in both cases.

On the other hand, if the scoped capability can (and does) ask the underlying mutex for its state before doing anything, we might be completely fine, as you stated.

So I'm really not sure, but I think there are some arguments that can be made for introducing this warning (and possibly more).

We could also consider making this part of the `-Wthread-safety-beta` warnings, so that we don't break anyone's `-Werror=thread-safety` builds.


Repository:
  rC Clang

https://reviews.llvm.org/D51187