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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D102026#2777438 <https://reviews.llvm.org/D102026#2777438>, @delesley wrote:

> 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?

Essentially I'm suggesting here to treat exclusive/shared joins the same way as locked/unlocked joins: we allow them for managed locks and take the "weaker" state afterwards to prevent false negatives, but not for unmanaged locks, where we take the "stronger" state afterwards to prevent spamming the user.

> 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().

This pattern already works, but not with exclusive/shared mismatch.

> 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.

Generally I agree with you. It's something that repeatedly came up though, people were having these functions that were called sometimes with lock, sometimes without. It's not incredibly common, but I saw it a few times. I recommended the above pattern to get rid of the warnings that popped up, when reworking this into a function with clear preconditions didn't work.

> The //only// valid condition in that case is to check whether mu_ is locked -- any other condition would be unsound.

Well, that information might not always come from `mu_` itself. It might be tracked in the class that owns `mu_`.

> 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,

It certainly is a workaround, but it's powerful enough to cover everything that we could do with `TEST_LOCKED_FUNCTION`, except that we can't find cases where the branches have been mixed up.

I've thought about such an attribute (after all it shouldn't be much different from `try_acquire` implementation-wise), but I've found it hard to justify because we can just use these assertions to achieve the same thing. You can even do

  if (mu.isLocked())
      []() ASSERT_CAPABILITY(mu) {}();
  else
      mu.Lock();



> and restrict the mixing of assert/lock to managed locks in the mean time.

I think we should disallow releasing asserted locks, and then naturally we can't allow assert/lock joins for unmanaged locks. I'm planning another change for this.

It is the status quo: we allow these joins, just not with mixed mode (shared/exclusive). So this change will allow shared/exclusive - asserted/unmanaged joins until my follow-up will disallow them again, though because of the asserted/unmanaged part and not the shared/exclusive part. If you're not happy about this gap in warning coverage, we can also postpone this change until we have the asserted/unmanaged joins disallowed.



================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4570
     mu_.AssertHeld();
-    mu_.Unlock();
-  }  // should this be a warning?
+    mu_.Unlock(); // should this be a warning?
+  }
----------------
delesley wrote:
> This function should have a warning because is not marked with UNLOCK_FUNCTION.  The lock was held on entry, and not held on exit.  So the original location of the comment, on the closing curly brace, was correct.  
Putting it on the `Unlock` call would some more natural to me, and also more helpful. Natural, because that's where we (should) discover the issue, and helpful, because in a longer function you wouldn't know what's going on if the unlock is somewhere in the middle.

If we didn't have `AssertHeld`, we would also warn on the `Unlock` call, and I think we should treat this no different (other than having a different warning message).


================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4629
+      mu_.AssertHeld(); // expected-note {{the other acquisition of mutex 'mu_' is here}}
+    // FIXME: should instead warn because it's unclear whether we need to release or not.
+    int b = a;
----------------
delesley wrote:
> I'm not wild about having FIXMEs in test code.  I would prefer to have the patch actually issue a warning here (but see below).
> 
> HOWEVER, does the current code issue a warning in this situation?  If not, leave it as-is and keep the FIXME.  You can't make the analysis more strict without checking with the C++ compiler team at Google (which I am no longer on), because it may break the build on our end.
> 
The comment is that the warning should be different. It's somewhat related with the other discussion: one shouldn't be able to release asserted locks, and thus the join here should fail for that reason and not because of a shared/exclusive mismatch.

Naturally I want to do those things together in one change, that's why I'm not addressing it here.

I'm not particularly worried that this is going to have a big impact, but we've had interactions with someone at Google whom I might ask to try it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102026