de4eef52d1 threading: use correct mutex name in reverse_lock fatal error messages (Cory Fields)
Pull request description:
"Now that REVERSE_LOCK requires the name of the actual mutex, it can be used for better error messages." - theuni
This is a follow-up to this comment https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2981287545
I just cherry-picked the commit 85c2848eb575f4abaa81fdd4e8f3b2048693dd98
ACKs for top commit:
theuni:
Re-ACK de4eef52d1
TheCharlatan:
ACK de4eef52d1
Tree-SHA512: 1109381e1f0589093f7c737cb1ebd1c43324a9e1ea34b5f05a9171d06ab44cca0c5ead43c581f6e37ded1f0463ab8a280f3319c288d39a4625109b5c08a7cb68
a201a99f8c thread-safety: fix annotations with REVERSE_LOCK (Cory Fields)
aeea5f0ec1 thread-safety: add missing lock annotation (Cory Fields)
832c57a534 thread-safety: modernize thread safety macros (Cory Fields)
Pull request description:
This is one of several PRs to cleanup/modernize our threading primitives.
While replacing the old critical section locks in the mining code with a `REVERSE_LOCK`, I noticed that our thread-safety annotations weren't hooked up to it. This PR gets `REVERSE_LOCK` working properly.
Firstly it modernizes the attributes as-recommended by the [clang docs](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) (ctrl+f for `USE_LOCK_STYLE_THREAD_SAFETY_ATTRIBUTES`). There's a subtle difference between the old `unlock_function` and new `release_capability`, where our `reverse_lock` only works with the latter. I believe this is an upstream bug. I've [reported and attempted a fix here](https://github.com/llvm/llvm-project/pull/139343), but either way it makes sense to me to modernize.
The second adds a missing annotation pointed out by a fixed `REVERSE_LOCK`. Because clang's thread-safety annotations aren't passed through a reference to `UniqueLock` as one may assume (see [here](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis) for more details), `cs_main` has to be listed explicitly as a requirement.
The last commit actually fixes the `reverse_lock` by making it a `SCOPED_LOCK` and using the pattern [found in a clang test](https://github.com/llvm/llvm-project/blob/main/clang/test/SemaCXX/warn-thread-safety-analysis.cpp#L3126). Though the docs don't describe how to accomplish it, the functionality was added [in this commit](6a68efc959). Due to aliasing issues (see link above), in order to work correctly, the original mutex has to be passed along with the lock, so all existing `REVERSE_LOCK`s have been updated. To ensure that the mutexes actually match, a runtime assertion is added.
ACKs for top commit:
fjahr:
re-ACK a201a99f8c
davidgumberg:
reACK a201a99f8c
theuni:
Ok, done. Those last pushes can be ignored. ACKs on a201a99 are still fresh.
ryanofsky:
Code review ACK a201a99f8c. Just dropping 0065b9673db5da2994b0b07c1d50ebfb19af39d0 and fixing incorrect `reverse_lock::lockname` initialization since last review.
TheCharlatan:
Re-ACK a201a99f8c
Tree-SHA512: 2755fae0c41021976a1a633014a86d927f104ccbc8014c01c06dae89af363f92e5bc5d4276ad6d759302ac4679fe02a543758124d48318074db1c370989af7a7
Without proper annotations, clang thinks that mutexes are still held for the
duration of a reverse_lock. This could lead to subtle bugs as
EXCLUSIVE_LOCKS_REQUIRED(foo) passes when it shouldn't.
As mentioned in the docs [0], clang's thread-safety analyzer is unable to deal
with aliases of mutexes, so it is not possible to use the lock's copy of the
mutex for that purpose. Instead, the original mutex needs to be passed back to
the reverse_lock for the sake of thread-safety analysis, but it is not actually
used otherwise.
[0]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
fd290730f5 validation: clean up and clarify CheckInputScripts logic (Cory Fields)
1a37507895 validation: use a lock for CCheckQueueControl (Cory Fields)
c3b0e6c7f4 validation: make CCheckQueueControl's CCheckQueue non-optional (Cory Fields)
4c8c90b556 validation: only create a CCheckQueueControl if it's actually going to be used (Cory Fields)
11fed833b3 threading: add LOCK_ARGS macro (Cory Fields)
Pull request description:
As part of an effort to cleanup our threading primitives and add safe `SharedMutex`/`SharedLock` impls, I'd like to get rid of the last of our legacy `ENTER_CRITICAL_SECTION`/`LEAVE_CRITICAL_SECTION` usage. This, along with a follow-up [after fixing REVERSE_LOCK](https://github.com/bitcoin/bitcoin/pull/32465) will allow us to do that.
This replaces the old macros with an RAII lock, while simplifying `CCheckQueueControl`. It now requires a `CCheckQueue`, and optionality is handled externally. In the case of validation, it is wrapped in a `std::optional`.
It also adds an `LOCK_ARGS` macro for `UniqueLock` initialization which may be helpful elsewhere.
ACKs for top commit:
fjahr:
re-ACK fd290730f5
ryanofsky:
Code review ACK fd290730f5, just removing assert since last review. Thanks for considering all the comments and feedback!
TheCharlatan:
Re-ACK fd290730f5
Tree-SHA512: 54b9db604ee1bda7d11bce1653a88d3dcbc4f525eed6a85abdd4d6409138674af4bb8b00afa4e0d3d29dadd045a3a39de253a45f0ef9c05f56cba1aac5b59303
91d0888921 sync: unpublish LocksHeld() which is used only in sync.cpp (Vasil Dimov)
3df37e0c78 doc: clarify that LOCK() does AssertLockNotHeld() internally (Vasil Dimov)
Pull request description:
Constructs like
```cpp
AssertLockNotHeld(m);
LOCK(m);
```
are equivalent to (almost, modulo some logging differences, see below)
```cpp
LOCK(m);
```
for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in such cases. Requests to do the former keep coming during review process. `developer-notes.md` explicitly states "Combine annotations in function declarations with run-time asserts in function definitions", but that seems to be too strong or unclear. `LOCK()` is also a run-time assert in this case.
Also remove `LocksHeld()` from the public interface in `sync.h` since it is only used in `sync.cpp`.
ACKs for top commit:
achow101:
ACK 91d0888921
hebasto:
ACK 91d0888921, I have reviewed the code and it looks OK.
Tree-SHA512: c4b7ef2c0bfeb28d1c4f55f497810f629873137e02f5a92137c02cb1ff603ac76473dcd2171e594491494a5cb87b8c0c803e06b86f190d4acb231791e28e802d
75c3f9f880 sync: rename AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock (Vasil Dimov)
8d9ee8efe8 sync: remove DebugLock alias template (Vasil Dimov)
4b2e16763f sync: avoid confusing name overlap (Mutex) (Vasil Dimov)
9d7ae4b66c sync: remove unused template parameter from ::UniqueLock (Vasil Dimov)
11c190e3f1 sync: simplify MaybeCheckNotHeld() definitions by using a template (Vasil Dimov)
Pull request description:
Summary:
* Reduce 4 of the `MaybeCheckNotHeld()` definitions to 2 by using a template.
* Remove unused template parameter from `::UniqueLock`.
* Use `MutexType` instead of `Mutex` for a template parameter name to avoid overlap/confusion with the `Mutex` class.
* Rename `AnnotatedMixin::UniqueLock` to `AnnotatedMixin::unique_lock` to avoid overlap/confusion with the global `UniqueLock` and for consistency with `UniqueLock::reverse_lock`.
The first commit `sync: simplify MaybeCheckNotHeld() definitions by using a template` is also part of https://github.com/bitcoin/bitcoin/pull/25390
ACKs for top commit:
aureleoules:
ACK 75c3f9f880 - LGTM
ryanofsky:
Code review ACK 75c3f9f880. Nice cleanups! Just suggested changes since last review: keeping UniqueLock name and fixing a missed rename in a code comment
Tree-SHA512: ec261f6a444bdfe4f06e844b57b3606fdd9b2f842647cae15266d9729970d87585c808d482fbba0b31c33a4aa03527c36e282c92b28d9052711f75a7048c96f1
Use `UniqueLock` directly. Type deduction works just fine from the first
argument to the constructor of `UniqueLock`, so there is no need to
repeat
```cpp
UniqueLock<typename std::remove_reference<typename std::remove_pointer<decltype(cs)>::type>::type>
```
five times in the `LOCK` macros. Just `UniqueLock` suffices.
Use `MutexType` instead of `Mutex` for the template parameter of
`UniqueLock` because there is already a class named `Mutex` and the
naming overlap is confusing. `MutexType` is used elsewhere in `sync.h`.
Microsoft's C++ Standard Library uses the `[[nodiscard]]` attribute for
`try_lock()`.
See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex
This change allows to drop the current suppression for the warning C4838
and helps to prevent the upcoming warning C4858.
See: 539c26c923
The template parameter `typename Base = typename Mutex::UniqueLock` is
not used, so remove it. Use internally defined type `Base` to avoid
repetitions of `Mutex::UniqueLock`.
in microseconds.
Change the function name in order to print "LockContention" instead
of "PrintLockContention" to the log. Add Doxygen documentation.
With this change, the lock contention log prints:
2021-09-01T11:29:03Z LockContention: pnode->cs_vSend, net.cpp:1373 started
2021-09-01T11:29:03Z LockContention: pnode->cs_vSend, net.cpp:1373 completed (31μs)
2021-09-01T11:29:03Z LockContention: cs_vNodes, net.cpp:2277 started
2021-09-01T11:29:03Z LockContention: cs_vNodes, net.cpp:2277 completed (6μs)
2021-09-01T11:29:04Z LockContention: cs_vNodes, net.cpp:2242 started
2021-09-01T11:29:04Z LockContention: cs_vNodes, net.cpp:2242 completed (3μs)
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
3eb94ec81b sync: Use decltype(auto) return type for WITH_LOCK (Carl Dong)
Pull request description:
> Now that we're using C++17, we can use the decltype(auto) return type
> for functions and lambda expressions.
>
> As demonstrated in this commit, this can simplify cases where previously
> the compiler failed to deduce the correct return type.
>
> Just for reference, for the "assign to ref" cases fixed here, there are
> 3 possible solutions:
>
> - Return a pointer and immediately deref as used before this commit
> - Make sure the function/lambda returns declspec(auto) as used after
> this commit
> - Class& i = WITH_LOCK(..., return std::ref(...));
>
> -----
>
> References:
> 1. https://en.cppreference.com/w/cpp/language/function#Return_type_deduction
> 2. https://en.cppreference.com/w/cpp/language/template_argument_deduction#Other_contexts
> 3. https://en.cppreference.com/w/cpp/language/auto
> 4. https://en.cppreference.com/w/cpp/language/decltype
>
> Explanations:
> 1. https://stackoverflow.com/a/21369192
> 2. https://stackoverflow.com/a/21369170
Thanks to sipa and ryanofsky for helping me understand this
ACKs for top commit:
jnewbery:
utACK 3eb94ec81b
hebasto:
ACK 3eb94ec81b, I have reviewed the code and it looks OK, I agree it can be merged. I have verified possible warnings:
ryanofsky:
Code review ACK 3eb94ec81b
Tree-SHA512: 5f55c7722aeca8ea70e5c1a8db93e93ba0e356e8967e7f607ada38003df4b153d73c29bd2cea8d7ec1344720d37d857ea7dbfd2a88da1d92e0e9cbb9abd287df
95975dd08d sync: detect double lock from the same thread (Vasil Dimov)
4df6567e4c sync: make EnterCritical() & push_lock() type safe (Vasil Dimov)
Pull request description:
Double lock of the same (non-recursive) mutex from the same thread would produce an undefined behavior. Detect this from `DEBUG_LOCKORDER` and react similarly to the deadlock detection.
This came up during discussion in another, related PR: https://github.com/bitcoin/bitcoin/pull/19238#discussion_r442394521.
ACKs for top commit:
laanwj:
code review ACK 95975dd08d
hebasto:
re-ACK 95975dd08d
Tree-SHA512: 375c62db7819e348bfaecc3bd82a7907fcd8f5af24f7d637ac82f3f16789da9fc127dbd0e37158a08e0dcbba01a55c6635caf1d8e9e827cf5a3747f7690a498e
The functions `EnterCritical()` and `push_lock()` take a pointer to a
mutex, but that pointer used to be of type `void*` because we use a few
different types for mutexes. This `void*` argument was not type safe
because somebody could have send a pointer to anything that is not a
mutex. Furthermore it wouldn't allow to check whether the passed mutex
is recursive or not.
Thus, change the functions to templated ones so that we can implement
stricter checks for non-recursive mutexes. This also simplifies the
callers of `EnterCritical()`.
