Skip to content

Commit 26d85ac

Browse files
rmcilroyCommit Bot
rmcilroy
authored and
Commit Bot
committed
[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 <[email protected]> Commit-Queue: Ross McIlroy <[email protected]> Cr-Commit-Position: refs/heads/master@{#73430}
1 parent de51603 commit 26d85ac

8 files changed

Lines changed: 177 additions & 31 deletions

File tree

‎BUILD.gn‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4497,6 +4497,7 @@ v8_component("v8_libbase") {
44974497
"src/base/hashmap.h",
44984498
"src/base/ieee754.cc",
44994499
"src/base/ieee754.h",
4500+
"src/base/immediate-crash.h",
45004501
"src/base/iterator.h",
45014502
"src/base/lazy-instance.h",
45024503
"src/base/logging.cc",

‎src/base/immediate-crash.h‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
// Copyright 2021 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#ifndef V8_BASE_IMMEDIATE_CRASH_H_
6+
#define V8_BASE_IMMEDIATE_CRASH_H_
7+
8+
#include "include/v8config.h"
9+
#include "src/base/build_config.h"
10+
11+
// Crashes in the fastest possible way with no attempt at logging.
12+
// There are several constraints; see http://crbug.com/664209 for more context.
13+
//
14+
// - TRAP_SEQUENCE_() must be fatal. It should not be possible to ignore the
15+
// resulting exception or simply hit 'continue' to skip over it in a debugger.
16+
// - Different instances of TRAP_SEQUENCE_() must not be folded together, to
17+
// ensure crash reports are debuggable. Unlike __builtin_trap(), asm volatile
18+
// blocks will not be folded together.
19+
// Note: TRAP_SEQUENCE_() previously required an instruction with a unique
20+
// nonce since unlike clang, GCC folds together identical asm volatile
21+
// blocks.
22+
// - TRAP_SEQUENCE_() must produce a signal that is distinct from an invalid
23+
// memory access.
24+
// - TRAP_SEQUENCE_() must be treated as a set of noreturn instructions.
25+
// __builtin_unreachable() is used to provide that hint here. clang also uses
26+
// this as a heuristic to pack the instructions in the function epilogue to
27+
// improve code density.
28+
//
29+
// Additional properties that are nice to have:
30+
// - TRAP_SEQUENCE_() should be as compact as possible.
31+
// - The first instruction of TRAP_SEQUENCE_() should not change, to avoid
32+
// shifting crash reporting clusters. As a consequence of this, explicit
33+
// assembly is preferred over intrinsics.
34+
// Note: this last bullet point may no longer be true, and may be removed in
35+
// the future.
36+
37+
// Note: TRAP_SEQUENCE Is currently split into two macro helpers due to the fact
38+
// that clang emits an actual instruction for __builtin_unreachable() on certain
39+
// platforms (see https://crbug.com/958675). In addition, the int3/bkpt/brk will
40+
// be removed in followups, so splitting it up like this now makes it easy to
41+
// land the followups.
42+
43+
#if V8_CC_GNU
44+
45+
#if V8_TARGET_ARCH_X64 || V8_TARGET_ARCH_IA32
46+
47+
// TODO(https://crbug.com/958675): In theory, it should be possible to use just
48+
// int3. However, there are a number of crashes with SIGILL as the exception
49+
// code, so it seems likely that there's a signal handler that allows execution
50+
// to continue after SIGTRAP.
51+
#define TRAP_SEQUENCE1_() asm volatile("int3")
52+
53+
#if V8_OS_MACOSX
54+
// Intentionally empty: __builtin_unreachable() is always part of the sequence
55+
// (see IMMEDIATE_CRASH below) and already emits a ud2 on Mac.
56+
#define TRAP_SEQUENCE2_() asm volatile("")
57+
#else
58+
#define TRAP_SEQUENCE2_() asm volatile("ud2")
59+
#endif // V8_OS_MACOSX
60+
61+
#elif V8_HOST_ARCH_ARM
62+
63+
// bkpt will generate a SIGBUS when running on armv7 and a SIGTRAP when running
64+
// as a 32 bit userspace app on arm64. There doesn't seem to be any way to
65+
// cause a SIGTRAP from userspace without using a syscall (which would be a
66+
// problem for sandboxing).
67+
// TODO(https://crbug.com/958675): Remove bkpt from this sequence.
68+
#define TRAP_SEQUENCE1_() asm volatile("bkpt #0")
69+
#define TRAP_SEQUENCE2_() asm volatile("udf #0")
70+
71+
#elif V8_HOST_ARCH_ARM64
72+
73+
// This will always generate a SIGTRAP on arm64.
74+
// TODO(https://crbug.com/958675): Remove brk from this sequence.
75+
#define TRAP_SEQUENCE1_() asm volatile("brk #0")
76+
#define TRAP_SEQUENCE2_() asm volatile("hlt #0")
77+
78+
#else
79+
80+
// Crash report accuracy will not be guaranteed on other architectures, but at
81+
// least this will crash as expected.
82+
#define TRAP_SEQUENCE1_() __builtin_trap()
83+
#define TRAP_SEQUENCE2_() asm volatile("")
84+
85+
#endif // V8_HOST_ARCH_*
86+
87+
#elif V8_CC_MSVC
88+
89+
#if !defined(__clang__)
90+
91+
// MSVC x64 doesn't support inline asm, so use the MSVC intrinsic.
92+
#define TRAP_SEQUENCE1_() __debugbreak()
93+
#define TRAP_SEQUENCE2_()
94+
95+
#elif V8_HOST_ARCH_ARM64
96+
97+
// Windows ARM64 uses "BRK #F000" as its breakpoint instruction, and
98+
// __debugbreak() generates that in both VC++ and clang.
99+
#define TRAP_SEQUENCE1_() __debugbreak()
100+
// Intentionally empty: __builtin_unreachable() is always part of the sequence
101+
// (see IMMEDIATE_CRASH below) and already emits a ud2 on Win64,
102+
// https://crbug.com/958373
103+
#define TRAP_SEQUENCE2_() __asm volatile("")
104+
105+
#else
106+
107+
#define TRAP_SEQUENCE1_() asm volatile("int3")
108+
#define TRAP_SEQUENCE2_() asm volatile("ud2")
109+
110+
#endif // __clang__
111+
112+
#else
113+
114+
#error No supported trap sequence!
115+
116+
#endif // V8_CC_GNU
117+
118+
#define TRAP_SEQUENCE_() \
119+
do { \
120+
TRAP_SEQUENCE1_(); \
121+
TRAP_SEQUENCE2_(); \
122+
} while (false)
123+
124+
// CHECK() and the trap sequence can be invoked from a constexpr function.
125+
// This could make compilation fail on GCC, as it forbids directly using inline
126+
// asm inside a constexpr function. However, it allows calling a lambda
127+
// expression including the same asm.
128+
// The side effect is that the top of the stacktrace will not point to the
129+
// calling function, but to this anonymous lambda. This is still useful as the
130+
// full name of the lambda will typically include the name of the function that
131+
// calls CHECK() and the debugger will still break at the right line of code.
132+
#if !V8_CC_GNU
133+
134+
#define WRAPPED_TRAP_SEQUENCE_() TRAP_SEQUENCE_()
135+
136+
#else
137+
138+
#define WRAPPED_TRAP_SEQUENCE_() \
139+
do { \
140+
[] { TRAP_SEQUENCE_(); }(); \
141+
} while (false)
142+
143+
#endif // !V8_CC_GCC
144+
145+
#if defined(__clang__) || V8_CC_GCC
146+
147+
// __builtin_unreachable() hints to the compiler that this is noreturn and can
148+
// be packed in the function epilogue.
149+
#define IMMEDIATE_CRASH() \
150+
({ \
151+
WRAPPED_TRAP_SEQUENCE_(); \
152+
__builtin_unreachable(); \
153+
})
154+
155+
#else
156+
157+
// This is supporting build with MSVC where there is no __builtin_unreachable().
158+
#define IMMEDIATE_CRASH() WRAPPED_TRAP_SEQUENCE_()
159+
160+
#endif // defined(__clang__) || defined(COMPILER_GCC)
161+
162+
#endif // V8_BASE_IMMEDIATE_CRASH_H_

‎src/base/logging.cc‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -167,15 +167,6 @@ void V8_Fatal(const char* format, ...) {
167167
v8::base::OS::Abort();
168168
}
169169

170-
#if !defined(DEBUG) && defined(OFFICIAL_BUILD)
171-
void V8_FatalNoContext() {
172-
v8::base::OS::PrintError("V8 CHECK or FATAL\n");
173-
if (v8::base::g_print_stack_trace) v8::base::g_print_stack_trace();
174-
fflush(stderr);
175-
v8::base::OS::Abort();
176-
}
177-
#endif
178-
179170
void V8_Dcheck(const char* file, int line, const char* message) {
180171
v8::base::g_dcheck_function(file, line, message);
181172
}

‎src/base/logging.h‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include "src/base/base-export.h"
1313
#include "src/base/build_config.h"
1414
#include "src/base/compiler-specific.h"
15+
#include "src/base/immediate-crash.h"
1516
#include "src/base/template-utils.h"
1617

1718
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,
2425
void V8_Fatal(const char* file, int line, const char* format, ...);
2526
#define FATAL(...) V8_Fatal(__FILE__, __LINE__, __VA_ARGS__)
2627

27-
#elif !defined(OFFICIAL_BUILD)
28+
#else
29+
[[noreturn]] PRINTF_FORMAT(1, 2) V8_BASE_EXPORT V8_NOINLINE
30+
void V8_Fatal(const char* format, ...);
31+
#if !defined(OFFICIAL_BUILD)
2832
// In non-official release, include full error message, but drop file & line
2933
// numbers. It saves binary size to drop the |file| & |line| as opposed to just
3034
// passing in "", 0 for them.
31-
[[noreturn]] PRINTF_FORMAT(1, 2) V8_BASE_EXPORT V8_NOINLINE
32-
void V8_Fatal(const char* format, ...);
3335
#define FATAL(...) V8_Fatal(__VA_ARGS__)
3436
#else
35-
// In official builds, include only messages that contain parameters because
36-
// single-message errors can always be derived from stack traces.
37-
[[noreturn]] V8_BASE_EXPORT V8_NOINLINE void V8_FatalNoContext();
38-
[[noreturn]] PRINTF_FORMAT(1, 2) V8_BASE_EXPORT V8_NOINLINE
39-
void V8_Fatal(const char* format, ...);
40-
// FATAL(msg) -> V8_FatalNoContext()
41-
// FATAL(msg, ...) -> V8_Fatal()
37+
// FATAL(msg) -> IMMEDIATE_CRASH()
38+
// FATAL(msg, ...) -> V8_Fatal(msg, ...)
4239
#define FATAL_HELPER(_7, _6, _5, _4, _3, _2, _1, _0, ...) _0
43-
#define FATAL_DISCARD_ARG(arg) V8_FatalNoContext()
40+
#define FATAL_DISCARD_ARG(arg) IMMEDIATE_CRASH()
4441
#define FATAL(...) \
4542
FATAL_HELPER(__VA_ARGS__, V8_Fatal, V8_Fatal, V8_Fatal, V8_Fatal, V8_Fatal, \
46-
V8_Fatal, V8_Fatal, FATAL_DISCARD_ARG) \
43+
V8_Fatal, FATAL_DISCARD_ARG) \
4744
(__VA_ARGS__)
48-
#endif
45+
#endif // !defined(OFFICIAL_BUILD)
46+
#endif // DEBUG
4947

5048
#define UNIMPLEMENTED() FATAL("unimplemented code")
5149
#define UNREACHABLE() FATAL("unreachable code")

‎src/base/macros.h‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -183,12 +183,6 @@ V8_INLINE Dest bit_cast(Source const& source) {
183183
#define DISABLE_CFI_ICALL V8_CLANG_NO_SANITIZE("cfi-icall")
184184
#endif
185185

186-
#if V8_CC_GNU
187-
#define V8_IMMEDIATE_CRASH() __builtin_trap()
188-
#else
189-
#define V8_IMMEDIATE_CRASH() ((void(*)())0)()
190-
#endif
191-
192186
// A convenience wrapper around static_assert without a string message argument.
193187
// Once C++17 becomes the default, this macro can be removed in favor of the
194188
// new static_assert(condition) overload.

‎src/base/platform/platform-posix.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -499,7 +499,7 @@ void OS::Sleep(TimeDelta interval) {
499499

500500
void OS::Abort() {
501501
if (g_hard_abort) {
502-
V8_IMMEDIATE_CRASH();
502+
IMMEDIATE_CRASH();
503503
}
504504
// Redirect to std abort to signal abnormal program termination.
505505
abort();

‎src/base/platform/platform-win32.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -929,7 +929,7 @@ void OS::Abort() {
929929
fflush(stderr);
930930

931931
if (g_hard_abort) {
932-
V8_IMMEDIATE_CRASH();
932+
IMMEDIATE_CRASH();
933933
}
934934
// Make the MSVCRT do a silent abort.
935935
raise(SIGABRT);

‎src/d8/d8.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2291,7 +2291,7 @@ void Shell::Fuzzilli(const v8::FunctionCallbackInfo<v8::Value>& args) {
22912291
.FromMaybe(0);
22922292
switch (arg) {
22932293
case 0:
2294-
V8_IMMEDIATE_CRASH();
2294+
IMMEDIATE_CRASH();
22952295
break;
22962296
case 1:
22972297
CHECK(0);

0 commit comments

Comments
 (0)