[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added a comment.

I have a few concerns.  First, this patch introduces an inconsistency between managed and unmanaged locks.  For unmanaged locks, we warn, and //then assume the lock is held exclusively//.  For managed locks, we don't warn, but //assume it is held shared//.  The warn/not-warn is consistent with more relaxed handling of managed locks, but exclusive/shared difference could lead to confusion.  Both mechanisms are sound; they're just not consistent.  Any thoughts?

Second, the mixing of AssertHeld() and Lock() on different control flow branches looks very weird to me.  I can see one obvious case where you might want to do that, e.g.  if (mu_.is_locked()) mu_.AssertHeld(); else mu_.Lock().  However, if a function locks mu_ on one branch, and assert mu_ on the other, then that usually indicates a problem.  It means that the lock-state going into that function was unknown; the programmer didn't know whether mu_ was locked or not, and that's never good.  The //only// valid condition in that case is to check whether mu_ is locked -- any other condition would be unsound.  The correct way to deal with this situation is to mark the is_locked() method with an attribute, e.g. TEST_LOCKED_FUNCTION, rather than relying on the AssertHeld mechanism.  That's a lot more work, but I would prefer to at least have a TODO comment indicating that the AssertHeld is a workaround, and restrict the mixing of assert/lock to managed locks in the mean time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102026