Introduce Clang Thread Safety Analysis, and apply it to two subsystems

CMakeLists.txt

@@ -169,6 +169,8 @@ option(EXTERNAL_LIBSWOC "Use external libswoc (default OFF)")
 option(LINK_PLUGINS "Link core libraries to plugins (default OFF)")
 option(ENABLE_PROBES "Enable ATS SystemTap probes (default OFF)")
 option(ENABLE_VERIFY_PLUGINS "Enable plugin verification tests (default ON)" ON)
+option(ENABLE_THREAD_SAFETY_ANALYSIS "Enable Clang -Wthread-safety analysis (Clang only, default ON)" ON)
+option(THREAD_SAFETY_ANALYSIS_AS_ERROR "Treat thread-safety findings as errors; for CI gating (default OFF)")
 
 # Setup user
 # NOTE: this is the user trafficserver runs as
@@ -358,6 +360,35 @@ elseif(ENABLE_TSAN)
   add_link_options(-g -fsanitize=thread)
 endif()
 
+# Clang Thread Safety Analysis. The TS_* annotations (tsutil/ts_thread_safety.h)
+# compile to nothing on GCC, and the flag is Clang-only on purpose (GCC does not
+# know -Wthread-safety and would itself error if passed it), so this whole block
+# is a no-op for GCC builds.
+#
+# On Clang it is on by default but only a WARNING: -Wno-error=thread-safety keeps
+# it a warning even in builds that otherwise use -Werror, so an in-progress
+# annotation never blocks a developer's build. CI sets
+# THREAD_SAFETY_ANALYSIS_AS_ERROR=ON to promote findings to errors and gate
+# merges.
+#
+# Skip FreeBSD: its libc annotates the pthread primitives themselves, so
+# -Wthread-safety there flags ATS's existing hand-rolled mutex wrappers
+# (tscore/ink_mutex.h, ink_rwlock, ...) tree-wide, not just newly-annotated code.
+# Enabling it on FreeBSD needs those legacy wrappers made analysis-clean first.
+# Elsewhere the platform leaves the pthread primitives un-annotated, so only
+# annotated code is analyzed and this stays quiet until a real violation.
+if(ENABLE_THREAD_SAFETY_ANALYSIS
+   AND CMAKE_CXX_COMPILER_ID MATCHES "Clang"
+   AND NOT CMAKE_SYSTEM_NAME STREQUAL "FreeBSD"
+)
+  add_compile_options(-Wthread-safety)
+  if(THREAD_SAFETY_ANALYSIS_AS_ERROR)
+    add_compile_options(-Werror=thread-safety)
+  else()
+    add_compile_options(-Wno-error=thread-safety)
+  endif()
+endif()
+
 if(ENABLE_PROBES)
   add_compile_options("-DENABLE_SYSTEMTAP_PROBES")
 endif()

CMakePresets.json

@@ -125,6 +125,8 @@
       "cacheVariables": {
         "CMAKE_BUILD_TYPE": "Debug",
         "CMAKE_COMPILE_WARNING_AS_ERROR": "ON",
+        "ENABLE_THREAD_SAFETY_ANALYSIS": "ON",
+        "THREAD_SAFETY_ANALYSIS_AS_ERROR": "ON",
         "ENABLE_CCACHE": "ON",
         "BUILD_EXPERIMENTAL_PLUGINS": "ON",
         "ENABLE_WASM_WAMR": "OFF",

include/tsutil/Metrics.h

@@ -38,6 +38,7 @@
 #include "swoc/MemSpan.h"
 
 #include "tsutil/Assert.h"
+#include "tsutil/TsMutex.h"
 
 namespace ts
 {
@@ -304,17 +305,17 @@ class Metrics
 
   class Storage
   {
-    BlobStorage        _blobs;
-    uint16_t           _cur_blob = 0;
-    uint16_t           _cur_off  = 0;
-    LookupTable        _lookups;
-    mutable std::mutex _mutex;
+    BlobStorage _blobs   TS_GUARDED_BY(_mutex);
+    uint16_t _cur_blob   TS_GUARDED_BY(_mutex) = 0;
+    uint16_t _cur_off    TS_GUARDED_BY(_mutex) = 0;
+    LookupTable _lookups TS_GUARDED_BY(_mutex);
+    mutable ts::mutex    _mutex;
 
   public:
     Storage(const Storage &)            = delete;
     Storage &operator=(const Storage &) = delete;
 
-    Storage()
+    Storage() TS_NO_THREAD_SAFETY_ANALYSIS // single-threaded construction; not yet shared
     {
       _blobs[0] = std::make_unique<NamesAndAtomics>();
       release_assert(_blobs[0]);
@@ -325,7 +326,7 @@ class Metrics
     ~Storage() {}
 
     IdType           create(const std::string_view name, const MetricType type = MetricType::COUNTER);
-    void             addBlob();
+    void             addBlob() TS_REQUIRES(_mutex);
     IdType           lookup(const std::string_view name) const;
     AtomicType      *lookup(const std::string_view name, IdType *out_id, MetricType *out_type = nullptr) const;
     AtomicType      *lookup(Metrics::IdType id, std::string_view *out_name = nullptr, MetricType *out_type = nullptr) const;
@@ -337,7 +338,7 @@ class Metrics
     std::pair<int16_t, int16_t>
     current() const
     {
-      std::lock_guard lock(_mutex);
+      ts::lock_guard lock(_mutex);
       return {_cur_blob, _cur_off};
     }
 
@@ -346,6 +347,7 @@ class Metrics
     {
       auto [blob, entry] = _splitID(id);
 
+      ts::lock_guard lock(_mutex);
       return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off)));
     }
   };

include/tsutil/TsMutex.h

@@ -0,0 +1,100 @@
+/** @file
+
+  A std::mutex annotated for Clang Thread Safety Analysis, with a matching
+  scoped lock guard.
+
+  These are the plain-mutex counterparts to ts::shared_mutex and its
+  reader/writer guards (TsSharedMutex.h): use ts::mutex with ts::lock_guard
+  wherever you would otherwise use std::mutex with std::lock_guard, but want the
+  data it protects checked by -Wthread-safety. The runtime behavior is exactly
+  that of std::mutex; the annotations are compile-time only (see
+  tsutil/ts_thread_safety.h).
+
+  @section license License
+
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+ */
+
+#pragma once
+
+#include <mutex>
+
+#include "tsutil/ts_thread_safety.h"
+
+namespace ts
+{
+// A std::mutex marked as a Clang thread-safety capability, so data guarded by it
+// can be checked by -Wthread-safety. Same interface and runtime behavior as
+// std::mutex.
+//
+class TS_CAPABILITY("mutex") mutex
+{
+public:
+  mutex()                         = default;
+  mutex(mutex const &)            = delete;
+  mutex &operator=(mutex const &) = delete;
+
+  // The lock/unlock bodies are the trusted implementation of this capability:
+  // exempt them from analysis so only the capability contract on each signature
+  // is checked. The actual data-race checking happens at the call sites.
+  void
+  lock() TS_ACQUIRE() TS_NO_THREAD_SAFETY_ANALYSIS
+  {
+    _m.lock();
+  }
+  bool
+  try_lock() TS_TRY_ACQUIRE(true) TS_NO_THREAD_SAFETY_ANALYSIS
+  {
+    return _m.try_lock();
+  }
+  void
+  unlock() TS_RELEASE() TS_NO_THREAD_SAFETY_ANALYSIS
+  {
+    _m.unlock();
+  }
+
+  using native_handle_type = std::mutex::native_handle_type;
+
+  native_handle_type
+  native_handle()
+  {
+    return _m.native_handle();
+  }
+
+private:
+  std::mutex _m;
+};
+
+// RAII guard for ts::mutex that carries Clang thread-safety capability state.
+// Prefer over std::lock_guard / std::unique_lock in code annotated for
+// -Wthread-safety (see tsutil/ts_thread_safety.h for why the std wrappers are
+// not tracked).
+//
+class TS_SCOPED_CAPABILITY lock_guard
+{
+public:
+  explicit lock_guard(mutex &m) TS_ACQUIRE(m) : _m(m) { _m.lock(); }
+  ~lock_guard() TS_RELEASE() { _m.unlock(); }
+
+  lock_guard(lock_guard const &)            = delete;
+  lock_guard &operator=(lock_guard const &) = delete;
+
+private:
+  mutex &_m;
+};
+
+} // end namespace ts

include/tsutil/TsSharedMutex.h

@@ -27,6 +27,7 @@
 #include <pthread.h>
 #include "tsutil/Strerror.h"
 #include "tsutil/Assert.h"
+#include "tsutil/ts_thread_safety.h"
 
 #ifdef X
 #error "X preprocessor symbol defined"
