[PATCH] D102026: Thread safety analysis: Allow exlusive/shared joins for managed and asserted capabilities
Delesley Hutchins via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon May 24 09:09:06 PDT 2021
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
More information about the cfe-commits
mailing list