From 985574c4f52fbe7d852a37ae881500fec2f31c6c Mon Sep 17 00:00:00 2001 From: Mikhail Krinkin Date: Mon, 20 Jan 2025 17:44:20 +0000 Subject: [PATCH] Change thread safety annotations in TLS code When I tried to build Envoy with Clang-18 I hit an issue that Clang thread safety analizer does not like the fact that we are returning a reference to a protected member (central_cache_) from centralCacheLockHeld method. While I do think that the code is correct, when looking at the thread safety annotations I think we could do a little bit better. Currently, centralCacheLockHeld is annotated with ABLS_ASSERT_EXCLUSIVE_LOCK. My understanding is that this annotation should be used on functions that during runtime check that the right lock is held and fail if it's not the case. centralCacheLockHeld currently does not actually check that the lock is held - this seems somewhat misleading and I don't think that thread safety analysis should derive anything from this annotation TBH, as there is no runtime check present there. We could add a runtime check to the function, but unfortunately it will not be enough to address Clang's warning (see https://github.com/llvm/llvm-project/issues/123512). Besides I think that we can do slightly better. This change replaces ABLS_ASSERT_EXCLUSIVE_LOCK with ABSL_EXCLUSIVE_LOCKS_REQUIRED, to let Clang thread safety analysis know during compilation time that this function should be called under a lock. That change triggered a few more warnings in various places that call into centralCacheLockHeld. In simple cases just adding ABSL_EXCLUSIVE_LOCKS_REQUIRED to the functions that call centralCacheLockHeld was enough. There were a couple of more complicated cases that Clang could not figure out because it does not support aliasing (i.e., when the same mutex is known under different names, Clang cannot always figure out that different names refer to the same lock). To deal with those cases I added assertHeld method with ABLS_ASSERT_EXCLUSIVE_LOCK annotation to the lock implementation and used it to help Clang to figure out what locks are held. All-in-all, I think relying on ABSL_EXCLUSIVE_LOCKS_REQUIRED is slightly better and it addresses the warning for me as well. Signed-off-by: Mikhail Krinkin --- source/common/common/thread.h | 2 ++ source/common/stats/thread_local_store.cc | 3 +++ source/common/stats/thread_local_store.h | 15 +++++++++------ 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 105f95ae89f6a..096f228eebacd 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -25,6 +25,8 @@ class MutexBasicLockable : public BasicLockable { bool tryLock() ABSL_EXCLUSIVE_TRYLOCK_FUNCTION(true) override { return mutex_.TryLock(); } void unlock() ABSL_UNLOCK_FUNCTION() override { mutex_.Unlock(); } + void assertHeld() ABSL_ASSERT_EXCLUSIVE_LOCK(this) { mutex_.AssertHeld(); } + private: friend class CondVar; absl::Mutex mutex_; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 09e235e6ac4c6..4dec99975b57d 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -54,6 +54,7 @@ ThreadLocalStoreImpl::~ThreadLocalStoreImpl() { void ThreadLocalStoreImpl::setHistogramSettings(HistogramSettingsConstPtr&& histogram_settings) { iterateScopes([](const ScopeImplSharedPtr& scope) -> bool { + scope->parent_.lock_.assertHeld(); ASSERT(scope->centralCacheLockHeld()->histograms_.empty()); return true; }); @@ -74,6 +75,7 @@ void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) { const uint32_t first_histogram_index = deleted_histograms_.size(); iterateScopesLockHeld([this](const ScopeImplSharedPtr& scope) ABSL_EXCLUSIVE_LOCKS_REQUIRED( lock_) -> bool { + scope->parent_.lock_.assertHeld(); const CentralCacheEntrySharedPtr& central_cache = scope->centralCacheLockHeld(); removeRejectedStats(central_cache->counters_, [this](const CounterSharedPtr& counter) mutable { @@ -293,6 +295,7 @@ void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) { // VirtualHosts. bool need_post = scopes_to_cleanup_.empty(); scopes_to_cleanup_.push_back(scope->scope_id_); + scope->parent_.lock_.assertHeld(); central_cache_entries_to_cleanup_.push_back(scope->centralCacheLockHeld()); lock.release(); diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index f8f27558b4005..4aab7f050f1a3 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -339,16 +339,16 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo return iterateLockHeld(fn); } - bool iterateLockHeld(const IterateFn& fn) const { + bool iterateLockHeld(const IterateFn& fn) const ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) { return iterHelper(fn, centralCacheLockHeld()->counters_); } - bool iterateLockHeld(const IterateFn& fn) const { + bool iterateLockHeld(const IterateFn& fn) const ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) { return iterHelper(fn, centralCacheLockHeld()->gauges_); } - bool iterateLockHeld(const IterateFn& fn) const { + bool iterateLockHeld(const IterateFn& fn) const ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) { return iterHelper(fn, centralCacheLockHeld()->histograms_); } - bool iterateLockHeld(const IterateFn& fn) const { + bool iterateLockHeld(const IterateFn& fn) const ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) { return iterHelper(fn, centralCacheLockHeld()->text_readouts_); } ThreadLocalStoreImpl& store() override { return parent_; } @@ -421,7 +421,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo // scope->central_cache_, the analysis system cannot understand that the // scope's parent_.lock_ is held, so we assert that here. const CentralCacheEntrySharedPtr& centralCacheLockHeld() const - ABSL_ASSERT_EXCLUSIVE_LOCK(parent_.lock_) { + ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) { return central_cache_; } @@ -482,7 +482,10 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo // The Store versions of iterate cover all the scopes in the store. template bool iterHelper(StatFn fn) const { return iterateScopes( - [fn](const ScopeImplSharedPtr& scope) -> bool { return scope->iterateLockHeld(fn); }); + [fn](const ScopeImplSharedPtr& scope) -> bool { + scope->parent_.lock_.assertHeld(); + return scope->iterateLockHeld(fn); + }); } std::string getTagsForName(const std::string& name, TagVector& tags) const;