From 4fc90a2597787670baebdc3574fc4dd659b2653d Mon Sep 17 00:00:00 2001 From: Andreas Haas Date: Fri, 26 Oct 2018 13:04:26 +0200 Subject: [PATCH] [wasm] Refactor trap-handler to allow an extension to windows This CL refactors the existing trap handler code for Linux to allow a cleaner extension to Windows. 1) The CL extracts platform-specific code into separate files, see https://docs.google.com/document/d/1HCgKIpdjy_CEodTLvZ5VuykDI6gGTHrTtau2j0zwm28. Specifically this means: * Move posix-specific API functions from v8.h to v8-wasm-trap-handler-posix.h. Deprecate the existing TryHandleSignal API function. * Move posix-specific function declarations from trap-handler-internal.h to handler-inside-posix.h * Move posix-specific function definitions from handler-shared.cc to handler-outside-posix.cc 2) The CL changes filenames from *-linux.* to *-posix.*. I expect that most of the implementation for MacOS will be the same as for Linux. Bug: v8:6743 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I4bb7f199564a2f01042084d15a82311d11a93c7b Reviewed-on: https://chromium-review.googlesource.com/c/1280324 Commit-Queue: Andreas Haas Reviewed-by: Ben Titzer Reviewed-by: Ulan Degenbaev Cr-Commit-Position: refs/heads/master@{#57028} --- BUILD.gn | 10 +++++-- include/v8-wasm-trap-handler-posix.h | 29 +++++++++++++++++++ include/v8.h | 4 ++- src/DEPS | 1 + src/api.cc | 16 ++++++++-- ...nside-linux.cc => handler-inside-posix.cc} | 15 +++++----- src/trap-handler/handler-inside-posix.h | 22 ++++++++++++++ ...side-linux.cc => handler-outside-posix.cc} | 21 +++++++++++++- src/trap-handler/handler-outside.cc | 1 - src/trap-handler/handler-shared.cc | 15 ---------- src/trap-handler/trap-handler-internal.h | 11 ------- src/trap-handler/trap-handler.h | 11 +------ test/unittests/wasm/trap-handler-unittest.cc | 2 +- 13 files changed, 106 insertions(+), 52 deletions(-) create mode 100644 include/v8-wasm-trap-handler-posix.h rename src/trap-handler/{handler-inside-linux.cc => handler-inside-posix.cc} (92%) create mode 100644 src/trap-handler/handler-inside-posix.h rename src/trap-handler/{handler-outside-linux.cc => handler-outside-posix.cc} (82%) diff --git a/BUILD.gn b/BUILD.gn index c15c7d7afd..52495f9921 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1524,6 +1524,10 @@ v8_header_set("v8_headers") { "include/v8config.h", ] + if (is_linux) { + sources += [ "include/v8-wasm-trap-handler-posix.h" ] + } + deps = [ ":v8_version", ] @@ -1547,6 +1551,7 @@ v8_source_set("v8_base") { "include/v8-profiler.h", "include/v8-testing.h", "include/v8-util.h", + "include/v8-wasm-trap-handler-posix.h", "include/v8.h", "include/v8config.h", "src/accessors.cc", @@ -2743,8 +2748,9 @@ v8_source_set("v8_base") { ] if (is_linux) { sources += [ - "src/trap-handler/handler-inside-linux.cc", - "src/trap-handler/handler-outside-linux.cc", + "src/trap-handler/handler-inside-posix.cc", + "src/trap-handler/handler-inside-posix.h", + "src/trap-handler/handler-outside-posix.cc", ] } if (is_win) { diff --git a/include/v8-wasm-trap-handler-posix.h b/include/v8-wasm-trap-handler-posix.h new file mode 100644 index 0000000000..0db2e59ec5 --- /dev/null +++ b/include/v8-wasm-trap-handler-posix.h @@ -0,0 +1,29 @@ +// Copyright 2018 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef V8_WASM_TRAP_HANDLER_POSIX_H_ +#define V8_WASM_TRAP_HANDLER_POSIX_H_ + +#include + +namespace v8 { +/** + * This function determines whether a memory access violation has been an + * out-of-bounds memory access in WebAssembly. If so, it will modify the context + * parameter and add a return address where the execution can continue after the + * signal handling, and return true. Otherwise, false will be returned. + * + * The parameters to this function correspond to those passed to a Posix signal + * handler. Use this function only on Linux and Mac. + * + * \param sig_code The signal code, e.g. SIGSEGV. + * \param info A pointer to the siginfo_t struct provided to the signal handler. + * \param context A pointer to a ucontext_t struct provided to the signal + * handler. + */ +bool TryHandleWebAssemblyTrapPosix(int sig_code, siginfo_t* info, + void* context); + +} // namespace v8 +#endif // V8_WASM_TRAP_HANDLER_POSIX_H_ diff --git a/include/v8.h b/include/v8.h index fb30bdd5c0..cbf75afbe8 100644 --- a/include/v8.h +++ b/include/v8.h @@ -8460,7 +8460,9 @@ class V8_EXPORT V8 { * \param context The third argument passed to the Linux signal handler, which * points to a ucontext_t structure. */ - static bool TryHandleSignal(int signal_number, void* info, void* context); + V8_DEPRECATE_SOON("Use TryHandleWebAssemblyTrapPosix", + static bool TryHandleSignal(int signal_number, void* info, + void* context)); #endif // V8_OS_POSIX /** diff --git a/src/DEPS b/src/DEPS index 99873803c9..03454d6dee 100644 --- a/src/DEPS +++ b/src/DEPS @@ -27,6 +27,7 @@ include_rules = [ "+src/interpreter/interpreter-generator.h", "+src/interpreter/setup-interpreter.h", "-src/trap-handler", + "+src/trap-handler/handler-inside-posix.h", "+src/trap-handler/trap-handler.h", "+testing/gtest/include/gtest/gtest_prod.h", "-src/libplatform", diff --git a/src/api.cc b/src/api.cc index 00543eaa3d..132488cf16 100644 --- a/src/api.cc +++ b/src/api.cc @@ -101,6 +101,11 @@ #include "src/wasm/wasm-result.h" #include "src/wasm/wasm-serialization.h" +#ifdef V8_OS_POSIX +#include +#include "src/trap-handler/handler-inside-posix.h" +#endif + namespace v8 { /* @@ -5887,14 +5892,19 @@ bool v8::V8::Initialize() { } #if V8_OS_POSIX -bool V8::TryHandleSignal(int signum, void* info, void* context) { +bool TryHandleWebAssemblyTrapPosix(int sig_code, siginfo_t* info, + void* context) { #if V8_OS_LINUX && V8_TARGET_ARCH_X64 && !V8_OS_ANDROID - return v8::internal::trap_handler::TryHandleSignal( - signum, static_cast(info), static_cast(context)); + return i::trap_handler::TryHandleSignal(sig_code, info, context); #else // V8_OS_LINUX && V8_TARGET_ARCH_X64 && !V8_OS_ANDROID return false; #endif } + +bool V8::TryHandleSignal(int signum, void* info, void* context) { + return TryHandleWebAssemblyTrapPosix( + signum, reinterpret_cast(info), context); +} #endif bool V8::RegisterDefaultSignalHandler() { diff --git a/src/trap-handler/handler-inside-linux.cc b/src/trap-handler/handler-inside-posix.cc similarity index 92% rename from src/trap-handler/handler-inside-linux.cc rename to src/trap-handler/handler-inside-posix.cc index 867f90bfe7..75ada35738 100644 --- a/src/trap-handler/handler-inside-linux.cc +++ b/src/trap-handler/handler-inside-posix.cc @@ -23,6 +23,8 @@ // context. Some additional code is used both inside and outside the signal // handler. This code can be found in handler-shared.cc. +#include "src/trap-handler/handler-inside-posix.h" + #include #include #include @@ -59,7 +61,7 @@ class SigUnmaskStack { void operator=(const SigUnmaskStack&) = delete; }; -bool TryHandleSignal(int signum, siginfo_t* info, ucontext_t* context) { +bool TryHandleSignal(int signum, siginfo_t* info, void* context) { // Bail out early in case we got called for the wrong kind of signal. if (signum != SIGSEGV) { return false; @@ -91,11 +93,12 @@ bool TryHandleSignal(int signum, siginfo_t* info, ucontext_t* context) { sigaddset(&sigs, SIGSEGV); SigUnmaskStack unmask(sigs); - uintptr_t fault_addr = context->uc_mcontext.gregs[REG_RIP]; + ucontext_t* uc = reinterpret_cast(context); + uintptr_t fault_addr = uc->uc_mcontext.gregs[REG_RIP]; uintptr_t landing_pad = 0; if (TryFindLandingPad(fault_addr, &landing_pad)) { // Tell the caller to return to the landing pad. - context->uc_mcontext.gregs[REG_RIP] = landing_pad; + uc->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; @@ -109,9 +112,7 @@ bool TryHandleSignal(int signum, siginfo_t* info, ucontext_t* context) { } void HandleSignal(int signum, siginfo_t* info, void* context) { - ucontext_t* uc = reinterpret_cast(context); - - if (!TryHandleSignal(signum, info, uc)) { + if (!TryHandleSignal(signum, info, context)) { // Since V8 didn't handle this signal, we want to re-raise the same signal. // For kernel-generated SEGV signals, we do this by restoring the original // SEGV handler and then returning. The fault will happen again and the @@ -120,7 +121,7 @@ void HandleSignal(int signum, siginfo_t* info, void* context) { // We handle user-generated signals by calling raise() instead. This is for // completeness. We should never actually see one of these, but just in // case, we do the right thing. - RestoreOriginalSignalHandler(); + RemoveTrapHandler(); if (!IsKernelGeneratedSignal(info)) { raise(signum); } diff --git a/src/trap-handler/handler-inside-posix.h b/src/trap-handler/handler-inside-posix.h new file mode 100644 index 0000000000..f88ea60d81 --- /dev/null +++ b/src/trap-handler/handler-inside-posix.h @@ -0,0 +1,22 @@ +// Copyright 2018 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef V8_TRAP_HANDLER_HANDLER_INSIDE_POSIX_H_ +#define V8_TRAP_HANDLER_HANDLER_INSIDE_POSIX_H_ + +#include + +namespace v8 { +namespace internal { +namespace trap_handler { + +void HandleSignal(int signum, siginfo_t* info, void* context); + +bool TryHandleSignal(int signum, siginfo_t* info, void* context); + +} // namespace trap_handler +} // namespace internal +} // namespace v8 + +#endif // V8_TRAP_HANDLER_HANDLER_INSIDE_POSIX_H_ diff --git a/src/trap-handler/handler-outside-linux.cc b/src/trap-handler/handler-outside-posix.cc similarity index 82% rename from src/trap-handler/handler-outside-linux.cc rename to src/trap-handler/handler-outside-posix.cc index 34f3983315..7bc64fd0f2 100644 --- a/src/trap-handler/handler-outside-linux.cc +++ b/src/trap-handler/handler-outside-posix.cc @@ -21,14 +21,23 @@ #include +#include "src/trap-handler/handler-inside-posix.h" #include "src/trap-handler/trap-handler-internal.h" -#include "src/trap-handler/trap-handler.h" namespace v8 { namespace internal { namespace trap_handler { #if V8_TRAP_HANDLER_SUPPORTED +namespace { +struct sigaction g_old_handler; + +// When using the default signal handler, we save the old one to restore in case +// V8 chooses not to handle the signal. +bool g_is_default_signal_handler_registered; + +} // namespace + bool RegisterDefaultTrapHandler() { CHECK(!g_is_default_signal_handler_registered); @@ -66,7 +75,17 @@ bool RegisterDefaultTrapHandler() { g_is_default_signal_handler_registered = true; return true; } +#endif // V8_TRAP_HANDLER_SUPPORTED + +void RemoveTrapHandler() { +#if V8_TRAP_HANDLER_SUPPORTED + if (g_is_default_signal_handler_registered) { + if (sigaction(SIGSEGV, &g_old_handler, nullptr) == 0) { + g_is_default_signal_handler_registered = false; + } + } #endif +} } // namespace trap_handler } // namespace internal diff --git a/src/trap-handler/handler-outside.cc b/src/trap-handler/handler-outside.cc index 2d75d2d7e4..4d29238f84 100644 --- a/src/trap-handler/handler-outside.cc +++ b/src/trap-handler/handler-outside.cc @@ -19,7 +19,6 @@ // // For the code that runs in the signal handler itself, see handler-inside.cc. -#include #include #include #include diff --git a/src/trap-handler/handler-shared.cc b/src/trap-handler/handler-shared.cc index d07f7ae131..1d245d0a91 100644 --- a/src/trap-handler/handler-shared.cc +++ b/src/trap-handler/handler-shared.cc @@ -28,21 +28,6 @@ namespace trap_handler { // 1 byte in size; see https://sourceware.org/bugzilla/show_bug.cgi?id=14898. THREAD_LOCAL int g_thread_in_wasm_code; -#if V8_TRAP_HANDLER_SUPPORTED -// When using the default signal handler, we save the old one to restore in case -// V8 chooses not to handle the signal. -struct sigaction g_old_handler; -bool g_is_default_signal_handler_registered; -#endif - -V8_EXPORT_PRIVATE void RestoreOriginalSignalHandler() { -#if V8_TRAP_HANDLER_SUPPORTED - if (sigaction(SIGSEGV, &g_old_handler, nullptr) == 0) { - g_is_default_signal_handler_registered = false; - } -#endif -} - static_assert(sizeof(g_thread_in_wasm_code) > 1, "sizeof(thread_local_var) must be > 1, see " "https://sourceware.org/bugzilla/show_bug.cgi?id=14898"); diff --git a/src/trap-handler/trap-handler-internal.h b/src/trap-handler/trap-handler-internal.h index 66ae4f652a..e41e76bc8a 100644 --- a/src/trap-handler/trap-handler-internal.h +++ b/src/trap-handler/trap-handler-internal.h @@ -41,10 +41,6 @@ class MetadataLock { void operator=(const MetadataLock&) = delete; }; -#if V8_TRAP_HANDLER_SUPPORTED -void HandleSignal(int signum, siginfo_t* info, void* context); -#endif - // To enable constant time registration of handler data, we keep a free list of // entries in the gCodeObjects table. Each entry contains a {next_free} field, // which can be used to figure out where the next entry should be inserted. @@ -68,13 +64,6 @@ extern std::atomic_size_t gRecoveredTrapCount; // unchanged. bool TryFindLandingPad(uintptr_t fault_addr, uintptr_t* landing_pad); -#if V8_TRAP_HANDLER_SUPPORTED -// When using the default signal handler, we save the old one to restore in case -// V8 chooses not to handle the signal. -extern struct sigaction g_old_handler; -extern bool g_is_default_signal_handler_registered; -#endif - } // namespace trap_handler } // namespace internal } // namespace v8 diff --git a/src/trap-handler/trap-handler.h b/src/trap-handler/trap-handler.h index 0e9dbf248c..a2cfda8223 100644 --- a/src/trap-handler/trap-handler.h +++ b/src/trap-handler/trap-handler.h @@ -5,7 +5,6 @@ #ifndef V8_TRAP_HANDLER_TRAP_HANDLER_H_ #define V8_TRAP_HANDLER_TRAP_HANDLER_H_ -#include #include #include @@ -13,10 +12,6 @@ #include "src/flags.h" #include "src/globals.h" -#if V8_OS_LINUX -#include -#endif - namespace v8 { namespace internal { namespace trap_handler { @@ -101,11 +96,7 @@ inline void ClearThreadInWasm() { } bool RegisterDefaultTrapHandler(); -V8_EXPORT_PRIVATE void RestoreOriginalSignalHandler(); - -#if V8_OS_LINUX -bool TryHandleSignal(int signum, siginfo_t* info, ucontext_t* context); -#endif // V8_OS_LINUX +V8_EXPORT_PRIVATE void RemoveTrapHandler(); size_t GetRecoveredTrapCount(); diff --git a/test/unittests/wasm/trap-handler-unittest.cc b/test/unittests/wasm/trap-handler-unittest.cc index 07e3ca888d..7d4dc4df2a 100644 --- a/test/unittests/wasm/trap-handler-unittest.cc +++ b/test/unittests/wasm/trap-handler-unittest.cc @@ -58,7 +58,7 @@ TEST_F(SignalHandlerFallbackTest, DoTest) { FAIL(); } else { // Our signal handler ran. - v8::internal::trap_handler::RestoreOriginalSignalHandler(); + v8::internal::trap_handler::RemoveTrapHandler(); SUCCEED(); return; }