From 26d85acee2c052aa128b77e2e5fde1d4c9306910 Mon Sep 17 00:00:00 2001 From: Ross McIlroy Date: Fri, 12 Mar 2021 13:34:23 +0000 Subject: [PATCH] [base] Use IMMEDIATE_CRASH on official build FATAL errors. Release-official builds strip error messages from CHECK messages. This can make it difficult to distinguish a CHECK crash location in crash reports. As such, instead of using V8_FatalNoContext, import the IMMEDIATE_CRASH macro from chromium and use that instead, which should cause a crash directly in the instruction stream so that the top stackframe on the crash report directly identifies the CHECK location that failed. More details here: https://docs.google.com/document/d/1tyMwzxUNH8BctM_urSQIYdcbwmzP4kTnwEjnFamBpKY Change-Id: I5b8175f19571834f790060d641db08d0b9c2c17b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2756223 Reviewed-by: Mythri Alle Commit-Queue: Ross McIlroy Cr-Commit-Position: refs/heads/master@{#73430} --- BUILD.gn | 1 + src/base/immediate-crash.h | 162 ++++++++++++++++++++++++++++ src/base/logging.cc | 9 -- src/base/logging.h | 24 ++--- src/base/macros.h | 6 -- src/base/platform/platform-posix.cc | 2 +- src/base/platform/platform-win32.cc | 2 +- src/d8/d8.cc | 2 +- 8 files changed, 177 insertions(+), 31 deletions(-) create mode 100644 src/base/immediate-crash.h diff --git a/BUILD.gn b/BUILD.gn index da6ec0b67d..92c73a9453 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -4497,6 +4497,7 @@ v8_component("v8_libbase") { "src/base/hashmap.h", "src/base/ieee754.cc", "src/base/ieee754.h", + "src/base/immediate-crash.h", "src/base/iterator.h", "src/base/lazy-instance.h", "src/base/logging.cc", diff --git a/src/base/immediate-crash.h b/src/base/immediate-crash.h new file mode 100644 index 0000000000..ef1f922317 --- /dev/null +++ b/src/base/immediate-crash.h @@ -0,0 +1,162 @@ +// Copyright 2021 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_BASE_IMMEDIATE_CRASH_H_ +#define V8_BASE_IMMEDIATE_CRASH_H_ + +#include "include/v8config.h" +#include "src/base/build_config.h" + +// Crashes in the fastest possible way with no attempt at logging. +// There are several constraints; see http://crbug.com/664209 for more context. +// +// - TRAP_SEQUENCE_() must be fatal. It should not be possible to ignore the +// resulting exception or simply hit 'continue' to skip over it in a debugger. +// - Different instances of TRAP_SEQUENCE_() must not be folded together, to +// ensure crash reports are debuggable. Unlike __builtin_trap(), asm volatile +// blocks will not be folded together. +// Note: TRAP_SEQUENCE_() previously required an instruction with a unique +// nonce since unlike clang, GCC folds together identical asm volatile +// blocks. +// - TRAP_SEQUENCE_() must produce a signal that is distinct from an invalid +// memory access. +// - TRAP_SEQUENCE_() must be treated as a set of noreturn instructions. +// __builtin_unreachable() is used to provide that hint here. clang also uses +// this as a heuristic to pack the instructions in the function epilogue to +// improve code density. +// +// Additional properties that are nice to have: +// - TRAP_SEQUENCE_() should be as compact as possible. +// - The first instruction of TRAP_SEQUENCE_() should not change, to avoid +// shifting crash reporting clusters. As a consequence of this, explicit +// assembly is preferred over intrinsics. +// Note: this last bullet point may no longer be true, and may be removed in +// the future. + +// Note: TRAP_SEQUENCE Is currently split into two macro helpers due to the fact +// that clang emits an actual instruction for __builtin_unreachable() on certain +// platforms (see https://crbug.com/958675). In addition, the int3/bkpt/brk will +// be removed in followups, so splitting it up like this now makes it easy to +// land the followups. + +#if V8_CC_GNU + +#if V8_TARGET_ARCH_X64 || V8_TARGET_ARCH_IA32 + +// TODO(https://crbug.com/958675): In theory, it should be possible to use just +// int3. However, there are a number of crashes with SIGILL as the exception +// code, so it seems likely that there's a signal handler that allows execution +// to continue after SIGTRAP. +#define TRAP_SEQUENCE1_() asm volatile("int3") + +#if V8_OS_MACOSX +// Intentionally empty: __builtin_unreachable() is always part of the sequence +// (see IMMEDIATE_CRASH below) and already emits a ud2 on Mac. +#define TRAP_SEQUENCE2_() asm volatile("") +#else +#define TRAP_SEQUENCE2_() asm volatile("ud2") +#endif // V8_OS_MACOSX + +#elif V8_HOST_ARCH_ARM + +// bkpt will generate a SIGBUS when running on armv7 and a SIGTRAP when running +// as a 32 bit userspace app on arm64. There doesn't seem to be any way to +// cause a SIGTRAP from userspace without using a syscall (which would be a +// problem for sandboxing). +// TODO(https://crbug.com/958675): Remove bkpt from this sequence. +#define TRAP_SEQUENCE1_() asm volatile("bkpt #0") +#define TRAP_SEQUENCE2_() asm volatile("udf #0") + +#elif V8_HOST_ARCH_ARM64 + +// This will always generate a SIGTRAP on arm64. +// TODO(https://crbug.com/958675): Remove brk from this sequence. +#define TRAP_SEQUENCE1_() asm volatile("brk #0") +#define TRAP_SEQUENCE2_() asm volatile("hlt #0") + +#else + +// Crash report accuracy will not be guaranteed on other architectures, but at +// least this will crash as expected. +#define TRAP_SEQUENCE1_() __builtin_trap() +#define TRAP_SEQUENCE2_() asm volatile("") + +#endif // V8_HOST_ARCH_* + +#elif V8_CC_MSVC + +#if !defined(__clang__) + +// MSVC x64 doesn't support inline asm, so use the MSVC intrinsic. +#define TRAP_SEQUENCE1_() __debugbreak() +#define TRAP_SEQUENCE2_() + +#elif V8_HOST_ARCH_ARM64 + +// Windows ARM64 uses "BRK #F000" as its breakpoint instruction, and +// __debugbreak() generates that in both VC++ and clang. +#define TRAP_SEQUENCE1_() __debugbreak() +// Intentionally empty: __builtin_unreachable() is always part of the sequence +// (see IMMEDIATE_CRASH below) and already emits a ud2 on Win64, +// https://crbug.com/958373 +#define TRAP_SEQUENCE2_() __asm volatile("") + +#else + +#define TRAP_SEQUENCE1_() asm volatile("int3") +#define TRAP_SEQUENCE2_() asm volatile("ud2") + +#endif // __clang__ + +#else + +#error No supported trap sequence! + +#endif // V8_CC_GNU + +#define TRAP_SEQUENCE_() \ + do { \ + TRAP_SEQUENCE1_(); \ + TRAP_SEQUENCE2_(); \ + } while (false) + +// CHECK() and the trap sequence can be invoked from a constexpr function. +// This could make compilation fail on GCC, as it forbids directly using inline +// asm inside a constexpr function. However, it allows calling a lambda +// expression including the same asm. +// The side effect is that the top of the stacktrace will not point to the +// calling function, but to this anonymous lambda. This is still useful as the +// full name of the lambda will typically include the name of the function that +// calls CHECK() and the debugger will still break at the right line of code. +#if !V8_CC_GNU + +#define WRAPPED_TRAP_SEQUENCE_() TRAP_SEQUENCE_() + +#else + +#define WRAPPED_TRAP_SEQUENCE_() \ + do { \ + [] { TRAP_SEQUENCE_(); }(); \ + } while (false) + +#endif // !V8_CC_GCC + +#if defined(__clang__) || V8_CC_GCC + +// __builtin_unreachable() hints to the compiler that this is noreturn and can +// be packed in the function epilogue. +#define IMMEDIATE_CRASH() \ + ({ \ + WRAPPED_TRAP_SEQUENCE_(); \ + __builtin_unreachable(); \ + }) + +#else + +// This is supporting build with MSVC where there is no __builtin_unreachable(). +#define IMMEDIATE_CRASH() WRAPPED_TRAP_SEQUENCE_() + +#endif // defined(__clang__) || defined(COMPILER_GCC) + +#endif // V8_BASE_IMMEDIATE_CRASH_H_ diff --git a/src/base/logging.cc b/src/base/logging.cc index 9e1cf59f64..a2766477de 100644 --- a/src/base/logging.cc +++ b/src/base/logging.cc @@ -167,15 +167,6 @@ void V8_Fatal(const char* format, ...) { v8::base::OS::Abort(); } -#if !defined(DEBUG) && defined(OFFICIAL_BUILD) -void V8_FatalNoContext() { - v8::base::OS::PrintError("V8 CHECK or FATAL\n"); - if (v8::base::g_print_stack_trace) v8::base::g_print_stack_trace(); - fflush(stderr); - v8::base::OS::Abort(); -} -#endif - void V8_Dcheck(const char* file, int line, const char* message) { v8::base::g_dcheck_function(file, line, message); } diff --git a/src/base/logging.h b/src/base/logging.h index 8386a3c2df..d7161c5b4b 100644 --- a/src/base/logging.h +++ b/src/base/logging.h @@ -12,6 +12,7 @@ #include "src/base/base-export.h" #include "src/base/build_config.h" #include "src/base/compiler-specific.h" +#include "src/base/immediate-crash.h" #include "src/base/template-utils.h" V8_BASE_EXPORT V8_NOINLINE void V8_Dcheck(const char* file, int line, @@ -24,28 +25,25 @@ V8_BASE_EXPORT V8_NOINLINE void V8_Dcheck(const char* file, int line, void V8_Fatal(const char* file, int line, const char* format, ...); #define FATAL(...) V8_Fatal(__FILE__, __LINE__, __VA_ARGS__) -#elif !defined(OFFICIAL_BUILD) +#else +[[noreturn]] PRINTF_FORMAT(1, 2) V8_BASE_EXPORT V8_NOINLINE + void V8_Fatal(const char* format, ...); +#if !defined(OFFICIAL_BUILD) // In non-official release, include full error message, but drop file & line // numbers. It saves binary size to drop the |file| & |line| as opposed to just // passing in "", 0 for them. -[[noreturn]] PRINTF_FORMAT(1, 2) V8_BASE_EXPORT V8_NOINLINE - void V8_Fatal(const char* format, ...); #define FATAL(...) V8_Fatal(__VA_ARGS__) #else -// In official builds, include only messages that contain parameters because -// single-message errors can always be derived from stack traces. -[[noreturn]] V8_BASE_EXPORT V8_NOINLINE void V8_FatalNoContext(); -[[noreturn]] PRINTF_FORMAT(1, 2) V8_BASE_EXPORT V8_NOINLINE - void V8_Fatal(const char* format, ...); -// FATAL(msg) -> V8_FatalNoContext() -// FATAL(msg, ...) -> V8_Fatal() +// FATAL(msg) -> IMMEDIATE_CRASH() +// FATAL(msg, ...) -> V8_Fatal(msg, ...) #define FATAL_HELPER(_7, _6, _5, _4, _3, _2, _1, _0, ...) _0 -#define FATAL_DISCARD_ARG(arg) V8_FatalNoContext() +#define FATAL_DISCARD_ARG(arg) IMMEDIATE_CRASH() #define FATAL(...) \ FATAL_HELPER(__VA_ARGS__, V8_Fatal, V8_Fatal, V8_Fatal, V8_Fatal, V8_Fatal, \ - V8_Fatal, V8_Fatal, FATAL_DISCARD_ARG) \ + V8_Fatal, FATAL_DISCARD_ARG) \ (__VA_ARGS__) -#endif +#endif // !defined(OFFICIAL_BUILD) +#endif // DEBUG #define UNIMPLEMENTED() FATAL("unimplemented code") #define UNREACHABLE() FATAL("unreachable code") diff --git a/src/base/macros.h b/src/base/macros.h index b370d8818b..248a23a1f4 100644 --- a/src/base/macros.h +++ b/src/base/macros.h @@ -183,12 +183,6 @@ V8_INLINE Dest bit_cast(Source const& source) { #define DISABLE_CFI_ICALL V8_CLANG_NO_SANITIZE("cfi-icall") #endif -#if V8_CC_GNU -#define V8_IMMEDIATE_CRASH() __builtin_trap() -#else -#define V8_IMMEDIATE_CRASH() ((void(*)())0)() -#endif - // A convenience wrapper around static_assert without a string message argument. // Once C++17 becomes the default, this macro can be removed in favor of the // new static_assert(condition) overload. diff --git a/src/base/platform/platform-posix.cc b/src/base/platform/platform-posix.cc index 9e3a04579f..ee787f7d9a 100644 --- a/src/base/platform/platform-posix.cc +++ b/src/base/platform/platform-posix.cc @@ -499,7 +499,7 @@ void OS::Sleep(TimeDelta interval) { void OS::Abort() { if (g_hard_abort) { - V8_IMMEDIATE_CRASH(); + IMMEDIATE_CRASH(); } // Redirect to std abort to signal abnormal program termination. abort(); diff --git a/src/base/platform/platform-win32.cc b/src/base/platform/platform-win32.cc index 7f6c0e97d2..50da60c72f 100644 --- a/src/base/platform/platform-win32.cc +++ b/src/base/platform/platform-win32.cc @@ -929,7 +929,7 @@ void OS::Abort() { fflush(stderr); if (g_hard_abort) { - V8_IMMEDIATE_CRASH(); + IMMEDIATE_CRASH(); } // Make the MSVCRT do a silent abort. raise(SIGABRT); diff --git a/src/d8/d8.cc b/src/d8/d8.cc index bd1c29a662..5f1b567202 100644 --- a/src/d8/d8.cc +++ b/src/d8/d8.cc @@ -2291,7 +2291,7 @@ void Shell::Fuzzilli(const v8::FunctionCallbackInfo& args) { .FromMaybe(0); switch (arg) { case 0: - V8_IMMEDIATE_CRASH(); + IMMEDIATE_CRASH(); break; case 1: CHECK(0);