Add compile time checking for run time locking assertions #12665

practicalswift · 2018-03-10T17:17:32Z

Add compile time checking for all run time locking assertions.

The intention is that literally all EXCLUSIVE_LOCKS_REQUIRED/LOCKS_EXCLUDED:s added in this PR should follow either directly or indirectly from AssertLockHeld(…)/AssertLockNotHeld(…):s already existing in the repo.

Consider the case where function A(…) contains AssertLockHeld(cs_foo) (without
first locking cs_foo in A), and that B(…) calls A(…) (without first locking cs_main):

It directly follows that: A(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.
It indirectly follows that: B(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.

maflcko · 2018-03-11T21:26:00Z

src/txmempool.cpp

This 1 line diff should be a separate pull request

That assertion does not seem to hold, so I think it should be removed. I'll investigate.

maflcko · 2018-03-11T21:27:01Z

Concept ACK

practicalswift · 2018-03-12T06:59:26Z

@MarcoFalke Updated! Please review :-)

laanwj · 2018-03-13T11:49:13Z

Concept ACK - Unlike the title implies, not all the added run time checking requirements have an associated AssertLockHeld/AssertLockNotHeld (e.g. CoinSelection) is this on purpose?

practicalswift · 2018-03-13T13:52:03Z

@laanwj Thanks for reporting. The annotations for CoinSelection and two other functions were incorrect. That is now fixed.

The intention is that literally all EXCLUSIVE_LOCKS_REQUIRED/LOCKS_EXCLUDED:s added in this PR should follow either directly or indirectly from AssertLockHeld(…)/AssertLockNotHeld(…):s already existing in the repo.

Consider the case where function A(…) contains AssertLockHeld(cs_foo) (without
first locking cs_foo in A), and that B(…) calls A(…) (without first locking cs_main):

It directly follows that: A(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.
It indirectly follows that: B(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.

I'll run through all annotations again to verify that they all follow directly or indirectly from AssertLockHeld(…)/AssertLockNotHeld(…):s.

These are the annotations added – let me know if something sticks out/looks weird:

[Edit: List moved to PR description. See above!]

Sjors · 2018-03-13T20:57:49Z

Sadly that still didn't catch my mistake here, but I assume this is a step in that direction.

Lightly tested on macOS (no change, as expected).

@practicalswift maybe you can put that overview and additional explanation ("directly follows that...") in the commit message or PR description?

I didn't see a EXCLUSIVE_LOCKS_REQUIRED in LoadChainTip; I assume that's done in a some dependent function? Maybe that can be made explicit in your overview if it's not too tedious?

The EXLCUSIVE_LOCKS... stuff in validation.h is not repeated in validation.cpp, that's intentional?

Should violations merely throw warnings?

meshcollider · 2018-03-13T21:11:53Z

Concept ACK

practicalswift · 2018-03-14T09:19:32Z

@Sjors No intention to repeat annotations :-) Which specific annotations did you see repeated in validation.cpp? Did you check them in the files or just in the summary?

Violations are checked by Travis and cause a Travis build failure if found – see configure.ac.

practicalswift · 2018-03-14T09:22:51Z

@Sjors Did you mean that you didn't see any AssertLockHeld(…) in LoadChainTip? :-)

Sjors · 2018-03-14T14:10:36Z

Which specific annotations did you see repeated in validation.cpp

None, so that's all good. I just found it slightly confusing that some annotations are in .h files and others in .cpp files.

Did you mean that you didn't see any AssertLockHeld(…) in LoadChainTip?

Yes, sorry. To more specific, no AssertLockHeld(mempool.cs). But if you're happy and the compiler is happy, I'm happy :-)

practicalswift · 2018-04-25T21:41:28Z

@MarcoFalke Thanks for the utACK!

Added a commit which changed from mempool.cs to ::mempool.cs.

I've now split up this PR into four separate PRs – one per lock:

cs_KeyStore: Add compile time checking for all cs_KeyStore runtime locking assertions #13077
::mempool.cs: mempool: Add compile time checking for ::mempool.cs runtime locking assertions #13080
cs_wallet: wallet: Add compile time checking for cs_wallet runtime locking assertions #13081
cs_main: Add compile time checking for cs_main runtime locking assertions #13083

Please review these four :-)

…ocking assertions 66dc662 Add compile time checking for all cs_KeyStore runtime locking assertions (practicalswift) Pull request description: Add compile time checking for all `cs_KeyStore` runtime locking assertions. This PR is a subset of #12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: 84ee5459e7f75f9affaa4275cb760576ab0e26da8a48b9a4a59b51a25418fe4b862ba832cb01312479a8c9b0e38ee8b6c4076bcbbd73213346d09ec2057fc9f1

…time locking assertions cbba1d2 Add compile time checking for all ::mempool.cs runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `::mempool.cs` runtime locking assertions. This PR is a subset of #12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: 1b5ec1cfca6be67edd1298fea1a52b5572ce833dd4ad05c4583f753c2d3229402663373675df87e950151d5c41aeb3ee02f0ad935ed83fe2f45ca8e4d55d901e

