Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ThreadSanitizer support #2406

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+240 −60
Open

Add ThreadSanitizer support #2406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bfa6ef
Add ThreadSanitizer support
devDDen Aug 22, 2024
4506cd3
tests: fix data races with BOOST_REQUIRE
devDDen Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 70 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,45 @@ mark_as_advanced (
CMAKE_EXE_LINKER_FLAGS_SANITIZE
CMAKE_SHARED_LINKER_FLAGS_SANITIZE)

set (CMAKE_CXX_FLAGS_SANITIZETHREAD
"-Os -g"
CACHE
STRING
"Flags used by the C++ compiler during sanitize-thread builds."
FORCE)

set (CMAKE_C_FLAGS_SANITIZETHREAD
"-Os -g"
CACHE
STRING
"Flags used by the C compiler during sanitize-thread builds."
FORCE)

set (CMAKE_EXE_LINKER_FLAGS_SANITIZETHREAD
""
CACHE
STRING
"Flags used for linking binaries during sanitize-thread builds."
FORCE)

set (CMAKE_SHARED_LINKER_FLAGS_SANITIZETHREAD
""
CACHE
STRING
"Flags used by the shared libraries linker during sanitize-thread builds."
FORCE)

mark_as_advanced (
CMAKE_CXX_FLAGS_SANITIZETHREAD
CMAKE_C_FLAGS_SANITIZETHREAD
CMAKE_EXE_LINKER_FLAGS_SANITIZETHREAD
CMAKE_SHARED_LINKER_FLAGS_SANITIZETHREAD)
Copy link
Contributor

@tchaikov tchaikov Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd prefer keeping using the "sanitize" mode and "Sanitize" config. instead of adding yet another one dedicating for building with TSan enabled, can we just add an option named Seastar_SANITIZERS as a list of sanitizers for configuring the build?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov if you're thinking of enabling this sanitizer by default when sanitizers are enabled - please try to measure the performance impact of this sanitizer on some "typical" (however you define that) Seastar test code. The majority of the Seastar code (and code using Seastar) does not use share data between threads, so although catching every bug is great, I doubt this sanitizer will help catch many bugs so we need some justification to turn it on by

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov if you're thinking of enabling this sanitizer by default when sanitizers are enabled

that's not my plan. my plan is to keep the existing behavior.

  • please try to measure the performance impact of this sanitizer on some "typical" (however you define that) Seastar test code. The majority of the Seastar code (and code using Seastar) does not use share data between threads, so although catching every bug is great, I doubt this sanitizer will help catch many bugs so we need some justification to turn it on by


set (CMAKE_BUILD_TYPE
"${CMAKE_BUILD_TYPE}"
CACHE
STRING
"Choose the type of build, options are: None Debug Release RelWithDebInfo MinSizeRel Dev Sanitize."
"Choose the type of build, options are: None Debug Release RelWithDebInfo Dev Sanitize SanitizeThread."
FORCE)

if (NOT CMAKE_BUILD_TYPE)
Expand Down Expand Up @@ -356,7 +390,7 @@ endif ()


set (Seastar_TEST_ENVIRONMENT
"ASAN_OPTIONS=${Seastar_ASAN_OPTIONS};UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1;BOOST_TEST_CATCH_SYSTEM_ERRORS=no"
"ASAN_OPTIONS=${Seastar_ASAN_OPTIONS};UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1;TSAN_OPTIONS=halt_on_error=1;BOOST_TEST_CATCH_SYSTEM_ERRORS=no"
CACHE
STRING
"Environment variables for running tests")
Expand All @@ -377,6 +411,12 @@ set (Seastar_SANITIZE
STRING
"Enable ASAN and UBSAN. Can be ON, OFF or DEFAULT (which enables it for Debug and Sanitize)")

set (Seastar_SANITIZE_THREAD
"DEFAULT"
CACHE
STRING
"Enable TSAN. Can be ON, OFF or DEFAULT (which enables it for SanitizeThread)")

set (Seastar_DEBUG_SHARED_PTR
"DEFAULT"
CACHE
Expand Down Expand Up @@ -888,6 +928,26 @@ if (condition)
$<${condition}:Sanitizers::undefined_behavior>)
endif ()

tri_state_option (${Seastar_SANITIZE_THREAD}
DEFAULT_BUILD_TYPES "SanitizeThread"
CONDITION condition)
if (condition)
if (NOT ThreadSanitizer_FOUND)
message (FATAL_ERROR "Thread sanitizer not found!")
endif ()
set (Seastar_Sanitizers_OPTIONS ${ThreadSanitizer_COMPILER_OPTIONS})
target_link_libraries (seastar
PUBLIC
$<${condition}:Sanitizers::thread>)
target_compile_definitions(seastar
PUBLIC
SEASTAR_NO_EXCEPTION_HACK)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why make it public? AFAICT, it's not part of the public interface, as this macro definition is only checked by reactor.cc.

if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
# Disabled because tsan doesn't support std::atomic_thread_fence
list (APPEND Seastar_PRIVATE_CXX_FLAGS -Wno-error=tsan)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you quote the link to the related document and/or the bug report? and why is it a private compiler flag?

endif ()
endif ()

# We only need valgrind to find uninitialized memory uses, so disable
# the leak sanitizer.
# To test with valgrind run "ctest -T memcheck"
Expand Down Expand Up @@ -963,6 +1023,10 @@ if (Sanitizers_FIBER_SUPPORT)
list (APPEND Seastar_PRIVATE_COMPILE_DEFINITIONS SEASTAR_HAVE_ASAN_FIBER_SUPPORT)
endif ()

if (ThreadSanitizer_FIBER_SUPPORT)
list (APPEND Seastar_PRIVATE_COMPILE_DEFINITIONS SEASTAR_HAVE_TSAN_FIBER_SUPPORT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this definition only applies to ThreadSanitizer, i'd suggest add this definition only if ThreadSanitizer is enabled. so we don't need to check both of them in the C++ code.

endif ()

if (Seastar_ALLOC_PAGE_SIZE)
target_compile_definitions (seastar
PUBLIC SEASTAR_OVERRIDE_ALLOCATOR_PAGE_SIZE=${Seastar_ALLOC_PAGE_SIZE})
Expand Down Expand Up @@ -1079,11 +1143,11 @@ foreach (definition
SEASTAR_SHUFFLE_TASK_QUEUE)
target_compile_definitions (seastar
PUBLIC
$<$<IN_LIST:$<CONFIG>,Debug;Sanitize>:${definition}>)
$<$<IN_LIST:$<CONFIG>,Debug;Sanitize;SanitizeThread>:${definition}>)
endforeach ()

