refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it #16034
+33
−30
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
This change seems reasonable, but I don't think LockAnnotation is a good name for this class if it can potentially assert and abort the program. Would suggest adding a scripted diff to rename LockAnnotation to |
Contributor
Author
|
@ryanofsky Thanks for the quick review! I've now added a |
Contributor
|
Please update OP with the rename to |
practicalswift
changed the title
Contributor
Author
|
@promag Done! Please review :-) |
Contributor
|
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. |
ryanofsky
approved these changes
May 16, 2019
Contributor
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK last 4 commits of 8e81c1a87b45fea00bc0719569bd98ed61c1f6f4
…s hold also in practice at runtime (ifdef DEBUG_LOCKORDER)
-BEGIN VERIFY SCRIPT- git grep -l LockAnnotation | xargs sed -i 's/LockAnnotation/LockAssertion/' -END VERIFY SCRIPT-
practicalswift
force-pushed
the
make-sure-LockAnnotation-promises-are-truthful
branch
from
8e81c1a to
9f85e9c
Compare
Contributor
Author
|
Rebased! Please re-review :-) |
maflcko
changed the title
maflcko
pushed a commit
that referenced
this pull request
May 23, 2019
… add run-time check to it 9f85e9c scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift) de9b5db Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift) 3a80944 Move LockAnnotation to make it reflect the truth (practicalswift) cc25885 Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift) Pull request description: `LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise). Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises. This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`). Issues like the one described in #16028 will be discovered immediately with this PR merged. Changes in this PR: * Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code) * Move `LockAnnotation` in `wallet_tests` to make it reflect the truth * Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`) * Rename `LockAnnotation` to `LockAssertion` ACKs for commit 9f85e9: ryanofsky: utACK 9f85e9c. No changes at all since last review except clean rebase after base PR #16033 was merged Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 24, 2019
…ion and add run-time check to it 9f85e9c scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift) de9b5db Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift) 3a80944 Move LockAnnotation to make it reflect the truth (practicalswift) cc25885 Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift) Pull request description: `LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise). Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises. This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`). Issues like the one described in bitcoin#16028 will be discovered immediately with this PR merged. Changes in this PR: * Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code) * Move `LockAnnotation` in `wallet_tests` to make it reflect the truth * Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`) * Rename `LockAnnotation` to `LockAssertion` ACKs for commit 9f85e9: ryanofsky: utACK 9f85e9c. No changes at all since last review except clean rebase after base PR bitcoin#16033 was merged Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
May 27, 2020
…de) to sync.h (our code) Summary: bitcoin/bitcoin@cc25885 --- Move LockAnnotation to make it reflect the truth bitcoin/bitcoin@3a80944 --- Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) bitcoin/bitcoin@de9b5db --- Depends on D6259 Partial backport of Core [[bitcoin/bitcoin#16034 | PR16034]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6261
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
May 28, 2020
Summary: -BEGIN VERIFY SCRIPT- git grep -l LockAnnotation | xargs sed -i 's/LockAnnotation/LockAssertion/' -END VERIFY SCRIPT- bitcoin/bitcoin@9f85e9c --- Depends on D6261 Concludes backport of Core [[bitcoin/bitcoin#16034 | PR16034]] Test Plan: cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DENABLE_WERROR=ON -DENABLE_SANITIZERS=thread ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6262
practicalswift
deleted the
make-sure-LockAnnotation-promises-are-truthful
branch
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
May 17, 2022
…ion and add run-time check to it 9f85e9c scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift) de9b5db Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift) 3a80944 Move LockAnnotation to make it reflect the truth (practicalswift) cc25885 Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift) Pull request description: `LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise). Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises. This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`). Issues like the one described in bitcoin#16028 will be discovered immediately with this PR merged. Changes in this PR: * Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code) * Move `LockAnnotation` in `wallet_tests` to make it reflect the truth * Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`) * Rename `LockAnnotation` to `LockAssertion` ACKs for commit 9f85e9: ryanofsky: utACK 9f85e9c. No changes at all since last review except clean rebase after base PR bitcoin#16033 was merged Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a
malbit
added a commit
to malbit/raptoreum
that referenced
this pull request
Aug 7, 2022
[bitcoin#14624](bitcoin/bitcoin#14624) bls refactoring to resolve clang warnings [bitcoin#16426](bitcoin/bitcoin#16426) - cs_wallet lock order refactoring and reduce cs_main locking atomic smartnode_connection (allow read/write by different threads simultaneously) cs_mnauth locks on verifiedProRegTxHash read RecursiveMutex at locking access to activeSmartNode [bitcoin#14307](bitcoin/bitcoin#14307) - Consolidate redundant implementation of ParseHashStr [bitcoin#13399](bitcoin/bitcoin#13399) - rpc: Add submitheader built-in miner deleted [bitcoin#17781](bitcoin/bitcoin#17781) - Remove mempool global from miner [bitcoin#16623](bitcoin/bitcoin#16624) - encapsulate transactions state [bitcoin#15931](bitcoin/bitcoin#15931) - Remove GetDepthInMainChain dependency on locked chain interface Pass CConnman to function against global pointer [bitcoin#16839](bitcoin/bitcoin#16839) - Replace Connman and BanMan globals with NodeContext local [bitcoin#16034](bitcoin/bitcoin#16034) - refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it [bitcoin#17564](bitcoin/bitcoin#17564) - Use mempool from node context instead of global [bitcoin#18740](bitcoin/bitcoin#18740) - Remove g_rpc_node global [bitcoin#19096](bitcoin/bitcoin#19096) - Remove g_rpc_chain global [bitcoin#18338](bitcoin/bitcoin#18338) - Fix wallet unload race condition other fixes and tweaks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
LockAnnotation lock(mutex);is a guarantee to the compiler thread-analysis thatmutexis locked (when it couldn't be determined otherwise).Before this PR it was possible to make the mistake of adding a
LockAnnotationwhere the correct mutex is not held. This in turn makes the thread-analysis reasoning being based on incorrect premises.This PR adds an assertion in the
LockAnnotationctor which checks that the guarantees given by us at compile-time are held also in practice (ifdef DEBUG_LOCKORDER).Issues like the one described in #16028 will be discovered immediately with this PR merged.
Changes in this PR:
LockAnnotationfromthreadsafety.h(imported code) tosync.h(our code)LockAnnotationinwallet_teststo make it reflect the truthLockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER)LockAnnotationtoLockAssertion