… locking assertions 66b0b1b Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` runtime locking assertions. This PR is a subset of #12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd

…ssertions 9e0a514 Add compile time checking for all cs_main runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_main` runtime locking assertions. This PR is a subset of #12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6

….cs runtime locking assertions cbba1d2 Add compile time checking for all ::mempool.cs runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `::mempool.cs` runtime locking assertions. This PR is a subset of bitcoin#12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: 1b5ec1cfca6be67edd1298fea1a52b5572ce833dd4ad05c4583f753c2d3229402663373675df87e950151d5c41aeb3ee02f0ad935ed83fe2f45ca8e4d55d901e

…runtime locking assertions 66b0b1b Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` runtime locking assertions. This PR is a subset of bitcoin#12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd

…cking assertions 9e0a514 Add compile time checking for all cs_main runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_main` runtime locking assertions. This PR is a subset of bitcoin#12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6

…cking assertions 9e0a514 Add compile time checking for all cs_main runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_main` runtime locking assertions. This PR is a subset of bitcoin#12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6 # Conflicts: # src/validation.cpp # src/validation.h # src/wallet/feebumper.cpp # src/wallet/rpcwallet.cpp # src/wallet/wallet.h # Conflicts: # src/wallet/wallet.h

…runtime locking assertions 66b0b1b Add compile time checking for all cs_wallet runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_wallet` runtime locking assertions. This PR is a subset of bitcoin#12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: d561d89e98a823922107e56dbd493f0f82e22edac91e51e6422f17daf2b446a70c143b7b157ca618fadd33d0ec63eb7a57dde5a83bfdf1fc19d71459b43e21fd

….cs runtime locking assertions cbba1d2 Add compile time checking for all ::mempool.cs runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `::mempool.cs` runtime locking assertions. This PR is a subset of bitcoin#12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: 1b5ec1cfca6be67edd1298fea1a52b5572ce833dd4ad05c4583f753c2d3229402663373675df87e950151d5c41aeb3ee02f0ad935ed83fe2f45ca8e4d55d901e

…cking assertions 9e0a514 Add compile time checking for all cs_main runtime locking assertions (practicalswift) Pull request description: Add compile time checking for `cs_main` runtime locking assertions. This PR is a subset of bitcoin#12665. The PR was broken up to make reviewing easier. The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo. Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`): * It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. * It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation. Tree-SHA512: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6

fanquake added the Refactoring label Mar 10, 2018

practicalswift changed the title ~~Add compile time checking for all run time locking assertions~~ Add compile time checking for all run time locking assertions [wip] Mar 11, 2018

practicalswift force-pushed the compile-time-checking-of-runtime-assertions branch 4 times, most recently from 3e4a3f7 to aa224a2 Compare March 11, 2018 16:20

maflcko reviewed Mar 11, 2018

View reviewed changes

practicalswift mentioned this pull request Mar 11, 2018

mempool: Avoid locking mutex that is already held by the same thread #12671

Closed

practicalswift force-pushed the compile-time-checking-of-runtime-assertions branch 5 times, most recently from ad1df86 to 7443c01 Compare March 11, 2018 23:01

practicalswift changed the title ~~Add compile time checking for all run time locking assertions [wip]~~ Add compile time checking for all run time locking assertions Mar 11, 2018

practicalswift mentioned this pull request Mar 12, 2018

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

Closed

practicalswift changed the title ~~Add compile time checking for all run time locking assertions~~ Add compile time checking for run time locking assertions Mar 13, 2018

practicalswift force-pushed the compile-time-checking-of-runtime-assertions branch from 7443c01 to abaef4e Compare March 13, 2018 12:28

practicalswift force-pushed the compile-time-checking-of-runtime-assertions branch 2 times, most recently from 9fa76f6 to 38f1ab0 Compare March 14, 2018 09:20

practicalswift force-pushed the compile-time-checking-of-runtime-assertions branch from 38f1ab0 to 7286205 Compare March 14, 2018 11:28

practicalswift force-pushed the compile-time-checking-of-runtime-assertions branch from 7286205 to c75b691 Compare March 14, 2018 18:32

This was referenced Apr 25, 2018

wallet: Add compile time checking for cs_wallet runtime locking assertions #13081

Merged

Add compile time checking for cs_main runtime locking assertions #13083

Merged

practicalswift closed this May 2, 2018

practicalswift deleted the compile-time-checking-of-runtime-assertions branch April 10, 2021 19:34

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

Add compile time checking for run time locking assertions #12665

Add compile time checking for run time locking assertions #12665

Uh oh!

Conversation

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

Uh oh!

Uh oh!

maflcko Mar 11, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift Mar 11, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 11, 2018

Uh oh!

practicalswift commented Mar 12, 2018

Uh oh!

laanwj commented Mar 13, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

meshcollider commented Mar 13, 2018

Uh oh!

practicalswift commented Mar 14, 2018

Uh oh!

practicalswift commented Mar 14, 2018

Uh oh!

Sjors commented Mar 14, 2018

Uh oh!

practicalswift commented Apr 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

practicalswift commented Mar 10, 2018 •

edited

Loading

practicalswift commented Mar 13, 2018 •

edited

Loading

Sjors commented Mar 13, 2018 •

edited

Loading