sync.h: fix LockAssertion error reporting #19970

1. 
   ajtowns commented at 4:26 am on September 18, 2020: member

   This fixes LockAssertion’s incorrect line number reporting, with minimal changes.

   PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs

2. DrahtBot added the label P2P on Sep 18, 2020
3. ajtowns force-pushed on Sep 18, 2020
4. sync.h: fix LockAssertion error reporting df7a0ab346
5. ajtowns force-pushed on Sep 18, 2020
6. vasild approved
7. 
   vasild commented at 7:24 am on September 18, 2020: member

   ACK df7a0ab34

   That is an improvement over the current state of affairs.

   Feel free to rename LOCK_ASSERTION() to SUPPRESS_THREAD_SAFETY_ANALYSIS_AND_FORCE_THE_COMPILER_TO_THINK_THAT_WE_OWN_THE_MUTEX_HERE().

8. 
   ryanofsky commented at 12:26 pm on September 18, 2020: member

   Code review ACK df7a0ab34606c6203eb53ff4061f656293ab0f7f. Code change looks fine, but I think this is a bad approach for a few reasons:

   • The approach is more complicated than it needs to be. There is no need to have a macro-wrapped fake lock class just to assert a mutex is locked. The ASSERT_CAPABILITY annotation literally exists for this purpose. If you have a can of beans, I’m sure you can probably open it with a hammer and screwdriver, but you are probably better off using the can opener.

   • This is introducing a new macro to do what clang developers specifically told us not to do: “please don’t use ACQUIRE when the capability is assumed to be held previously”

   • This change makes LOCK_ASSERTION look like safer alternative to AssertLockHeld when actually it more dangerous (it is not checked at compile time unlike AssertLockHeld). It would be nice if the naming could give an indication of which assert should be preferred. (Of course we can get rid of two asserts with #19865).

   • Unclear why it’s called a “quick fix”. It’s basically the same size as #19918 except it has fewer comments and an incomplete description with no stated rationale

9. 
   ajtowns commented at 4:57 am on September 19, 2020: member

   Code review ACK df7a0ab. Code change looks fine, but I think this is a bad approach for a few reasons:

   * The approach is more complicated than it needs to be. There is no need to have a macro-wrapped fake lock class just to assert a mutex is locked. The ASSERT_CAPABILITY annotation literally exists for this purpose. If you have a can of beans, I'm sure you can probably open it with a hammer and screwdriver, but you are probably better off using the can opener.
   * This is introducing a new macro to do what clang developers specifically told us [not to do](https://reviews.llvm.org/D87629#2272676): "please don't use [ACQUIRE](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#acquire-acquire-shared-release-release-shared-release-generic) when the capability is assumed to be held previously"
   

   Both of those are appropriate justifications for a PR to change the approach. They’d be a valid objection to something that somehow prevents future PRs from changing the approach, but this PR doesn’t do that.

   * This change makes LOCK_ASSERTION look like safer alternative to AssertLockHeld when actually it more dangerous (it is not checked at compile time unlike AssertLockHeld). It would be nice if the naming could give an indication of which assert should be preferred. (Of course we can get rid of two asserts with [#19865](/bitcoin-bitcoin/19865/)).
   

   The developer notes state that using compile-time annotations are to be used consistently, and LOCK_ASSERTION is only to be used when that fails. The implementation of LOCK_ASSERTION as it stands will cause compile errors if you use the wrong assertion. I think you’re completely mistaken in thinking LOCK_ASSERTION looks “safer” than “AssertLockHeld”.

   * Unclear why it's called a "quick fix". It's basically the same size as [#19918](/bitcoin-bitcoin/19918/) except it has fewer comments and an incomplete description with no stated rationale
   

   It’s a “fix” because it corrects obviously broken behaviour, and it’s “quick” because it leaves the issues where there’s debate about what to do them to be addressed elsewhere. This isn’t an “alternative” to other approaches as you suggest in your edit to the PR description, it’s just a first step.

10. MarcoFalke removed the label P2P on Sep 19, 2020
11. MarcoFalke added the label Refactoring on Sep 19, 2020
12. MarcoFalke approved
13. 
    MarcoFalke commented at 8:53 am on September 19, 2020: member

    ACK df7a0ab34606c6203eb53ff4061f656293ab0f7f

    This doesn’t change the use of EXCLUSIVE_LOCK_FUNCTION, which can be changed in independent follow-up prs. This only passes the correct line numbers to AssertLockHeldInternal, which is a fix.

14. 
    DrahtBot commented at 1:24 pm on September 19, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19979 (Use proper TSA attributes (attempt two) by hebasto)
    • #19918 (sync: Replace LockAssertion with AssertLockHeldUnverified by ryanofsky)
    • #19865 (scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion by ryanofsky)

    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.

15. 
    DrahtBot commented at 4:14 pm on September 23, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

16. DrahtBot added the label Needs rebase on Sep 23, 2020
17. 
    ajtowns commented at 3:23 am on September 24, 2020: member

    No longer relevant
18. ajtowns closed this on Sep 24, 2020

    ---

19. tecnovert referenced this in commit 1c89361d73 on Oct 20, 2020
20. tecnovert referenced this in commit 28c0cd9cf8 on Oct 20, 2020
21. tecnovert referenced this in commit f1c21406d7 on Oct 20, 2020
22. tecnovert referenced this in commit 8a72a73165 on Oct 21, 2020
23. DrahtBot locked this on Feb 15, 2022

Contributors
ajtowns vasild ryanofsky MarcoFalke DrahtBot

Labels
Refactoring Needs rebase