[WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) #11226

practicalswift · 2017-09-03T16:51:58Z

Add additional Clang thread safety analysis annotations as discussed with @sipa (#10866 (comment)) and others.

This is a follow-up to #10866 ~~which this PR is rebased upon (awaiting merge of #10866). The first three commits (8a1dc09, e022990 and cbae151) can be reviewed in #10866.~~

This PR contains three types of changes:

Add missing locks (based on the GUARDED_BY(...) analysis below). Please review thoroughly.
Add GUARDED_BY(...) annotations to allow for Clang's thread safety analysis at compile-time. Does not change run-time behaviour.
Add EXCLUSIVE_LOCKS_REQUIRED(...) annotations to allow for Clang's thread safety analysis at compile-time. Does not change run-time behaviour.

Background reading: The "C/C++ Thread Safety Analysis" paper (Hutchins, Ballman & Sutherland, 2014) paper describing Clang thread safety analysis and how it is used for the Google C++ codebase:

They essentially provide a static type system for threads, and can detect potential race conditions and deadlocks. This paper describes Clang Thread Safety Analysis, a tool which uses annotations to declare and enforce thread safety policies in C and C++ programs.
[…]
It has been deployed on a large scale at Google; all C++ code at Google is now compiled with thread safety analysis enabled by default.
[…]
The GUARDED_BY attribute declares that a thread must lock mu before it can read or write to balance, thus ensuring that the increment and decrement operations are atomic. Similarly, REQUIRES declares that the calling thread must lock mu before calling withdrawImpl. Because the caller is assumed to have locked mu, it is safe to modify balance within the body of the method.

sipa · 2017-09-04T00:21:02Z

Awesome, I'm very happy to see someone work on this again.

sipa · 2017-09-04T09:00:02Z

You must have introduced a lock inversion, based on the Travis output.

promag · 2017-09-04T09:24:09Z

Squash commits Add GUARDED_BY(cs_KeyStore) and also Add GUARDED_BY(cs) annotation.

practicalswift · 2017-09-04T15:25:54Z

@sipa Thanks for letting me know! I've now removed the commit with the incorrectly added LOCK(walletInstance->cs_KeyStore) in wallet.cpp that caused the lock inversion. Do you which variables cs_KeyStore is meant to guard?

practicalswift · 2017-09-04T15:28:56Z

@promag Fixed! :-) Regarding the repeated GUARDED_BY(cs) commit messages - these actually refer to different cs:es. To make things less confusing I've now disambiguated them by adding the class names in the commit message.

Furthermore I've added an commit which renames renames CAddrMan.cs to CAddrMan.cs_addrMan, and CTxMemPool.cs to CTxMemPool.cs_txMemPool.

TheBlueMatt · 2017-09-04T20:17:19Z

Concept ACK. This will generate some turbulence in terms of needing to rebase a ton of existing PRs, so maybe it makes sense to break this into several PRs which only touch given modules? In any case, excited to see this stuff happen.

laanwj · 2017-09-06T22:28:28Z

Concept ACK. Though I'm not sure what a good time to merge a huge, spread-out (though low-risk) change like this is. Maybe just before the 0.16 split?

practicalswift · 2017-09-11T08:34:47Z

@laanwj Good point! When is the 0.16 split expected to happen?

The smaller PR #10866 lays the groundwork for the subsequent thread-safety-analysis annotation work. Having that PR merged a bit earlier would help a lot :-)

practicalswift · 2017-09-11T08:40:19Z

@TheBlueMatt Sure, I can split this PR into multiple parts to ease reviewing! Just let me know which parts/modules to split it on :-) To avoid duplicating the commits in #10866 we should probably wait until #10866 is merged before splitting this PR up. Sounds reasonable?

practicalswift · 2017-09-11T09:13:39Z

@sipa @TheBlueMatt @laanwj Thanks for reviewing and thanks for the encouragement! Do you happen to know which core developers that would be appropriate reviewers for this work (on a deeper GUARDED_BY(…) level for each module)?

The types of errors I might need some help from reviewers to avoid:

False positives: Too restrictive lock requirements. Requiring a lock where a lock is not needed. More concretely: unnecessary GUARDED_BY(…) annotations. (The EXCLUSIVE_LOCKS_REQUIRED(…) annotations follow more or less mechanically for the GUARDED_BY(…).)
False negatives: Missing lock requirements. Not requiring a lock where a lock is needed. More concretely: missing GUARDED_BY(…) annotations.

And perhaps most importantly: help reviewing that the added locks in this PR looks reasonable. In constrast to the annotations they alter run-time behaviour.

There are a couple of AssertLockHeld(…):s that I've been unable to map to variables being guarded. I might need some help to track those down to add the corresponding GUARDED_BY(…):s (and hence EXCLUSIVE_LOCKS_REQUIRED(…)). I'm a bit hesitant to just add EXCLUSIVE_LOCKS_REQUIRED(…) to match the AssertLockHeld(…) (which would be trivial) since that methodology hides the fact that the guard required hasn't been explicitly stated using an GUARDED_BY(…) annotation yet.

practicalswift · 2017-09-13T20:39:19Z

The code base contains 37 locks and in the process of documenting them I compiled this list of the initial creators of each lock. These lock introducers would likely be the best possible reviewers for the GUARDED_BY(…):s in this PR for "their" respective locks :-)

Bitcoin locks hall of fame

Locks introduced by currently active core devs:

@TheBlueMatt (10 locks): cs_filter, cs_args, cs_vSend, cs_most_recent_block, cs_callbacks_pending (previously m_cs_callbacks_pending), cs_vAddedNodes, cs_SubVer, cs_addrName, cs_addrLocal, cs_sendProcessing
@sipa (8 locks): cs_KeyStore, cs_addrMan (previously cs), cs_mapLocalHost, cs_LastBlockFile, cs_vOneShots, cs_pathCached (previously csPathCached), cs_nTimeOffset, cs_nBlockSequenceId
@theuni (3 locks): cs_hSocket, cs_vRecv, cs_vProcessMsg
@morcos (2 locks): cs_feeEstimator, cs_feeFilter
@gmaxwell (1 lock): cs_warnings

Previous lock introducers:

Locks included in the first commit in the repo (Satoshi locks!): cs_main, cs_vNodes, cs_db, cs_inventory, cs_Shutdown
gavinandresen: cs_wallet, cs_txMemPool (previously cs), cs_setBanned
sje397: cs_totalBytesSent, cs_totalBytesRecv
Clark Gaebel: cs_test (previously cs)
domob1812: cs_rpcWarmup
Philip Kaufmann: cs_proxyInfos

practicalswift · 2017-09-14T20:27:50Z

I'm now down to only four remaining AssertLockHeld(…):s for which I'm unable to identify which specific variable the lock in question is being a guard for.

These are the four cases I need help with:

[validation.cpp] GetBlockScriptFlags(...) – AssertLockHeld(cs_main);
[validation.cpp] AcceptBlockHeader(...) – AssertLockHeld(cs_main);
[wallet/wallet.cpp] CWallet::RemoveWatchOnly(...) – AssertLockHeld(cs_wallet);
[wallet/wallet.cpp] CWallet::RescanFromTime(...) – AssertLockHeld(cs_wallet);

For each of the above cases I see two alternatives:

A GUARDED_BY(…) for some variable accessed directly or indirectly is missing.
The lock is not technically necessary.

practicalswift · 2017-09-15T09:17:58Z

I've now documented the five places where I'm opting out of thread-safety analysis for various reasons (using the NO_THREAD_SAFETY_ANALYSIS annotation): b72ad1f43d96b6f63e7a4046f31af085da4e86db

Please let me know if there is a way to remove any of these opt-outs.

TheBlueMatt · 2017-09-15T18:25:55Z

@practicalswift

GetBlockScriptFlags() needs cs_main for versionbitscache (maybe others, but probably just that?)
AcceptBlockHeader is mostly for mapBlockIndex it looks like
RemoveWatchOnly looks like it may need to be an atomic operation, but I haven't dug into it too much. It may very well be the case that you'd miss a NotifyWatchonlyChanged fire if you have an AddWatchOnly and a RemoveWatchOnly call at the same time (though I don't know if there are any negative effects from doing so).
RescanFromTime indeed does not appear to require cs_wallet (obviously ScanForWalletTransactions does, but it takes it itself). It obviously needs cs_main, though.

practicalswift · 2017-09-15T18:48:58Z

@TheBlueMatt Thanks! I'll look into the suggested fixes.

practicalswift · 2017-09-16T14:22:15Z

@jonasschnelli @TheBlueMatt In CDB::PeriodicFlush(CWalletDBWrapper& dbw) (code) we are currently trying to lock bitdb.cs_db – shouldn't that lock be on env->cs_db instead (CDBEnv *env = dbw.env)? Or am I reading it wrong? :-)

practicalswift · 2017-09-16T18:12:00Z

@laanwj, after some digging I found your bitdb reference cleanup commit be9e1a9, perhaps you know the answer to the question about the bitdb.cs_db lock above? :-)

