Replace LockAssertion with AssertLockHeld, remove LockAssertion #19979
Conversation
There was a problem hiding this comment.
Code review ACK 774face
Love this approach! This solves the main problem I'm concerned about, which is having multiple nearly identical assert implementations. The minor downsides to this approach are:
-
May be less efficient because invoking a callback through a std::function can require a heap allocation and indirect call through a function pointer instead of an inlined call or call to a fixed address.
-
This pattern for bypassing thread safety analysis--annotating a lambda with a EXCLUSIVE_LOCKS_REQUIRED and assigning the lambda to a std::function which ignores the annotation--isn't as nice as directly telling the analysis a lock is held using an ASSERT_EXCLUSIVE_LOCK function. An ASSERT_EXCLUSIVE_LOCK call makes sure unprovable assertions look different than proven annotation, and ensures they are accompanied by runtime check.
Because of these limitations, it's possible in the future we may want to provide an assert implementation annotated with ASSERT_EXCLUSIVE_LOCK. But this seems like a good step to eventually getting there (again).
|
As mentioned in previous prs, clang may have accidental bugs or intentional limitations when checking for locks held at compile time. So removing the run-time checks seems dangerous, see also #19865 (comment) . For example, the following diff compiles, but is obviously wrong: diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b256d719a6..b6006427a7 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1336,6 +1336,8 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
std::shared_ptr<const CBlockHeaderAndShortTxIDs> pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs> (*pblock, true);
const CNetMsgMaker msgMaker(PROTOCOL_VERSION);
+ CConnman::NodeFn fun;
+ {
LOCK(cs_main);
static int nHighestFastAnnounce = 0;
@@ -1353,8 +1355,7 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
most_recent_compact_block = pcmpctblock;
fWitnessesPresentInMostRecentCompactBlock = fWitnessEnabled;
}
-
- m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+ fun =[this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
// TODO: Avoid the repeated-serialization here
if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
return;
@@ -1370,7 +1371,10 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
m_connman.PushMessage(pnode, msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock));
state.pindexBestHeaderSent = pindex;
}
- });
+ };
+ }// release cs_main
+
+ m_connman.ForEachNode(fun);
}
/** |
This moves the run time checks outside of the lambdas. I don't think it's fair to say it removes them. PR could be tweaked to move the checks inside the lambdas instead of outside if you prefer that. It is true in general that the approach of annotating a lambda and calling it through a std::function so the annotation will not be enforced is less safe than using an ASSERT_EXCLUSIVE_LOCK assert, because an ASSERT_EXCLUSIVE_LOCK assert pairs any unproven declaration that a mutex is locked together with a runtime check. With this approach in this PR, you are responsible for doing the check separately yourself. |
|
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. |
|
Updated 774face -> 0bd1184 (pr19979.01 -> pr19979.02, diff):
|
If that's a concern, use something like LLVM's
Right, casting away the annotations is not nice, the assertion is probably the best you can do here. Since the annotations live on declarations and not on types, we can't warn about these casts because there is no way to fix that warning. You can't annotate the function pointer type. Edit: thinking about it again, we could warn and for example require an explicit cast or |
|
ACK 0bd1184 |
There was a problem hiding this comment.
ACK 0bd1184
Looks good - this leaves just one lock assertion method - AssertLockHeld() and uses some hacktweak to deal with the few exceptional cases where lambda callbacks are involved. I think those few cases fall within the clang's limitations and we shouldn't extort the compiler to provide maximum amount of warnings in those cases. In #19929 I tagged them with NO_THREAD_SAFETY_ANALYSIS.
Thanks!
This may provide some justification for the "always do an AssertLockHeld(cs) in the functions with EXCLUSIVE_LOCKS_REQUIRED(cs)" rule -- any function that gets called via a std::function cast or a function pointer generally won't have the lock validity checked at compile time, and (I think) there's no way to prevent functions from being called that way, so having the runtime check is a good backup. I guess there's no easy way to add a linter to check that we don't have functions with EXCLUSIVE_LOCKS_REQUIRED that lack an AssertLockHeld? If there were a way to prevent an EXCLUSIVE_LOCKS_REQUIRED function from being called via std::function etc and having its annotation discarded, then I think we'd want to enable that, and switch to an ASSERT_LOCK_HELD approach for the lambdas, since then we could guarantee all the EXCLUSIVE_LOCKS_REQUIRED things are actually checked at compile time. But assuming we can't do that today, then this approach is no worse at catching bugs than other alternatives, and simpler to deal with than any of them (since it's just "always use EXCLUSIVE_LOCKS_REQUIRED, always use AssertLockHeld") as far as I can see. I don't think the description ("eliminates from the code base all cases when Clang Thread Safety Analysis cannot determine if a lock is held") is quite accurate though -- clang still isn't able to determine the lock is held in these lambdas. ACK 0bd1184 |
What about "Replace LockAssertion with AssertLockHeld, remove LockAssertion" as OP? Other than that, I think this can go in. |
Done. |
…e LockAssertion 0bd1184 Remove unused LockAssertion struct (Hennadii Stepanov) ab2a442 Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov) 73f71e1 refactor: Use explicit function type instead of template (Hennadii Stepanov) Pull request description: This PR replaces `LockAssertion` with `AssertLockHeld`, and removes `LockAssertion`. This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs ACKs for top commit: MarcoFalke: ACK 0bd1184 ajtowns: ACK 0bd1184 vasild: ACK 0bd1184 Tree-SHA512: ef7780dd689faf0bb479fdb97c49bc652e2dd10c148234bb95502dfbb676442d8565ee37864d923ca21a25f9dc2a335bf46ee82c095e387b59a664ab05c0ae41
We can warn, but we need a good idea on a way out. Since template<typename F>
F thread_safety_cast(F&& f) NO_THREAD_SAFETY_ANALYSIS
{
return std::forward<F>(f);
}that would allow casting away annotations. But I haven't thought this through yet. |
| @@ -793,25 +793,6 @@ bool ChainstateManager::ProcessNewBlock(...) | |||
| } | |||
| ``` | |||
|
|
|||
| - When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances: | |||
There was a problem hiding this comment.
In commit "Remove unused LockAssertion struct" (0bd1184)
Instead of removing this section, maybe it would have been better to update. The example could be updated with EXCLUSIVE_LOCKS_REQUIRED to be consistent with the code. And the text could be something like "When Clang Thread Safety Analysis is unable to determine if a mutex is locked, it is recommended to disable the analysis or to call the code through a function annotated with EXCLUSIVE_LOCKS_REQUIRED, using an indirect call so the requirement is not checked."
There was a problem hiding this comment.
Imo using NO_THREAD_SAFETY_ANALYSIS (or thread_safety_cast above) or EXCLUSIVE_LOCKS_REQUIRED on the lambda is equivalent, as the resulting binary, as well as any compile time errors/warnings should be the same.
As there is no observable difference (except in the written code itself), is there a reason to mention it in the dev notes?
There was a problem hiding this comment.
Imo, they're both not ideal. EXCLUSIVE_LOCKS_REQUIRED is preferable to NO_THREAD_SAFETY_ANALYSIS because it makes which mutex is assumed to be held explicit, and it doesn't disable checking for other mutexes. You might need to assume cs_main is held somewhere, and still want errors if you forget LOCK(cs_wallet) in the same function.
If it was useful before to have a section in developer notes advising how to deal with TSA errors, it's probably more useful now that the current practice is to add annotations that look like they might be enforced, but aren't enforced. But I don't know if the section was useful before it was removed. Just wanted to suggest here that it could be updated if it was useful.
There was a problem hiding this comment.
You might need to assume cs_main is held somewhere, and still want errors if you forget LOCK(cs_wallet) in the same function.
Good point. Yeah, I agree the new "style" could be mentioned in the docs.
Summary: PR description: > This PR replaces LockAssertion with AssertLockHeld, and removes LockAssertion. > > This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs This is a backport of [[bitcoin/bitcoin#19979 | core#19979]] [1/3] bitcoin/bitcoin@73f71e1 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10171
Summary: This is a backport of [[bitcoin/bitcoin#19979 | core#19979]] [2/3] bitcoin/bitcoin@ab2a442 This partially revert D10161 Depends on D10171 Test Plan: With TSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10172
Summary: This is a backport of [[bitcoin/bitcoin#19979 | core#19979]] [3/3] bitcoin/bitcoin@0bd1184 Depends on D10172 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10173
This PR replaces
LockAssertionwithAssertLockHeld, and removesLockAssertion.This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs