ThreadSanitizer support

[bnoordhuis]

Tracking issue because I'm trying to add TSan support but I'm hitting a snag - it doesn't like how we implement JS atomics and that may or may not be a legitimate complaint:

WARNING: ThreadSanitizer: data race (pid=1075168)                                                                                                              
  Atomic write of size 4 at 0x7b0400000ae0 by main thread:                                                                                                     
    #0 __tsan_atomic32_fetch_xor ../../../../src/libsanitizer/tsan/tsan_interface_atomic.cpp:723 (libtsan.so.0+0x85269)
    #1 js_atomics_op /home/bnoordhuis/src/quickjs/quickjs.c:53424 (run-test262+0x11c646)              
    #2 js_call_c_function /home/bnoordhuis/src/quickjs/quickjs.c:14573 (run-test262+0x6a95b)                                                                   
    #3 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:14774 (run-test262+0x6b602)                                                                      
    #4 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:15186 (run-test262+0x6f5f0)             
    #5 JS_CallFree /home/bnoordhuis/src/quickjs/quickjs.c:17293 (run-test262+0x7f765)      
    #6 JS_EvalFunctionInternal /home/bnoordhuis/src/quickjs/quickjs.c:33085 (run-test262+0xc1e9d)            
    #7 __JS_EvalInternal /home/bnoordhuis/src/quickjs/quickjs.c:33219 (run-test262+0xc29f1)            
    #8 JS_EvalInternal /home/bnoordhuis/src/quickjs/quickjs.c:33237 (run-test262+0xc2b47)                    
    #9 JS_EvalThis /home/bnoordhuis/src/quickjs/quickjs.c:33267 (run-test262+0xc2d51)                
    #10 JS_Eval /home/bnoordhuis/src/quickjs/quickjs.c:33275 (run-test262+0xc2ddc)                  
    #11 eval_buf /home/bnoordhuis/src/quickjs/run-test262.c:1214 (run-test262+0x16bd9)           
    #12 run_test_buf /home/bnoordhuis/src/quickjs/run-test262.c:1583 (run-test262+0x198c6) 
    #13 run_test /home/bnoordhuis/src/quickjs/run-test262.c:1819 (run-test262+0x1a859)            
    #14 run_test_dir_list /home/bnoordhuis/src/quickjs/run-test262.c:1973 (run-test262+0x1b1b5)
    #15 main /home/bnoordhuis/src/quickjs/run-test262.c:2144 (run-test262+0x1be4d)        
                                                                                                                                                               
  Previous read of size 4 at 0x7b0400000ae0 by thread T16 (mutexes: write M0):                                                                                 
    #0 js_atomics_wait /home/bnoordhuis/src/quickjs/quickjs.c:53622 (run-test262+0x11d657)
    #1 js_call_c_function /home/bnoordhuis/src/quickjs/quickjs.c:14560 (run-test262+0x6a8ba)   
    #2 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:14774 (run-test262+0x6b602)
    #3 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:15186 (run-test262+0x6f5f0)                                                                      
    #4 JS_Call /home/bnoordhuis/src/quickjs/quickjs.c:17286 (run-test262+0x7f6ca)    
    #5 agent_start /home/bnoordhuis/src/quickjs/run-test262.c:510 (run-test262+0x13da7)                                                                        

  As if synchronized via sleep:                                                                                                                                
    #0 usleep ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:359 (libtsan.so.0+0x66547)                                                         
    #1 js_agent_sleep /home/bnoordhuis/src/quickjs/run-test262.c:654 (run-test262+0x147bc)                                                                     
    #2 js_call_c_function /home/bnoordhuis/src/quickjs/quickjs.c:14560 (run-test262+0x6a8ba)                                                                   
    #3 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:14774 (run-test262+0x6b602)                                                                      
    #4 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:15186 (run-test262+0x6f5f0)                                                                      
    #5 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:15186 (run-test262+0x6f5f0)
    #6 JS_CallFree /home/bnoordhuis/src/quickjs/quickjs.c:17293 (run-test262+0x7f765)
    #7 JS_EvalFunctionInternal /home/bnoordhuis/src/quickjs/quickjs.c:33085 (run-test262+0xc1e9d)
    #8 __JS_EvalInternal /home/bnoordhuis/src/quickjs/quickjs.c:33219 (run-test262+0xc29f1)
    #9 JS_EvalInternal /home/bnoordhuis/src/quickjs/quickjs.c:33237 (run-test262+0xc2b47)
    #10 JS_EvalThis /home/bnoordhuis/src/quickjs/quickjs.c:33267 (run-test262+0xc2d51)
    #11 JS_Eval /home/bnoordhuis/src/quickjs/quickjs.c:33275 (run-test262+0xc2ddc)
    #12 eval_buf /home/bnoordhuis/src/quickjs/run-test262.c:1214 (run-test262+0x16bd9)
    #13 run_test_buf /home/bnoordhuis/src/quickjs/run-test262.c:1583 (run-test262+0x198c6)
    #14 run_test /home/bnoordhuis/src/quickjs/run-test262.c:1819 (run-test262+0x1a859)
    #15 run_test_dir_list /home/bnoordhuis/src/quickjs/run-test262.c:1973 (run-test262+0x1b1b5)
    #16 main /home/bnoordhuis/src/quickjs/run-test262.c:2144 (run-test262+0x1be4d)

  Location is heap block of size 16 at 0x7b0400000ae0 allocated by main thread: 
    #0 calloc ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:672 (libtsan.so.0+0x31edc)
    #1 js_def_calloc /home/bnoordhuis/src/quickjs/quickjs.c:1807 (run-test262+0x34417)
    #2 js_calloc_rt /home/bnoordhuis/src/quickjs/quickjs.c:1409 (run-test262+0x32e27)
    #3 js_mallocz_rt /home/bnoordhuis/src/quickjs/quickjs.c:1501 (run-test262+0x33415)
    #4 js_mallocz /home/bnoordhuis/src/quickjs/quickjs.c:1539 (run-test262+0x335cd)
    #5 js_array_buffer_constructor3 /home/bnoordhuis/src/quickjs/quickjs.c:50667 (run-test262+0x1101f8)
    #6 js_array_buffer_constructor2 /home/bnoordhuis/src/quickjs/quickjs.c:50702 (run-test262+0x11049c)
    #7 js_shared_array_buffer_constructor /home/bnoordhuis/src/quickjs/quickjs.c:50755 (run-test262+0x110824)
    #8 js_call_c_function /home/bnoordhuis/src/quickjs/quickjs.c:14560 (run-test262+0x6a8ba)
    #9 JS_CallConstructorInternal /home/bnoordhuis/src/quickjs/quickjs.c:17398 (run-test262+0x7fd5a)
    #10 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:15168 (run-test262+0x6f3a0)
    #11 JS_CallFree /home/bnoordhuis/src/quickjs/quickjs.c:17293 (run-test262+0x7f765)
    #12 JS_EvalFunctionInternal /home/bnoordhuis/src/quickjs/quickjs.c:33085 (run-test262+0xc1e9d)
    #13 __JS_EvalInternal /home/bnoordhuis/src/quickjs/quickjs.c:33219 (run-test262+0xc29f1)
    #14 JS_EvalInternal /home/bnoordhuis/src/quickjs/quickjs.c:33237 (run-test262+0xc2b47)
    #15 JS_EvalThis /home/bnoordhuis/src/quickjs/quickjs.c:33267 (run-test262+0xc2d51)
    #16 JS_Eval /home/bnoordhuis/src/quickjs/quickjs.c:33275 (run-test262+0xc2ddc)
    #17 eval_buf /home/bnoordhuis/src/quickjs/run-test262.c:1214 (run-test262+0x16bd9)
    #18 run_test_buf /home/bnoordhuis/src/quickjs/run-test262.c:1583 (run-test262+0x198c6)
    #19 run_test /home/bnoordhuis/src/quickjs/run-test262.c:1819 (run-test262+0x1a859)
    #20 run_test_dir_list /home/bnoordhuis/src/quickjs/run-test262.c:1973 (run-test262+0x1b1b5)
    #21 main /home/bnoordhuis/src/quickjs/run-test262.c:2144 (run-test262+0x1be4d)

  Mutex M0 (0x55da1530d740) created at: 
    #0 pthread_mutex_init ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1227 (libtsan.so.0+0x4bee1)
    #1 js_mutex_init /home/bnoordhuis/src/quickjs/cutils.c:1273 (run-test262+0x309fd)
    #2 js__atomics_init /home/bnoordhuis/src/quickjs/quickjs.c:53720 (run-test262+0x11dc33)
    #3 pthread_once ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1449 (libtsan.so.0+0x42e6a)
    #4 js_once /home/bnoordhuis/src/quickjs/cutils.c:1268 (run-test262+0x309bf) 
    #5 JS_AddIntrinsicAtomics /home/bnoordhuis/src/quickjs/quickjs.c:53726 (run-test262+0x11dc70)
    #6 JS_AddIntrinsicTypedArrays /home/bnoordhuis/src/quickjs/quickjs.c:53818 (run-test262+0x11e3b6)
    #7 JS_NewContext /home/bnoordhuis/src/quickjs/quickjs.c:2241 (run-test262+0x35f97)
    #8 run_test_buf /home/bnoordhuis/src/quickjs/run-test262.c:1563 (run-test262+0x1973d)
    #9 run_test /home/bnoordhuis/src/quickjs/run-test262.c:1819 (run-test262+0x1a859)
    #10 run_test_dir_list /home/bnoordhuis/src/quickjs/run-test262.c:1973 (run-test262+0x1b1b5)
    #11 main /home/bnoordhuis/src/quickjs/run-test262.c:2144 (run-test262+0x1be4d)

  Thread T16 (tid=1075248, running) created by main thread at:
    #0 pthread_create ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:969 (libtsan.so.0+0x605b8)
    #1 js_agent_start /home/bnoordhuis/src/quickjs/run-test262.c:553 (run-test262+0x140d8)
    #2 js_call_c_function /home/bnoordhuis/src/quickjs/quickjs.c:14560 (run-test262+0x6a8ba)
    #3 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:14774 (run-test262+0x6b602)
    #4 JS_CallInternal /home/bnoordhuis/src/quickjs/quickjs.c:15186 (run-test262+0x6f5f0)
    #5 JS_CallFree /home/bnoordhuis/src/quickjs/quickjs.c:17293 (run-test262+0x7f765)
    #6 JS_EvalFunctionInternal /home/bnoordhuis/src/quickjs/quickjs.c:33085 (run-test262+0xc1e9d)
    #7 __JS_EvalInternal /home/bnoordhuis/src/quickjs/quickjs.c:33219 (run-test262+0xc29f1)
    #8 JS_EvalInternal /home/bnoordhuis/src/quickjs/quickjs.c:33237 (run-test262+0xc2b47)
    #9 JS_EvalThis /home/bnoordhuis/src/quickjs/quickjs.c:33267 (run-test262+0xc2d51)
    #10 JS_Eval /home/bnoordhuis/src/quickjs/quickjs.c:33275 (run-test262+0xc2ddc)
    #11 eval_buf /home/bnoordhuis/src/quickjs/run-test262.c:1214 (run-test262+0x16bd9)
    #12 run_test_buf /home/bnoordhuis/src/quickjs/run-test262.c:1583 (run-test262+0x198c6)
    #13 run_test /home/bnoordhuis/src/quickjs/run-test262.c:1819 (run-test262+0x1a859)
    #14 run_test_dir_list /home/bnoordhuis/src/quickjs/run-test262.c:1973 (run-test262+0x1b1b5)
    #15 main /home/bnoordhuis/src/quickjs/run-test262.c:2144 (run-test262+0x1be4d)

