sync: Replace LockAssertion with AssertLockHeldUnverified #19918

ryanofsky · 2020-09-08T15:43:57Z

AssertLockHeldUnverified does the same thing LockAssertion except it:

Correctly reports file and line number where assertion fails instead of location in sync.h.
Has a simpler syntax that doesn't require declaring an unused variable name.
Should be harder to confuse with AssertLockHeld. Name should indicate it's an unverified assertion not to be preferred when the safer verified assert is available.

This also adds doxygen comments describing AssertLockHeld, AssertLockHeldUnverified, and AssertLockNotHeld macros.

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

AssertLockHeldUnverified does the same thing LockAssertion except it: - Correctly reports file and line number where assertion fails instead of location in sync.h. - Has a simpler syntax that doesn't require declaring an unused variable name. - Should be harder to confuse with AssertLockHeld. Name should indicate it's a weaker assertion not to be preferred when stronger compile time checks are available. This also adds doxygen comments describing AssertLockHeld, AssertLockHeldUnverified, and AssertLockNotHeld macros.

ryanofsky · 2020-09-08T16:22:44Z

This PR is an alternative to #19865. I still think #19865 is a better approach would prefer it. I think having two lock assertions is unnecessarily confusing and while AssertLockHeld was useful historically before we had compile-time checks, now it just adds noise and inconsistency. If we are going to have two assertions, though, at least this PR should make it easier to compare and choose between them.

hebasto · 2020-09-08T16:38:12Z

LockAssertion was used (in a controversial way) in #19668 as it resolved problem. The initial #19668 version (https://github.com/hebasto/bitcoin/commits/pr19668.01) also had a commit that removes LockAssertion.

So, Concept ACK on better tooling and naming.

ajtowns

Concept ACK

ajtowns · 2020-09-08T16:58:10Z

src/sync.h

+ * Use of this macro is neither encouraged nor discouraged in new code. It was
+ * historically used before compile time checks were available, but it may
+ * still be be useful for virtual methods or std::function callbacks where
+ * EXCLUSIVE_LOCKS_REQUIRED annotations are not enforced.


developer-notes currently recommends "Combine annotations in function declarations with run-time asserts in function definitions", think these recommendations are better off there, and certainly having conflicting recommendations is worse.

I think the only case where it's useful for virtual methods is if the parent class doesn't need the lock, but the child class does -- in that case invoking Parent* p = new Child(); p->func(); won't trigger compile-time warnings but you could at least catch it with a runtime check. But I think that pattern probably should be considered a mistake anyway.

As far as I know callbacks will need the WeaklyAssertLockHeld variant?

Maybe just drop the above paragraph as it contradicts with the developer nodes? IMO the latter is correct.

re: #19918 (comment)

developer-notes currently recommends "Combine annotations in function declarations with run-time asserts in function definitions", think these recommendations are better off there, and certainly having conflicting recommendations is worse.

I don't love that recommendation, and the text doesn't give a rationale for following it, but I dropped this paragraph. I wasn't aware of other guidance and didn't mean to conflict.

I think the only case where it's useful for virtual methods is if the parent class doesn't need the lock, but the child class does -- in that case invoking Parent* p = new Child(); p->func(); won't trigger compile-time warnings but you could at least catch it with a runtime check. But I think that pattern probably should be considered a mistake anyway.

As far as I know callbacks will need the WeaklyAssertLockHeld variant?

re: "pattern probably should be considered a mistake." IMO, the idea of having a compile time annotation whose only purpose at compile time is checking for the presence of a different compile-time annotation is a mistake. But given that, it makes sense to use AssertLockHeld instead of WeaklyAssertLockHeld to provide runtime checking inside a lambda annotated with EXCLUSIVE_LOCKS_REQUIRED called through a std::function, or inside an overridden virtual method annotated EXCLUSIVE_LOCKS_REQUIRED called through a base method which is not annotated.

Deleted the comment, though, because I'm becoming convinced these asserts are hopeless to explain in detail. As long we're are going to reject #19865 and have these ubiquitous asserts, the most useful practical advice will probably be to prefer using AssertLockHeld wherever possible and just fall back to WeaklyAssertLockHeld when the compiler gives an error you don't understand.

re: #19918 (comment)

Maybe just drop the above paragraph as it contradicts with the developer nodes? IMO the latter is correct.

Thanks, dropped.

But given that, it makes sense to use AssertLockHeld instead of WeaklyAssertLockHeld to provide runtime checking inside a lambda annotated with EXCLUSIVE_LOCKS_REQUIRED called through a std::function

Both the current LockAssertion and your proposed WeaklyAssertLockHeld do runtime checking, so there's no advantage there.

I don't think AssertLockHeld in lambdas ever makes much sense: there are two ways to use lambdas -- one where thread safety annotations work as expected, and one where they're broken because the lambda's a callback passed to an unannotated function. In the former case you're writing:

LOCK(cs); [&]() EXCLUSIVE_LOCKS_REQUIRED(cs) { ++foo; }();

where it's already obvious that cs is held when the lambda is called, and you're just adding the annotation to make the compiler happy.

In the latter case, you're writing:

LOCK(cs_main); connman->ForEachNode([&](CNode* pnode) { WeaklyAssertLockHeld(cs_main); ... });

because you can't pass the fact that cs_main is still held through the un-annotated ForEachNode.

inside an overridden virtual method annotated EXCLUSIVE_LOCKS_REQUIRED called through a base method which is not annotated.

I think that is something we should go out of our way to avoid. Either the base method should be annotated as requiring the lock, or the overriding method should acquire the lock itself -- with annotations, locking is part of the function signature, and you shouldn't change the signature when overriding a method. We don't have any code like this now, and I don't think there's any reason ever to, so I'm not sure it's worth addressing it explicitly, but if we are, better to discourage it outright.

Thread safety attributes sit on declarations and not on types, but we can check the virtual overriding case. I'll have to look into that. (Theoretically it should be part of the type, but that would interact with other language features like overloading, so it was decided not to put it there.)

Looking through the "thread safety" issues on bugzilla, this seems similar to the idea "We could warn in -Wthread-safety-attributes if a declaration has attributes that weren't visible on an earlier declaration." from https://bugs.llvm.org/show_bug.cgi?id=42849

ajtowns · 2020-09-08T18:04:08Z

src/sync.h

+ * EXCLUSIVE_LOCKS_REQUIRED errors when the compiler can't determine that a
+ * mutex is locked.
+ */
+#define WeaklyAssertLockHeld(mutex) [&]() ASSERT_EXCLUSIVE_LOCK(mutex) { AssertLockHeldInternal(#mutex, __FILE__, __LINE__, &mutex); }()


Maybe LOCK_ALREADY_HELD(mutex) would be better, comparable with LOCK(mutex)? This isn't a particularly weak assertion in anyway I can see, it's just one the compiler isn't able to verify at compile time.

"Assert that a mutex was already locked in any possible way this code could be reached. If DEBUG_LOCKORDER is defined, a runtime check that the mutex was actually locked by this thread will be made." ? Without DEBUG_LOCKORDER this does nothing at runtime.

Note that ASSERT_EXCLUSIVE_LOCK has slightly weird scoping behaviour; if x requires mutex m then:

if (1) { WeaklyAssertLockHeld(m); } ++x;

works fine, while

extern int n; if (n > 1) { WeaklyAssertLockHeld(m); } ++x;

tells you you need to hold the mutex to do ++x. I couldn't come up with a case where this results in missing a warning though; but I'm not super-confident in it, as compared to being tightly bounded by scopes the way SCOPED_LOCKABLE is.

In non-debug build a code like:

WeaklyAssertLockHeld(mutex_required_for_x); x = 1;

where the mutex is not held will not produce a compilation warning and will not assert at runtime. So the unprotected access will actually happen. This is because ASSERT_EXCLUSIVE_LOCK tells the compiler we are going to check, but we actually don't check (in non-debug build). Maybe remove ASSERT_EXCLUSIVE_LOCK in non-debug builds, or at least clearly explain in the comment the dangers of using this.

re: #19918 (comment)

Maybe LOCK_ALREADY_HELD(mutex) would be better, comparable with LOCK(mutex)? [...]

This seems like a brain dump and I can't figure out the suggestion. Is it to rename one macro or rename two macros?

