Skip to content

[refactor] interfaces: Add missing LockAnnotation for cs_main #15855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
May 14, 2019
Merged

[refactor] interfaces: Add missing LockAnnotation for cs_main #15855

maflcko merged 1 commit into from
May 14, 2019

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 19, 2019

This adds missing LockAnnotation lock(::cs_main); to src/interfaces/chain.cpp (as well as tests and benchmarks)

@maflcko maflcko force-pushed the 1904-interfacesLock branch from fad8bc2 to fa0a3ba Compare April 19, 2019 17:18
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 19, 2019 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15931 (Remove GetDepthInMainChain dependency on locked chain interface by ariard)
  • #15921 (Tidy up ValidationState interface by jnewbery)

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.

@maflcko maflcko force-pushed the 1904-interfacesLock branch from fa0a3ba to fa90b1a Compare April 19, 2019 17:46
promag
promag reviewed Apr 21, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.

@maflcko maflcko force-pushed the 1904-interfacesLock branch from fa90b1a to fa7a61d Compare April 22, 2019 13:06
@maflcko
Copy link
Member Author

maflcko commented Apr 22, 2019

Addressed feedback by @promag

@maflcko maflcko force-pushed the 1904-interfacesLock branch 2 times, most recently from bab981d to fab495f Compare May 1, 2019 19:10
@maflcko maflcko force-pushed the 1904-interfacesLock branch from fab495f to fa2633e Compare May 7, 2019 16:29
@practicalswift
Copy link
Contributor

practicalswift commented May 7, 2019

Concept ACK

@maflcko maflcko force-pushed the 1904-interfacesLock branch from fa2633e to fa58deb Compare May 7, 2019 16:31
@maflcko maflcko force-pushed the 1904-interfacesLock branch 2 times, most recently from 5b0d796 to face05a Compare May 10, 2019 20:48
@promag
Copy link
Contributor

promag commented May 13, 2019

utACK face05a080c081fc426ff34ecb39ac072710034b.

practicalswift
practicalswift reviewed May 13, 2019
View reviewed changes
@maflcko maflcko force-pushed the 1904-interfacesLock branch from face05a to fad1d26 Compare May 13, 2019 17:33
@practicalswift
Copy link
Contributor

practicalswift commented May 13, 2019 •
edited
Loading

@MarcoFalke The added comment makes it easier to understand for human reviewers. That is good, but I still think we would be better off if we used different annotations for the "opt-out of analysis" and "give the compiler a locking guarantee when it fails to understand our code" use cases:

When analysing locks using static analysis etc I want to be able to let LockAnnotation imply the stronger "AssertLockHeld(…)" (instead of the weaker NO_THREAD_SAFETY_ANALYSIS).

This change breaks that implication.

Perhaps --enable-debug builds should do AssertLockHeld(…) as part of the LockAnnotationctor to make sure the LockAnnotation guarantees hold up over time? The opt-out variant shouldn't have that (obviously) :-)

@maflcko maflcko force-pushed the 1904-interfacesLock branch from fad1d26 to fa3c651 Compare May 13, 2019 18:47
@maflcko
Copy link
Member Author

maflcko commented May 13, 2019

Perhaps --enable-debug builds should do AssertLockHeld(…) as part of the LockAnnotationctor to make sure the LockAnnotation guarantees hold up over time

Sounds good. I have addressed your concerns, but this suggestion should go into a follow-up pull request?

ryanofsky
ryanofsky approved these changes May 13, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fa3c651

@practicalswift
Copy link
Contributor

practicalswift commented May 14, 2019

utACK fa3c651

@maflcko maflcko merged commit fa3c651 into bitcoin:master May 14, 2019
maflcko pushed a commit that referenced this pull request May 14, 2019
Merge #15855: [refactor] interfaces: Add missing LockAnnotation for c…
40c66bb 
…s_main

fa3c651 [refactor] interfaces: Add missing LockAnnotation for cs_main (MarcoFalke)

Pull request description:

  This adds missing `LockAnnotation lock(::cs_main);` to `src/interfaces/chain.cpp` (as well as tests and  benchmarks)

ACKs for commit fa3c65:
  practicalswift:
    utACK fa3c651
  ryanofsky:
    utACK fa3c651

Tree-SHA512: b67082fe3718c94b4addf7f2530593915225c25080f20c3ffa4ff7e08f1f49548f255fb285f89a8feff84be3f6c91e1792495ced9f6bf396732396d1356d597a
@maflcko maflcko deleted the 1904-interfacesLock branch May 14, 2019 12:27
@UdjinM6 UdjinM6 mentioned this pull request Jan 22, 2020
Merged
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 27, 2020
@majcosta
[backport#15855] [refactor] interfaces: Add missing LockAnnotation fo…
0ae0f30 
…r cs_main

Summary:
bitcoin/bitcoin@fa3c651

---

Depends on D6260

Backport of Core [[bitcoin/bitcoin#15855 | PR15855]]

Test Plan:
  cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DENABLE_WERROR=ON -DENABLE_SANITIZERS=thread
  ninja check-all
  ninja
  ./src/bench/bitcoin-bench

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6259
kwvg added a commit to kwvg/dash that referenced this pull request Oct 24, 2021
@kwvg
merge bitcoin#15855: Add missing LockAnnotation for cs_main
07ee416
UdjinM6 pushed a commit to dashpay/dash that referenced this pull request Oct 25, 2021
@kwvg
merge bitcoin#15855: Add missing LockAnnotation for cs_main (#4530)
f1a2cdb 
* merge bitcoin#15855: Add missing LockAnnotation for cs_main

* mutex: update cs_main locks, assertions and annotations

This commit is a squash between 8c98823 and 90d0535
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Nov 3, 2021
@PastaPastaPasta
partial Merge bitcoin#15855: [refactor] interfaces: Add missing LockA…
0bc782d 
…nnotation for cs_main

fa3c651 [refactor] interfaces: Add missing LockAnnotation for cs_main (MarcoFalke)

Pull request description:

  This adds missing `LockAnnotation lock(::cs_main);` to `src/interfaces/chain.cpp` (as well as tests and  benchmarks)

ACKs for commit fa3c65:
  practicalswift:
    utACK fa3c651
  ryanofsky:
    utACK fa3c651

Tree-SHA512: b67082fe3718c94b4addf7f2530593915225c25080f20c3ffa4ff7e08f1f49548f255fb285f89a8feff84be3f6c91e1792495ced9f6bf396732396d1356d597a
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@kwvg @pravblockc
merge bitcoin#15855: Add missing LockAnnotation for cs_main (dashpay#…
aaee20b 
…4530)

* merge bitcoin#15855: Add missing LockAnnotation for cs_main

* mutex: update cs_main locks, assertions and annotations

This commit is a squash between 8c98823 and 90d0535
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@pravblockc
partial Merge bitcoin#15855: [refactor] interfaces: Add missing LockA…
ff1be08 
…nnotation for cs_main

fa3c651 [refactor] interfaces: Add missing LockAnnotation for cs_main (MarcoFalke)

Pull request description:

  This adds missing `LockAnnotation lock(::cs_main);` to `src/interfaces/chain.cpp` (as well as tests and  benchmarks)

ACKs for commit fa3c65:
  practicalswift:
    utACK fa3c651
  ryanofsky:
    utACK fa3c651

Tree-SHA512: b67082fe3718c94b4addf7f2530593915225c25080f20c3ffa4ff7e08f1f49548f255fb285f89a8feff84be3f6c91e1792495ced9f6bf396732396d1356d597a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+2 more reviewers

@promag promag promag left review comments

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @DrahtBot @practicalswift @promag @ryanofsky