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
llvm/llvm-project#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 <mkrinkin@microsoft.com>
Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>

Author: krinkinmu

source/common/stats/thread_local_store.cc

@@ -53,10 +53,12 @@ ThreadLocalStoreImpl::~ThreadLocalStoreImpl() {
 }
 
 void ThreadLocalStoreImpl::setHistogramSettings(HistogramSettingsConstPtr&& histogram_settings) {
-  iterateScopes([](const ScopeImplSharedPtr& scope) -> bool {
-    ASSERT(scope->centralCacheLockHeld()->histograms_.empty());
-    return true;
-  });
+  iterateScopes([this](const ScopeImplSharedPtr& scope)
+                    ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) -> bool {
+                      assertLocked(*scope);
+                      ASSERT(scope->centralCacheLockHeld()->histograms_.empty());
+                      return true;
+                    });
   histogram_settings_ = std::move(histogram_settings);
 }
 
@@ -74,6 +76,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 {
+    assertLocked(*scope);
     const CentralCacheEntrySharedPtr& central_cache = scope->centralCacheLockHeld();
     removeRejectedStats<CounterSharedPtr>(central_cache->counters_,
                                           [this](const CounterSharedPtr& counter) mutable {
@@ -293,6 +296,7 @@ void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) {
     // VirtualHosts.
     bool need_post = scopes_to_cleanup_.empty();
     scopes_to_cleanup_.push_back(scope->scope_id_);
+    assertLocked(*scope);
     central_cache_entries_to_cleanup_.push_back(scope->centralCacheLockHeld());
     lock.release();
 

source/common/stats/thread_local_store.h

@@ -339,16 +339,20 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
       return iterateLockHeld(fn);
     }
 
-    bool iterateLockHeld(const IterateFn<Counter>& fn) const {
+    bool iterateLockHeld(const IterateFn<Counter>& fn) const
+        ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) {
       return iterHelper(fn, centralCacheLockHeld()->counters_);
     }
-    bool iterateLockHeld(const IterateFn<Gauge>& fn) const {
+    bool iterateLockHeld(const IterateFn<Gauge>& fn) const
+        ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) {
       return iterHelper(fn, centralCacheLockHeld()->gauges_);
     }
-    bool iterateLockHeld(const IterateFn<Histogram>& fn) const {
+    bool iterateLockHeld(const IterateFn<Histogram>& fn) const
+        ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) {
       return iterHelper(fn, centralCacheLockHeld()->histograms_);
     }
-    bool iterateLockHeld(const IterateFn<TextReadout>& fn) const {
+    bool iterateLockHeld(const IterateFn<TextReadout>& fn) const
+        ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_.lock_) {
       return iterHelper(fn, centralCacheLockHeld()->text_readouts_);
     }
     ThreadLocalStoreImpl& store() override { return parent_; }
@@ -421,7 +425,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, 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_;
     }
 
@@ -463,6 +467,19 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
 
   using ScopeImplSharedPtr = std::shared_ptr<ScopeImpl>;
 
+  /**
+   * assertLocked exists to help compiler figure out that lock_ and scope->parent_.lock_ is
+   * actually the same lock known under two different names. This function requires lock_ to
+   * be held when it's called and at the same time it is annotated as if it checks in runtime
+   * that scope->parent_.lock_ is held. It does not actually perform any runtime checks, because
+   * those aren't needed since we know that scope->parent_ refers to ThreadLockStoreImpl and
+   * therefore scope->parent_.lock is the same as lock_.
+   */
+  void assertLocked(const ScopeImpl& scope) const ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_)
+      ABSL_ASSERT_EXCLUSIVE_LOCK(scope.parent_.lock_) {
+    (void)scope;
+  }
+
   /**
    * Calls fn_lock_held for every scope with, lock_ held. This avoids iterate/destruct
    * races for scopes.
@@ -481,8 +498,11 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
 
   // The Store versions of iterate cover all the scopes in the store.
   template <class StatFn> bool iterHelper(StatFn fn) const {
-    return iterateScopes(
-        [fn](const ScopeImplSharedPtr& scope) -> bool { return scope->iterateLockHeld(fn); });
+    return iterateScopes([this, fn](const ScopeImplSharedPtr& scope)
+                             ABSL_EXCLUSIVE_LOCKS_REQUIRED(lock_) -> bool {
+                               assertLocked(*scope);
+                               return scope->iterateLockHeld(fn);
+                             });
   }
 
   std::string getTagsForName(const std::string& name, TagVector& tags) const;