From fb4ab3be3fc0024d00358d5b7816d3215a8ff20c Mon Sep 17 00:00:00 2001 From: Paolo Severini Date: Tue, 16 Apr 2019 00:17:49 +0000 Subject: [PATCH] V8 x64 backend doesn't emit ABI compliant stack frames On 64 bit Windows, stack walking does not work across stack frames generated by V8 because the V8 x64 backend doesn't emit unwinding info and because it does not emi ABI compliant stack frames. (bug v8:3598). This should be fixed with this CL: https://chromium-review.googlesource.com/c/v8/v8/+/1469329 The fix consists in having V8 register dynamically PDATA/XDATA for the whole code-range address space of an isolate every time a new isolate is initialized, and unregister them when the Isolate is destroyed. A more detailed description of the V8 fix can be found here: https://docs.google.com/document/d/1-wf50jFlii0c_Pr52lm2ZU-49m220nhYMrHDi3vXnh0/edit This V8 changes are currently experimental, behind the v8_win64_unwinding_info build flag and the '--win64-unwinding-info' command line flag. However Crashpad already registers PDATA/XDATA for the code range of a V8 isolate, in order to be able to handle and report unhandled exceptions that have V8 dynamic code in the call stack. For more details, see: https://chromium.googlesource.com/v8/v8.git/+/9b32bb22c1e516a4931ac647656bdf07bd7332be Since it is not possible to register multiple PDATA entries for the same address range, a new functions has been added to the V8 API: - SetUnhandledExceptionCallback() can be used by an embedder to register its own unhandled exception handler for exceptions that arise in V8-generated code. This CL contains a few small changes to use this updated V8 API: Crashpad calls v8::Isolate::SetUnhandledExceptionCallback() to register its own custom exception handler for V8-code. - When the '--win64-unwinding-info' flag is set, V8 will register the specified exception handler as part of the Win64 unwind info, for jitted code and for embedded builtins code. - When the '--win64-unwinding-info' flag is not set, V8 will still register the specified exception handler (but no precise unwind data) for the code range of jitted code only, as Crashpad currently does. Bug: v8:3598 Change-Id: Iba4a724a04a3bc3420c986d3e3b22f3b4aea279a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1474703 Reviewed-by: Jochen Eisinger Reviewed-by: Jeremy Roman Reviewed-by: Ross McIlroy Commit-Queue: Paolo Severini Cr-Commit-Position: refs/heads/master@{#651075} --- chrome/child/v8_crashpad_support_win.cc | 5 +- chrome_elf/chrome_elf_x64.def | 4 - components/crash/content/app/breakpad_win.cc | 70 ---------------- .../crash/content/app/crash_export_stubs.cc | 9 --- .../crash/content/app/crash_export_thunks.cc | 14 ---- .../crash/content/app/crash_export_thunks.h | 7 -- components/crash/content/app/crashpad.h | 6 -- components/crash/content/app/crashpad_win.cc | 80 ------------------- .../shell/common/v8_crashpad_support_win.cc | 16 +--- gin/debug_impl.cc | 26 +----- gin/debug_impl.h | 4 - gin/isolate_holder.cc | 22 ----- gin/public/debug.h | 22 +---- 13 files changed, 10 insertions(+), 275 deletions(-) diff --git a/chrome/child/v8_crashpad_support_win.cc b/chrome/child/v8_crashpad_support_win.cc index d455be6df31f24..e110b18826e38c 100644 --- a/chrome/child/v8_crashpad_support_win.cc +++ b/chrome/child/v8_crashpad_support_win.cc @@ -12,10 +12,7 @@ namespace v8_crashpad_support { void SetUp() { #if defined(ARCH_CPU_X86_64) - gin::Debug::SetCodeRangeCreatedCallback( - &RegisterNonABICompliantCodeRange_ExportThunk); - gin::Debug::SetCodeRangeDeletedCallback( - &UnregisterNonABICompliantCodeRange_ExportThunk); + gin::Debug::SetUnhandledExceptionCallback(&CrashForException_ExportThunk); #endif } diff --git a/chrome_elf/chrome_elf_x64.def b/chrome_elf/chrome_elf_x64.def index 94ca0b213279f4..4d9bd77c3c25d5 100644 --- a/chrome_elf/chrome_elf_x64.def +++ b/chrome_elf/chrome_elf_x64.def @@ -17,10 +17,6 @@ EXPORTS RequestSingleCrashUpload_ExportThunk SetUploadConsent_ExportThunk - ; X64-only exports - RegisterNonABICompliantCodeRange_ExportThunk - UnregisterNonABICompliantCodeRange_ExportThunk - ; From chrome_elf/crash/crash_helper.cc SetMetricsClientId diff --git a/components/crash/content/app/breakpad_win.cc b/components/crash/content/app/breakpad_win.cc index 2231f14e9633fb..d7560691c01d3e 100644 --- a/components/crash/content/app/breakpad_win.cc +++ b/components/crash/content/app/breakpad_win.cc @@ -631,74 +631,4 @@ ClearBreakpadPipeEnvironmentVariable() { env->UnSetVar(kPipeNameVar); } -#ifdef _M_X64 -int CrashForExceptionInNonABICompliantCodeRange( - PEXCEPTION_RECORD ExceptionRecord, - ULONG64 EstablisherFrame, - PCONTEXT ContextRecord, - PDISPATCHER_CONTEXT DispatcherContext) { - EXCEPTION_POINTERS info = { ExceptionRecord, ContextRecord }; - return CrashForException(&info); -} - -struct ExceptionHandlerRecord { - RUNTIME_FUNCTION runtime_function; - UNWIND_INFO unwind_info; - unsigned char thunk[12]; -}; - -extern "C" void __declspec(dllexport) __cdecl -RegisterNonABICompliantCodeRange(void* start, size_t size_in_bytes) { - ExceptionHandlerRecord* record = - reinterpret_cast(start); - - // We assume that the first page of the code range is executable and - // committed and reserved for breakpad. What could possibly go wrong? - - // All addresses are 32bit relative offsets to start. - record->runtime_function.BeginAddress = 0; - record->runtime_function.EndAddress = - base::checked_cast(size_in_bytes); - record->runtime_function.UnwindData = - offsetof(ExceptionHandlerRecord, unwind_info); - - // Create unwind info that only specifies an exception handler. - record->unwind_info.Version = 1; - record->unwind_info.Flags = UNW_FLAG_EHANDLER; - record->unwind_info.SizeOfProlog = 0; - record->unwind_info.CountOfCodes = 0; - record->unwind_info.FrameRegister = 0; - record->unwind_info.FrameOffset = 0; - record->unwind_info.ExceptionHandler = - offsetof(ExceptionHandlerRecord, thunk); - - // Hardcoded thunk. - // mov imm64, rax - record->thunk[0] = 0x48; - record->thunk[1] = 0xb8; - void* handler = - reinterpret_cast(&CrashForExceptionInNonABICompliantCodeRange); - memcpy(&record->thunk[2], &handler, 8); - - // jmp rax - record->thunk[10] = 0xff; - record->thunk[11] = 0xe0; - - // Protect reserved page against modifications. - DWORD old_protect; - CHECK(VirtualProtect( - start, sizeof(ExceptionHandlerRecord), PAGE_EXECUTE_READ, &old_protect)); - CHECK(RtlAddFunctionTable( - &record->runtime_function, 1, reinterpret_cast(start))); -} - -extern "C" void __declspec(dllexport) __cdecl -UnregisterNonABICompliantCodeRange(void* start) { - ExceptionHandlerRecord* record = - reinterpret_cast(start); - - CHECK(RtlDeleteFunctionTable(&record->runtime_function)); -} -#endif - } // namespace breakpad diff --git a/components/crash/content/app/crash_export_stubs.cc b/components/crash/content/app/crash_export_stubs.cc index a4080e3c6a9652..4f9ff3cb4f7d0f 100644 --- a/components/crash/content/app/crash_export_stubs.cc +++ b/components/crash/content/app/crash_export_stubs.cc @@ -41,12 +41,3 @@ bool DumpHungProcessWithPtype_ExportThunk(HANDLE process_handle, const char* ptype) { return false; } - -#if defined(ARCH_CPU_X86_64) - -void RegisterNonABICompliantCodeRange_ExportThunk(void* start, - size_t size_in_bytes) {} - -void UnregisterNonABICompliantCodeRange_ExportThunk(void* start) {} - -#endif // defined(ARCH_CPU_X86_64) diff --git a/components/crash/content/app/crash_export_thunks.cc b/components/crash/content/app/crash_export_thunks.cc index 039f1806256b5d..f2f88878bda215 100644 --- a/components/crash/content/app/crash_export_thunks.cc +++ b/components/crash/content/app/crash_export_thunks.cc @@ -76,17 +76,3 @@ bool DumpHungProcessWithPtype_ExportThunk(HANDLE process_handle, return crash_reporter::DumpHungProcessWithPtypeImpl(process, ptype); } - -#if defined(ARCH_CPU_X86_64) - -void RegisterNonABICompliantCodeRange_ExportThunk(void* start, - size_t size_in_bytes) { - crash_reporter::internal::RegisterNonABICompliantCodeRangeImpl(start, - size_in_bytes); -} - -void UnregisterNonABICompliantCodeRange_ExportThunk(void* start) { - crash_reporter::internal::UnregisterNonABICompliantCodeRangeImpl(start); -} - -#endif // ARCH_CPU_X86_64 diff --git a/components/crash/content/app/crash_export_thunks.h b/components/crash/content/app/crash_export_thunks.h index 85c5c03e794385..5e9b7aa05c107e 100644 --- a/components/crash/content/app/crash_export_thunks.h +++ b/components/crash/content/app/crash_export_thunks.h @@ -68,13 +68,6 @@ void ClearReportsBetween_ExportThunk(time_t begin, time_t end); // |process|. bool DumpHungProcessWithPtype_ExportThunk(HANDLE process, const char* ptype); -#if defined(ARCH_CPU_X86_64) -// V8 support functions. -void RegisterNonABICompliantCodeRange_ExportThunk(void* start, - size_t size_in_bytes); -void UnregisterNonABICompliantCodeRange_ExportThunk(void* start); -#endif // defined(ARCH_CPU_X86_64) - } // extern "C" #endif // COMPONENTS_CRASH_CONTENT_APP_CRASH_EXPORT_THUNKS_H_ diff --git a/components/crash/content/app/crashpad.h b/components/crash/content/app/crashpad.h index 2a2675cf6f0e71..2324c1eb56c420 100644 --- a/components/crash/content/app/crashpad.h +++ b/components/crash/content/app/crashpad.h @@ -184,12 +184,6 @@ void GetPlatformCrashpadAnnotations( // target process. DWORD WINAPI DumpProcessForHungInputThread(void* param); -#if defined(ARCH_CPU_X86_64) -// V8 support functions. -void RegisterNonABICompliantCodeRangeImpl(void* start, size_t size_in_bytes); -void UnregisterNonABICompliantCodeRangeImpl(void* start); -#endif // defined(ARCH_CPU_X86_64) - #endif // defined(OS_WIN) #if defined(OS_LINUX) || defined(OS_ANDROID) diff --git a/components/crash/content/app/crashpad_win.cc b/components/crash/content/app/crashpad_win.cc index cc16c40ea320bb..8f55759a6f4a62 100644 --- a/components/crash/content/app/crashpad_win.cc +++ b/components/crash/content/app/crashpad_win.cc @@ -172,85 +172,5 @@ NOINLINE DWORD WINAPI DumpProcessForHungInputThread(void* param) { return 0; } -#if defined(ARCH_CPU_X86_64) - -static int CrashForExceptionInNonABICompliantCodeRange( - PEXCEPTION_RECORD ExceptionRecord, - ULONG64 EstablisherFrame, - PCONTEXT ContextRecord, - PDISPATCHER_CONTEXT DispatcherContext) { - EXCEPTION_POINTERS info = { ExceptionRecord, ContextRecord }; - return CrashForException_ExportThunk(&info); -} - -// See https://msdn.microsoft.com/en-us/library/ddssxxy8.aspx -typedef struct _UNWIND_INFO { - unsigned char Version : 3; - unsigned char Flags : 5; - unsigned char SizeOfProlog; - unsigned char CountOfCodes; - unsigned char FrameRegister : 4; - unsigned char FrameOffset : 4; - ULONG ExceptionHandler; -} UNWIND_INFO, *PUNWIND_INFO; - -struct ExceptionHandlerRecord { - RUNTIME_FUNCTION runtime_function; - UNWIND_INFO unwind_info; - unsigned char thunk[12]; -}; - -void RegisterNonABICompliantCodeRangeImpl(void* start, size_t size_in_bytes) { - ExceptionHandlerRecord* record = - reinterpret_cast(start); - - // We assume that the first page of the code range is executable and - // committed and reserved for breakpad. What could possibly go wrong? - - // All addresses are 32bit relative offsets to start. - record->runtime_function.BeginAddress = 0; - record->runtime_function.EndAddress = - base::checked_cast(size_in_bytes); - record->runtime_function.UnwindData = - offsetof(ExceptionHandlerRecord, unwind_info); - - // Create unwind info that only specifies an exception handler. - record->unwind_info.Version = 1; - record->unwind_info.Flags = UNW_FLAG_EHANDLER; - record->unwind_info.SizeOfProlog = 0; - record->unwind_info.CountOfCodes = 0; - record->unwind_info.FrameRegister = 0; - record->unwind_info.FrameOffset = 0; - record->unwind_info.ExceptionHandler = - offsetof(ExceptionHandlerRecord, thunk); - - // Hardcoded thunk. - // mov imm64, rax - record->thunk[0] = 0x48; - record->thunk[1] = 0xb8; - void* handler = - reinterpret_cast(&CrashForExceptionInNonABICompliantCodeRange); - memcpy(&record->thunk[2], &handler, 8); - - // jmp rax - record->thunk[10] = 0xff; - record->thunk[11] = 0xe0; - - // Protect reserved page against modifications. - DWORD old_protect; - CHECK(VirtualProtect( - start, sizeof(ExceptionHandlerRecord), PAGE_EXECUTE_READ, &old_protect)); - CHECK(RtlAddFunctionTable( - &record->runtime_function, 1, reinterpret_cast(start))); -} - -void UnregisterNonABICompliantCodeRangeImpl(void* start) { - ExceptionHandlerRecord* record = - reinterpret_cast(start); - - CHECK(RtlDeleteFunctionTable(&record->runtime_function)); -} -#endif // ARCH_CPU_X86_64 - } // namespace internal } // namespace crash_reporter diff --git a/content/shell/common/v8_crashpad_support_win.cc b/content/shell/common/v8_crashpad_support_win.cc index 33eab60a86dddd..5c1e8ab4bc3192 100644 --- a/content/shell/common/v8_crashpad_support_win.cc +++ b/content/shell/common/v8_crashpad_support_win.cc @@ -5,27 +5,15 @@ #include "content/shell/common/v8_crashpad_support_win.h" #include - #include "base/logging.h" +#include "components/crash/content/app/crash_export_thunks.h" #include "gin/public/debug.h" namespace v8_crashpad_support { void SetUp() { #ifdef _WIN64 - // Get the breakpad pointer from content_shell.exe - gin::Debug::CodeRangeCreatedCallback create_callback = - reinterpret_cast( - ::GetProcAddress(::GetModuleHandle(L"content_shell.exe"), - "RegisterNonABICompliantCodeRange")); - gin::Debug::CodeRangeDeletedCallback delete_callback = - reinterpret_cast( - ::GetProcAddress(::GetModuleHandle(L"content_shell.exe"), - "UnregisterNonABICompliantCodeRange")); - if (create_callback && delete_callback) { - gin::Debug::SetCodeRangeCreatedCallback(create_callback); - gin::Debug::SetCodeRangeDeletedCallback(delete_callback); - } + gin::Debug::SetUnhandledExceptionCallback(&CrashForException_ExportThunk); #endif } diff --git a/gin/debug_impl.cc b/gin/debug_impl.cc index 5c3b7ffc932f06..ca0577ea4caabc 100644 --- a/gin/debug_impl.cc +++ b/gin/debug_impl.cc @@ -8,10 +8,6 @@ namespace gin { namespace { v8::JitCodeEventHandler g_jit_code_event_handler = NULL; -#if defined(OS_WIN) -Debug::CodeRangeCreatedCallback g_code_range_created_callback = NULL; -Debug::CodeRangeDeletedCallback g_code_range_deleted_callback = NULL; -#endif } // namespace // static @@ -21,13 +17,9 @@ void Debug::SetJitCodeEventHandler(v8::JitCodeEventHandler event_handler) { #if defined(OS_WIN) // static -void Debug::SetCodeRangeCreatedCallback(CodeRangeCreatedCallback callback) { - g_code_range_created_callback = callback; -} - -// static -void Debug::SetCodeRangeDeletedCallback(CodeRangeDeletedCallback callback) { - g_code_range_deleted_callback = callback; +void Debug::SetUnhandledExceptionCallback( + v8::UnhandledExceptionCallback callback) { + v8::V8::SetUnhandledExceptionCallback(callback); } #endif @@ -36,16 +28,4 @@ v8::JitCodeEventHandler DebugImpl::GetJitCodeEventHandler() { return g_jit_code_event_handler; } -#if defined(OS_WIN) -// static -Debug::CodeRangeCreatedCallback DebugImpl::GetCodeRangeCreatedCallback() { - return g_code_range_created_callback; -} - -// static -Debug::CodeRangeDeletedCallback DebugImpl::GetCodeRangeDeletedCallback() { - return g_code_range_deleted_callback; -} -#endif - } // namespace gin diff --git a/gin/debug_impl.h b/gin/debug_impl.h index b88c0b6c0896f6..b0b7931f3e1059 100644 --- a/gin/debug_impl.h +++ b/gin/debug_impl.h @@ -13,10 +13,6 @@ namespace gin { class DebugImpl { public: static v8::JitCodeEventHandler GetJitCodeEventHandler(); -#if defined(OS_WIN) - static Debug::CodeRangeCreatedCallback GetCodeRangeCreatedCallback(); - static Debug::CodeRangeDeletedCallback GetCodeRangeDeletedCallback(); -#endif }; } // namespace gin diff --git a/gin/isolate_holder.cc b/gin/isolate_holder.cc index 5577bbbb4eb0a9..5f8aea7105a188 100644 --- a/gin/isolate_holder.cc +++ b/gin/isolate_holder.cc @@ -86,31 +86,9 @@ IsolateHolder::IsolateHolder( isolate_memory_dump_provider_.reset( new V8IsolateMemoryDumpProvider(this, task_runner)); -#if defined(OS_WIN) - { - void* code_range; - size_t size; - isolate_->GetCodeRange(&code_range, &size); - Debug::CodeRangeCreatedCallback callback = - DebugImpl::GetCodeRangeCreatedCallback(); - if (code_range && size && callback) - callback(code_range, size); - } -#endif } IsolateHolder::~IsolateHolder() { -#if defined(OS_WIN) - { - void* code_range; - size_t size; - isolate_->GetCodeRange(&code_range, &size); - Debug::CodeRangeDeletedCallback callback = - DebugImpl::GetCodeRangeDeletedCallback(); - if (code_range && callback) - callback(code_range); - } -#endif isolate_memory_dump_provider_.reset(); isolate_data_.reset(); isolate_->Dispose(); diff --git a/gin/public/debug.h b/gin/public/debug.h index 8c2eee341c3bc6..4e567876f7ac0f 100644 --- a/gin/public/debug.h +++ b/gin/public/debug.h @@ -24,25 +24,11 @@ class GIN_EXPORT Debug { static void SetJitCodeEventHandler(v8::JitCodeEventHandler event_handler); #if defined(OS_WIN) - typedef void (__cdecl *CodeRangeCreatedCallback)(void*, size_t); - - /* Sets a callback that is invoked for every new code range being created. - * - * On Win64, exception handling in jitted code is broken due to the fact - * that JS stack frames are not ABI compliant. It is possible to install - * custom handlers for the code range which holds the jitted code to work - * around this issue. - * - * https://code.google.com/p/v8/issues/detail?id=3598 - */ - static void SetCodeRangeCreatedCallback(CodeRangeCreatedCallback callback); - - typedef void (__cdecl *CodeRangeDeletedCallback)(void*); - - /* Sets a callback that is invoked for every previously registered code range - * when it is deleted. + /* Sets a callback that is invoked for exceptions that arise in V8-generated + * code (jitted code or embedded builtins). */ - static void SetCodeRangeDeletedCallback(CodeRangeDeletedCallback callback); + static void SetUnhandledExceptionCallback( + v8::UnhandledExceptionCallback callback); #endif };