Replace LockAssertion with AssertLockHeld, remove LockAssertion

hebasto commented at 9:20 am on September 19, 2020: member

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

DrahtBot added the label Docs on Sep 19, 2020

DrahtBot added the label P2P on Sep 19, 2020

hebasto commented at 10:05 am on September 19, 2020: member

cc @ajtowns @MarcoFalke @ryanofsky @vasild

MarcoFalke removed the label Docs on Sep 19, 2020

MarcoFalke removed the label P2P on Sep 19, 2020

MarcoFalke added the label Refactoring on Sep 19, 2020

ryanofsky approved

ryanofsky commented at 11:14 am on September 19, 2020: member

Code review ACK 774face46174120ea7f548911895416bc4794eaa

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

MarcoFalke commented at 12:35 pm on September 19, 2020: member

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:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index b256d719a6..b6006427a7 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -1336,6 +1336,8 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
 5     std::shared_ptr<const CBlockHeaderAndShortTxIDs> pcmpctblock = std::make_shared<const CBlockHeaderAndShortTxIDs> (*pblock, true);
 6     const CNetMsgMaker msgMaker(PROTOCOL_VERSION);
 7 
 8+    CConnman::NodeFn fun;
 9+    {
10     LOCK(cs_main);
11 
12     static int nHighestFastAnnounce = 0;
13@@ -1353,8 +1355,7 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
14         most_recent_compact_block = pcmpctblock;
15         fWitnessesPresentInMostRecentCompactBlock = fWitnessEnabled;
16     }
17-
18-    m_connman.ForEachNode([this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
19+    fun =[this, &pcmpctblock, pindex, &msgMaker, fWitnessEnabled, &hashBlock](CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
20         // TODO: Avoid the repeated-serialization here
21         if (pnode->nVersion < INVALID_CB_NO_BAN_VERSION || pnode->fDisconnect)
22             return;
23@@ -1370,7 +1371,10 @@ void PeerManager::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_
24             m_connman.PushMessage(pnode, msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock));
25             state.pindexBestHeaderSent = pindex;
26         }
27-    });
28+    };
29+    }// release cs_main
30+
31+    m_connman.ForEachNode(fun);
32 }
33 
34 /**

in src/net.h:272 in 774face461 outdated

268@@ -268,8 +269,7 @@ class CConnman
269         }
270     };
271 
272-    template<typename Callable>
273-    void ForEachNode(Callable&& func) const
274+    void ForEachNode(NodeFn func) const

MarcoFalke commented at 12:37 pm on September 19, 2020:

As the func is not consumed, shouldn’t this keep the uref &&?

ryanofsky commented at 1:05 pm on September 19, 2020:

This has to take a std::function argument so the lambda object is converted to a std::function and the EXCLUSIVE_LOCKS_REQUIRED requirement can by bypassed, otherwise static analysis will not accept this

MarcoFalke commented at 1:17 pm on September 19, 2020:

0    void ForEachNode(const NodeFn& func) const

should work and be preferable? Obviously a style nit, as it doesn’t make a difference with the current code.

hebasto commented at 3:12 pm on September 19, 2020:

Thanks! Updated.

ryanofsky commented at 12:59 pm on September 19, 2020: member

So removing the run-time checks seems dangerous

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.

DrahtBot commented at 1:22 pm on September 19, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19970 (sync.h: fix LockAssertion error reporting by ajtowns)
#19918 (sync: Replace LockAssertion with AssertLockHeldUnverified by ryanofsky)
#19865 (scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion by ryanofsky)

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.

refactor: Use explicit function type instead of template 73f71e1996

Replace LockAssertion with a proper thread safety annotations ab2a44297f

Remove unused LockAssertion struct 0bd1184adf

hebasto force-pushed on Sep 19, 2020

hebasto commented at 3:11 pm on September 19, 2020: member

Updated 774face46174120ea7f548911895416bc4794eaa -> 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25 (pr19979.01 -> pr19979.02, diff):

addressed @MarcoFalke’s comment:

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)

addressed @MarcoFalke’s style nit

aaronpuchert commented at 5:36 pm on September 19, 2020: none

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.

If that’s a concern, use something like LLVM’s function_ref. Still an indirect call, but without heap allocation.

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.

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 NO_THREAD_SAFETY_ANALYSIS. But it would be an uphill battle.

hebasto commented at 9:30 pm on September 19, 2020: member

FWIW, the same approach has already been used in the project code base in 6a72f26968cf931c985d8d4797b6264274cabd06 from #16426 by @ariard.

MarcoFalke commented at 6:40 am on September 20, 2020: member

ACK 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25

vasild approved

vasild commented at 12:41 pm on September 21, 2020: member

ACK 0bd1184ad

Looks good - this leaves just one lock assertion method - AssertLockHeld() and uses some ~~hack~~tweak 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!

MarcoFalke requested review from ajtowns on Sep 22, 2020

ajtowns commented at 11:16 am on September 23, 2020: member

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.

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 0bd1184adf6610c0bd14f4e9a25c0a200e65ae25

MarcoFalke commented at 1:57 pm on September 23, 2020: member

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.

What about “Replace LockAssertion with AssertLockHeld, remove LockAssertion” as OP?

Other than that, I think this can go in.

hebasto renamed this:
~~Use proper TSA attributes (attempt two)~~
Replace LockAssertion with AssertLockHeld, remove LockAssertion
on Sep 23, 2020

hebasto commented at 2:01 pm on September 23, 2020: member

@MarcoFalke

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.

What about “Replace LockAssertion with AssertLockHeld, remove LockAssertion” as OP?

Done.

MarcoFalke merged this on Sep 23, 2020

MarcoFalke closed this on Sep 23, 2020

hebasto deleted the branch on Sep 23, 2020

sidhujag referenced this in commit f43f51f706 on Sep 23, 2020

aaronpuchert commented at 0:00 am on September 24, 2020: none

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.

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?

We can warn, but we need a good idea on a way out. Since NO_THREAD_SAFETY_ANALYSIS also disables checking for arguments, users could have a function

0template<typename F>
1F thread_safety_cast(F&& f) NO_THREAD_SAFETY_ANALYSIS
2{
3    return std::forward<F>(f);
4}

that would allow casting away annotations. But I haven’t thought this through yet.

in doc/developer-notes.md:796 in 0bd1184adf

792@@ -793,25 +793,6 @@ bool ChainstateManager::ProcessNewBlock(...)
793 }
794 ```
795 
796-- When Clang Thread Safety Analysis is unable to determine if a mutex is locked, use `LockAssertion` class instances:

ryanofsky commented at 3:49 pm on September 24, 2020:

In commit “Remove unused LockAssertion struct” (0bd1184adf6610c0bd14f4e9a25c0a200e65ae25)

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

MarcoFalke commented at 3:55 pm on September 24, 2020:

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?

ryanofsky commented at 4:17 pm on September 24, 2020:

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.

MarcoFalke commented at 4:34 pm on September 24, 2020:

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.

deadalnix referenced this in commit f417c6b567 on Sep 22, 2021

deadalnix referenced this in commit a43673237b on Sep 22, 2021

deadalnix referenced this in commit 4e2a295338 on Sep 22, 2021

DrahtBot locked this on Feb 15, 2022

Replace LockAssertion with AssertLockHeld, remove LockAssertion #19979

Conflicts