@@ -48,7 +49,7 @@ namespace ts
 {
 // A class with the same interface as std::shared_mutex, but which is not prone to writer starvation.
 //
-class shared_mutex
+class TS_CAPABILITY("shared_mutex") shared_mutex
 {
 public:
   shared_mutex() {}
@@ -58,8 +59,13 @@ class shared_mutex
   shared_mutex(shared_mutex const &)            = delete;
   shared_mutex &operator=(shared_mutex const &) = delete;
 
+  // The lock/unlock methods are the trusted implementation of this capability:
+  // their bodies drive the raw pthread_rwlock_t, which some libc headers (e.g.
+  // FreeBSD's <pthread.h>) annotate as a capability in its own right. Exempt the
+  // bodies from analysis so only the capability contract on each signature is
+  // checked; the actual data-race checking happens at the call sites.
   void
-  lock()
+  lock() TS_ACQUIRE() TS_NO_THREAD_SAFETY_ANALYSIS
   {
     int error = pthread_rwlock_wrlock(&_lock);
     if (error != 0) {
@@ -69,7 +75,7 @@ class shared_mutex
   }
 
   bool
-  try_lock()
+  try_lock() TS_TRY_ACQUIRE(true) TS_NO_THREAD_SAFETY_ANALYSIS
   {
     int error = pthread_rwlock_trywrlock(&_lock);
     if (EBUSY == error) {
@@ -84,7 +90,7 @@ class shared_mutex
   }
 
   void
-  unlock()
+  unlock() TS_RELEASE() TS_NO_THREAD_SAFETY_ANALYSIS
   {
     X(debug_assert(_exclusive);)
     X(_exclusive = false;)
@@ -93,7 +99,7 @@ class shared_mutex
   }
 
   void
-  lock_shared()
+  lock_shared() TS_ACQUIRE_SHARED() TS_NO_THREAD_SAFETY_ANALYSIS
   {
     int error = pthread_rwlock_rdlock(&_lock);
     if (error != 0) {
@@ -105,7 +111,7 @@ class shared_mutex
   }
 
   bool
-  try_lock_shared()
+  try_lock_shared() TS_TRY_ACQUIRE_SHARED(true) TS_NO_THREAD_SAFETY_ANALYSIS
   {
     int error = pthread_rwlock_tryrdlock(&_lock);
     if (EBUSY == error) {
@@ -121,7 +127,7 @@ class shared_mutex
   }
 
   void
-  unlock_shared()
+  unlock_shared() TS_RELEASE_SHARED() TS_NO_THREAD_SAFETY_ANALYSIS
   {
     X(debug_assert(_shared > 0);)
     X(--_shared;)
@@ -148,7 +154,7 @@ class shared_mutex
 
 private:
   void
-  _unlock()
+  _unlock() TS_NO_THREAD_SAFETY_ANALYSIS
   {
     int error = pthread_rwlock_unlock(&_lock);
     if (error != 0) {
@@ -178,6 +184,38 @@ class shared_mutex
   X(std::atomic<int> _shared{0};)
 };
 
+// RAII guards for ts::shared_mutex that carry Clang thread-safety capability
+// state. Prefer these over std::unique_lock / std::shared_lock in code annotated
+// for -Wthread-safety: the analysis does not reliably track the std wrappers
+// (see tsutil/ts_thread_safety.h).
+//
+class TS_SCOPED_CAPABILITY write_guard
+{
+public:
+  explicit write_guard(shared_mutex &m) TS_ACQUIRE(m) : _m(m) { _m.lock(); }
+  ~write_guard() TS_RELEASE() { _m.unlock(); }
+
+  write_guard(write_guard const &)            = delete;
+  write_guard &operator=(write_guard const &) = delete;
+
+private:
+  shared_mutex &_m;
+};
+
+class TS_SCOPED_CAPABILITY read_guard
+{
+public:
+  explicit read_guard(shared_mutex &m) TS_ACQUIRE_SHARED(m) : _m(m) { _m.lock_shared(); }
+  // A scoped-capability destructor uses the plain release form even for a shared acquire.
+  ~read_guard() TS_RELEASE() { _m.unlock_shared(); }
+
+  read_guard(read_guard const &)            = delete;
+  read_guard &operator=(read_guard const &) = delete;
+
+private:
+  shared_mutex &_m;
+};
+
 } // end namespace ts
 
 #undef X