tri_state_option (${Seastar_DEBUG_SHARED_PTR}
DEFAULT_BUILD_TYPES "Debug" "Sanitize"
DEFAULT_BUILD_TYPES "Debug" "Sanitize" "SanitizeThread"
CONDITION condition)
if (condition)
target_compile_definitions (seastar
Expand All @@ -1092,7 +1156,7 @@ if (condition)
endif ()

tri_state_option (${Seastar_DEBUG_SHARED_PTR}
DEFAULT_BUILD_TYPES "Debug" "Sanitize"
DEFAULT_BUILD_TYPES "Debug" "Sanitize" "SanitizeThread"
CONDITION condition)
if (condition)
target_compile_definitions (seastar
Expand All @@ -1103,7 +1167,7 @@ endif ()
include (CheckLibc)

tri_state_option (${Seastar_STACK_GUARDS}
DEFAULT_BUILD_TYPES "Debug" "Sanitize" "Dev"
DEFAULT_BUILD_TYPES "Debug" "Sanitize" "SanitizeThread" "Dev"
CONDITION condition)
if (condition)
# check for -fstack-clash-protection together with -Werror, because
Expand Down
13 changes: 7 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,13 @@ Build modes
The configure.py script is a wrapper around cmake. The --mode argument
maps to CMAKE_BUILD_TYPE, and supports the following modes

| | CMake mode | Debug info | Optimi&shy;zations | Sanitizers | Allocator | Checks | Use for |
| -------- | ------------------- | ---------- | ------------------ |------------- | --------- | -------- | -------------------------------------- |
| debug | `Debug` | Yes | `-O0` | ASAN, UBSAN | System | All | gdb |
| release | `RelWithDebInfo` | Yes | `-O3` | None | Seastar | Asserts | production |
| dev | `Dev` (Custom) | No | `-O1` | None | Seastar | Asserts | build and test cycle |
| sanitize | `Sanitize` (Custom) | Yes | `-Os` | ASAN, UBSAN | System | All | second level of tests, track down bugs |
| | CMake mode | Debug info | Optimi&shy;zations | Sanitizers | Allocator | Checks | Use for |
| --------------- | ------------------------- | ---------- | ------------------ | ----------- | --------- | ------- | -------------------------------------- |
| debug | `Debug` | Yes | `-O0` | ASAN, UBSAN | System | All | gdb |
| release | `RelWithDebInfo` | Yes | `-O3` | None | Seastar | Asserts | production |
| dev | `Dev` (Custom) | No | `-O1` | None | Seastar | Asserts | build and test cycle |
| sanitize | `Sanitize` (Custom) | Yes | `-Os` | ASAN, UBSAN | System | All | second level of tests, track down bugs |
| sanitize-thread | `SanitizeThread` (Custom) | Yes | `-Os` | TSAN | System | All | race detection |

Note that seastar is more sensitive to allocators and optimizations than
usual. A quick rule of the thumb of the relative performances is that
Expand Down
26 changes: 26 additions & 0 deletions cmake/FindSanitizers.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ if (Sanitizers_UNDEFINED_BEHAVIOR_FOUND)
set (Sanitizers_UNDEFINED_BEHAVIOR_COMPILER_OPTIONS "-fsanitize=undefined;-fno-sanitize=vptr")
endif ()

set (CMAKE_REQUIRED_FLAGS -fsanitize=thread)
check_cxx_source_compiles ("int main() {}" Sanitizers_THREAD_FOUND)

if (Sanitizers_THREAD_FOUND)
set (ThreadSanitizer_COMPILER_OPTIONS -fsanitize=thread)
endif ()

set (Sanitizers_COMPILER_OPTIONS
${Sanitizers_ADDRESS_COMPILER_OPTIONS}
${Sanitizers_UNDEFINED_BEHAVIOR_COMPILER_OPTIONS})
Expand All @@ -45,13 +52,21 @@ file (READ ${CMAKE_CURRENT_LIST_DIR}/code_tests/Sanitizers_fiber_test.cc _saniti
set (CMAKE_REQUIRED_FLAGS ${Sanitizers_COMPILER_OPTIONS})
check_cxx_source_compiles ("${_sanitizers_fiber_test_code}" Sanitizers_FIBER_SUPPORT)

file (READ ${CMAKE_CURRENT_LIST_DIR}/code_tests/ThreadSanitizer_fiber_test.cc _thread_sanitizer_fiber_test_code)
set (CMAKE_REQUIRED_FLAGS ${ThreadSanitizer_COMPILER_OPTIONS})
check_cxx_source_compiles ("${_thread_sanitizer_fiber_test_code}" ThreadSanitizer_FIBER_SUPPORT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as explained, i'd suggest define this variable only if ThreadSanitizer is listed in the COMPONENT parameter.


include (FindPackageHandleStandardArgs)

find_package_handle_standard_args (Sanitizers
REQUIRED_VARS
Sanitizers_ADDRESS_COMPILER_OPTIONS
Sanitizers_UNDEFINED_BEHAVIOR_COMPILER_OPTIONS)

find_package_handle_standard_args (ThreadSanitizer
REQUIRED_VARS
ThreadSanitizer_COMPILER_OPTIONS)
Comment on lines +66 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, FindSanitizers.cmake should only check only for a package named "Sanitizers", not packages with other name. so i think a better alternative is to support the "COMPONENT" parameter. #2408 was created to address this.


if (Sanitizers_FOUND)
if (NOT (TARGET Sanitizers::address))
add_library (Sanitizers::address INTERFACE IMPORTED)
Expand All @@ -71,3 +86,14 @@ if (Sanitizers_FOUND)
INTERFACE_LINK_LIBRARIES "${Sanitizers_UNDEFINED_BEHAVIOR_COMPILER_OPTIONS}")
endif ()
endif ()

if (ThreadSanitizer_FOUND)
if (NOT (TARGET Sanitizers::thread))
add_library (Sanitizers::thread INTERFACE IMPORTED)

set_target_properties (Sanitizers::thread
PROPERTIES
INTERFACE_COMPILE_OPTIONS ${ThreadSanitizer_COMPILER_OPTIONS}
INTERFACE_LINK_LIBRARIES ${ThreadSanitizer_COMPILER_OPTIONS})
endif ()
endif ()
14 changes: 14 additions & 0 deletions cmake/code_tests/ThreadSanitizer_fiber_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
extern "C" {
void* __tsan_get_current_fiber();
void* __tsan_create_fiber(unsigned flags);
void __tsan_destroy_fiber(void* fiber);
void __tsan_switch_to_fiber(void* fiber, unsigned flags);
}

int main() {
void* new_fiber = __tsan_create_fiber(0);
void* main_fiber = __tsan_get_current_fiber();
__tsan_switch_to_fiber(new_fiber, 0);
__tsan_switch_to_fiber(main_fiber, 0);
__tsan_destroy_fiber(new_fiber);
}
2 changes: 1 addition & 1 deletion configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def identify_best_standard(cpp_standards, compiler):
# For convenience.
tr = seastar_cmake.translate_arg

MODE_TO_CMAKE_BUILD_TYPE = {'release': 'RelWithDebInfo', 'debug': 'Debug', 'dev': 'Dev', 'sanitize': 'Sanitize' }
MODE_TO_CMAKE_BUILD_TYPE = {'release': 'RelWithDebInfo', 'debug': 'Debug', 'dev': 'Dev', 'sanitize': 'Sanitize', 'sanitize-thread' : 'SanitizeThread'}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd suggest to add an option to configure.py and cmake to list the enabled sanitizers. by default, if "Sanitize" is specified, both UBSan and ASan are enabled, but one can override the option by specifying it explicitly to use TSan, like

./configure.py --mode sanitize --sanitizer UndefinedBehavior --sanitizer Thread
./configure.py --mode sanitize --sanitizer Address
# -DCMAKE_BUILD_TYPE is but a convenient way to set a bunch of 
# preconfigured settings, but one can fine-tune the build with 
# these lower-level options
# just enable these two sanitizers without any predefined configurations
cmake -DCMAKE_BUILD_TYPE=None -DSeastar_SANITIZERS=Address;UndefinedBehavior
# inherit the settings of Sanitize, but customize the enabled Sanitizers
cmake -DCMAKE_BUILD_TYPE=Sanitize -DSeastar_SANITIZERS=Address;UndefinedBehavior



def configure_mode(mode):
Expand Down
11 changes: 10 additions & 1 deletion include/seastar/core/thread_impl.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,17 @@ class thread_context;
class scheduling_group;

struct jmp_buf_link {
#ifdef SEASTAR_ASAN_ENABLED
#if defined(SEASTAR_ASAN_ENABLED) || defined(SEASTAR_TSAN_ENABLED)
ucontext_t context;
#ifdef SEASTAR_ASAN_ENABLED
void* fake_stack = nullptr;
const void* stack_bottom;
size_t stack_size;
#endif
#ifdef SEASTAR_TSAN_ENABLED
void* fiber = nullptr;
bool destroy_tsan_fiber = false;
#endif
#else
jmp_buf jmpbuf;
#endif
Expand All @@ -52,6 +58,9 @@ public:
void switch_out();
void initial_switch_in_completed();
void final_switch_out();
#ifdef SEASTAR_TSAN_ENABLED
~jmp_buf_link();
#endif
};

extern thread_local jmp_buf_link* g_current_context;
Expand Down
4 changes: 4 additions & 0 deletions include/seastar/util/std-compat.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ namespace std::pmr {
#define SEASTAR_ASAN_ENABLED
#endif

#if __has_feature(thread_sanitizer) || defined(__SANITIZE_THREAD__)
#define SEASTAR_TSAN_ENABLED
#endif

#if __has_include(<source_location>)
#include <source_location>
#endif
Expand Down
2 changes: 1 addition & 1 deletion seastar_cmake.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import os

SUPPORTED_MODES = ['release', 'debug', 'dev', 'sanitize']
SUPPORTED_MODES = ['release', 'debug', 'dev', 'sanitize', 'sanitize-thread']

ROOT_PATH = os.path.realpath(os.path.dirname(__file__))

Expand Down
12 changes: 8 additions & 4 deletions src/core/exception_hacks.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ module seastar;
#endif

namespace seastar {

#ifndef SEASTAR_NO_EXCEPTION_HACK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restore SEASTAR_NO_EXCEPTION_HACK behavior

i'd suggest extract this into its own commit. and explain why we want to bring it back. and how this change helps.

using dl_iterate_fn = int (*) (int (*callback) (struct dl_phdr_info *info, size_t size, void *data), void *data);

[[gnu::no_sanitize_address]]
Expand Down Expand Up @@ -102,6 +104,9 @@ void init_phdr_cache() {
return 0;
}, nullptr);
}
#else
void init_phdr_cache() {}
#endif

void internal::increase_thrown_exceptions_counter() noexcept {
seastar::engine()._cxx_exceptions++;
Expand All @@ -124,6 +129,7 @@ void log_exception_trace() noexcept {}

}

#ifndef SEASTAR_NO_EXCEPTION_HACK
extern "C"
[[gnu::visibility("default")]]
[[gnu::used]]
Expand All @@ -146,18 +152,16 @@ int dl_iterate_phdr(int (*callback) (struct dl_phdr_info *info, size_t size, voi
}
return r;
}
#endif

#ifndef NO_EXCEPTION_INTERCEPT
extern "C"
[[gnu::visibility("default")]]
[[gnu::used]]
int _Unwind_RaiseException(struct ::_Unwind_Exception *h) {
using throw_fn = int (*)(void *);
static throw_fn org = nullptr;
static throw_fn org = (throw_fn)dlsym (RTLD_NEXT, "_Unwind_RaiseException");

if (!org) {
org = (throw_fn)dlsym (RTLD_NEXT, "_Unwind_RaiseException");
}
if (seastar::local_engine) {
seastar::internal::increase_thrown_exceptions_counter();
seastar::log_exception_trace();
Expand Down
2 changes: 1 addition & 1 deletion src/core/posix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ posix_thread::posix_thread(attr a, std::function<void ()> func)
throw std::system_error(r, std::system_category());
}

#ifndef SEASTAR_ASAN_ENABLED
#if !defined(SEASTAR_ASAN_ENABLED) && !defined(SEASTAR_TSAN_ENABLED)
auto stack_size = a._stack_size.size;
if (!stack_size) {
stack_size = 2 << 20;
Expand Down
8 changes: 4 additions & 4 deletions src/core/reactor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,7 +867,7 @@ static void print_with_backtrace(const char* cause, bool oneline = false) noexce
print_with_backtrace(buf, oneline);
}

#ifndef SEASTAR_ASAN_ENABLED
#if !defined(SEASTAR_ASAN_ENABLED) && !defined(SEASTAR_TSAN_ENABLED)
// Installs signal handler stack for current thread.
// The stack remains installed as long as the returned object is kept alive.
// When it goes out of scope the previous handler is restored.
Expand All @@ -893,7 +893,7 @@ static decltype(auto) install_signal_handler_stack() {
}
#else
// SIGSTKSZ is too small when using asan. We also don't need to
// handle SIGSEGV ourselves when using asan, so just don't install
// handle SIGSEGV ourselves when using asan/tsan, so just don't install
// a signal handler stack.
auto install_signal_handler_stack() {
struct nothing { ~nothing() {} };
Expand Down Expand Up @@ -3960,8 +3960,8 @@ static void sigabrt_action() noexcept {
reraise_signal(SIGABRT);
}

// We don't need to handle SIGSEGV when asan is enabled.
#ifdef SEASTAR_ASAN_ENABLED
// We don't need to handle SIGSEGV when asan/tsan is enabled.
#if defined(SEASTAR_ASAN_ENABLED) || defined(SEASTAR_TSAN_ENABLED)
template<>
void install_oneshot_signal_handler<SIGSEGV, sigsegv_action>() {
(void)sigsegv_action;
Expand Down
Loading