SUMMARY: ThreadSanitizer: data race /home/bnoordhuis/src/quickjs/quickjs.c:53424 in js_atomics_op

⬆️ excerpt from running build/run-test262 -c test262.conf -d test262/test/built-ins/Atomics/wait

I'm undecided if the as if synchronized via sleep warning is a red herring or not but the observation that we mix atomic ops and mutexes is legit.

CMakeLists.txt diff:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1f6d377..5d9daea 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -115,6 +115,7 @@ xoption(BUILD_CLI_WITH_MIMALLOC "Build the qjs executable with mimalloc" OFF)
 xoption(BUILD_CLI_WITH_STATIC_MIMALLOC "Build the qjs executable with mimalloc (statically linked)" OFF)
 xoption(CONFIG_ASAN "Enable AddressSanitizer (ASan)" OFF)
 xoption(CONFIG_MSAN "Enable MemorySanitizer (MSan)" OFF)
+xoption(CONFIG_TSAN "Enable ThreadSanitizer (TSan)" OFF)
 xoption(CONFIG_UBSAN "Enable UndefinedBehaviorSanitizer (UBSan)" OFF)
 
 if(CONFIG_ASAN)
@@ -144,6 +145,18 @@ add_link_options(
     -fno-sanitize-recover=all
     -fno-omit-frame-pointer
 )
