scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
-0.5 on concept, I don't think dropping the runtime checks has any advantage. For approach, I think just doing The updated docs in doc/developer-notes.md would also need updating if this change were to be merged. |
|
I agree with @ajtowns' comment:
Probably,
Concept NACK. |
|
If a function definition is placed in a
You are literally talking about adding an annotation to check for the presence of another annotation. This is an absurd idea to me, but to take it seriously, what should the developer guidelines say about using AssertLockHeld this way? Should every function that is annotated with EXCLUSIVE_LOCKS_REQUIRED also have an AssertLockHeld at the top? Is there going to be a linter to check for this, or is this going to be another source of nits in review comments?
Before the lock annotations added in #19668 this was true. It was a good way to discover where to add EXCLUSIVE_LOCKS_REQUIRED annotations. But now those annotations are added AssertLockHeld is only functioning as annotation checking the presence of another annotation, and doesn't impact the work in those other PRs or improve thread safety in any way. |
This is one benefit among others (run time check is the main purpose of
Annotations that was missed and added in 3ddc150 and 2ee7743 justify the #19668 approach.
Why not?
I think it is desirable for a new code. |
|
Good! So we agree this PR has no detrimental effects on thread safety, and the NACK is based on a style preference? |
No. It is based on thread safety. While migrating from
At least, could this change be postponed until getting rid of |
If it is annotated with EXCLUSIVE_LOCKS_REQUIRED, it seems you should be confident either that the mutex is actually locked or that LockAssertion was used earlier and would have triggered a runtime error where it was used. This PR isn't removing all runtime checks, just runtime checks redundant with compile time checks. AssertLockHeld is still available whenever you want to use it. It just returns to functioning like a normal runtime check, and not a strange compile time check enforcing the presence of a different compile time check. I can see how the strange check was useful during development of #19668, but it doesn't serve a purpose for thread safety going forward or help with future PRs. If you want to make an argument for keeping all AssertLockHelds based on readability, that's fine, but then I think you should make a developer guideline saying that AssertLockHeld should be called first thing in any function annotated with EXCLUSIVE_LOCKS_REQUIRED, and ideally have a linter to enforce this. Otherwise if the assert is only used in some places but not others, that is just adding confusion and inconsistency. |
|
I need to review this PR just for what I'll learn. 🐳 |
|
I think by default The redundant run time checks also serve as a insurance against bugs in clang. Ideally, they'd be inserted by the compiler whenever a function is annotated. Though, I don't see a way to do this in C++ without wrapping everything into more macros. Another option would be to have a preprocessing step in our ci scripts to insert the redundant run-time checks in enable-debug builds. At least that would make me feel more comfortable removing them. |
Can you clarify what is uncomfortable? If a function is annotated with EXCLUSIVE_LOCKS_REQUIRED(mutex), and the developer is not using clang, and the developer makes a change that calls the function without locking So you are uncomfortable about the inconvenience that removing asserts which are already haphazardly and inconsistently placed will cause for developers who are not using clang, but who are building in debug mode, and who are removing locks or calling functions in new places, and who are doing some kind of manual or automated testing that would happen to trigger these assertions at runtime? Or uncomfortable about something different? |
|
I simply wouldn't put too much trust into clang, since it may have bugs. |
|
We managed to misuse the compiler directives:
I think we shouldn't be doing that for no matter what reason - misleading the compiler that we do one thing while we do another. The OP in #19668 boils down to: 1 int x GUARDED_BY(cs_main);
2
3 void f() // not annotated with EXCLUSIVE_LOCKS_REQUIRED(cs_main)
4 {
5 // AssertLockHeld was properly annotated with ASSERT_EXCLUSIVE_LOCK() before #19668
6 AssertLockHeld(cs_main);
7
8 // no warning here that we access x without holding cs_main
9 x = 5;
10 }The compiler does not issue a compile-time warning for line 9 because it knows line 9 is unreachable at run-time if I agree with @MarcoFalke that clang may have bugs and we better not have it as our sole protection. The thread safety analysis look a bit immature to me - for example the warnings produced depend on the order in which attributes are defined (:-O). IMO compile time checks are good and they are an addition to runtime checks, not a replacement. What about going back to using the proper attributes? Tag |
But it is not known for a code reader. |
Line 9 is reachable at run-time if |
|
@ajtowns this is because of another misuse (or "trying to fool the compiler") - in void static inline AssertLockHeldInternal(...) ASSERT_EXCLUSIVE_LOCK(cs) {}So, we lied the compiler that we will check and In this case, IMO |
So, the 23d71d1 commit from the #19668 is a correct change, and it should not be reverted, right? |
|
23d71d1 contains 2 changes. IMO the first one should be reverted and the second change should stay. |
I don't think it's useful to put this in moral terms. We're trying to prevent buggy code, by in order of preference, (a) making it impossible to write (eg RAII so locks are free automatically); (b) making the compiler complain about it (eg thread safety annotations); (c) getting predictable safe errors at runtime rather than crashes, hangs or undefined behaviour that only happen randomly (eg lock order checks). In our code, it's never correct to call |
…kAssertion -BEGIN VERIFY SCRIPT- git grep -l AssertLockHeldInternal | xargs sed -i /AssertLockHeldInternal/s/EXCLUSIVE_LOCKS_REQUIRED/ASSERT_EXCLUSIVE_LOCK/ git grep -l AssertLockHeld ':!src/sync.h' | xargs sed -i '/^ *AssertLockHeld(.*);/d' git grep -l LockAssertion | xargs sed -i 's/LockAssertion lock(\(.*\));/AssertLockHeld(\1);/g' -END VERIFY SCRIPT-
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Can be closed? |
|
Let me know if I should reopen |
This PR is a simple 3-line scripted diff followed by a documentation cleanup. The scripted diff does three things:
AssertLockHeldcalls. Since Do not hide compile-time thread safety warnings #19668,AssertLockHeldhas had anEXCLUSIVE_LOCKS_REQUIREDannotation which has guaranteed (though ongoing travis and cirrus CI builds since Do not hide compile-time thread safety warnings #19668 was merged) that theseAssertLockHeldcalls are redundant withEXCLUSIVE_LOCKS_REQUIREDorEXCLUSIVE_LOCK_FUNCTIONannotations already present in surrounding code, and specifically that:Reverts
AssertLockHeldimplementation which got changed in Do not hide compile-time thread safety warnings #19668 to it's original pre-Do not hide compile-time thread safety warnings #19668 state. In Do not hide compile-time thread safety warnings #19668,AssertLockHeldwas changed to have anEXCLUSIVE_LOCKS_REQUIREDannotation instead of having anASSERT_EXCLUSIVE_LOCKannotation. As described above, having anEXCLUSIVE_LOCKS_REQUIREDannotation on an assert statement is only useful as proof that the assert statement doesn't do anything or convey any new information. By contrast, having anASSERT_EXCLUSIVE_LOCKannotation on an assert statement can be an actually useful way of conveying to the compiler than a specific mutex is locked at a specific place in the code, when the compiler thread safety analysis can't determine that on its own (because of lost type information).Removes
LockAssertionclass, replacing all current uses with calls toAssertLockHeld. Ever since theLockAssertionclass was first introduced in Refactor: Start to separate wallet from node #14437 (asLockAnnotation), it has always been an inferior alternative toAssertLockHeldand not had a reason to exist. (Refactor: Start to separate wallet from node #14437 was originally written before [net] Thread safety annotations in net_processing #13423 which added ASSERT_EXCLUSIVE_LOCK annotation. It was justified when the code was first written, but no longer necessary by the time it was merged).Motivation for this PR is to get rid of confusion between different types of lock assertions and only keep one assertion:
AssertLockHeldwhich goes back to the implementation it's had since #13423 was merged until #19668 was merged. After this PR,AssertLockHeldonly needs to be used sparingly to augment compile-time thread safety annotations, which are a superior alternative to runtime checks for guaranteeing thread safety.PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs