Skip to content

Commit

Permalink
V8 x64 backend doesn't emit ABI compliant stack frames
Browse files Browse the repository at this point in the history 
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 <jochen@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Commit-Queue: Paolo Severini <paolosev@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#651075}
  • Loading branch information
@paolosevMSFT
paolosevMSFT authored and Commit Bot committed Apr 16, 2019
1 parent 77dccc4 commit fb4ab3b
Show file tree
Hide file tree
Showing 13 changed files with 10 additions and 275 deletions.
5 changes: 1 addition & 4 deletions chrome/child/v8_crashpad_support_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
4 changes: 0 additions & 4 deletions chrome_elf/chrome_elf_x64.def
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ EXPORTS
RequestSingleCrashUpload_ExportThunk
SetUploadConsent_ExportThunk

; X64-only exports
RegisterNonABICompliantCodeRange_ExportThunk
UnregisterNonABICompliantCodeRange_ExportThunk

; From chrome_elf/crash/crash_helper.cc
SetMetricsClientId

Expand Down
70 changes: 0 additions & 70 deletions components/crash/content/app/breakpad_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExceptionHandlerRecord*>(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<DWORD>(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<void*>(&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<DWORD64>(start)));
}

extern "C" void __declspec(dllexport) __cdecl
UnregisterNonABICompliantCodeRange(void* start) {
ExceptionHandlerRecord* record =
reinterpret_cast<ExceptionHandlerRecord*>(start);

CHECK(RtlDeleteFunctionTable(&record->runtime_function));
}
#endif

} // namespace breakpad
9 changes: 0 additions & 9 deletions components/crash/content/app/crash_export_stubs.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
14 changes: 0 additions & 14 deletions components/crash/content/app/crash_export_thunks.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
7 changes: 0 additions & 7 deletions components/crash/content/app/crash_export_thunks.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_
6 changes: 0 additions & 6 deletions components/crash/content/app/crashpad.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
80 changes: 0 additions & 80 deletions components/crash/content/app/crashpad_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExceptionHandlerRecord*>(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<DWORD>(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<void*>(&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<DWORD64>(start)));
}

void UnregisterNonABICompliantCodeRangeImpl(void* start) {
ExceptionHandlerRecord* record =
reinterpret_cast<ExceptionHandlerRecord*>(start);

CHECK(RtlDeleteFunctionTable(&record->runtime_function));
}
#endif // ARCH_CPU_X86_64

} // namespace internal
} // namespace crash_reporter
16 changes: 2 additions & 14 deletions content/shell/common/v8_crashpad_support_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,15 @@
#include "content/shell/common/v8_crashpad_support_win.h"

#include <windows.h>

#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<gin::Debug::CodeRangeCreatedCallback>(
::GetProcAddress(::GetModuleHandle(L"content_shell.exe"),
"RegisterNonABICompliantCodeRange"));
gin::Debug::CodeRangeDeletedCallback delete_callback =
reinterpret_cast<gin::Debug::CodeRangeDeletedCallback>(
::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
}

Expand Down
26 changes: 3 additions & 23 deletions gin/debug_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand All @@ -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
4 changes: 0 additions & 4 deletions gin/debug_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 0 additions & 22 deletions gin/isolate_holder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
22 changes: 4 additions & 18 deletions gin/public/debug.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

Expand Down

0 comments on commit fb4ab3b

Please sign in to comment.