Address annotation gaps found by concurrency analysis#5151
Address annotation gaps found by concurrency analysis#5151marta-lokhova wants to merge 2 commits intostellar:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses concurrency analysis gaps by adding thread safety annotations to various classes. The changes convert standard library mutex types to custom wrapper types that support Clang's thread safety analysis, add GUARDED_BY annotations to protected fields, and replace standard lock guards with macro-based equivalents that work with the thread safety analysis framework.
Changes:
- Convert mutexes in VirtualClock, ProcessManagerImpl, and LiveBucketIndex to thread-safety-annotated wrapper types (Mutex, RecursiveMutex, SharedMutex)
- Add GUARDED_BY annotations to fields protected by these mutexes
- Replace std::lock_guard usages with LOCK_GUARD/RECURSIVE_LOCK_GUARD macros
- Document lock ordering between Peer::mStateMutex and Hmac::mMutex using ACQUIRED_BEFORE annotation
- Add thread assertion to LedgerManagerImpl::isApplying()
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/Timer.h | Convert VirtualClock mutexes to Mutex type and add GUARDED_BY annotations |
| src/util/Timer.cpp | Replace std::lock_guard with LOCK_GUARD macro |
| src/process/ProcessManagerImpl.h | Convert mProcessesMutex to RecursiveMutex and add GUARDED_BY annotations |
| src/process/ProcessManagerImpl.cpp | Replace std::lock_guard with RECURSIVE_LOCK_GUARD macro |
| src/overlay/Peer.h | Add ACQUIRED_BEFORE annotation to document lock ordering with Hmac::mMutex |
| src/overlay/Hmac.h | Expose mMutex as public under THREAD_SAFETY and add GUARDED_BY to fields |
| src/overlay/Hmac.cpp | Add LOCK_GUARD to damageRecvMacKey test method |
| src/ledger/LedgerManagerImpl.h | Add thread assertion to isApplying() getter |
| src/bucket/LiveBucketIndex.h | Convert mCacheMutex to SharedMutex and add GUARDED_BY annotation |
| src/bucket/LiveBucketIndex.cpp | Replace std::shared_lock/std::unique_lock with SharedLockShared/SharedLockExclusive |
SirTyson
left a comment
SirTyson
left a comment
There was a problem hiding this comment.
LGTM, but I think while we're here we can add much better tracy visibility into our locks.
marta-lokhova
force-pushed
the
staticThreadSafety
branch
3 times, most recently
from
397bf2e to
f448eb2
Compare
marta-lokhova
force-pushed
the
staticThreadSafety
branch
from
f448eb2 to
0b59f27
Compare
| AC_MSG_NOTICE([enabling thread safety static analysis, see https://clang.llvm.org/docs/ThreadSafetyAnalysis.html]) | ||
| AS_CASE(["$CXX"], | ||
| [clang*], [CXXFLAGS="$CXXFLAGS -Wthread-safety -Werror=thread-safety -DTHREAD_SAFETY"], | ||
| [clang*], [CXXFLAGS="$CXXFLAGS -Wthread-safety -Werror=thread-safety -Wthread-safety-beta -Werror=thread-safety-beta -DTHREAD_SAFETY"], |
There was a problem hiding this comment.
A slightly annoying discovery - some features like ACQUIRED_BEFORE/ACQUIRED_AFTER are not actually implemented in clang20 (as per llvm/llvm-project#51788). These are implemented in thread-safety-beta though, and newer clang versions properly promote them stable (see llvm/llvm-project#152853). Either way, I think it doesn't hurt to enable thread-safety-beta for more coverage.
SirTyson
left a comment
SirTyson
left a comment
There was a problem hiding this comment.
LGTM overall! I think if we generate tracy graphs that are too large, we can do a followup where we narrow down our uses of the macro. Imo tracy graphs are already huge and I'd rather have larger traces with full visibility instead of having blind spots when possible. It's a micro-benchmarking tool anyway, so I think we should avoid optimizing for size/noise unless it's really necessary.
Couple small nits, and the macro accidentally upgraded some pre-existing mutexes to mutable. Probably not a big deal, but we can be a little more conservative.
| // The annotations parameter can be empty or contain one or more annotations. | ||
| #ifndef USE_TRACY | ||
| #define ANNOTATED_MUTEX(VarName, ...) \ | ||
| Mutex mutable VarName ANNOTATE_FOR_THREAD_SAFETY(__VA_ARGS__) |
There was a problem hiding this comment.
ProcessManagerImpl::mProcessesMutex was not mutable, but now is. Can we drop the mutable from the macro so we can maintain both mutable ANNOTATED_MUTEX and ANNOTATED_MUTEX? In practice it's probably fine for mutexes to always be mutable, but this is "downgrading" mutexes that were previously not mutable.
There was a problem hiding this comment.
ya I think it's harmless, but don't feel strongly
| // https://clang.llvm.org/docs/ThreadSafetyAnalysis.html | ||
|
|
||
| #ifndef THREAD_ANNOTATIONS_H_ | ||
| #define THREAD_ANNOTATIONS_H_ |
There was a problem hiding this comment.
Get rid of THREAD_ANNOTATIONS_H_ since we already have #pragma once
| Mutex mMutex; | ||
| #else | ||
| TracyLockable(std::mutex, mMutex); | ||
| #ifdef THREAD_SAFETY |
There was a problem hiding this comment.
Nit: This friend declaration is not intuitive, a short comment would be nice.
| // When Tracy is enabled, we use TracyLockable for profiling visibility. | ||
| // Note: We still provide a wrapper class for consistency with non-Tracy builds. | ||
| // Users should access the underlying TracyLockable directly for profiling. | ||
| class Mutex : public stellar::NonMovableOrCopyable |
There was a problem hiding this comment.
This is now dead code since we have the maco. We never use the Mutex type when tracy is enabled.
There was a problem hiding this comment.
Yeah, see the comment - support is added for completeness (I don't mind removing it though if you feel strongly)
No description provided.