From b8fbad35fb3e8ec73b07b32f4c45e9aa134c5626 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 22 Apr 2021 23:05:52 +0000 Subject: [PATCH 1/3] wasm: fix "thread_in_wasm flag was not set" crash in debug builds. Signed-off-by: Piotr Sikora --- bazel/external/wee8.patch | 56 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/bazel/external/wee8.patch b/bazel/external/wee8.patch index a7776c892483c..b9f62dedcbd69 100644 --- a/bazel/external/wee8.patch +++ b/bazel/external/wee8.patch @@ -1,6 +1,7 @@ # 1. Fix linking with unbundled toolchain on macOS. # 2. Increase VSZ limit to 64 TiB (allows us to start up to 6,553 VMs). # 3. Fix MSAN linking. +# 4. Fix "thread_in_wasm flag was not set" crash in debug builds (https://chromium-review.googlesource.com/c/v8/v8/+/2817598). --- wee8/build/toolchain/gcc_toolchain.gni +++ wee8/build/toolchain/gcc_toolchain.gni @@ -348,6 +348,8 @@ template("gcc_toolchain") { @@ -53,3 +54,58 @@ if (use_libfuzzer && (is_linux || is_chromeos)) { if (is_asan) { +--- wee8/src/execution/isolate.cc ++++ wee8/src/execution/isolate.cc +@@ -1672,10 +1672,36 @@ Object Isolate::ReThrow(Object exception) { + return ReadOnlyRoots(heap()).exception(); + } + ++namespace { ++// This scope will set the thread-in-wasm flag after the execution of all ++// destructors. The thread-in-wasm flag is only set when the scope gets enabled. ++class SetThreadInWasmFlagScope { ++ public: ++ SetThreadInWasmFlagScope() { ++ DCHECK_IMPLIES(trap_handler::IsTrapHandlerEnabled(), ++ !trap_handler::IsThreadInWasm()); ++ } ++ ++ ~SetThreadInWasmFlagScope() { ++ if (enabled_) trap_handler::SetThreadInWasm(); ++ } ++ ++ void Enable() { enabled_ = true; } ++ ++ private: ++ bool enabled_ = false; ++}; ++} // namespace ++ + Object Isolate::UnwindAndFindHandler() { ++ // Create the {SetThreadInWasmFlagScope} first in this function so that its ++ // destructor gets called after all the other destructors. It is important ++ // that the destructor sets the thread-in-wasm flag after all other ++ // destructors. The other destructors may cause exceptions, e.g. ASan on ++ // Windows, which would invalidate the thread-in-wasm flag when the wasm trap ++ // handler handles such non-wasm exceptions. ++ SetThreadInWasmFlagScope set_thread_in_wasm_flag_scope; + Object exception = pending_exception(); +- DCHECK_IMPLIES(trap_handler::IsTrapHandlerEnabled(), +- !trap_handler::IsThreadInWasm()); + + auto FoundHandler = [&](Context context, Address instruction_start, + intptr_t handler_offset, +@@ -1768,9 +1794,10 @@ Object Isolate::UnwindAndFindHandler() { + StandardFrameConstants::kFixedFrameSizeAboveFp - + wasm_code->stack_slots() * kSystemPointerSize; + +- // This is going to be handled by Wasm, so we need to set the TLS flag. +- trap_handler::SetThreadInWasm(); +- ++ // This is going to be handled by WebAssembly, so we need to set the TLS ++ // flag. The {SetThreadInWasmFlagScope} will set the flag after all ++ // destructors have been executed. ++ set_thread_in_wasm_flag_scope.Enable(); + return FoundHandler(Context(), wasm_code->instruction_start(), offset, + wasm_code->constant_pool(), return_sp, frame->fp()); + } From 308267c1c3c96702b43a9bfb5eb54350f08de372 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 3 May 2021 22:16:03 +0000 Subject: [PATCH 2/3] review: cherry-pick another fix. Signed-off-by: Piotr Sikora --- bazel/external/wee8.patch | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/bazel/external/wee8.patch b/bazel/external/wee8.patch index b9f62dedcbd69..bf17ebe0b3d58 100644 --- a/bazel/external/wee8.patch +++ b/bazel/external/wee8.patch @@ -1,7 +1,7 @@ # 1. Fix linking with unbundled toolchain on macOS. # 2. Increase VSZ limit to 64 TiB (allows us to start up to 6,553 VMs). # 3. Fix MSAN linking. -# 4. Fix "thread_in_wasm flag was not set" crash in debug builds (https://chromium-review.googlesource.com/c/v8/v8/+/2817598). +# 4. Fix "thread_in_wasm flag was not set" crash in debug builds (https://chromium-review.googlesource.com/c/v8/v8/+/2817598, https://chromium-review.googlesource.com/c/v8/v8/+/2867468). --- wee8/build/toolchain/gcc_toolchain.gni +++ wee8/build/toolchain/gcc_toolchain.gni @@ -348,6 +348,8 @@ template("gcc_toolchain") { @@ -109,3 +109,25 @@ return FoundHandler(Context(), wasm_code->instruction_start(), offset, wasm_code->constant_pool(), return_sp, frame->fp()); } +--- wee8/src/compiler/wasm-compiler.cc ++++ wee8/src/compiler/wasm-compiler.cc +@@ -7030,6 +7030,7 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder { + + BuildModifyThreadInWasmFlag(true); + ++ Node* old_effect = effect(); + Node* exception_branch = graph()->NewNode( + mcgraph()->common()->Branch(BranchHint::kTrue), + gasm_->WordEqual(return_value, mcgraph()->IntPtrConstant(0)), +@@ -7046,9 +7047,8 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder { + gasm_->Call(call_descriptor, call_target, return_value); + TerminateThrow(effect(), control()); + +- SetEffectControl( +- return_value, +- graph()->NewNode(mcgraph()->common()->IfTrue(), exception_branch)); ++ SetEffectControl(old_effect, graph()->NewNode(mcgraph()->common()->IfTrue(), ++ exception_branch)); + DCHECK_LT(sig_->return_count(), wasm::kV8MaxWasmFunctionMultiReturns); + size_t return_count = sig_->return_count(); + if (return_count == 0) { From f17cd4532a03e6111ea8e070250ab403e0f02f10 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Fri, 14 May 2021 08:59:51 +0000 Subject: [PATCH 3/3] review: add TODO. Signed-off-by: Piotr Sikora --- bazel/external/wee8.patch | 1 + 1 file changed, 1 insertion(+) diff --git a/bazel/external/wee8.patch b/bazel/external/wee8.patch index 6d7ce1f090442..9aac1bea90c64 100644 --- a/bazel/external/wee8.patch +++ b/bazel/external/wee8.patch @@ -1,6 +1,7 @@ # 1. Fix linking with unbundled toolchain on macOS. # 2. Increase VSZ limit to 64 TiB (allows us to start up to 6,553 VMs). # 3. Fix building and linking with MSAN. +# TODO(PiotrSikora): remove when not needed anymore (most likely in v9.2 branch): # 4. Fix "thread_in_wasm flag was not set" crash in debug builds (https://chromium-review.googlesource.com/c/v8/v8/+/2817598, https://chromium-review.googlesource.com/c/v8/v8/+/2867468). --- wee8/build/toolchain/gcc_toolchain.gni +++ wee8/build/toolchain/gcc_toolchain.gni