The point of this PR is to be a fallback if #19865 is rejected. #19865 says there should only be one assert: AssertLockHeld, and you should never use it unless you need to call a EXCLUSIVE_LOCKS_REQUIRED function and the compiler doesn't know the mutex is locked, and you can't annotate the current function.

Because there has been pushback against #19865, this PR keeps both asserts, but makes them more consistent with each other and chooses names that contrasts the stronger, preferred assert with the weaker, less-preferred assert.

re: #19918 (comment)

In non-debug build a code like:

Thanks for pointing it out, I added a note. To be sure, though, your build is different different from my build, and different from the CI builds used to verify changes.

This seems like a brain dump and I can't figure out the suggestion. Is it to rename one macro or rename two macros?

Just to rename "WeaklyAssertLockHeld" to "LOCK_ALREADY_HELD" or similar.

I don't think it's accurate to say one is "less-preferred" to the other -- they do different things, and are appropriate in different circumstances. Hopefully the circumstances where WeaklyAssertLockHeld is appropriate are more rare, is all.

"Assert that a mutex was already locked in any possible way this code could be reached. If DEBUG_LOCKORDER is defined, a runtime check that the mutex was actually locked by this thread will be made."

was a suggestion regarding the code docs, since it only has any runtime effect when DEBUG_LOCKORDER is defined.

Note that ASSERT_EXCLUSIVE_LOCK has slightly weird scoping behaviour [...]

This is unrelated to scopes, the capability is assumed to be held after the assertion, regardless of whether we leave any scopes. (Think about it, there is no destructor for the assertion, so why would the scope ending change anything?)

This analysis works on the source-level CFG. We can inspect this by passing --analyze -Xanalyzer -analyzer-checker=debug.DumpCFG to Clang. For the first example (using the vocabulary from the documentation):

[B4 (ENTRY)] Succs (1): B3 [B1] 1: x 2: ++[B1.1] Preds (2): B2 B3(Unreachable) Succs (1): B0 [B2] 1: m 2: [B2.1].AssertHeld 3: [B2.2]() Preds (1): B3 Succs (1): B1 [B3] 1: 1 2: [B3.1] (ImplicitCastExpr, IntegralToBoolean, _Bool) T: if [B3.2] Preds (1): B4 Succs (2): B2 B1(Unreachable) [B0 (EXIT)] Preds (1): B1

The if is in B3 and branches on a compile-time constant, thus we see that the branch to B1 (where the increment happens) is unreachable. In your second example the branch condition is no longer a compile-time constant. (Try adding const or constexpr, then it should work.)

This is because ASSERT_EXCLUSIVE_LOCK tells the compiler we are going to check, but we actually don't check (in non-debug build).

Using ASSERT_EXCLUSIVE_LOCK even though you don't always check is fine. It would be strange if it wasn't, because that's how the standard assert behaves.

Maybe remove ASSERT_EXCLUSIVE_LOCK in non-debug builds, or at least clearly explain in the comment the dangers of using this.

Having static analysis depend on the build profile doesn't really make sense IMO. If you don't have the same annotations in all profiles you're just going to make your life harder.

re: #19918 (comment)

Just to rename "WeaklyAssertLockHeld" to "LOCK_ALREADY_HELD" or similar.

I'm still not understanding why this is a good idea, but I made a space for it https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs

ajtowns · 2020-09-08T18:11:27Z

This PR is an alternative to #19865. I still think #19865 is a better approach would prefer it.

I think there's two separate issues, (1) naming/behaviour for "we can't prove to the compiler this is correct, so we'll just tell it to trust us" and (2) "do we need to manually add code for runtime checks that just duplicate the compile time checks?". I think this improves (1) and #19685 is mostly about (2), and that it makes sense to keep them separate. YMMV.

vasild · 2020-09-14T07:50:02Z

src/sync.h

+ * Assert at compile time and run time that a mutex is locked. Has no effect
+ * when EXCLUSIVE_LOCKS_REQUIRED annotations are enforced because the runtime
+ * assertions will never trigger (analogous to asserting an unsigned int is
+ * greator or equal to 0).


