refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it #16034

practicalswift · 2019-05-16T11:45:49Z

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

ryanofsky · 2019-05-16T12:13:21Z

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 LockAssert or something similar.

practicalswift · 2019-05-16T13:18:44Z

@ryanofsky Thanks for the quick review! I've now added a scripted-diff which renames LockAnnotation to LockAssertion. Please re-review :-)

promag · 2019-05-16T14:26:27Z

Please update OP with the rename to LockAssertion.

practicalswift · 2019-05-16T14:49:41Z

@promag Done! Please review :-)

DrahtBot · 2019-05-16T16:15:46Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15921 (Tidy up ValidationState interface by jnewbery)
#15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)

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

utACK last 4 commits of 8e81c1a

…r code)

…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 · 2019-05-17T11:45:21Z

Rebased! Please re-review :-)

ryanofsky

utACK 9f85e9c. No changes at all since last review except clean rebase after base PR #16033 was merged

… 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

…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

…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

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

…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

…ckAssertion and add run-time check to it

…on to LockAssertion and add run-time check to it

…Annotation to LockAssertion and add run-time check to it

…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

[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

fanquake added the Refactoring label May 16, 2019

practicalswift changed the title ~~Add assertion to make sure the LockAnnotation guarantees we give are truthful (ifdef DEBUG_LOCKORDER)~~ Add assertion to make sure the LockAnnotation guarantees we give are truthful (ifdef DEBUG_LOCKORDER). Rename LockAnnotation to LockAssertion. May 16, 2019

ryanofsky approved these changes May 16, 2019

View reviewed changes

practicalswift added 4 commits May 17, 2019 13:29

Move LockAnnotation from threadsafety.h (imported code) to sync.h (ou…

cc25885

…r code)

Move LockAnnotation to make it reflect the truth

3a80944

Make sure the compile-time locking promises given via LockAnnotation:…

de9b5db

…s hold also in practice at runtime (ifdef DEBUG_LOCKORDER)

scripted-diff: Rename LockAnnotation to LockAssertion

9f85e9c

-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 May 17, 2019 11:43

ryanofsky approved these changes May 23, 2019

View reviewed changes

maflcko changed the title ~~Add assertion to make sure the LockAnnotation guarantees we give are truthful (ifdef DEBUG_LOCKORDER). Rename LockAnnotation to LockAssertion.~~ refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it May 23, 2019

maflcko merged commit 9f85e9c into bitcoin:master May 23, 2019

maflcko mentioned this pull request Aug 29, 2020

Do not hide compile-time thread safety warnings #19668

Merged

practicalswift deleted the make-sure-LockAnnotation-promises-are-truthful branch April 10, 2021 19:38

Munkybooty added a commit to Munkybooty/dash that referenced this pull request Feb 18, 2022

squash! Merge bitcoin#16034: refactoring: Rename LockAnnotation to Lo…

0f222b4

…ckAssertion and add run-time check to it

Munkybooty added a commit to Munkybooty/dash that referenced this pull request Feb 18, 2022

squash! squash! Merge bitcoin#16034: refactoring: Rename LockAnnotati…

e0b084d

…on to LockAssertion and add run-time check to it

Munkybooty added a commit to Munkybooty/dash that referenced this pull request Feb 18, 2022

squash! squash! squash! Merge bitcoin#16034: refactoring: Rename Lock…

ee28ba8

…Annotation to LockAssertion and add run-time check to it

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it #16034

refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it #16034

Uh oh!

practicalswift commented May 16, 2019 •

edited

Loading

Uh oh!

ryanofsky commented May 16, 2019

Uh oh!

practicalswift commented May 16, 2019

Uh oh!

promag commented May 16, 2019

Uh oh!

practicalswift commented May 16, 2019

Uh oh!

DrahtBot commented May 16, 2019 •

edited

Loading

Uh oh!

ryanofsky left a comment

Uh oh!

practicalswift commented May 17, 2019

Uh oh!

ryanofsky left a comment •

edited

Loading

Uh oh!

Uh oh!

refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it #16034

refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it #16034

Uh oh!

Conversation

practicalswift commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented May 16, 2019

Uh oh!

practicalswift commented May 16, 2019

Uh oh!

promag commented May 16, 2019

Uh oh!

practicalswift commented May 16, 2019

Uh oh!

DrahtBot commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented May 17, 2019

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented May 16, 2019 •

edited

Loading

DrahtBot commented May 16, 2019 •

edited

Loading

ryanofsky left a comment •

edited

Loading