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