The compile time check is not an assertion (like static_assert), but is a warning. The code will get compiled with it if --enable-werror is not used during ./configure. Maybe reword to:

Produce a compilation warning and assert at run time (only if DEBUG_LOCKORDER is defined) that a mutex is locked.

re: #19918 (comment)

The compile time check is not an assertion (like static_assert), but is a warning.

Thanks, on CI these trigger fatal build errors and that seems like the most important thing, but I added a note about varying build behavior.

vasild · 2020-09-14T07:54:53Z

src/sync.h

+ * Use of this macro is neither encouraged nor discouraged in new code. It was
+ * historically used before compile time checks were available, but it may
+ * still be be useful for virtual methods or std::function callbacks where
+ * EXCLUSIVE_LOCKS_REQUIRED annotations are not enforced.


Maybe just drop the above paragraph as it contradicts with the developer nodes? IMO the latter is correct.

vasild · 2020-09-14T08:02:18Z

src/sync.h

+ * EXCLUSIVE_LOCKS_REQUIRED errors when the compiler can't determine that a
+ * mutex is locked.
+ */
+#define WeaklyAssertLockHeld(mutex) [&]() ASSERT_EXCLUSIVE_LOCK(mutex) { AssertLockHeldInternal(#mutex, __FILE__, __LINE__, &mutex); }()


In non-debug build a code like:

WeaklyAssertLockHeld(mutex_required_for_x); x = 1;

where the mutex is not held will not produce a compilation warning and will not assert at runtime. So the unprotected access will actually happen. This is because ASSERT_EXCLUSIVE_LOCK tells the compiler we are going to check, but we actually don't check (in non-debug build). Maybe remove ASSERT_EXCLUSIVE_LOCK in non-debug builds, or at least clearly explain in the comment the dangers of using this.

vasild · 2020-09-14T08:09:39Z

src/sync.h

+ */
+#define WeaklyAssertLockHeld(mutex) [&]() ASSERT_EXCLUSIVE_LOCK(mutex) { AssertLockHeldInternal(#mutex, __FILE__, __LINE__, &mutex); }()
+
+/** Assert at compile time and run time that a mutex is not locked. */


As above, maybe replace "assert at compile time" with "warn at compile time"?

re: #19918 (comment)

As above, maybe replace "assert at compile time" with "warn at compile time"?

Thanks, added a note about build differences. Assert is in the name of the function and is the commonly used name for a correctness check which isn't supposed to affect normal behavior, so I didn't change the top description.

ryanofsky

Updated 070803d -> 5c9eb25 (pr/lockb.1 -> pr/lockb.2, compare) with comment fixes

re: #19918 (comment)

LockAssertion was used (in a controversial way) in #19668 as it resolved problem.

Yes, but the problem LockAssertion was solving in #19668 was a problem created by #19668 (changing AssertLockHeld to no longer be annotated with ASSERT_EXCLUSIVE_LOCK). Ever since LockAssertion was introduced in #14437 (as LockAnnotation) it has been redundant with AssertLockHeld, which first got the ASSERT_EXCLUSIVE_LOCK annotation in #13423.

ryanofsky · 2020-09-14T20:23:26Z

src/sync.h

+ * Assert at compile time and run time that a mutex is locked. Has no effect
+ * when EXCLUSIVE_LOCKS_REQUIRED annotations are enforced because the runtime
+ * assertions will never trigger (analogous to asserting an unsigned int is
+ * greator or equal to 0).


re: #19918 (comment)

The compile time check is not an assertion (like static_assert), but is a warning.

Thanks, on CI these trigger fatal build errors and that seems like the most important thing, but I added a note about varying build behavior.

ryanofsky · 2020-09-14T20:23:43Z

src/sync.h

+ * Use of this macro is neither encouraged nor discouraged in new code. It was
+ * historically used before compile time checks were available, but it may
+ * still be be useful for virtual methods or std::function callbacks where
+ * EXCLUSIVE_LOCKS_REQUIRED annotations are not enforced.


re: #19918 (comment)

developer-notes currently recommends "Combine annotations in function declarations with run-time asserts in function definitions", think these recommendations are better off there, and certainly having conflicting recommendations is worse.

