Add ThreadSanitizer support

.github/workflows/ci.yml

@@ -39,6 +39,7 @@ jobs:
     env:
       ASAN_OPTIONS: halt_on_error=1
       MSAN_OPTIONS: halt_on_error=1
+      TSAN_OPTIONS: halt_on_error=1
       UBSAN_OPTIONS: halt_on_error=1
     defaults:
       run:
@@ -55,6 +56,7 @@ jobs:
           - { os: ubuntu-latest, configType: asan, runTest262: true }
           - { os: ubuntu-latest, configType: ubsan, runTest262: true }
           - { os: ubuntu-latest, configType: msan }
+          - { os: ubuntu-24.04, configType: tsan, runTest262: true }
           - { os: ubuntu-latest, configType: tcc }
           - { os: ubuntu-latest, arch: x86 }
           - { os: ubuntu-latest, arch: riscv64 }
@@ -78,9 +80,9 @@ jobs:
         with:
           submodules: true
 
-      # ASLR with big PIE slides does not work well with [AM]San
+      # ASLR with big PIE slides does not work well with [AMT]San
       - name: disable ASLR
-        if: ${{ matrix.config.os == 'ubuntu-latest' && (matrix.config.configType == 'asan' || matrix.config.configType == 'ubsan' || matrix.config.configType == 'msan')}}
+        if: ${{ matrix.config.os == 'ubuntu-latest' && (matrix.config.configType == 'asan' || matrix.config.configType == 'ubsan' || matrix.config.configType == 'msan' || matrix.config.configType == 'tsan')}}
         run: |
           sudo sysctl -w kernel.randomize_va_space=0
 
@@ -123,6 +125,9 @@ jobs:
             echo "BUILD_TYPE=RelWithDebInfo" >> $GITHUB_ENV;
             echo "CONFIG_MSAN=ON" >> $GITHUB_ENV;
             echo "CC=clang" >> $GITHUB_ENV;
+          elif [ "${{ matrix.config.configType }}" = "tsan" ]; then
+            echo "BUILD_TYPE=RelWithDebInfo" >> $GITHUB_ENV;
+            echo "CONFIG_TSAN=ON" >> $GITHUB_ENV;
           fi
 
       - name: build
@@ -133,7 +138,8 @@ jobs:
             BUILD_SHARED_LIBS=$BUILD_SHARED_LIBS \
             CONFIG_ASAN=$CONFIG_ASAN \
             CONFIG_UBSAN=$CONFIG_UBSAN \
-            CONFIG_MSAN=$CONFIG_MSAN
+            CONFIG_MSAN=$CONFIG_MSAN \
+            CONFIG_TSAN=$CONFIG_TSAN
 
       - name: stats
         if: ${{ matrix.config.configType != 'examples' }}

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(

quickjs-libc.c

@@ -87,11 +87,11 @@ extern char **environ;
 
 #ifdef USE_WORKER
 #include <pthread.h>
-#include "quickjs-c-atomics.h"
 #endif
 
 #include "cutils.h"
 #include "list.h"
+#include "quickjs-c-atomics.h"
 #include "quickjs-libc.h"
 
 #define MAX_SAFE_INTEGER (((int64_t) 1 << 53) - 1)
@@ -167,7 +167,7 @@ typedef struct JSThreadState {
 } JSThreadState;
 
 static uint64_t os_pending_signals;
-static int (*os_poll_func)(JSContext *ctx);
+static _Atomic int can_js_os_poll;
 
 static void js_std_dbuf_init(JSContext *ctx, DynBuf *s)
 {
@@ -3827,7 +3827,7 @@ static const JSCFunctionListEntry js_os_funcs[] = {
 
 static int js_os_init(JSContext *ctx, JSModuleDef *m)
 {
-    os_poll_func = js_os_poll;
+    can_js_os_poll = TRUE;
 
 #ifdef USE_WORKER
     {
@@ -4058,7 +4058,7 @@ JSValue js_std_loop(JSContext *ctx)
             }
         }
 
-        if (!os_poll_func || os_poll_func(ctx))
+        if (!can_js_os_poll || js_os_poll(ctx))
             break;
     }
 done:
@@ -4090,8 +4090,8 @@ JSValue js_std_await(JSContext *ctx, JSValue obj)
             if (err < 0) {
                 js_std_dump_error(ctx1);
             }
-            if (os_poll_func)
-                os_poll_func(ctx);
+            if (can_js_os_poll)
+                js_os_poll(ctx);
         } else {
             /* not a promise */
             ret = obj;

quickjs.c

@@ -67,6 +67,12 @@
 #define CONFIG_ATOMICS
 #endif
 
+#if defined(__GNUC__) || defined(__TINYC__)
+#define JS_EXTERN __attribute__((visibility("default")))
+#else
+#define JS_EXTERN
+#endif
+
 #ifndef __GNUC__
 #define __extension__
 #endif
@@ -53574,7 +53580,7 @@ typedef struct JSAtomicsWaiter {
 } JSAtomicsWaiter;
 
 static js_once_t js_atomics_once = JS_ONCE_INIT;
-static js_mutex_t js_atomics_mutex;
+JS_EXTERN js_mutex_t js_atomics_mutex; // non-static to give run-test262 access
 static struct list_head js_atomics_waiter_list =
     LIST_HEAD_INIT(js_atomics_waiter_list);
 

quickjs.h

@@ -36,7 +36,7 @@
 extern "C" {
 #endif
 
-#if defined(__GNUC__) || defined(__clang__)
+#if defined(__GNUC__) || defined(__TINYC__)
 #define js_force_inline       inline __attribute__((always_inline))
 #define __js_printf_like(f, a)   __attribute__((format(printf, f, a)))
 #define JS_EXTERN __attribute__((visibility("default")))

run-test262.c

@@ -39,6 +39,9 @@
 #include "list.h"
 #include "quickjs-libc.h"
 
+// defined in quickjs.c
+extern js_mutex_t js_atomics_mutex;
+
 /* enable test262 thread support to test SharedArrayBuffer and Atomics */
 #define CONFIG_AGENT
 
@@ -660,7 +663,21 @@ static JSValue js_agent_sleep(JSContext *ctx, JSValue this_val,
     uint32_t duration;
     if (JS_ToUint32(ctx, &duration, argv[0]))
         return JS_EXCEPTION;
+    // this is here to silence a TSan warning: we mix mutexes
+    // and atomic ops and that confuses poor TSan, see
+    // https://github.com/quickjs-ng/quickjs/issues/557
+    //
+    // Sleeping while holding a lock looks wrong but
+    // test262/harness/atomicsHelper.js only sleeps
+    // for 1 ms periods so it's probably fine.
+    //
+    // We could alternatively expose os.setTimeout as
+    // globalThis.setTimeout, because test262 will use
+    // it when available, but then we also have to drive
+    // an event loop. Maybe in the future.
+    js_mutex_lock(&js_atomics_mutex);
     usleep(duration * 1000);
+    js_mutex_unlock(&js_atomics_mutex);
     return JS_UNDEFINED;
 }