[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#2780384 <https://reviews.llvm.org/D102026#2780384>, @delesley wrote:

> Thanks for taking the time to discuss things with me.  :-)

Thank you as well!

> Wrt. to the TEST_LOCKED_FUNCTION, I agree that you can simulate the behavior using Assert and Lock.  But that pattern is too general/powerful, because it also allows you to write nonsensical and unsound code.  Philosophically, static analysis is concerned with allowing things that are sound, but preventing things that are not, so I would prefer to allow the valid case, but warn on the nonsensical/unsound code. The goal is not to provide powerful back doors so that you can squeeze anything past the checker -- doing so kind of defeats the point.  :-)

You're right, it does allow nonsensical code. But in some sense it's just a combination of two backdoors that we already have:

- Without `-Wthread-safety-negative` you don't have to declare preconditions of the kind "mutex should not be locked". We're basically assuming that locking is fine by default, thereby risking double locks.
- The `assert_capability` attribute is also a bit of a backdoor. Instead of statically propagating through the code that a mutex is held, we can just get that fact "out of thin air".

> That being said, I'm not certainly no asking you to implement TEST_LOCKED functionality in this particular patch, and I totally understand that it may simply not be worth the effort.

It certainly is appealing, especially because in our code we don't really have `AssertHeld`-like functions, but only `isLocked`-like functions, so our assertions are typically of the form `assert(mu.isLocked())`. But I haven't made up my mind yet.

Either way I think this change is just a consistent extension of the current behavior.

> Wrt. to unlocking an Assert, I see no problem.  It makes perfect sense to dynamically assert that a mutex is held, then do something, and then unlock the mutex afterwards; after all, the caller asserted that it was held before unlocking.  You shouldn't disallow that.

Let's discuss this when I put forward the change, I'll add you as reviewer as usual. I think I have good arguments why we shouldn't be allowing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102026