From 41f46ec08501e33c9147ffa8dad2d2888bcf3d4e Mon Sep 17 00:00:00 2001 From: Alastair Houghton Date: Fri, 16 Jun 2023 13:30:36 +0100 Subject: [PATCH 1/7] [Threading][TSan] Fix TSan errors from lazy init on Linux. Move the TSan functionality from Concurrency into Threading. Use it in the Linux `ulock` implementation so that TSan knows about `ulock` and will tolerate the newer `swift_once` implementation that uses it. rdar://110665213 --- include/swift/Threading/Impl/Linux/ulock.h | 29 ++++-- include/swift/Threading/TSan.h | 95 +++++++++++++++++++ lib/Threading/CMakeLists.txt | 3 +- lib/Threading/Linux.cpp | 3 +- lib/Threading/TSan.cpp | 49 ++++++++++ stdlib/public/Concurrency/CMakeLists.txt | 1 - stdlib/public/Concurrency/TaskPrivate.h | 22 +++-- stdlib/public/Concurrency/ThreadSanitizer.cpp | 50 ---------- stdlib/public/Threading/CMakeLists.txt | 1 + test/Sanitizers/tsan/once.swift | 50 ++++++++++ 10 files changed, 231 insertions(+), 72 deletions(-) create mode 100644 include/swift/Threading/TSan.h create mode 100644 lib/Threading/TSan.cpp delete mode 100644 stdlib/public/Concurrency/ThreadSanitizer.cpp create mode 100644 test/Sanitizers/tsan/once.swift diff --git a/include/swift/Threading/Impl/Linux/ulock.h b/include/swift/Threading/Impl/Linux/ulock.h index a9020965ec5d4..1616b1e7a678a 100644 --- a/include/swift/Threading/Impl/Linux/ulock.h +++ b/include/swift/Threading/Impl/Linux/ulock.h @@ -34,6 +34,8 @@ #include #include +#include "swift/Threading/TSan.h" + namespace swift { namespace threading_impl { namespace linux { @@ -59,32 +61,39 @@ inline void ulock_lock(ulock_t *lock) { const ulock_t tid = ulock_get_tid(); do { ulock_t zero = 0; - if (ulock_fastpath(__atomic_compare_exchange_n( - lock, &zero, tid, true, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED))) - return; - + if (ulock_fastpath(__atomic_compare_exchange_n(lock, &zero, tid, + true, __ATOMIC_ACQUIRE, + __ATOMIC_RELAXED))) + break; } while (ulock_futex(lock, FUTEX_LOCK_PI) != 0); + + tsan::acquire(lock); } inline bool ulock_trylock(ulock_t *lock) { ulock_t zero = 0; if (ulock_fastpath(__atomic_compare_exchange_n(lock, &zero, ulock_get_tid(), true, __ATOMIC_ACQUIRE, - __ATOMIC_RELAXED))) + __ATOMIC_RELAXED)) + || ulock_futex(lock, FUTEX_TRYLOCK_PI) == 0) { + tsan::acquire(lock); return true; + } - return ulock_futex(lock, FUTEX_TRYLOCK_PI) == 0; + return false; } inline void ulock_unlock(ulock_t *lock) { const ulock_t tid = ulock_get_tid(); do { ulock_t expected = tid; - if (ulock_fastpath(__atomic_compare_exchange_n( - lock, &expected, 0, true, __ATOMIC_RELEASE, __ATOMIC_RELAXED))) - return; - + if (ulock_fastpath(__atomic_compare_exchange_n(lock, &expected, 0, + true, __ATOMIC_RELEASE, + __ATOMIC_RELAXED))) + break; } while (ulock_futex(lock, FUTEX_UNLOCK_PI) != 0); + + tsan::release(lock); } } // namespace linux diff --git a/include/swift/Threading/TSan.h b/include/swift/Threading/TSan.h new file mode 100644 index 0000000000000..7bf8db8120da0 --- /dev/null +++ b/include/swift/Threading/TSan.h @@ -0,0 +1,95 @@ +//===--- TSan.h - TSan support functions ---------------------- -*- C++ -*-===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2023 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// +// +// Helper functions for code that needs to integrate with the thread +// sanitizer. In particular, TSan can't see inside the runtime libraries, +// so we occasionally need to give it a hint that we're doing synchronization +// in order to avoid false positives. +// +//===----------------------------------------------------------------------===// + +#ifndef SWIFT_THREADING_TSAN_H +#define SWIFT_THREADING_TSAN_H + +namespace swift { + +#if defined(_WIN32) || defined(__wasi__) || !__has_include() +namespace tsan { + +inline bool enabled() { return false; } +template T *acquire(T *ptr) { return ptr; } +template T *release(T *ptr) { return ptr; } +template T *consume(T *ptr) { return ptr; } + +} // namespace tsan +#else + +namespace threading_impl { + +extern bool tsan_enabled; +extern void (*tsan_acquire)(const void *ptr); +extern void (*tsan_release)(const void *ptr); + +} // namespace threading_impl + +namespace tsan { + +/// Returns true if TSan is enabled +inline bool enabled() { + return threading_impl::tsan_enabled; +} + +/// Indicate to TSan that an acquiring load has occurred on the current +/// thread. If some other thread does a releasing store with the same +/// pointer, we are indicating to TSan that all writes that happened +/// before that store will be visible to the current thread after the +/// `acquire()`. +template +T *acquire(T *ptr) { + if (threading_impl::tsan_acquire) { + threading_impl::tsan_acquire(ptr); + } + return ptr; +} + +/// Indicate to TSan that a releasing store has occurred on the current +/// thread. If some other thread does an acquiring load with the same +/// pointer, we are indicating to TSan that that thread will be able to +/// see all writes that happened before the `release()`. +template +T *release(T *ptr) { + if (threading_impl::tsan_release) { + threading_impl::tsan_release(ptr); + } + return ptr; +} + +/// Indicate to TSan that a consuming load has occurred on the current +/// thread. If some other thread does a releasing store with the same +/// pointer, we are indicating to TSan that all writes that happened +/// before that store will be visible *to those operations that carry a +/// dependency on the loaded value*. +/// +/// TSan doesn't currently know about consume, so we lie and say it's an +/// acquire instead. +template +T *consume(T *ptr) { + return acquire(ptr); +} + +} // namespace tsan + +#endif + +} // namespace swift + +#endif diff --git a/lib/Threading/CMakeLists.txt b/lib/Threading/CMakeLists.txt index 029a52b68e1e3..9b0828945af56 100644 --- a/lib/Threading/CMakeLists.txt +++ b/lib/Threading/CMakeLists.txt @@ -10,4 +10,5 @@ add_swift_host_library(swiftThreading STATIC Linux.cpp Pthreads.cpp Win32.cpp - Errors.cpp) + Errors.cpp + TSan.cpp) diff --git a/lib/Threading/Linux.cpp b/lib/Threading/Linux.cpp index 811f470ba0832..008231be7157a 100644 --- a/lib/Threading/Linux.cpp +++ b/lib/Threading/Linux.cpp @@ -18,6 +18,7 @@ #include "swift/Threading/Impl.h" #include "swift/Threading/Errors.h" +#include "swift/Threading/TSan.h" namespace { @@ -61,7 +62,7 @@ void swift::threading_impl::once_slow(once_t &predicate, void (*fn)(void *), #endif if (predicate.flag.load(std::memory_order_acquire) == 0) { fn(context); - predicate.flag.store(-1, std::memory_order_release); + predicate.flag.store(tsan::enabled() ? 1 : -1, std::memory_order_release); } #if defined(__LP64__) || defined(_LP64) linux::ulock_unlock(&predicate.lock); diff --git a/lib/Threading/TSan.cpp b/lib/Threading/TSan.cpp new file mode 100644 index 0000000000000..ad713b674d6e3 --- /dev/null +++ b/lib/Threading/TSan.cpp @@ -0,0 +1,49 @@ +//===--- TSan.h - TSan support functions ----------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2023 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// +// +// Helper functions for code that needs to integrate with the thread +// sanitizer. In particular, TSan can't see inside the runtime libraries, +// so we occasionally need to give it a hint that we're doing synchronization +// in order to avoid false positives. +// +//===----------------------------------------------------------------------===// + +#include "swift/Threading/TSan.h" +#include "swift/shims/Visibility.h" + +namespace swift { +namespace threading_impl { + +bool tsan_enabled = false; +void (*tsan_acquire)(const void *) = nullptr; +void (*tsan_release)(const void *) = nullptr; + +#if __has_include() +#include + +// The TSan library code will call this function when it starts up +extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS +void __tsan_on_initialize() { + tsan_enabled = true; + tsan_acquire = (void (*)(const void *))dlsym(RTLD_DEFAULT, "__tsan_acquire"); + tsan_release = (void (*)(const void *))dlsym(RTLD_DEFAULT, "__tsan_release"); + + // Always call through to the next image + void (*next_init)(void); + next_init = (void (*)(void))dlsym(RTLD_NEXT, "__tsan_on_initialize"); + if (next_init) + next_init(); +} +#endif + +} // namespace threading_impl +} // namespace swift diff --git a/stdlib/public/Concurrency/CMakeLists.txt b/stdlib/public/Concurrency/CMakeLists.txt index 7060ece89d636..0ac762e1cff1d 100644 --- a/stdlib/public/Concurrency/CMakeLists.txt +++ b/stdlib/public/Concurrency/CMakeLists.txt @@ -120,7 +120,6 @@ add_swift_target_library(swift_Concurrency ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} I TaskLocal.cpp TaskLocal.swift TaskSleep.swift - ThreadSanitizer.cpp ThreadingError.cpp TracingSignpost.cpp AsyncStreamBuffer.swift diff --git a/stdlib/public/Concurrency/TaskPrivate.h b/stdlib/public/Concurrency/TaskPrivate.h index 450fa2a7111d6..8ed5e45110fbc 100644 --- a/stdlib/public/Concurrency/TaskPrivate.h +++ b/stdlib/public/Concurrency/TaskPrivate.h @@ -29,6 +29,7 @@ #include "swift/Runtime/Exclusivity.h" #include "swift/Runtime/HeapObject.h" #include "swift/Threading/Thread.h" +#include "swift/Threading/TSan.h" #include #include @@ -99,15 +100,18 @@ void _swift_taskGroup_cancelAllChildren(TaskGroup *group); /// should generally use a higher-level function. void _swift_taskGroup_detachChild(TaskGroup *group, AsyncTask *child); -/// release() establishes a happens-before relation with a preceding acquire() -/// on the same address. -void _swift_tsan_acquire(void *addr); -void _swift_tsan_release(void *addr); -/// Technically, this consume relies on implicit HW address dependency ordering -/// and is paired with a corresponding release. Since TSAN doesn't know how to -/// reason about this, we tell TSAN it's an acquire instead. See also -/// SWIFT_MEMORY_ORDER_CONSUME definition. -#define _swift_tsan_consume _swift_tsan_acquire +/// Tell TSAN about an acquiring load +inline void _swift_tsan_acquire(void *addr) { + swift::tsan::acquire(addr); +} +/// Tell TSAN about a releasing store +inline void _swift_tsan_release(void *addr) { + swift::tsan::release(addr); +} +/// Tell TSAN about a consuming load +inline void _swift_tsan_consume(void *addr) { + swift::tsan::consume(addr); +} /// Special values used with DispatchQueueIndex to indicate the global and main /// executors. diff --git a/stdlib/public/Concurrency/ThreadSanitizer.cpp b/stdlib/public/Concurrency/ThreadSanitizer.cpp deleted file mode 100644 index c9d1d5027732b..0000000000000 --- a/stdlib/public/Concurrency/ThreadSanitizer.cpp +++ /dev/null @@ -1,50 +0,0 @@ -//===--- ThreadSanitizer.cpp - Thread Sanitizer support -------------------===// -// -// This source file is part of the Swift.org open source project -// -// Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors -// Licensed under Apache License v2.0 with Runtime Library Exception -// -// See https://swift.org/LICENSE.txt for license information -// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors -// -//===----------------------------------------------------------------------===// -// -// Thread Sanitizer support for the Swift Task runtime. -// -//===----------------------------------------------------------------------===// - -#include "TaskPrivate.h" - -// Thread Sanitizer is not supported on Windows or WASI. -#if defined(_WIN32) || defined(__wasi__) || !__has_include() -void swift::_swift_tsan_acquire(void *addr) {} -void swift::_swift_tsan_release(void *addr) {} -#else -#include - -namespace { -using TSanFunc = void(void *); -TSanFunc *tsan_acquire, *tsan_release; -} // anonymous namespace - -void swift::_swift_tsan_acquire(void *addr) { - if (tsan_acquire) { - tsan_acquire(addr); - SWIFT_TASK_DEBUG_LOG("tsan_acquire on %p", addr); - } -} - -void swift::_swift_tsan_release(void *addr) { - if (tsan_release) { - tsan_release(addr); - SWIFT_TASK_DEBUG_LOG("tsan_release on %p", addr); - } -} - -SWIFT_EXPORT_FROM(swift_Concurrency) SWIFT_CC(c) -void __tsan_on_initialize() { - tsan_acquire = (TSanFunc *)dlsym(RTLD_DEFAULT, "__tsan_acquire"); - tsan_release = (TSanFunc *)dlsym(RTLD_DEFAULT, "__tsan_release"); -} -#endif diff --git a/stdlib/public/Threading/CMakeLists.txt b/stdlib/public/Threading/CMakeLists.txt index 007289ed3fac7..ae9d8b4e8f4d9 100644 --- a/stdlib/public/Threading/CMakeLists.txt +++ b/stdlib/public/Threading/CMakeLists.txt @@ -9,4 +9,5 @@ add_swift_target_library(swiftThreading OBJECT_LIBRARY "${SWIFT_SOURCE_DIR}/lib/Threading/Linux.cpp" "${SWIFT_SOURCE_DIR}/lib/Threading/Pthreads.cpp" "${SWIFT_SOURCE_DIR}/lib/Threading/Win32.cpp" + "${SWIFT_SOURCE_DIR}/lib/Threading/TSan.cpp" INSTALL_IN_COMPONENT never_install) diff --git a/test/Sanitizers/tsan/once.swift b/test/Sanitizers/tsan/once.swift new file mode 100644 index 0000000000000..bb54500daadb2 --- /dev/null +++ b/test/Sanitizers/tsan/once.swift @@ -0,0 +1,50 @@ +// RUN: %target-swiftc_driver %s -Xfrontend -parse-as-library -g -sanitize=thread %import-libdispatch -o %t_tsan-binary +// RUN: %target-codesign %t_tsan-binary +// RUN: env %env-TSAN_OPTIONS=abort_on_error=0 %target-run %t_tsan-binary 2>&1 | %FileCheck %s --implicit-check-not='ThreadSanitizer' +// REQUIRES: executable_test +// REQUIRES: tsan_runtime + +// rdar://101876380 +// UNSUPPORTED: OS=ios + +// FIXME: This should be covered by "tsan_runtime"; older versions of Apple OSs +// don't support TSan. +// UNSUPPORTED: remote_run + +// Test that we do not report a race on initialization; Swift doesn't initialize +// globals at start-up, but rather uses swift_once(). This is thread safe, but +// on some platforms TSan wasn't seeing the synchronization, so would report +// a false positive. + +import Dispatch + +var count = 0 +let foo = { + count += 1 + return count +}() + +@main +struct Main { + static func main() { + let q = DispatchQueue(label: "q", attributes: .concurrent) + let finished = DispatchSemaphore(value: 0) + let count = 100 + + for _ in 0.. Date: Mon, 19 Jun 2023 11:14:34 +0100 Subject: [PATCH 2/7] [Threading][TSan] Update after review comments. * Use the longer name ThreadSanitizer rather than TSan for the new files. * Don't implement `tsan::consume` at all for now. * Do the `tsan::release` for `ulock_unlock()` at the head of the function, not at the tail. * Add a comment to test/Sanitizers/tsan/once.swift to explain the test a little more clearly. rdar://110665213 --- include/swift/Threading/Impl/Linux/ulock.h | 6 +++--- .../Threading/{TSan.h => ThreadSanitizer.h} | 20 +++---------------- lib/Threading/CMakeLists.txt | 2 +- lib/Threading/Linux.cpp | 2 +- .../{TSan.cpp => ThreadSanitizer.cpp} | 4 ++-- stdlib/public/Concurrency/TaskPrivate.h | 15 +++++++++----- stdlib/public/Threading/CMakeLists.txt | 2 +- test/Sanitizers/tsan/once.swift | 7 ++++++- 8 files changed, 27 insertions(+), 31 deletions(-) rename include/swift/Threading/{TSan.h => ThreadSanitizer.h} (77%) rename lib/Threading/{TSan.cpp => ThreadSanitizer.cpp} (93%) diff --git a/include/swift/Threading/Impl/Linux/ulock.h b/include/swift/Threading/Impl/Linux/ulock.h index 1616b1e7a678a..af04e74aa08ef 100644 --- a/include/swift/Threading/Impl/Linux/ulock.h +++ b/include/swift/Threading/Impl/Linux/ulock.h @@ -34,7 +34,7 @@ #include #include -#include "swift/Threading/TSan.h" +#include "swift/Threading/ThreadSanitizer.h" namespace swift { namespace threading_impl { @@ -84,6 +84,8 @@ inline bool ulock_trylock(ulock_t *lock) { } inline void ulock_unlock(ulock_t *lock) { + tsan::release(lock); + const ulock_t tid = ulock_get_tid(); do { ulock_t expected = tid; @@ -92,8 +94,6 @@ inline void ulock_unlock(ulock_t *lock) { __ATOMIC_RELAXED))) break; } while (ulock_futex(lock, FUTEX_UNLOCK_PI) != 0); - - tsan::release(lock); } } // namespace linux diff --git a/include/swift/Threading/TSan.h b/include/swift/Threading/ThreadSanitizer.h similarity index 77% rename from include/swift/Threading/TSan.h rename to include/swift/Threading/ThreadSanitizer.h index 7bf8db8120da0..19923c3689cee 100644 --- a/include/swift/Threading/TSan.h +++ b/include/swift/Threading/ThreadSanitizer.h @@ -1,4 +1,4 @@ -//===--- TSan.h - TSan support functions ---------------------- -*- C++ -*-===// +//===--- ThreadSanitizer.h - Thread Sanitizer support --------- -*- C++ -*-===// // // This source file is part of the Swift.org open source project // @@ -17,8 +17,8 @@ // //===----------------------------------------------------------------------===// -#ifndef SWIFT_THREADING_TSAN_H -#define SWIFT_THREADING_TSAN_H +#ifndef SWIFT_THREADING_THREAD_SANITIZER_H +#define SWIFT_THREADING_THREAD_SANITIZER_H namespace swift { @@ -28,7 +28,6 @@ namespace tsan { inline bool enabled() { return false; } template T *acquire(T *ptr) { return ptr; } template T *release(T *ptr) { return ptr; } -template T *consume(T *ptr) { return ptr; } } // namespace tsan #else @@ -73,19 +72,6 @@ T *release(T *ptr) { return ptr; } -/// Indicate to TSan that a consuming load has occurred on the current -/// thread. If some other thread does a releasing store with the same -/// pointer, we are indicating to TSan that all writes that happened -/// before that store will be visible *to those operations that carry a -/// dependency on the loaded value*. -/// -/// TSan doesn't currently know about consume, so we lie and say it's an -/// acquire instead. -template -T *consume(T *ptr) { - return acquire(ptr); -} - } // namespace tsan #endif diff --git a/lib/Threading/CMakeLists.txt b/lib/Threading/CMakeLists.txt index 9b0828945af56..4e0e6aed05e3b 100644 --- a/lib/Threading/CMakeLists.txt +++ b/lib/Threading/CMakeLists.txt @@ -11,4 +11,4 @@ add_swift_host_library(swiftThreading STATIC Pthreads.cpp Win32.cpp Errors.cpp - TSan.cpp) + ThreadSanitizer.cpp) diff --git a/lib/Threading/Linux.cpp b/lib/Threading/Linux.cpp index 008231be7157a..93963a7c30ad2 100644 --- a/lib/Threading/Linux.cpp +++ b/lib/Threading/Linux.cpp @@ -18,7 +18,7 @@ #include "swift/Threading/Impl.h" #include "swift/Threading/Errors.h" -#include "swift/Threading/TSan.h" +#include "swift/Threading/ThreadSanitizer.h" namespace { diff --git a/lib/Threading/TSan.cpp b/lib/Threading/ThreadSanitizer.cpp similarity index 93% rename from lib/Threading/TSan.cpp rename to lib/Threading/ThreadSanitizer.cpp index ad713b674d6e3..bee0b8529c52d 100644 --- a/lib/Threading/TSan.cpp +++ b/lib/Threading/ThreadSanitizer.cpp @@ -1,4 +1,4 @@ -//===--- TSan.h - TSan support functions ----------------------------------===// +//===--- ThreadSanitizer.cpp - Thread Sanitizer support -------------------===// // // This source file is part of the Swift.org open source project // @@ -17,7 +17,7 @@ // //===----------------------------------------------------------------------===// -#include "swift/Threading/TSan.h" +#include "swift/Threading/ThreadSanitizer.h" #include "swift/shims/Visibility.h" namespace swift { diff --git a/stdlib/public/Concurrency/TaskPrivate.h b/stdlib/public/Concurrency/TaskPrivate.h index 8ed5e45110fbc..414bf3dc75c11 100644 --- a/stdlib/public/Concurrency/TaskPrivate.h +++ b/stdlib/public/Concurrency/TaskPrivate.h @@ -29,7 +29,7 @@ #include "swift/Runtime/Exclusivity.h" #include "swift/Runtime/HeapObject.h" #include "swift/Threading/Thread.h" -#include "swift/Threading/TSan.h" +#include "swift/Threading/ThreadSanitizer.h" #include #include @@ -100,17 +100,22 @@ void _swift_taskGroup_cancelAllChildren(TaskGroup *group); /// should generally use a higher-level function. void _swift_taskGroup_detachChild(TaskGroup *group, AsyncTask *child); -/// Tell TSAN about an acquiring load +/// Tell TSan about an acquiring load inline void _swift_tsan_acquire(void *addr) { swift::tsan::acquire(addr); } -/// Tell TSAN about a releasing store +/// Tell TSan about a releasing store inline void _swift_tsan_release(void *addr) { swift::tsan::release(addr); } -/// Tell TSAN about a consuming load +/// Tell TSan about a consuming load inline void _swift_tsan_consume(void *addr) { - swift::tsan::consume(addr); + // TSan doesn't support consume, so pretend it's an acquire. + // + // Note that that means that TSan won't generate errors for non-dependent + // reads, so this isn't entirely safe if you're relying solely on TSan to + // spot bugs. + swift::tsan::acquire(addr); } /// Special values used with DispatchQueueIndex to indicate the global and main diff --git a/stdlib/public/Threading/CMakeLists.txt b/stdlib/public/Threading/CMakeLists.txt index ae9d8b4e8f4d9..961c643b756ee 100644 --- a/stdlib/public/Threading/CMakeLists.txt +++ b/stdlib/public/Threading/CMakeLists.txt @@ -9,5 +9,5 @@ add_swift_target_library(swiftThreading OBJECT_LIBRARY "${SWIFT_SOURCE_DIR}/lib/Threading/Linux.cpp" "${SWIFT_SOURCE_DIR}/lib/Threading/Pthreads.cpp" "${SWIFT_SOURCE_DIR}/lib/Threading/Win32.cpp" - "${SWIFT_SOURCE_DIR}/lib/Threading/TSan.cpp" + "${SWIFT_SOURCE_DIR}/lib/Threading/ThreadSanitizer.cpp" INSTALL_IN_COMPONENT never_install) diff --git a/test/Sanitizers/tsan/once.swift b/test/Sanitizers/tsan/once.swift index bb54500daadb2..f3bcc562403f5 100644 --- a/test/Sanitizers/tsan/once.swift +++ b/test/Sanitizers/tsan/once.swift @@ -12,13 +12,18 @@ // UNSUPPORTED: remote_run // Test that we do not report a race on initialization; Swift doesn't initialize -// globals at start-up, but rather uses swift_once(). This is thread safe, but +// globals at start-up, but rather uses `swift_once()`. This is thread safe, but // on some platforms TSan wasn't seeing the synchronization, so would report // a false positive. import Dispatch var count = 0 + +// This initialization will be done via a call to `swift_once()`. Prior to +// the fix for rdar://110665213, the addition to `count` would trigger a +// TSan message on Linux because the sanitizer couldn't see the lock we're +// using to make `swift_once()` thread safe. let foo = { count += 1 return count From 8ed8a284782add334120e63f2007a0760f366964 Mon Sep 17 00:00:00 2001 From: Alastair Houghton Date: Mon, 19 Jun 2023 15:20:17 +0100 Subject: [PATCH 3/7] [Threading][TSan] Move ThreadSanitizer.cpp into the main runtime. On Darwin, `RTLD_NEXT` doesn't do what we need it to here, with the result that if `libswiftCore`'s TSan initializer gets found first, then `libswift_Concurrency` won't have its initializer called at all, in spite of us using `RTLD_NEXT` to find the next definition. Fix this by centralising the initializer in `libswiftCore` instead. rdar://110665213 --- include/swift/Threading/ThreadSanitizer.h | 18 +++++++------ lib/Threading/CMakeLists.txt | 3 +-- stdlib/public/Threading/CMakeLists.txt | 1 - stdlib/public/runtime/CMakeLists.txt | 1 + .../public/runtime}/ThreadSanitizer.cpp | 25 ++++++++++++------- 5 files changed, 28 insertions(+), 20 deletions(-) rename {lib/Threading => stdlib/public/runtime}/ThreadSanitizer.cpp (63%) diff --git a/include/swift/Threading/ThreadSanitizer.h b/include/swift/Threading/ThreadSanitizer.h index 19923c3689cee..c97ab2c4241cf 100644 --- a/include/swift/Threading/ThreadSanitizer.h +++ b/include/swift/Threading/ThreadSanitizer.h @@ -20,6 +20,8 @@ #ifndef SWIFT_THREADING_THREAD_SANITIZER_H #define SWIFT_THREADING_THREAD_SANITIZER_H +#include "swift/shims/Visibility.h" + namespace swift { #if defined(_WIN32) || defined(__wasi__) || !__has_include() @@ -34,9 +36,9 @@ template T *release(T *ptr) { return ptr; } namespace threading_impl { -extern bool tsan_enabled; -extern void (*tsan_acquire)(const void *ptr); -extern void (*tsan_release)(const void *ptr); +SWIFT_RUNTIME_EXPORT bool _swift_tsan_enabled; +SWIFT_RUNTIME_EXPORT void (*_swift_tsan_acquire)(const void *ptr); +SWIFT_RUNTIME_EXPORT void (*_swift_tsan_release)(const void *ptr); } // namespace threading_impl @@ -44,7 +46,7 @@ namespace tsan { /// Returns true if TSan is enabled inline bool enabled() { - return threading_impl::tsan_enabled; + return threading_impl::_swift_tsan_enabled; } /// Indicate to TSan that an acquiring load has occurred on the current @@ -54,8 +56,8 @@ inline bool enabled() { /// `acquire()`. template T *acquire(T *ptr) { - if (threading_impl::tsan_acquire) { - threading_impl::tsan_acquire(ptr); + if (threading_impl::_swift_tsan_acquire) { + threading_impl::_swift_tsan_acquire(ptr); } return ptr; } @@ -66,8 +68,8 @@ T *acquire(T *ptr) { /// see all writes that happened before the `release()`. template T *release(T *ptr) { - if (threading_impl::tsan_release) { - threading_impl::tsan_release(ptr); + if (threading_impl::_swift_tsan_release) { + threading_impl::_swift_tsan_release(ptr); } return ptr; } diff --git a/lib/Threading/CMakeLists.txt b/lib/Threading/CMakeLists.txt index 4e0e6aed05e3b..029a52b68e1e3 100644 --- a/lib/Threading/CMakeLists.txt +++ b/lib/Threading/CMakeLists.txt @@ -10,5 +10,4 @@ add_swift_host_library(swiftThreading STATIC Linux.cpp Pthreads.cpp Win32.cpp - Errors.cpp - ThreadSanitizer.cpp) + Errors.cpp) diff --git a/stdlib/public/Threading/CMakeLists.txt b/stdlib/public/Threading/CMakeLists.txt index 961c643b756ee..007289ed3fac7 100644 --- a/stdlib/public/Threading/CMakeLists.txt +++ b/stdlib/public/Threading/CMakeLists.txt @@ -9,5 +9,4 @@ add_swift_target_library(swiftThreading OBJECT_LIBRARY "${SWIFT_SOURCE_DIR}/lib/Threading/Linux.cpp" "${SWIFT_SOURCE_DIR}/lib/Threading/Pthreads.cpp" "${SWIFT_SOURCE_DIR}/lib/Threading/Win32.cpp" - "${SWIFT_SOURCE_DIR}/lib/Threading/ThreadSanitizer.cpp" INSTALL_IN_COMPONENT never_install) diff --git a/stdlib/public/runtime/CMakeLists.txt b/stdlib/public/runtime/CMakeLists.txt index 18320c6d7689f..05fa2f65b9aa4 100644 --- a/stdlib/public/runtime/CMakeLists.txt +++ b/stdlib/public/runtime/CMakeLists.txt @@ -76,6 +76,7 @@ set(swift_runtime_sources SwiftDtoa.cpp SwiftTLSContext.cpp ThreadingError.cpp + ThreadSanitizer.cpp Tracing.cpp AccessibleFunction.cpp Win32.cpp) diff --git a/lib/Threading/ThreadSanitizer.cpp b/stdlib/public/runtime/ThreadSanitizer.cpp similarity index 63% rename from lib/Threading/ThreadSanitizer.cpp rename to stdlib/public/runtime/ThreadSanitizer.cpp index bee0b8529c52d..75b282f3e46f7 100644 --- a/lib/Threading/ThreadSanitizer.cpp +++ b/stdlib/public/runtime/ThreadSanitizer.cpp @@ -20,28 +20,35 @@ #include "swift/Threading/ThreadSanitizer.h" #include "swift/shims/Visibility.h" +#include + namespace swift { namespace threading_impl { -bool tsan_enabled = false; -void (*tsan_acquire)(const void *) = nullptr; -void (*tsan_release)(const void *) = nullptr; +SWIFT_RUNTIME_EXPORT bool _swift_tsan_enabled = false; +SWIFT_RUNTIME_EXPORT void (*_swift_tsan_acquire)(const void *) = nullptr; +SWIFT_RUNTIME_EXPORT void (*_swift_tsan_release)(const void *) = nullptr; #if __has_include() #include // The TSan library code will call this function when it starts up -extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS +SWIFT_RUNTIME_EXPORT void __tsan_on_initialize() { - tsan_enabled = true; - tsan_acquire = (void (*)(const void *))dlsym(RTLD_DEFAULT, "__tsan_acquire"); - tsan_release = (void (*)(const void *))dlsym(RTLD_DEFAULT, "__tsan_release"); + _swift_tsan_enabled = true; + _swift_tsan_acquire = (void (*)(const void *))dlsym(RTLD_DEFAULT, + "__tsan_acquire"); + _swift_tsan_release = (void (*)(const void *))dlsym(RTLD_DEFAULT, + "__tsan_release"); - // Always call through to the next image + // Always call through to the next image; this won't work on macOS, but it's + // important on Linux to allow others to hook into the thread sanitizer if + // they wish. void (*next_init)(void); next_init = (void (*)(void))dlsym(RTLD_NEXT, "__tsan_on_initialize"); - if (next_init) + if (next_init) { next_init(); + } } #endif From bad716f2cd4998153e6e7bf64eeee34f2b1207f3 Mon Sep 17 00:00:00 2001 From: Alastair Houghton Date: Mon, 19 Jun 2023 17:00:39 +0100 Subject: [PATCH 4/7] [Threading][TSan] Rearrange things again. We need ThreadSanitizer.cpp in libswiftCore for the runtime case, but we also need it in libswiftThreading for non-runtime cases. rdar://1106655213 --- lib/Threading/CMakeLists.txt | 3 ++- .../runtime => lib/Threading}/ThreadSanitizer.cpp | 11 +++++++---- stdlib/public/Threading/CMakeLists.txt | 1 + stdlib/public/runtime/CMakeLists.txt | 8 +++++++- 4 files changed, 17 insertions(+), 6 deletions(-) rename {stdlib/public/runtime => lib/Threading}/ThreadSanitizer.cpp (86%) diff --git a/lib/Threading/CMakeLists.txt b/lib/Threading/CMakeLists.txt index 029a52b68e1e3..4e0e6aed05e3b 100644 --- a/lib/Threading/CMakeLists.txt +++ b/lib/Threading/CMakeLists.txt @@ -10,4 +10,5 @@ add_swift_host_library(swiftThreading STATIC Linux.cpp Pthreads.cpp Win32.cpp - Errors.cpp) + Errors.cpp + ThreadSanitizer.cpp) diff --git a/stdlib/public/runtime/ThreadSanitizer.cpp b/lib/Threading/ThreadSanitizer.cpp similarity index 86% rename from stdlib/public/runtime/ThreadSanitizer.cpp rename to lib/Threading/ThreadSanitizer.cpp index 75b282f3e46f7..127cc4df1a964 100644 --- a/stdlib/public/runtime/ThreadSanitizer.cpp +++ b/lib/Threading/ThreadSanitizer.cpp @@ -25,15 +25,18 @@ namespace swift { namespace threading_impl { -SWIFT_RUNTIME_EXPORT bool _swift_tsan_enabled = false; -SWIFT_RUNTIME_EXPORT void (*_swift_tsan_acquire)(const void *) = nullptr; -SWIFT_RUNTIME_EXPORT void (*_swift_tsan_release)(const void *) = nullptr; +extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS +bool _swift_tsan_enabled = false; +extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS +void (*_swift_tsan_acquire)(const void *) = nullptr; +extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS +void (*_swift_tsan_release)(const void *) = nullptr; #if __has_include() #include // The TSan library code will call this function when it starts up -SWIFT_RUNTIME_EXPORT +extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS void __tsan_on_initialize() { _swift_tsan_enabled = true; _swift_tsan_acquire = (void (*)(const void *))dlsym(RTLD_DEFAULT, diff --git a/stdlib/public/Threading/CMakeLists.txt b/stdlib/public/Threading/CMakeLists.txt index 007289ed3fac7..96ea52d1d8f7e 100644 --- a/stdlib/public/Threading/CMakeLists.txt +++ b/stdlib/public/Threading/CMakeLists.txt @@ -4,6 +4,7 @@ list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/../../cmake/modules") include(AddSwiftStdlib) +# This should *not* include ThreadSanitizer.cpp, as that is part of libswiftCore add_swift_target_library(swiftThreading OBJECT_LIBRARY "${SWIFT_SOURCE_DIR}/lib/Threading/C11.cpp" "${SWIFT_SOURCE_DIR}/lib/Threading/Linux.cpp" diff --git a/stdlib/public/runtime/CMakeLists.txt b/stdlib/public/runtime/CMakeLists.txt index 05fa2f65b9aa4..44c16efa7986c 100644 --- a/stdlib/public/runtime/CMakeLists.txt +++ b/stdlib/public/runtime/CMakeLists.txt @@ -76,11 +76,16 @@ set(swift_runtime_sources SwiftDtoa.cpp SwiftTLSContext.cpp ThreadingError.cpp - ThreadSanitizer.cpp Tracing.cpp AccessibleFunction.cpp Win32.cpp) +# We pull this in separately here because other dylibs will need it, but only +# will have the __tsan_on_initialize called, and on Darwin, RTLD_NEXT can't be +# used to call subsequence dylibs' copies of that. +set(swift_runtime_threading_sources + ${SWIFT_SOURCE_DIR}/lib/Threading/ThreadSanitizer.cpp) + set(swift_runtime_backtracing_sources Backtrace.cpp CrashHandlerMacOS.cpp @@ -133,6 +138,7 @@ add_swift_target_library(swiftRuntime OBJECT_LIBRARY ${swift_runtime_sources} ${swift_runtime_objc_sources} ${swift_runtime_leaks_sources} + ${swift_runtime_threading_sources} C_COMPILE_FLAGS ${swift_runtime_library_compile_flags} LINK_FLAGS ${swift_runtime_linker_flags} From 3f0018df97cd52812f98c4b62e46269589136e03 Mon Sep 17 00:00:00 2001 From: Alastair Houghton Date: Mon, 19 Jun 2023 17:24:35 +0100 Subject: [PATCH 5/7] [Threading][TSan] Fix linkage issue. We need to pick up the `_swift_tsan_xxx` symbols from libswiftCore in most cases, but sometimes we're statically linked and in that case we want to use a local copy. rdar://1106655213 --- cmake/modules/AddSwiftUnittests.cmake | 3 ++- include/swift/Threading/ThreadSanitizer.h | 15 ++++++++++++--- lib/Threading/ThreadSanitizer.cpp | 9 +++------ 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/cmake/modules/AddSwiftUnittests.cmake b/cmake/modules/AddSwiftUnittests.cmake index b0ed92b913f8f..e7a98367337e9 100644 --- a/cmake/modules/AddSwiftUnittests.cmake +++ b/cmake/modules/AddSwiftUnittests.cmake @@ -58,7 +58,8 @@ function(add_swift_unittest test_dirname) string(TOUPPER "${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_THREADING_PACKAGE}" _threading_package) target_compile_definitions("${test_dirname}" PRIVATE - "SWIFT_THREADING_${_threading_package}") + "SWIFT_THREADING_${_threading_package}" + "SWIFT_THREADING_STATIC") if(NOT SWIFT_COMPILER_IS_MSVC_LIKE) if(SWIFT_USE_LINKER) diff --git a/include/swift/Threading/ThreadSanitizer.h b/include/swift/Threading/ThreadSanitizer.h index c97ab2c4241cf..c07103d862052 100644 --- a/include/swift/Threading/ThreadSanitizer.h +++ b/include/swift/Threading/ThreadSanitizer.h @@ -34,11 +34,20 @@ template T *release(T *ptr) { return ptr; } } // namespace tsan #else +// If we're static linking to libswiftThreading.a, these symbols can come +// from there. If, on the other hand, we're dynamically linked, we want +// to get them from libswiftCore.dylib instead. +#if SWIFT_THREADING_STATIC +#define SWIFT_THREADING_EXPORT extern "C" +#else +#define SWIFT_THREADING_EXPORT SWIFT_RUNTIME_EXPORT +#endif + namespace threading_impl { -SWIFT_RUNTIME_EXPORT bool _swift_tsan_enabled; -SWIFT_RUNTIME_EXPORT void (*_swift_tsan_acquire)(const void *ptr); -SWIFT_RUNTIME_EXPORT void (*_swift_tsan_release)(const void *ptr); +SWIFT_THREADING_EXPORT bool _swift_tsan_enabled; +SWIFT_THREADING_EXPORT void (*_swift_tsan_acquire)(const void *ptr); +SWIFT_THREADING_EXPORT void (*_swift_tsan_release)(const void *ptr); } // namespace threading_impl diff --git a/lib/Threading/ThreadSanitizer.cpp b/lib/Threading/ThreadSanitizer.cpp index 127cc4df1a964..99213d0976abb 100644 --- a/lib/Threading/ThreadSanitizer.cpp +++ b/lib/Threading/ThreadSanitizer.cpp @@ -25,12 +25,9 @@ namespace swift { namespace threading_impl { -extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS -bool _swift_tsan_enabled = false; -extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS -void (*_swift_tsan_acquire)(const void *) = nullptr; -extern "C" SWIFT_ATTRIBUTE_FOR_EXPORTS -void (*_swift_tsan_release)(const void *) = nullptr; +SWIFT_THREADING_EXPORT bool _swift_tsan_enabled = false; +SWIFT_THREADING_EXPORT void (*_swift_tsan_acquire)(const void *) = nullptr; +SWIFT_THREADING_EXPORT void (*_swift_tsan_release)(const void *) = nullptr; #if __has_include() #include From 18b359ffcf8b8dc36d774d19e1f1fa208c2c4082 Mon Sep 17 00:00:00 2001 From: Alastair Houghton Date: Tue, 20 Jun 2023 09:12:29 +0100 Subject: [PATCH 6/7] [Threading][TSan] Fix builds where TSan isn't supported. On builds where TSan isn't supported, don't include any code in ThreadSanitizer.cpp. rdar://110665213 --- include/swift/Threading/ThreadSanitizer.h | 5 +++++ lib/Threading/ThreadSanitizer.cpp | 7 ++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/include/swift/Threading/ThreadSanitizer.h b/include/swift/Threading/ThreadSanitizer.h index c07103d862052..e7f95dec29ddc 100644 --- a/include/swift/Threading/ThreadSanitizer.h +++ b/include/swift/Threading/ThreadSanitizer.h @@ -25,6 +25,9 @@ namespace swift { #if defined(_WIN32) || defined(__wasi__) || !__has_include() + +#define SWIFT_THREADING_TSAN_SUPPORT 0 + namespace tsan { inline bool enabled() { return false; } @@ -34,6 +37,8 @@ template T *release(T *ptr) { return ptr; } } // namespace tsan #else +#define SWIFT_THREADING_TSAN_SUPPORT 1 + // If we're static linking to libswiftThreading.a, these symbols can come // from there. If, on the other hand, we're dynamically linked, we want // to get them from libswiftCore.dylib instead. diff --git a/lib/Threading/ThreadSanitizer.cpp b/lib/Threading/ThreadSanitizer.cpp index 99213d0976abb..a103c161dce91 100644 --- a/lib/Threading/ThreadSanitizer.cpp +++ b/lib/Threading/ThreadSanitizer.cpp @@ -18,6 +18,9 @@ //===----------------------------------------------------------------------===// #include "swift/Threading/ThreadSanitizer.h" + +#if SWIFT_THREADING_TSAN_SUPPORT + #include "swift/shims/Visibility.h" #include @@ -50,7 +53,9 @@ void __tsan_on_initialize() { next_init(); } } -#endif +#endif // __has_include() } // namespace threading_impl } // namespace swift + +#endif // SWIFT_THREADING_TSAN_SUPPORT From f38b9a960df744e22c0663b6cec1b129aec6975e Mon Sep 17 00:00:00 2001 From: Alastair Houghton Date: Thu, 22 Jun 2023 12:28:06 +0100 Subject: [PATCH 7/7] [Threading][TSan] Update comments. Updated the comments for tsan::acquire and tsan::release to better reflect what TSan is actually doing. rdar://110665213 --- include/swift/Threading/ThreadSanitizer.h | 41 ++++++++++++++++++----- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/include/swift/Threading/ThreadSanitizer.h b/include/swift/Threading/ThreadSanitizer.h index e7f95dec29ddc..0a6a8028ea183 100644 --- a/include/swift/Threading/ThreadSanitizer.h +++ b/include/swift/Threading/ThreadSanitizer.h @@ -63,11 +63,35 @@ inline bool enabled() { return threading_impl::_swift_tsan_enabled; } -/// Indicate to TSan that an acquiring load has occurred on the current -/// thread. If some other thread does a releasing store with the same -/// pointer, we are indicating to TSan that all writes that happened -/// before that store will be visible to the current thread after the -/// `acquire()`. +/// Inform TSan about a synchronization operation. +/// +/// This is used when TSan cannot see the synchronization operation, for +/// example, if it is using a custom primitive for which TSan doesn't have +/// a built-in interceptor. This does not necessarily mean a lock or a C(++) +/// atomic operation - it could be any kind of synchronization mechanism. +/// +/// An acquire-release pair using the same address establishes an ordering +/// constraint in TSan's happens-before graph, which TSan uses to determine +/// whether two memory accesses from different threads have a well-defined +/// order. +/// +/// For instance, in +/// +/// Thread 1 Thread 2 +/// +/// access to y +/// tsan::release(x) +/// lock given away +/// +/// --> sync point --> +/// +/// lock taken +/// tsan::acquire(x) +/// access to y +/// +/// the access to y from Thread 2 is safe relative to the preceding access to +/// y on Thread 1 because it is preceded by an acquire of x that was itself +/// preceded by a release of x. template T *acquire(T *ptr) { if (threading_impl::_swift_tsan_acquire) { @@ -76,10 +100,9 @@ T *acquire(T *ptr) { return ptr; } -/// Indicate to TSan that a releasing store has occurred on the current -/// thread. If some other thread does an acquiring load with the same -/// pointer, we are indicating to TSan that that thread will be able to -/// see all writes that happened before the `release()`. +/// Inform TSan about a synchronization operation. +/// +/// This is the counterpart to tsan::acquire. template T *release(T *ptr) { if (threading_impl::_swift_tsan_release) {