feat tsan: annotate mutex

Tests: протестировано CI Pull Request resolved: #105 Co-authored-by: Antony Polukhin <antoshkka@gmail.com>
userver-framework · Dec 26, 2023 · 0f138e9 · 0f138e9
1 parent 834672a
commit 0f138e9
Show file tree

Hide file tree

Showing 9 changed files with 124 additions and 12 deletions.
diff --git a/.mapping.json b/.mapping.json
@@ -3082,6 +3082,7 @@
   "universal/include/userver/compiler/impl/constexpr.hpp":"taxi/uservices/userver/universal/include/userver/compiler/impl/constexpr.hpp",
   "universal/include/userver/compiler/impl/lifetime.hpp":"taxi/uservices/userver/universal/include/userver/compiler/impl/lifetime.hpp",
   "universal/include/userver/compiler/impl/tls.hpp":"taxi/uservices/userver/universal/include/userver/compiler/impl/tls.hpp",
+  "universal/include/userver/compiler/impl/tsan.hpp":"taxi/uservices/userver/universal/include/userver/compiler/impl/tsan.hpp",
   "universal/include/userver/compiler/select.hpp":"taxi/uservices/userver/universal/include/userver/compiler/select.hpp",
   "universal/include/userver/compiler/thread_local.hpp":"taxi/uservices/userver/universal/include/userver/compiler/thread_local.hpp",
   "universal/include/userver/crypto/algorithm.hpp":"taxi/uservices/userver/universal/include/userver/crypto/algorithm.hpp",

diff --git a/cmake/sanitize.blacklist.txt b/cmake/sanitize.blacklist.txt
@@ -10,6 +10,15 @@ src:*bits/stl_iterator.h
 [undefined]
 src:*bits/stl_vector.h
 
+[thread]
+mainfile:*core/src/engine/task/task_context.cpp
+
+[thread]
+mainfile:*core/src/engine/task/task_context.hpp
+
+[thread]
+src:*third_party/moodycamel/include/moodycamel/concurrentqueue.h
+
 # libstdc++ 8.0.1 casts an under-aligned pointer to type type_info:
 # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85930
 [undefined]

diff --git a/core/src/engine/impl/mutex_impl.hpp b/core/src/engine/impl/mutex_impl.hpp
@@ -9,6 +9,7 @@
 #include <engine/impl/wait_list.hpp>
 #include <engine/impl/wait_list_light.hpp>
 #include <engine/task/task_context.hpp>
+#include <userver/compiler/impl/tsan.hpp>
 
 USERVER_NAMESPACE_BEGIN
 
@@ -91,11 +92,18 @@ class MutexImpl<WaitListLight>::MutexWaitStrategy final : public WaitStrategy {
 };
 
 template <class Waiters>
-MutexImpl<Waiters>::MutexImpl() : owner_(nullptr) {}
+MutexImpl<Waiters>::MutexImpl() : owner_(nullptr) {
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_mutex_create(this, __tsan_mutex_not_static);
+#endif
+}
 
 template <class Waiters>
 MutexImpl<Waiters>::~MutexImpl() {
   UASSERT(!owner_);
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_mutex_destroy(this, __tsan_mutex_not_static);
+#endif
 }
 
 template <class Waiters>
@@ -133,6 +141,9 @@ void MutexImpl<Waiters>::lock() {
 
 template <class Waiters>
 void MutexImpl<Waiters>::unlock() {
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_mutex_pre_unlock(this, 0);
+#endif
   auto* old_owner = owner_.exchange(nullptr, std::memory_order_acq_rel);
   UASSERT(old_owner && old_owner->IsCurrent());
 
@@ -145,18 +156,46 @@ void MutexImpl<Waiters>::unlock() {
     static_assert(std::is_same_v<Waiters, WaitListLight>);
     lock_waiters_.WakeupOne();
   }
+
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_mutex_post_unlock(this, 0);
+#endif
 }
 
 template <class Waiters>
 bool MutexImpl<Waiters>::try_lock() {
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_mutex_pre_lock(this, __tsan_mutex_try_lock);
+#endif
+
   auto& current = current_task::GetCurrentTaskContext();
-  return LockFastPath(current);
+  const auto result = LockFastPath(current);
+
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_mutex_post_lock(
+      this, __tsan_mutex_try_lock | (result ? 0 : __tsan_mutex_try_lock_failed),
+      0);
+#endif
+
+  return result;
 }
 
 template <class Waiters>
 bool MutexImpl<Waiters>::try_lock_until(Deadline deadline) {
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_mutex_pre_lock(this, __tsan_mutex_try_lock);
+#endif
+
   auto& current = current_task::GetCurrentTaskContext();
-  return LockFastPath(current) || LockSlowPath(current, deadline);
+  const auto result = LockFastPath(current) || LockSlowPath(current, deadline);
+
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_mutex_post_lock(
+      this, __tsan_mutex_try_lock | (result ? 0 : __tsan_mutex_try_lock_failed),
+      0);
+#endif
+
+  return result;
 }
 
 }  // namespace engine::impl

diff --git a/core/src/engine/task/exception_hacks.cpp b/core/src/engine/task/exception_hacks.cpp
@@ -1,9 +1,8 @@
 #include <engine/task/exception_hacks.hpp>
 
+#include <userver/compiler/impl/tsan.hpp>
+
 #if defined(__has_feature)
-#if __has_feature(thread_sanitizer)
-#define HAS_TSAN 1
-#endif
 #if __has_feature(memory_sanitizer)
 #define HAS_MSAN 1
 #endif
@@ -16,7 +15,7 @@
 // segfaults with stackoverflow trying to report the backtrace from within
 // dl_iterate_phdr.
 #if !defined(USERVER_DISABLE_PHDR_CACHE) && defined(__linux__) && \
-    !defined(HAS_TSAN) && !defined(HAS_MSAN)
+    !USERVER_IMPL_HAS_TSAN && !defined(HAS_MSAN)
 #define USE_PHDR_CACHE 1  // NOLINT
 #endif
 

diff --git a/core/src/engine/task/task_context.cpp b/core/src/engine/task/task_context.cpp
@@ -9,6 +9,7 @@
 #include <engine/coro/pool.hpp>
 #include <logging/log_extra_stacktrace.hpp>
 #include <userver/compiler/impl/tls.hpp>
+#include <userver/compiler/impl/tsan.hpp>
 #include <userver/engine/exception.hpp>
 #include <userver/engine/impl/task_context_factory.hpp>
 #include <userver/engine/task/cancel.hpp>
@@ -120,6 +121,8 @@ TaskContext::TaskContext(TaskProcessor& task_processor,
               << ReadableTaskId(current_task::GetCurrentTaskContextUnchecked())
               << " created task with task_id=" << ReadableTaskId(this)
               << logging::LogExtra::Stacktrace();
+
+  TsanReleaseBarrier();
 }
 
 TaskContext::~TaskContext() noexcept {
@@ -229,11 +232,15 @@ void TaskContext::DoStep() {
     CurrentTaskScope current_task_scope(*this, eh_globals_);
     try {
       SetState(Task::State::kRunning);
-      (*coro_)(this);
+      auto& coro_ref = *coro_;
+      TsanAcquireBarrier();
+      coro_ref(this);
     } catch (...) {
       uncaught = std::current_exception();
     }
+    TsanReleaseBarrier();
   }
+
   if (uncaught) std::rethrow_exception(uncaught);
 
   switch (yield_reason_) {
@@ -329,7 +336,12 @@ TaskContext::WakeupSource TaskContext::Sleep(WaitStrategy& wait_strategy) {
   UASSERT(task_pipe_);
   TraceStateTransition(Task::State::kSuspended);
   ProfilerStopExecution();
-  [[maybe_unused]] TaskContext* context = (*task_pipe_)().get();
+
+  auto& task_pipe_ref = *task_pipe_;
+  TsanAcquireBarrier();
+  [[maybe_unused]] TaskContext* context = task_pipe_ref().get();
+  TsanReleaseBarrier();
+
   ProfilerStartExecution();
   TraceStateTransition(Task::State::kRunning);
   UASSERT(context == this);
@@ -486,6 +498,7 @@ class TaskContext::LocalStorageGuard {
 void TaskContext::CoroFunc(TaskPipe& task_pipe) {
   for (TaskContext* context : task_pipe) {
     UASSERT(context);
+    context->TsanReleaseBarrier();
     context->yield_reason_ = YieldReason::kNone;
     context->task_pipe_ = &task_pipe;
 
@@ -527,6 +540,7 @@ void TaskContext::CoroFunc(TaskPipe& task_pipe) {
     context->ProfilerStopExecution();
 
     context->task_pipe_ = nullptr;
+    context->TsanAcquireBarrier();
   }
 }
 
@@ -771,6 +785,20 @@ bool HasWaitSucceeded(TaskContext::WakeupSource wakeup_source) noexcept {
   return false;
 }
 
+void TaskContext::TsanAcquireBarrier() noexcept {
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_acquire(this);
+  __tsan_acquire(&coro_);
+#endif
+}
+
+void TaskContext::TsanReleaseBarrier() noexcept {
+#if USERVER_IMPL_HAS_TSAN
+  __tsan_release(&coro_);
+  __tsan_release(this);
+#endif
+}
+
 }  // namespace impl
 }  // namespace engine
 

diff --git a/core/src/engine/task/task_context.hpp b/core/src/engine/task/task_context.hpp
@@ -208,6 +208,9 @@ class TaskContext final : public ContextAccessor {
 
   void TraceStateTransition(Task::State state);
 
+  void TsanAcquireBarrier() noexcept;
+  void TsanReleaseBarrier() noexcept;
+
   const uint64_t magic_{kMagic};
   TaskProcessor& task_processor_;
   TaskCounter::Token task_counter_token_;
@@ -232,7 +235,7 @@ class TaskContext final : public ContextAccessor {
   std::chrono::steady_clock::time_point execute_started_;
   std::chrono::steady_clock::time_point last_state_change_timepoint_;
 
-  size_t trace_csw_left_;
+  std::size_t trace_csw_left_;
 
   AtomicSleepState sleep_state_{
       SleepState{SleepFlags::kSleeping, SleepState::Epoch{0}}};

diff --git a/postgresql/src/storages/postgres/tests/cluster_pgtest.cpp b/postgresql/src/storages/postgres/tests/cluster_pgtest.cpp
@@ -403,7 +403,7 @@ UTEST_F(PostgreCluster, ListenNotify) {
   constexpr auto kListenChannel = std::string_view{"foo"};
   constexpr auto kNotifyPayload = std::string_view{"bar"};
   static const auto kNotifyDeadline =
-      engine::Deadline::FromDuration(std::chrono::milliseconds{100});
+      engine::Deadline::FromDuration(utest::kMaxTestWaitTime);
 
   testsuite::TestsuiteTasks testsuite_tasks{true};
   auto cluster = CreateCluster(GetDsnListFromEnv(), GetTaskProcessor(), 2,

diff --git a/universal/include/userver/compiler/impl/tsan.hpp b/universal/include/userver/compiler/impl/tsan.hpp
@@ -0,0 +1,33 @@
+#pragma once
+
+/// @file userver/compiler/impl/tsan.hpp
+/// @brief Defines USERVER_IMPL_HAS_TSAN to 1 and includes
+/// <sanitizer/tsan_interface.h> if Thread Sanitizer is enabled; otherwise
+/// defines USERVER_IMPL_HAS_TSAN to 0.
+
+#if defined(__has_feature)
+
+#if __has_feature(thread_sanitizer)
+// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
+#define USERVER_IMPL_HAS_TSAN 1
+#else
+// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
+#define USERVER_IMPL_HAS_TSAN 0
+#endif
+
+#else
+// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
+#define USERVER_IMPL_HAS_TSAN 0
+#endif
+
+#if USERVER_IMPL_HAS_TSAN
+#include <sanitizer/tsan_interface.h>
+#endif
+
+#if USERVER_IMPL_HAS_TSAN && !defined(BOOST_USE_TSAN)
+#error Broken CMake: thread sanitizer is used however Boost is unaware of it
+#endif
+
+#if !USERVER_IMPL_HAS_TSAN && defined(BOOST_USE_TSAN)
+#error Broken CMake: thread sanitizer is not used however Boost thinks it is
+#endif
diff --git a/universal/include/userver/utils/encoding/tskv.hpp b/universal/include/userver/utils/encoding/tskv.hpp
@@ -28,7 +28,7 @@
 #ifdef __clang__
 // NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
 #define USERVER_IMPL_DISABLE_ASAN \
-  __attribute__((no_sanitize_address, no_sanitize_memory))
+  __attribute__((no_sanitize_address, no_sanitize_memory, no_sanitize_thread))
 #else
 // NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
 #define USERVER_IMPL_DISABLE_ASAN __attribute__((no_sanitize_address))