From 0738f0f6685ec84daefbb3db5f3328143b96fda1 Mon Sep 17 00:00:00 2001 From: Clemens Hammacher Date: Thu, 12 Oct 2017 15:32:14 +0200 Subject: [PATCH] [wasm] Move "thread in wasm" flag handling out of compiled code Instead of modifying this flag in compiled wasm code, we can just change it in the caller / called code. This saves code space and compilation time and fixes the referenced bug. R=titzer@chromium.org, eholk@chromium.org Bug: chromium:773631, v8:5277 Change-Id: I095158ac01eecd21a92649a3990e8d7c593db912 Reviewed-on: https://chromium-review.googlesource.com/712597 Reviewed-by: Ben Titzer Reviewed-by: Eric Holk Commit-Queue: Clemens Hammacher Cr-Commit-Position: refs/heads/master@{#48602} --- src/compiler/wasm-compiler.cc | 27 ++------------------------ src/runtime/runtime-wasm.cc | 31 +++++++++++++++++++++++++----- src/trap-handler/handler-inside.cc | 6 ++++-- src/wasm/wasm-interpreter.cc | 11 ++++++++++- 4 files changed, 42 insertions(+), 33 deletions(-) diff --git a/src/compiler/wasm-compiler.cc b/src/compiler/wasm-compiler.cc index 7bfa42f713..32f6b95e55 100644 --- a/src/compiler/wasm-compiler.cc +++ b/src/compiler/wasm-compiler.cc @@ -2796,8 +2796,8 @@ void WasmGraphBuilder::BuildJSToWasmWrapper(Handle wasm_code, // Throw a TypeError. Use the js_context of the calling javascript function // (passed as a parameter), such that the generated code is js_context // independent. - BuildCallToRuntimeWithContextFromJS(Runtime::kWasmThrowTypeError, - js_context, nullptr, 0); + BuildCallToRuntimeWithContext(Runtime::kWasmThrowTypeError, js_context, + nullptr, 0); // Add a dummy call to the wasm function so that the generated wrapper // contains a reference to the wrapped wasm function. Without this reference @@ -3133,9 +3133,6 @@ void WasmGraphBuilder::BuildCWasmEntry(Address wasm_context_address) { Node* code_obj = Param(CWasmEntryParameters::kCodeObject + 1); Node* arg_buffer = Param(CWasmEntryParameters::kArgumentsBuffer + 1); - // Set the ThreadInWasm flag before we do the actual call. - BuildModifyThreadInWasmFlag(true); - int wasm_arg_count = static_cast(sig_->parameter_count()); int arg_count = wasm_arg_count + 4; // code, wasm_context, control, effect Node** args = Buffer(arg_count); @@ -3165,9 +3162,6 @@ void WasmGraphBuilder::BuildCWasmEntry(Address wasm_context_address) { graph()->NewNode(jsgraph()->common()->Call(desc), arg_count, args); *effect_ = call; - // Clear the ThreadInWasmFlag - BuildModifyThreadInWasmFlag(false); - // Store the return value. DCHECK_GE(1, sig_->return_count()); if (sig_->return_count() == 1) { @@ -3320,23 +3314,6 @@ Node* WasmGraphBuilder::BuildCallToRuntimeWithContext(Runtime::FunctionId f, Node* js_context, Node** parameters, int parameter_count) { - // We're leaving Wasm code, so clear the flag. - *control_ = BuildModifyThreadInWasmFlag(false); - // Since the thread-in-wasm flag is clear, it is as if we are calling from JS. - Node* call = BuildCallToRuntimeWithContextFromJS(f, js_context, parameters, - parameter_count); - - // Restore the thread-in-wasm flag, since we have returned to Wasm. - *control_ = BuildModifyThreadInWasmFlag(true); - - return call; -} - -// This version of BuildCallToRuntime does not clear and set the thread-in-wasm -// flag. -Node* WasmGraphBuilder::BuildCallToRuntimeWithContextFromJS( - Runtime::FunctionId f, Node* js_context, Node* const* parameters, - int parameter_count) { const Runtime::Function* fun = Runtime::FunctionForId(f); CallDescriptor* desc = Linkage::GetRuntimeCallDescriptor( jsgraph()->zone(), f, fun->nargs, Operator::kNoProperties, diff --git a/src/runtime/runtime-wasm.cc b/src/runtime/runtime-wasm.cc index d37489a3c0..039a974417 100644 --- a/src/runtime/runtime-wasm.cc +++ b/src/runtime/runtime-wasm.cc @@ -41,6 +41,24 @@ Context* GetWasmContextOnStackTop(Isolate* isolate) { ->compiled_module() ->ptr_to_native_context(); } + +class ClearThreadInWasmScope { + public: + explicit ClearThreadInWasmScope(bool coming_from_wasm) + : coming_from_wasm_(coming_from_wasm) { + DCHECK_EQ(trap_handler::UseTrapHandler() && coming_from_wasm, + trap_handler::IsThreadInWasm()); + if (coming_from_wasm) trap_handler::ClearThreadInWasm(); + } + ~ClearThreadInWasmScope() { + DCHECK(!trap_handler::IsThreadInWasm()); + if (coming_from_wasm_) trap_handler::SetThreadInWasm(); + } + + private: + const bool coming_from_wasm_; +}; + } // namespace RUNTIME_FUNCTION(Runtime_WasmGrowMemory) { @@ -50,6 +68,9 @@ RUNTIME_FUNCTION(Runtime_WasmGrowMemory) { Handle instance(GetWasmInstanceOnStackTop(isolate), isolate); + // This runtime function is always being called from wasm code. + ClearThreadInWasmScope flag_scope(true); + // Set the current isolate's context. DCHECK_NULL(isolate->context()); isolate->set_context(instance->compiled_module()->ptr_to_native_context()); @@ -110,6 +131,7 @@ Object* ThrowRuntimeError(Isolate* isolate, int message_id, int byte_offset, RUNTIME_FUNCTION(Runtime_ThrowWasmErrorFromTrapIf) { DCHECK_EQ(1, args.length()); CONVERT_SMI_ARG_CHECKED(message_id, 0); + ClearThreadInWasmScope clear_wasm_flag(isolate->context() == nullptr); return ThrowRuntimeError(isolate, message_id, 0, false); } @@ -117,6 +139,7 @@ RUNTIME_FUNCTION(Runtime_ThrowWasmError) { DCHECK_EQ(2, args.length()); CONVERT_SMI_ARG_CHECKED(message_id, 0); CONVERT_SMI_ARG_CHECKED(byte_offset, 1); + ClearThreadInWasmScope clear_wasm_flag(isolate->context() == nullptr); return ThrowRuntimeError(isolate, message_id, byte_offset, true); } @@ -266,6 +289,8 @@ RUNTIME_FUNCTION(Runtime_WasmRunInterpreter) { CHECK(arg_buffer_obj->IsSmi()); uint8_t* arg_buffer = reinterpret_cast(*arg_buffer_obj); + ClearThreadInWasmScope wasm_flag(true); + // Set the current isolate's context. DCHECK_NULL(isolate->context()); isolate->set_context(instance->compiled_module()->ptr_to_native_context()); @@ -297,11 +322,7 @@ RUNTIME_FUNCTION(Runtime_WasmStackGuard) { DCHECK_EQ(0, args.length()); DCHECK(!trap_handler::UseTrapHandler() || trap_handler::IsThreadInWasm()); - struct ClearAndRestoreThreadInWasm { - ClearAndRestoreThreadInWasm() { trap_handler::ClearThreadInWasm(); } - - ~ClearAndRestoreThreadInWasm() { trap_handler::SetThreadInWasm(); } - } restore_thread_in_wasm; + ClearThreadInWasmScope wasm_flag(true); // Set the current isolate's context. DCHECK_NULL(isolate->context()); diff --git a/src/trap-handler/handler-inside.cc b/src/trap-handler/handler-inside.cc index 8a3a48c42b..d3c543f4f4 100644 --- a/src/trap-handler/handler-inside.cc +++ b/src/trap-handler/handler-inside.cc @@ -102,13 +102,15 @@ bool TryHandleSignal(int signum, siginfo_t* info, ucontext_t* context) { if (TryFindLandingPad(fault_addr, &landing_pad)) { // Tell the caller to return to the landing pad. context->uc_mcontext.gregs[REG_RIP] = landing_pad; + // We will return to wasm code, so restore the g_thread_in_wasm_code flag. + g_thread_in_wasm_code = true; return true; } } // end signal mask scope // If we get here, it's not a recoverable wasm fault, so we go to the next - // handler. - g_thread_in_wasm_code = true; + // handler. Leave the g_thread_in_wasm_code flag unset since we do not return + // to wasm code. return false; } diff --git a/src/wasm/wasm-interpreter.cc b/src/wasm/wasm-interpreter.cc index 3123ec6fee..1cce5c9da5 100644 --- a/src/wasm/wasm-interpreter.cc +++ b/src/wasm/wasm-interpreter.cc @@ -2224,9 +2224,18 @@ class ThreadImpl { args[compiler::CWasmEntryParameters::kArgumentsBuffer] = arg_buffer_obj; Handle receiver = isolate->factory()->undefined_value(); + trap_handler::SetThreadInWasm(); MaybeHandle maybe_retval = Execution::Call(isolate, wasm_entry, receiver, arraysize(args), args); - if (maybe_retval.is_null()) return TryHandleException(isolate); + TRACE(" => External wasm function returned%s\n", + maybe_retval.is_null() ? " with exception" : ""); + + if (maybe_retval.is_null()) { + DCHECK(!trap_handler::IsThreadInWasm()); + return TryHandleException(isolate); + } + + trap_handler::ClearThreadInWasm(); // Pop arguments off the stack. sp_ -= num_args;