Annotate BRAVO locks for thread-safety analysis#13330
Merged
Merged
Conversation
Enabling -Wthread-safety tree-wide surfaced findings in Bravo.h on macOS, where libc++ marks the underlying std::shared_mutex as a capability: the wrapper acquires and releases it across method boundaries and on fast/slow-path branches the analyzer cannot follow. Fedora CI uses libstdc++, which does not annotate std types, so it never saw these. Make ts::bravo::shared_mutex_impl a real capability so the contract is checked on every Clang, not papered over on one std lib; its lock-driving bodies are the trusted implementation and stay exempted, matching the ts::shared_mutex pattern. Make the reader guard a rigid scoped capability -- acquire in constructor, release in destructor, no copy/move/defer/release -- which the analysis tracks with no exemption; the movable std::shared_lock-style forms it replaced could not be tracked, and no caller used them. Restructure the unit test's try-lock asserts so the acquire gates a branch.
masaori335
added
Build
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the BRAVO reader/writer lock wrapper (ts::bravo::shared_mutex_impl) to participate cleanly in Clang’s -Wthread-safety analysis across different standard libraries (notably libc++ on macOS), by making the wrapper itself the tracked capability and using a rigid scoped reader guard that the analyzer can follow.
Changes:
- Annotate
ts::bravo::shared_mutex_implas a thread-safety capability and mark its lock/unlock methods with acquire/release contracts (exempting their bodies from analysis where needed). - Replace the flexible/movable
ts::bravo::shared_lockAPI with a strict acquire-in-ctor / release-in-dtor scoped capability to improve analyzability. - Restructure BRAVO unit test try-lock assertions so unlocks only occur on the success branch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
include/tsutil/Bravo.h |
Adds Clang thread-safety capability annotations and refactors BRAVO’s shared guard into a rigid scoped capability. |
src/tsutil/unit_tests/test_Bravo.cc |
Updates try-lock test structure to align with conditional-acquire modeling and avoid unconditional unlocks. |
lock_shared/try_lock_shared assigned the Token only on the fast path, so a reused non-zero Token surviving into a slow-path acquisition would make unlock_shared release a reader slot instead of the underlying mutex. Clear it on entry to enforce the documented 0-init contract.
moonchen
approved these changes
Jun 25, 2026
moonchen
left a comment
moonchen
left a comment
Contributor
There was a problem hiding this comment.
Thanks for adding these annotations. Confirmed the token = 0 reset is correct and doesn't affect existing callers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
-Wthread-safetyin BRAVO implementation.