…wallet when accessing m_last_block_processed.

maflcko · 2018-01-10T14:24:49Z

Needs rebase.
For the purpose of easing review and speeding up merge: What about submitting a subset of this that only renames the comments // protected by ${cs} to GUARDED_BY(${cs}). Assuming that the existing comments are reviewed such a subset should be refactoring-only and easier to merge.

practicalswift · 2018-01-31T16:22:26Z

@MarcoFalke Do you mean a new PR with the GUARDED_BY(…):s in this PR added instead as comments // GUARDED_BY(…)? Sure, confirm and I'll do that.

Regarding rebasing – I suggest we merge (or close in the case of NACK) the locking related PR:s I currently have open and I'll rebase this PR on top of master after that. Makes sense?

maflcko · 2018-02-02T23:38:09Z

Something like e7f4866, but at this point I am not sure if it is going to help review a lot.

practicalswift · 2018-02-22T21:46:15Z

Closing. Will split this into subsets and submit as separate PR:s.

practicalswift · 2018-03-12T22:37:04Z

To facilitate reviewing the most important parts of this thread-safety PR (milestone 0.17.0) have been submitted as individual PR:s –

Add compile time checking for run time locking assertions #12665 – Add compile time checking for all run time locking assertions
net: Avoid locking cs_vNodes twice when calling FindNode(...). Add NodeExists(...). #11795 – Avoid locking cs_vNodes twice when calling FindNode(...)
Avoid locking mutexes that are already held by the same thread #11762 – Avoid locking mutexes that are already held by the same thread
rpc: Add missing cs_main lock in getblocktemplate(...) #11694 – Add missing cs_main lock in getblocktemplate(...)
mempool: Fix missing locking in CTxMemPool::check(…) and CTxMemPool::setSanityCheck(…) #11689 – Fix missing locking in CTxMemPool::check(…) and CTxMemPool::setSanityCheck(…)
Add missing locks: validation.cpp + related #11652 – Add missing locks to init.cpp (in AppInitMain + ThreadImport) and validation.cpp
wallet: Add missing cs_wallet/cs_KeyStore locks to wallet #11634 – Add missing cs_wallet/cs_KeyStore locks to wallet
Avoid lock: Call FlushStateToDisk(...) regardless of fCheckForPruning #11617 – Call FlushStateToDisk(...) regardless of fCheckForPruning
Add missing cs_main locks when accessing chainActive #11596 – Add missing cs_main locks when accessing chainActive

Reviews of these PR:s would be really appreciated! :-)

I'll tackle the remaining parts once the above PR:s have been merged or closed. Complete thread safety annotation coverage is not far away now! :-)