I don't love that recommendation, and the text doesn't give a rationale for following it, but I dropped this paragraph. I wasn't aware of other guidance and didn't mean to conflict.

I think the only case where it's useful for virtual methods is if the parent class doesn't need the lock, but the child class does -- in that case invoking Parent* p = new Child(); p->func(); won't trigger compile-time warnings but you could at least catch it with a runtime check. But I think that pattern probably should be considered a mistake anyway.

As far as I know callbacks will need the WeaklyAssertLockHeld variant?

re: "pattern probably should be considered a mistake." IMO, the idea of having a compile time annotation whose only purpose at compile time is checking for the presence of a different compile-time annotation is a mistake. But given that, it makes sense to use AssertLockHeld instead of WeaklyAssertLockHeld to provide runtime checking inside a lambda annotated with EXCLUSIVE_LOCKS_REQUIRED called through a std::function, or inside an overridden virtual method annotated EXCLUSIVE_LOCKS_REQUIRED called through a base method which is not annotated.

Deleted the comment, though, because I'm becoming convinced these asserts are hopeless to explain in detail. As long we're are going to reject #19865 and have these ubiquitous asserts, the most useful practical advice will probably be to prefer using AssertLockHeld wherever possible and just fall back to WeaklyAssertLockHeld when the compiler gives an error you don't understand.

re: #19918 (comment)

Maybe just drop the above paragraph as it contradicts with the developer nodes? IMO the latter is correct.

Thanks, dropped.

ryanofsky · 2020-09-14T20:24:01Z

src/sync.h

+ * EXCLUSIVE_LOCKS_REQUIRED errors when the compiler can't determine that a
+ * mutex is locked.
+ */
+#define WeaklyAssertLockHeld(mutex) [&]() ASSERT_EXCLUSIVE_LOCK(mutex) { AssertLockHeldInternal(#mutex, __FILE__, __LINE__, &mutex); }()


re: #19918 (comment)

Maybe LOCK_ALREADY_HELD(mutex) would be better, comparable with LOCK(mutex)? [...]

This seems like a brain dump and I can't figure out the suggestion. Is it to rename one macro or rename two macros?

The point of this PR is to be a fallback if #19865 is rejected. #19865 says there should only be one assert: AssertLockHeld, and you should never use it unless you need to call a EXCLUSIVE_LOCKS_REQUIRED function and the compiler doesn't know the mutex is locked, and you can't annotate the current function.

Because there has been pushback against #19865, this PR keeps both asserts, but makes them more consistent with each other and chooses names that contrasts the stronger, preferred assert with the weaker, less-preferred assert.

re: #19918 (comment)

In non-debug build a code like:

Thanks for pointing it out, I added a note. To be sure, though, your build is different different from my build, and different from the CI builds used to verify changes.

ryanofsky · 2020-09-14T20:24:12Z

src/sync.h

+ */
+#define WeaklyAssertLockHeld(mutex) [&]() ASSERT_EXCLUSIVE_LOCK(mutex) { AssertLockHeldInternal(#mutex, __FILE__, __LINE__, &mutex); }()
+
+/** Assert at compile time and run time that a mutex is not locked. */


re: #19918 (comment)

As above, maybe replace "assert at compile time" with "warn at compile time"?

Thanks, added a note about build differences. Assert is in the name of the function and is the commonly used name for a correctness check which isn't supposed to affect normal behavior, so I didn't change the top description.

ryanofsky · 2020-09-17T20:36:42Z

In IRC http://www.erisian.com.au/bitcoin-core-dev/log-2020-09-17.html#l-644, it was mentioned that WeaklyAssertLockHeld name is confusing. The name isn't intended to be confusing, but it is intended to discourage use. If you are writing an assert you should always favor AssertLockHeld over WeaklyAssertLockHeld and only fall back when the compiler forces you to because it can't prove that the mutex is locked in particular location.

AssertLockHeld states that the lock is held with confidence, because the compiler can check that the statement is true at compile time. WeaklyAssertLockHeld makes the statement with less confidence because the compiler can't check that it is true.

Any case, https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs#disadvantages-of-2a-approach has a table of possible names where other suggestions can be added

vasild

This looks good now except:

The confusing name WeaklyAssertLockHeld. It is confusing because you can't weakly call abort() or semi-exit(). I think LOCK_ALREADY_HELD is better, already suggested.
The comments keep mentioning assert at compile time, already suggested.

ryanofsky · 2020-09-18T11:53:44Z

It is confusing because you can't weakly call abort() or semi-exit().

This is really funny. The function is not called WeaklyAbort or WeaklyExit! It is called WeaklyAssert!

Assert in english means to state or declare positively, "often forcefully or aggressively" like "The suspect continued to assert his innocence." You can strongly assert something if that you are confident it is true, or you can weakly assert something if you are not confident. (Asserting has nothing to do with exiting in spoken english.)

Anyway if there is another name you would prefer, please suggest it! There is a table of suggestions at https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs#disadvantages-of-2a-approach.

LOCK_ALREADY_HELD makes no sense to me be because:

The LOCK_ALREADY_HELD name hides the fact that it's an assert statement whose runtime behavior is identical to AssertLockHeld.
The LOCK_ALREADY_HELD name misleads you into thinking it may be a safer alternative to AssertLockHeld because it doesn't contain the word assert, when actually it is a more dangerous alternative. (Compiler can prove that AssertLockHeld will not trigger, but compiler has no way of knowing whether an LOCK_ALREADY_HELD will trigger)
I haven't seen anyone give a positive explanation why LOCK_ALREADY_HELD/AssertLockHeld is a good pairing of names for two assert statements that do the same thing, one with more compile time checking, one with less. I haven't seen any explanation of why the pairing of names makes sense for a different reason either. All I've seen are claims that other pairs of names are confusing, not that LOCK_ALREADY_HELD/AssertLockHeld is good or has any coherent logic

aaronpuchert · 2020-09-18T12:19:26Z

FWIW, I also think "weakly" is something I wouldn't know how to interpret into this context. I've added a suggestion {Proven,Unproven}AssertLockHeld that isn't perfect but goes into a direction that I find worth exploring: one is a proven assertion (you could say a theorem) and the other an unproven assertion (you could say a conjecture or maybe axiom).

ryanofsky · 2020-09-18T12:30:35Z

I've added a suggestion {Proven,Unproven}AssertLockHeld that isn't perfect but goes into a direction that I find worth exploring: one is a proven assertion (you could say a theorem) and the other an unproven assertion (you could say a conjecture or maybe axiom).

Nice. I like UnprovenAssertLockHeld. Maybe UnsafelyAssertLockHeld would work too.

vasild · 2020-09-18T14:52:33Z

Just to mention that, given that #19970 and #19918 overlap, I think #19918 (this PR) is better (if the weak name is changed).

My preference is for:
FORCE_THE_COMPILER_TO_THINK_THAT_WE_OWN_THE_MUTEX_AND_ASSERT_AT_RUNTIME_IF_WE_DONT(), it would discourage usage, right?

ryanofsky · 2020-09-18T15:28:53Z

Just to mention that, given that #19970 and #19918 overlap, I think #19918 (this PR) is better (if the weak name is changed).

My preference is for:
FORCE_THE_COMPILER_TO_THINK_THAT_WE_OWN_THE_MUTEX_AND_ASSERT_AT_RUNTIME_IF_WE_DONT(), it would discourage usage, right?

Sure, I assume that is a joke suggestion, but I am happy with all the alternate suggested pairs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs#disadvantages-of-2a-approach table except LOCK_ASSERTION/AssertLockHeld and LOCK_ALREADY_HELD/AssertLock because these pairs do not indicate which assert in the pair is the safer stronger preferred assert.

I'd welcome adding more suggested pairs to the table. If #19865 is rejected and closed, we can do a poll for which of those pairs is most popular and update this PR with the decision.

I am still holding out hope for #19865, though. Personally I've found compile time thread safety checking useful, and thread sanitizer checking useful, and debug_lockorder checking useful, and even found AssertLockHeld useful before it had the EXCLUSIVE_LOCK annotation. But now that it has an annotation proving it won't trigger at runtime, I don't see personally see the utility anymore.

ajtowns · 2020-09-19T06:34:48Z

