This is an automated email from the ASF dual-hosted git repository.
masaori335 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 4fabae0c72 Annotate BRAVO locks for thread-safety analysis (#13330)
4fabae0c72 is described below
commit 4fabae0c7299a45f4d448e9a8e6de0917b2416c9
Author: Masaori Koshiba <[email protected]>
AuthorDate: Fri Jun 26 07:52:16 2026 +0900
Annotate BRAVO locks for thread-safety analysis (#13330)
* Annotate BRAVO locks for thread-safety analysis
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.
* Reset BRAVO Token on entry to harden slow-path unlock
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.
---
include/tsutil/Bravo.h | 140 +++++++++++-------------------------
src/tsutil/unit_tests/test_Bravo.cc | 35 ++++++---
2 files changed, 65 insertions(+), 110 deletions(-)
diff --git a/include/tsutil/Bravo.h b/include/tsutil/Bravo.h
index 4660f69aca..1dc1043d68 100644
--- a/include/tsutil/Bravo.h
+++ b/include/tsutil/Bravo.h
@@ -39,6 +39,7 @@
#include "tsutil/DenseThreadId.h"
#include "tsutil/Assert.h"
+#include "tsutil/ts_thread_safety.h"
#include <array>
#include <atomic>
@@ -68,129 +69,54 @@ using Token = size_t;
/**
ts::bravo::shared_lock
+
+ Reader guard for shared_mutex_impl, carrying the BRAVO Token. A rigid scoped
+ capability: it acquires the shared lock in its constructor and releases it
in
+ its destructor, with no copy, move, defer or release. That rigidity is what
+ lets the analysis track it -- the deferred and movable forms of
+ std::shared_lock let the held state escape a single scope and cannot be
+ modelled -- and BRAVO readers need only this scoped form.
*/
-template <class Mutex> class shared_lock
+template <class Mutex> class TS_SCOPED_CAPABILITY shared_lock
{
public:
using mutex_type = Mutex;
- shared_lock() noexcept = default;
- shared_lock(Mutex &m) : _mutex(&m) { lock(); }
- shared_lock(Mutex &m, std::try_to_lock_t) : _mutex(&m) { try_lock(); }
- shared_lock(Mutex &m, std::defer_lock_t) noexcept : _mutex(&m) {}
-
- ~shared_lock()
- {
- if (_owns) {
- _mutex->unlock_shared(_token);
- }
- };
+ explicit shared_lock(Mutex &m) TS_ACQUIRE_SHARED(m) : _mutex(&m) {
_mutex->lock_shared(_token); }
+ ~shared_lock() TS_RELEASE() { _mutex->unlock_shared(_token); }
////
- // Not Copyable
+ // Neither copyable nor movable: the held shared lock must not escape this
scope.
//
shared_lock(shared_lock const &) = delete;
shared_lock &operator=(shared_lock const &) = delete;
-
- ////
- // Moveable
- //
- shared_lock(shared_lock &&s) : _mutex(s._mutex), _token(s._token),
_owns(s._owns)
- {
- s._mutex = nullptr;
- s._token = 0;
- s._owns = false;
- };
-
- shared_lock &
- operator=(shared_lock &&s)
- {
- if (_owns) {
- _mutex->unlock_shared(_token);
- }
- _mutex = s._mutex;
- _token = s._token;
- _owns = s._owns;
-
- s._mutex = nullptr;
- s._token = 0;
- s._owns = false;
- };
-
- ////
- // Shared locking
- //
- void
- lock()
- {
- _mutex->lock_shared(_token);
- _owns = true;
- }
-
- bool
- try_lock()
- {
- _owns = _mutex->try_lock_shared(_token);
- return _owns;
- }
-
- // not implemented yet
- bool try_lock_for() = delete;
- bool try_lock_until() = delete;
-
- void
- unlock()
- {
- _mutex->unlock_shared(_token);
- _owns = false;
- }
-
- ////
- // Modifiers
- //
- void
- swap(shared_lock &s)
- {
- std::swap(_mutex, s._mutex);
- std::swap(_token, s._token);
- std::swap(_owns, s._owns);
- }
-
- mutex_type *
- release()
- {
- mutex_type *m = _mutex;
- _mutex = nullptr;
- _token = 0;
- _owns = false;
- return m;
- }
+ shared_lock(shared_lock &&) = delete;
+ shared_lock &operator=(shared_lock &&) = delete;
////
// Observers
//
mutex_type *
- mutex()
+ mutex() const
{
return _mutex;
}
Token
- token()
+ token() const
{
return _token;
}
bool
- owns_lock()
+ owns_lock() const
{
- return _owns;
+ return _mutex != nullptr;
}
private:
mutex_type *_mutex = nullptr;
Token _token = 0;
- bool _owns = false;
};
/**
@@ -201,7 +127,8 @@ private:
Set the SLOT_SIZE larger than DenseThreadId::num_possible_values to go
fast-path.
*/
-template <typename T = std::shared_mutex, size_t SLOT_SIZE = 256, int
SLOWDOWN_GUARD = 7> class shared_mutex_impl
+template <typename T = std::shared_mutex, size_t SLOT_SIZE = 256, int
SLOWDOWN_GUARD = 7>
+class TS_CAPABILITY("shared_mutex") shared_mutex_impl
{
public:
shared_mutex_impl() = default;
@@ -219,15 +146,22 @@ public:
////
// Exclusive locking
//
+ // These lock/unlock methods are the trusted implementation of this
capability:
+ // their bodies drive the underlying lock (T, by default std::shared_mutex,
+ // which libc++ annotates as a capability) across method boundaries and along
+ // the BRAVO fast/slow-path branches -- patterns the analysis cannot follow.
+ // Exempt the bodies so only the capability contract on each signature is
+ // checked; the data-race checking happens at the call sites.
+ //
void
- lock()
+ lock() TS_ACQUIRE() TS_NO_THREAD_SAFETY_ANALYSIS
{
_mutex.underlying.lock();
_revoke();
}
bool
- try_lock()
+ try_lock() TS_TRY_ACQUIRE(true) TS_NO_THREAD_SAFETY_ANALYSIS
{
bool r = _mutex.underlying.try_lock();
if (!r) {
@@ -240,19 +174,22 @@ public:
}
void
- unlock()
+ unlock() TS_RELEASE() TS_NO_THREAD_SAFETY_ANALYSIS
{
_mutex.underlying.unlock();
}
////
- // Shared locking
+ // Shared locking (bodies exempted for the same reason as the exclusive ones
above)
//
void
- lock_shared(Token &token)
+ lock_shared(Token &token) TS_ACQUIRE_SHARED() TS_NO_THREAD_SAFETY_ANALYSIS
{
debug_assert(SLOT_SIZE >= DenseThreadId::num_possible_values());
+ // Clear up front so a slow-path acquisition never leaves a stale
fast-path slot for unlock_shared().
+ token = 0;
+
// Fast path
if (_mutex.read_bias.load(std::memory_order_acquire)) {
size_t index = DenseThreadId::self() % SLOT_SIZE;
@@ -277,10 +214,13 @@ public:
}
bool
- try_lock_shared(Token &token)
+ try_lock_shared(Token &token) TS_TRY_ACQUIRE_SHARED(true)
TS_NO_THREAD_SAFETY_ANALYSIS
{
debug_assert(SLOT_SIZE >= DenseThreadId::num_possible_values());
+ // Clear up front so a slow-path acquisition never leaves a stale
fast-path slot for unlock_shared().
+ token = 0;
+
// Fast path
if (_mutex.read_bias.load(std::memory_order_acquire)) {
size_t index = DenseThreadId::self() % SLOT_SIZE;
@@ -313,7 +253,7 @@ public:
}
void
- unlock_shared(const Token token)
+ unlock_shared(const Token token) TS_RELEASE_SHARED()
TS_NO_THREAD_SAFETY_ANALYSIS
{
if (token == 0) {
_mutex.underlying.unlock_shared();
diff --git a/src/tsutil/unit_tests/test_Bravo.cc
b/src/tsutil/unit_tests/test_Bravo.cc
index 1a825cd946..666ae4f5cc 100644
--- a/src/tsutil/unit_tests/test_Bravo.cc
+++ b/src/tsutil/unit_tests/test_Bravo.cc
@@ -41,8 +41,11 @@ TEST_CASE("BRAVO - simple check", "[libts][BRAVO]")
std::thread t{[](ts::bravo::shared_mutex &mutex) {
ts::bravo::Token token{0};
- CHECK(mutex.try_lock_shared(token) == true);
- mutex.unlock_shared(token);
+ bool locked = mutex.try_lock_shared(token);
+ CHECK(locked == true);
+ if (locked) {
+ mutex.unlock_shared(token);
+ }
},
std::ref(mutex)};
@@ -95,28 +98,40 @@ TEST_CASE("BRAVO - multiple try-lock", "[libts][BRAVO]")
{
ts::bravo::Token token{0};
- CHECK(mutex.try_lock_shared(token));
+ bool locked = mutex.try_lock_shared(token);
+ CHECK(locked);
CHECK(i == 0);
- mutex.unlock_shared(token);
+ if (locked) {
+ mutex.unlock_shared(token);
+ }
}
{
- CHECK(mutex.try_lock());
+ bool locked = mutex.try_lock();
+ CHECK(locked);
CHECK(++i == 1);
- mutex.unlock();
+ if (locked) {
+ mutex.unlock();
+ }
}
{
ts::bravo::Token token{0};
- CHECK(mutex.try_lock_shared(token));
+ bool locked = mutex.try_lock_shared(token);
+ CHECK(locked);
CHECK(i == 1);
- mutex.unlock_shared(token);
+ if (locked) {
+ mutex.unlock_shared(token);
+ }
}
{
- CHECK(mutex.try_lock());
+ bool locked = mutex.try_lock();
+ CHECK(locked);
CHECK(++i == 2);
- mutex.unlock();
+ if (locked) {
+ mutex.unlock();
+ }
}
CHECK(i == 2);
