Add ThreadSanitizer support #2406

devDDen · 2024-08-26T15:44:48Z

ThreadSanitizer helps to detect data races. The PR adds TSan support to seastar and a build mode where it is enabled. It allows to run seastar applications with ThreadSanitizer.

During testing, the sanitizer reported data races in the following places:

already fixed 48c6888
exception_hacks.cc in _Unwind_RaiseException (fixed here)
in tests/unit/* in BOOST_REQUIRE (fixed here) due to Boost.Test macros are not thread safe. To get an accurate stack trace, the Boost.Test library must be built with ThreadSanitizer

- restore SEASTAR_NO_EXCEPTION_HACK behavior - detect TSan and its fiber support - do not handle SIGSEGV, use increased stack size for TSan as for ASan - use TSan fiber api in context switch code - add sanitize-thread build mode with TSan and SEASTAR_NO_EXCEPTION_HACK

BOOST_REQUIRE macros are not thread safe, so replace them with asserts in multithreaded environment

avikivity · 2024-08-26T16:50:36Z

Looks good, @tchaikov please review, especially the cmake parts.

tchaikov

left some comments.

tchaikov · 2024-08-27T05:13:09Z

CMakeLists.txt

+    CMAKE_CXX_FLAGS_SANITIZETHREAD
+    CMAKE_C_FLAGS_SANITIZETHREAD
+    CMAKE_EXE_LINKER_FLAGS_SANITIZETHREAD
+    CMAKE_SHARED_LINKER_FLAGS_SANITIZETHREAD)


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?

@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

@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

tchaikov · 2024-08-27T05:30:38Z

src/core/thread.cc


 namespace {

-#ifdef SEASTAR_HAVE_ASAN_FIBER_SUPPORT
+#if defined(SEASTAR_ASAN_ENABLED) && defined(SEASTAR_HAVE_ASAN_FIBER_SUPPORT)


because #if at higher level has changed

- #ifdef SEASTAR_ASAN_ENABLED + #if defined(SEASTAR_ASAN_ENABLED) || defined(SEASTAR_TSAN_ENABLED)

I see the comment below about it

tchaikov · 2024-08-27T05:32:06Z

CMakeLists.txt

+      SEASTAR_NO_EXCEPTION_HACK)
+  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)


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

tchaikov · 2024-08-27T05:33:46Z

CMakeLists.txt

+      $<${condition}:Sanitizers::thread>)
+  target_compile_definitions(seastar
+    PUBLIC
+      SEASTAR_NO_EXCEPTION_HACK)


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

tchaikov · 2024-08-27T05:48:57Z

src/core/thread.cc

+{
+    // Do not destroy main context
+    if (destroy_tsan_fiber)
+    {


please attach the { at end of previous line.

tchaikov · 2024-08-27T06:11:22Z

cmake/FindSanitizers.cmake

+find_package_handle_standard_args (ThreadSanitizer
+  REQUIRED_VARS
+    ThreadSanitizer_COMPILER_OPTIONS)


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.

tchaikov · 2024-08-27T06:22:56Z

configure.py

@@ -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'}


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

tchaikov · 2024-08-27T06:56:28Z

src/core/exception_hacks.cc

@@ -71,6 +71,8 @@ module seastar;
 #endif

 namespace seastar {
+
+#ifndef SEASTAR_NO_EXCEPTION_HACK


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.

tchaikov · 2024-08-27T06:58:30Z

CMakeLists.txt

@@ -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)


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.

tchaikov · 2024-08-27T06:59:36Z

cmake/FindSanitizers.cmake

@@ -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)


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

tchaikov · 2024-08-27T07:01:52Z

tests/unit/distributed_test.cc

@@ -142,7 +142,7 @@ SEASTAR_TEST_CASE(test_map_reduce_lifetime) {
        }
        auto operator()(const X& x) {
            return yield().then([this, &x] {
-                BOOST_REQUIRE(!destroyed);
+                assert(!destroyed);


BOOST_REQUIRE macros are not thread safe, so replace them with
asserts in multithreaded environment

could you share the impact of "not thread safe" ? did you find the issue by inspecting the code, or by enabling ThreadSanitizer, and it warns at seeing this usage?

ThreadSanitizer warns

Boost.Test docs say about it

Normal Seastar tests run on a single reactor thread. But this specific test deliberately starts the same code on multiple threads. And then, supposedly, you're not supposed to use BOOST_REQUIRE from multiple threads.

However, I don't think replacing BOOST_REQUIRE by assert() is the right thing to do - it will replace a clean test failure of a single test function, by a crash of the entire executable... :-(

I think it more sense to have an (atomic-variable) counter of errors, and then BOOST_REQUIRE that the counter be 0 when the do_with_distributed ends (on a single thread).

BTW, I find it really shocking - that in 2024 Boost.Test doesn't support multiple threads out of the box. It's trivial to support (especially when performance is not even an issue). It reduces whatever little confidence I had in this test framework :-(

tchaikov · 2024-09-12T15:22:53Z

@devDDen #2408 has been merged. do you plan to continue working on this change?

devDDen · 2024-09-15T10:49:59Z

@devDDen #2408 has been merged. do you plan to continue working on this change?

Yes, at first I plan to do #2406 (comment) soon in a separate PR

tchaikov · 2024-09-15T11:48:05Z

@devDDen #2408 has been merged. do you plan to continue working on this change?

Yes, at first I plan to do #2406 (comment) soon in a separate PR

thanks. please let me know if i can help.

devDDen added 2 commits August 26, 2024 11:39

tests: fix data races with BOOST_REQUIRE

4506cd3

BOOST_REQUIRE macros are not thread safe, so replace them with asserts in multithreaded environment

tchaikov reviewed Aug 27, 2024

View reviewed changes

tchaikov mentioned this pull request Sep 2, 2024

build: check the combination of Sanitizers #2408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ThreadSanitizer support #2406

Add ThreadSanitizer support #2406

devDDen commented Aug 26, 2024

avikivity commented Aug 26, 2024

tchaikov left a comment

tchaikov Aug 27, 2024 •

edited

Loading

nyh Sep 15, 2024

tchaikov Sep 15, 2024

tchaikov Aug 27, 2024

devDDen Aug 27, 2024

devDDen Aug 27, 2024

tchaikov Aug 27, 2024

tchaikov Aug 27, 2024

tchaikov Aug 27, 2024

tchaikov Aug 27, 2024

tchaikov Aug 27, 2024

tchaikov Aug 27, 2024

tchaikov Aug 27, 2024

tchaikov Aug 27, 2024

tchaikov Aug 27, 2024 •

edited

Loading

devDDen Aug 27, 2024

nyh Sep 15, 2024

nyh Sep 15, 2024

tchaikov commented Sep 12, 2024

devDDen commented Sep 15, 2024

tchaikov commented Sep 15, 2024

Add ThreadSanitizer support #2406

Are you sure you want to change the base?

Add ThreadSanitizer support #2406

Conversation

devDDen commented Aug 26, 2024

avikivity commented Aug 26, 2024

tchaikov left a comment

Choose a reason for hiding this comment

tchaikov Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Sep 12, 2024

devDDen commented Sep 15, 2024

tchaikov commented Sep 15, 2024

tchaikov Aug 27, 2024 •

edited

Loading

tchaikov Aug 27, 2024 •

edited

Loading