From 8fcfdc7ec57386a1617094602433ae95b07c91ba Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 27 Sep 2024 13:45:44 +0200 Subject: [PATCH 1/7] Add ThreadSanitizer support Fixes: https://github.com/quickjs-ng/quickjs/issues/557 --- .github/workflows/ci.yml | 12 +++++++++--- CMakeLists.txt | 13 +++++++++++++ quickjs.c | 2 +- quickjs.h | 1 - run-test262.c | 8 ++++++++ 5 files changed, 31 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d52652a06..34f9f82b4 100644 --- a/.github/workflows/ci.yml +++ b/.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-latest, configType: tsan } - { 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' }} diff --git a/CMakeLists.txt b/CMakeLists.txt index 1f6d377bc..5d9daeab4 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( diff --git a/quickjs.c b/quickjs.c index 646766705..91db3066f 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_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); diff --git a/quickjs.h b/quickjs.h index d449205f3..bf3b377ca 100644 --- a/quickjs.h +++ b/quickjs.h @@ -1022,7 +1022,6 @@ JS_EXTERN int JS_SetModuleExportList(JSContext *ctx, JSModuleDef *m, JS_EXTERN const char* JS_GetVersion(void); -#undef JS_EXTERN #undef js_force_inline #undef __js_printf_like diff --git a/run-test262.c b/run-test262.c index 5d7ef906e..dd903bcb1 100644 --- a/run-test262.c +++ b/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,6 +663,11 @@ 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 + js_mutex_lock(&js_atomics_mutex); + js_mutex_unlock(&js_atomics_mutex); usleep(duration * 1000); return JS_UNDEFINED; } From 76e2e0217472b2656e44b574c25394b00c967a52 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 27 Sep 2024 17:52:27 +0200 Subject: [PATCH 2/7] fixup! fix tsan warning in quickjs-libc --- quickjs-libc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/quickjs-libc.c b/quickjs-libc.c index 2955e4e01..3283ffdd6 100644 --- a/quickjs-libc.c +++ b/quickjs-libc.c @@ -87,11 +87,11 @@ extern char **environ; #ifdef USE_WORKER #include -#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; From efdc8f4195edb852440a27b6d1ca6523aeed43b9 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 27 Sep 2024 18:05:43 +0200 Subject: [PATCH 3/7] fixup! run test262 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 34f9f82b4..fe761b7c6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,7 +56,7 @@ jobs: - { os: ubuntu-latest, configType: asan, runTest262: true } - { os: ubuntu-latest, configType: ubsan, runTest262: true } - { os: ubuntu-latest, configType: msan } - - { os: ubuntu-latest, configType: tsan } + - { os: ubuntu-latest, configType: tsan, runTest262: true } - { os: ubuntu-latest, configType: tcc } - { os: ubuntu-latest, arch: x86 } - { os: ubuntu-latest, arch: riscv64 } From d19712c4acfc39c7f4baf7754b8cdf19fb2397ba Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 27 Sep 2024 18:06:36 +0200 Subject: [PATCH 4/7] fixup! undo exposing JS_EXTERN --- quickjs.c | 2 +- quickjs.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/quickjs.c b/quickjs.c index 91db3066f..cada7bbfd 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; -JS_EXTERN js_mutex_t js_atomics_mutex; // non-static to give run-test262 access +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); diff --git a/quickjs.h b/quickjs.h index bf3b377ca..d449205f3 100644 --- a/quickjs.h +++ b/quickjs.h @@ -1022,6 +1022,7 @@ JS_EXTERN int JS_SetModuleExportList(JSContext *ctx, JSModuleDef *m, JS_EXTERN const char* JS_GetVersion(void); +#undef JS_EXTERN #undef js_force_inline #undef __js_printf_like From 8de99df667aabc68bdd3e82d36dafb8a8d1e1e27 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 27 Sep 2024 18:28:31 +0200 Subject: [PATCH 5/7] fixup! use __attribute__((visibility("default"))) --- quickjs.c | 8 +++++++- quickjs.h | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/quickjs.c b/quickjs.c index cada7bbfd..2795ebcad 100644 --- a/quickjs.c +++ b/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; -js_mutex_t js_atomics_mutex; // non-static to give run-test262 access +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); diff --git a/quickjs.h b/quickjs.h index d449205f3..d818b8162 100644 --- a/quickjs.h +++ b/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"))) From e89433c51a93cea6a0b47c46056c54d1c7b94864 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 28 Sep 2024 12:37:01 +0200 Subject: [PATCH 6/7] fixup! hold lock during sleep --- run-test262.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/run-test262.c b/run-test262.c index dd903bcb1..1f46711a8 100644 --- a/run-test262.c +++ b/run-test262.c @@ -666,9 +666,18 @@ static JSValue js_agent_sleep(JSContext *ctx, JSValue this_val, // 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); - js_mutex_unlock(&js_atomics_mutex); usleep(duration * 1000); + js_mutex_unlock(&js_atomics_mutex); return JS_UNDEFINED; } From 66133ed0585e0cd4ec5b1d0e0ffd189145f5399e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 28 Sep 2024 14:38:30 +0200 Subject: [PATCH 7/7] fixup! try ubuntu 24.04 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fe761b7c6..7d113b976 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -56,7 +56,7 @@ jobs: - { os: ubuntu-latest, configType: asan, runTest262: true } - { os: ubuntu-latest, configType: ubsan, runTest262: true } - { os: ubuntu-latest, configType: msan } - - { os: ubuntu-latest, configType: tsan, runTest262: true } + - { os: ubuntu-24.04, configType: tsan, runTest262: true } - { os: ubuntu-latest, configType: tcc } - { os: ubuntu-latest, arch: x86 } - { os: ubuntu-latest, arch: riscv64 }