109a858 tests: Add missing locks to tests (practicalswift) Pull request description: Add missing locks to tests to satisfy lock requirements (such as `EXCLUSIVE_LOCKS_REQUIRED(...)` (Clang Thread Safety Analysis, see bitcoin#11226), `AssertLockHeld(...)` and implicit lock assumptions). Tree-SHA512: 1aaeb1da89df1779f02fcceff9d2f8ea24a3926d421f9ea305a19be04dd0b3e63d91f6c1ed22fb7e6988343f6a5288829a387ef872cfa7b6add57bd01046b5d9 Signed-off-by: Pasta <pasta@dashboost.org> # Conflicts: # src/test/test_bitcoin.cpp

109a858 tests: Add missing locks to tests (practicalswift) Pull request description: Add missing locks to tests to satisfy lock requirements (such as `EXCLUSIVE_LOCKS_REQUIRED(...)` (Clang Thread Safety Analysis, see bitcoin#11226), `AssertLockHeld(...)` and implicit lock assumptions). Tree-SHA512: 1aaeb1da89df1779f02fcceff9d2f8ea24a3926d421f9ea305a19be04dd0b3e63d91f6c1ed22fb7e6988343f6a5288829a387ef872cfa7b6add57bd01046b5d9 Signed-off-by: Pasta <pasta@dashboost.org>

109a858 tests: Add missing locks to tests (practicalswift) Pull request description: Add missing locks to tests to satisfy lock requirements (such as `EXCLUSIVE_LOCKS_REQUIRED(...)` (Clang Thread Safety Analysis, see bitcoin#11226), `AssertLockHeld(...)` and implicit lock assumptions). Tree-SHA512: 1aaeb1da89df1779f02fcceff9d2f8ea24a3926d421f9ea305a19be04dd0b3e63d91f6c1ed22fb7e6988343f6a5288829a387ef872cfa7b6add57bd01046b5d9 Signed-off-by: Pasta <pasta@dashboost.org> # Conflicts: # src/test/test_bitcoin.cpp

practicalswift mentioned this pull request Sep 3, 2017

Fix -Wthread-safety-analysis warnings. Compile with -Wthread-safety-analysis if available. #10866

Merged

practicalswift force-pushed the guarded-by-trivial branch 2 times, most recently from 35a19eb to 2c4a137 Compare September 3, 2017 17:11

fanquake added the Refactoring label Sep 4, 2017

practicalswift force-pushed the guarded-by-trivial branch 2 times, most recently from 763b3d1 to 8729f81 Compare September 4, 2017 06:52

practicalswift force-pushed the guarded-by-trivial branch 3 times, most recently from b808f4c to d72e1e4 Compare September 4, 2017 14:33

practicalswift force-pushed the guarded-by-trivial branch from 09f74fe to 21083dd Compare September 4, 2017 19:29

practicalswift force-pushed the guarded-by-trivial branch from 76fc04e to d87a8ef Compare September 11, 2017 08:17

practicalswift force-pushed the guarded-by-trivial branch from 9262e91 to e555245 Compare September 13, 2017 07:40

practicalswift added 2 commits December 4, 2017 20:56

Set m_last_block_processed to nullptr in SetNull(). Acquire mutex cs_…

192a550

…wallet when accessing m_last_block_processed.

Avoid locking mutexes that are already held by the same thread

baf7c1f

practicalswift force-pushed the guarded-by-trivial branch from e329152 to baf7c1f Compare December 4, 2017 20:30

laanwj modified the milestones: 0.16.0, 0.17.0 Jan 11, 2018

practicalswift closed this Feb 22, 2018

practicalswift mentioned this pull request Apr 9, 2018

Add compile time checking for run time locking assertions #12665

Closed

practicalswift deleted the guarded-by-trivial branch April 10, 2021 19:33

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

[WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) #11226

[WIP] Add Clang thread safety analysis annotations: GUARDED_BY(lock) / EXCLUSIVE_LOCKS_REQUIRED(lock) #11226

Uh oh!

Conversation

practicalswift commented Sep 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Sep 4, 2017

Uh oh!

sipa commented Sep 4, 2017

Uh oh!

promag commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 4, 2017

Uh oh!

TheBlueMatt commented Sep 4, 2017

Uh oh!

laanwj commented Sep 6, 2017

Uh oh!

practicalswift commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 11, 2017

Uh oh!

practicalswift commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitcoin locks hall of fame

Uh oh!

practicalswift commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 15, 2017

Uh oh!

TheBlueMatt commented Sep 15, 2017

Uh oh!

practicalswift commented Sep 15, 2017

Uh oh!

practicalswift commented Sep 16, 2017

Uh oh!

practicalswift commented Sep 16, 2017

Uh oh!

maflcko commented Jan 10, 2018

Uh oh!

practicalswift commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 2, 2018

Uh oh!

practicalswift commented Feb 22, 2018

Uh oh!

practicalswift commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Sep 3, 2017 •

edited

Loading

promag commented Sep 4, 2017 •

edited

Loading

practicalswift commented Sep 4, 2017 •

edited

Loading

practicalswift commented Sep 11, 2017 •

edited

Loading

practicalswift commented Sep 11, 2017 •

edited

Loading

practicalswift commented Sep 13, 2017 •

edited

Loading

practicalswift commented Sep 14, 2017 •

edited

Loading

practicalswift commented Jan 31, 2018 •

edited

Loading

practicalswift commented Mar 12, 2018 •

edited

Loading