+elseif(CONFIG_TSAN)
+message(STATUS "Building with TSan")
+add_compile_options(
+    -fsanitize=thread
+    -fno-sanitize-recover=all
+    -fno-omit-frame-pointer
+)
+add_link_options(
+    -fsanitize=thread
+    -fno-sanitize-recover=all
+    -fno-omit-frame-pointer
+)
 elseif(CONFIG_UBSAN)
 message(STATUS "Building with UBSan")
 add_compile_definitions(

[bnoordhuis]

Observation: if I take and release the mutex inside js_agent_sleep, the warnings go away.

diff --git a/quickjs.c b/quickjs.c
index 6467667..61eb29e 100644
--- a/quickjs.c
+++ b/quickjs.c
@@ -53574,7 +53574,7 @@ typedef struct JSAtomicsWaiter {
 } JSAtomicsWaiter;
 
 static js_once_t js_atomics_once = JS_ONCE_INIT;
-static js_mutex_t js_atomics_mutex;
+js_mutex_t js_atomics_mutex;
 static struct list_head js_atomics_waiter_list =
     LIST_HEAD_INIT(js_atomics_waiter_list);
 
diff --git a/run-test262.c b/run-test262.c
index b9928a9..5fd98fc 100644
--- a/run-test262.c
+++ b/run-test262.c
@@ -39,6 +39,8 @@
 #include "list.h"
 #include "quickjs-libc.h"
 
+extern js_mutex_t js_atomics_mutex;
+
 /* enable test262 thread support to test SharedArrayBuffer and Atomics */
 #define CONFIG_AGENT
 
@@ -651,6 +653,8 @@ static JSValue js_agent_sleep(JSContext *ctx, JSValue this_val,
     uint32_t duration;
     if (JS_ToUint32(ctx, &duration, argv[0]))
         return JS_EXCEPTION;
+    js_mutex_lock(&js_atomics_mutex);
+    js_mutex_unlock(&js_atomics_mutex);
     usleep(duration * 1000);
     return JS_UNDEFINED;
 }

@saghul thoughts on whether that's a good idea or a bad idea? It doesn't materially affect run-test262 running time; it's around 20 seconds both ways for all of test262/test/built-ins/Atomics

real    0m20,143s                                                                                                                                              
user    0m7,219s                                                                                                                                               
sys     0m0,171s

And just Atomics.wait:

real    0m8,072s                                                               
user    0m2,609s                                                               
sys     0m0,140s

(pretty big wall-clock vs. CPU time difference)

[saghul]

I'll take a look! Shouldn't the unlock happen after the sleep?

[bnoordhuis]

I thought so too at first but I think it's really just acting as a fence / synchronization point and that's enough to make tsan happy.

[saghul]

I see. A quick look at https://github.com/google/sanitizers/wiki/ThreadSanitizerReportFormat suggests it might indeed be a false positive triggered by the mix of atomics and mutexes.

Suggestion: define CONFIG_AGENT (drop the define since it's now done always) in CMake when building the runner and only expose the mutex in quickjs.c in that case. An acompanying comment would also be nice.

[bnoordhuis]

I don't think that works: run-tes262 links against qjs, the static library target, so either the symbol is always exposed or never (and I don't want to build qjs twice because life's too short)

(Also, the no-CONFIG_AGENT build is broken. Might as well remove the define altogether.)

I'll include the patch from a few comments up and if you really hate it, you can always nack it :-)

[saghul]

I don't think that works: run-tes262 links against qjs, the static library target, so either the symbol is always exposed or never (and I don't want to build qjs twice because life's too short)

Ah true that. Let's land the change then.

(Also, the no-CONFIG_AGENT build is broken. Might as well remove the define altogether.)

Yeah, axe that too please :-)

[bnoordhuis]

axe that too please

Yes, I will in the "make run-test262 multi-threaded" PR. I'll be touching the agent code a great deal there anyway.

[saghul]

Yes, I will in the "make run-test262 multi-threaded" PR. I'll be touching the agent code a great deal there anyway.

Cool!

[bnoordhuis]

I have TSan working after #564 but runs take so godawful long that it's not realistic to test on CI, ergo... closing.