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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