AssertLockHeld states that the lock is held with confidence, because the compiler can check that the statement is true at compile time. WeaklyAssertLockHeld makes the statement with less confidence because the compiler can't check that it is true.

From the perspective of the person writing the code, this seems exactly backwards: if you're not getting compile time checks, and have disabled runtime checks in non-debug builds, you want to be extremely confident that the assertion is correct before relying on it in following code, so the WeaklyAssertLockHeld are actually the strongest assertions you're making.

I think the right analogy for LockAssertion isn't to LOCK or to AssertLockAlreadyHeld (despite the implementation details) but actually to EXCLUSIVE_LOCKS_REQUIRED -- in every place where we want to use LockAssertion we'd much rather use an EXCLUSIVE_LOCKS_REQUIRED annotation, but for whatever reason are prevented from that. So how about ASSERT_EXCLUSIVE_LOCK_REQUIREMENT_SATISFIED(cs_main) or something similar based on the function annotation we'd like to have, and changing the developer-notes recommendation along the lines of:

When unable to add an EXCLUSIVE_LOCKS_REQUIRED annotation (eg, due to passing a lambda that uses a lock via a generic function that can't require it), use ASSERT_EXCLUSIVE_LOCK_REQUIREMENT_SATISFIED(cs) at the start of the function to indicate to the compiler that lock correctness has been manually verified. In debug builds (with DEBUG_LOCKORDER set) this will be checked at runtime.

I think that makes it pretty obvious that ASSERT_EXCLUSIVE_LOCK_REQUIREMENT_SATISFIED is not the preferred thing to do when the alternative is possible, and I think having it at the start of a function minimises any potential leakage that might happen due to weird code paths and also makes it easy to move it to a compile-time-enforced annotation if that becomes possible.

ryanofsky · 2020-09-19T11:48:10Z

re: #19918 (comment)

Sure, I'm saying weak=unprovable and strong=provable, and you're saying actually strong=unprovable, and weak=provable. I think it's overthinking a little, but very neat, and I'm happy to use any names.

After names are chosen, everything else in this approach is pretty simple. It does have two asserts, so it isn't quite as simple as #19865 or #19979 where there is only one assert. But it's easy to always favor the provable assert, and fall back to the unprovable one when analysis doesn't accept it. If analysis is turned off, both asserts are identical.

ryanofsky · 2020-09-19T12:11:06Z

After tough work getting table not to horizontally scroll I added ASSERT_EXCLUSIVE_LOCK_REQUIREMENT_SATISFIED to table of suggested names in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs#disadvantages-of-2a-approach

DrahtBot · 2020-09-19T13:43:55Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Replace LockAssertion with AssertLockHeld, remove LockAssertion #19979 (Use proper TSA attributes (attempt two) by hebasto)
sync.h: fix LockAssertion error reporting #19970 (sync.h: fix LockAssertion error reporting by ajtowns)
scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865 (scripted-diff: Restore AssertLockHeld after Do not hide compile-time thread safety warnings #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.

ryanofsky · 2020-09-21T05:05:35Z

Strongly updated 5c9eb25 -> 3f8f78f (pr/lockb.2 -> pr/lockb.3, compare). Just a few changes to wording to address some concerns.

vasild

ACK 3f8f78f

This looks good and is an improvement over master (thus ACK). It overlaps with #19979 which I think is a better alternative because it leaves just one assert method.

"If you can't figure a good name for a function then something's wrong with that function."

ryanofsky · 2020-09-21T14:30:44Z

@vasild, I don't understand the way you are looking at the ASSERT_EXCLUSIVE_LOCK function (regardless of which name you would choose for the function). You seem to believe according to your comments here and in #19979 (review) that ASSERT_EXCLUSIVE_LOCK is less preferable to NO_THREAD_SAFETY_ANALYSIS because ASSERT_EXCLUSIVE_LOCK is "extorting the compiler".

This sounds to me like saying if a seatbelt doesn't fit you, instead of using a seatbelt extender, you should not wear a seatbelt, because using a seatbelt extender would be extorting the seatbelt.

I think this PR is using ASSERT_EXCLUSIVE_LOCK exactly the way it's intended to be used: to inform compiler thread analysis that a mutex is locked in cases where static analysis can't prove that it's locked. Do you believe this PR is misusing ASSERT_EXCLUSIVE_LOCK? Or maybe that ASSERT_EXCLUSIVE_LOCK is never acceptable to use? Why would giving the analysis (and giving developers reading the code) information be less preferable to disabling the analysis?

vasild · 2020-09-23T09:08:53Z

You seem to believe ... that ASSERT_EXCLUSIVE_LOCK is less preferable to NO_THREAD_SAFETY_ANALYSIS

No, I believe [having two assert functions] is less preferable to [one assert function + NO_THREAD_SAFETY_ANALYSIS in some exceptional places].

Overall I think that the most important thing is to have a clean interface (whatever is exported by sync.h) - one that is impossible to misuse and does not require mentions in developer-notes or long comments in the code. That means having just one function to "assert that the mutex is held". Even if that has some limitations and in a few cases we need to use NO_THREAD_SAFETY_ANALYSIS or tweaks like in #19979.

I think that this sole function should be tagged with ASSERT_EXCLUSIVE_LOCK because it best describes what the function does.

I think that tagging it with EXCLUSIVE_LOCKS_REQUIRED is incorrect to some extent because internally the assert function does not do anything that requires holding the mutex (like accessing variables protected by that mutex). I.e. it can perform the checks correctly (no races would occur) without the mutex being held. However EXCLUSIVE_LOCKS_REQUIRED provides stronger checks at compile time and also does not silence warnings in the code that follow it and also @aaronpuchert himself said it is ok to tag the assert function with EXCLUSIVE_LOCKS_REQUIRED, so I accept that.

I think this PR is using ASSERT_EXCLUSIVE_LOCK exactly the way it's intended to be used: to inform compiler thread analysis that a mutex is locked in cases where static analysis can't prove that it's locked. Do you believe this PR is misusing ASSERT_EXCLUSIVE_LOCK?

No, this PR is using ASSERT_EXCLUSIVE_LOCK exactly as intended, as I say above, but it is using it on AssertLockHeld2() while still having AssertLockHeld1() tagged with another attribute. Two functions is what I think should be avoided.

DrahtBot · 2020-09-23T16:14:51Z

🐙 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".}

maflcko · 2020-10-19T09:53:51Z

maflcko · 2020-10-25T13:42:33Z

Closing for now. Let me know if I should reopen

DrahtBot added Docs P2P labels Sep 8, 2020

ajtowns reviewed Sep 8, 2020

View reviewed changes

maflcko added Utils/log/libs and removed Docs P2P labels Sep 8, 2020

vasild mentioned this pull request Sep 9, 2020

sync: use proper TSA attributes #19929

Closed

hebasto mentioned this pull request Sep 10, 2020

Improve thread safety annotation coverage #19838

Closed

vasild reviewed Sep 14, 2020

View reviewed changes

ryanofsky force-pushed the pr/lockb branch from 070803d to 5c9eb25 Compare September 14, 2020 21:36

ryanofsky commented Sep 14, 2020

View reviewed changes

vasild reviewed Sep 18, 2020

View reviewed changes

ryanofsky mentioned this pull request Sep 18, 2020

sync.h: fix LockAssertion error reporting #19970

Closed

hebasto mentioned this pull request Sep 19, 2020

Replace LockAssertion with AssertLockHeld, remove LockAssertion #19979

Merged

DrahtBot mentioned this pull request Sep 19, 2020

scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865

Closed

ryanofsky changed the title ~~sync: Replace LockAssertion with WeaklyAssertLockHeld~~ sync: Replace LockAssertion with AssertLockHeldUnverified Sep 21, 2020

ryanofsky force-pushed the pr/lockb branch from 5c9eb25 to 3f8f78f Compare September 21, 2020 05:04

vasild approved these changes Sep 21, 2020

View reviewed changes

DrahtBot added the Needs rebase label Sep 23, 2020

maflcko closed this Oct 25, 2020

ryanofsky mentioned this pull request Jul 19, 2021

validation: UpdateTip/CheckBlockIndex assumeutxo support #21526

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

sync: Replace LockAssertion with AssertLockHeldUnverified #19918

sync: Replace LockAssertion with AssertLockHeldUnverified #19918

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!