FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix backtrace with tail [[noreturn]] abort · nodejs/node@d82b0d4 · GitHub

/ node Public

Commit d82b0d4

Browse files
legendecas
authored and
richardlau
committed
src: fix backtrace with tail [[noreturn]] abort
A function tail calls [[noreturn]] node::Abort will print an incorrect call stack because the frame pc was advanced when calling node::Abort to an invalid op, which may vary on different platforms. Dumps the backtrace in the ABORT macro instead to avoid calling backtrace in a tail [[noreturn]] call. Removes the [[noreturn]] attribute if a function calls backtrace and may be called as a tail statement. [[noreturn]] attribute of public functions like `napi_fatal_error` and `node::OnFatalError` can not be removed as compilers may complain about no return values after the removal. PR-URL: #50849 Refs: #50761 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
1 parent 3f4e254 commit d82b0d4

7 files changed

Lines changed: 45 additions & 35 deletions

File tree

‎src/api/environment.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -734,7 +734,7 @@ Maybe<bool> InitializeContextRuntime(Local<Context> context) {
734734
}
735735
} else if (per_process::cli_options->disable_proto != "") {
736736
// Validated in ProcessGlobalArgs
737-
OnFatalError("InitializeContextRuntime()", "invalid --disable-proto mode");
737+
UNREACHABLE("invalid --disable-proto mode");
738738
}
739739

740740
return Just(true);

‎src/inspector_agent.cc‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,6 @@
3636
namespace node {
3737
namespace inspector {
3838
namespace {
39-
40-
using node::OnFatalError;
41-
4239
using v8::Context;
4340
using v8::Function;
4441
using v8::HandleScope;
@@ -917,8 +914,7 @@ void Agent::ToggleAsyncHook(Isolate* isolate, Local<Function> fn) {
917914
USE(fn->Call(context, Undefined(isolate), 0, nullptr));
918915
if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
919916
PrintCaughtException(isolate, context, try_catch);
920-
OnFatalError("\nnode::inspector::Agent::ToggleAsyncHook",
921-
"Cannot toggle Inspector's AsyncHook, please report this.");
917+
UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
922918
}
923919
}
924920

‎src/node_errors.cc‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -376,14 +376,7 @@ void AppendExceptionLine(Environment* env,
376376
.FromMaybe(false));
377377
}
378378

379-
[[noreturn]] void Abort() {
380-
DumpNativeBacktrace(stderr);
381-
DumpJavaScriptBacktrace(stderr);
382-
fflush(stderr);
383-
ABORT_NO_BACKTRACE();
384-
}
385-
386-
[[noreturn]] void Assert(const AssertionInfo& info) {
379+
void Assert(const AssertionInfo& info) {
387380
std::string name = GetHumanReadableProcessName();
388381

389382
fprintf(stderr,
@@ -396,15 +389,15 @@ void AppendExceptionLine(Environment* env,
396389
info.message);
397390

398391
fflush(stderr);
399-
Abort();
392+
ABORT();
400393
}
401394

402395
enum class EnhanceFatalException { kEnhance, kDontEnhance };
403396

404397
/**
405398
* Report the exception to the inspector, then print it to stderr.
406399
* This should only be used when the Node.js instance is about to exit
407-
* (i.e. this should be followed by a env->Exit() or an Abort()).
400+
* (i.e. this should be followed by a env->Exit() or an ABORT()).
408401
*
409402
* Use enhance_stack = EnhanceFatalException::kDontEnhance
410403
* when it's unsafe to call into JavaScript.
@@ -576,8 +569,7 @@ static void ReportFatalException(Environment* env,
576569
ABORT();
577570
}
578571

579-
[[noreturn]] void OOMErrorHandler(const char* location,
580-
const v8::OOMDetails& details) {
572+
void OOMErrorHandler(const char* location, const v8::OOMDetails& details) {
581573
// We should never recover from this handler so once it's true it's always
582574
// true.
583575
is_in_oom.store(true);
@@ -1063,7 +1055,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {
10631055
if (env != nullptr && env->abort_on_uncaught_exception()) {
10641056
ReportFatalException(
10651057
env, exception, message, EnhanceFatalException::kEnhance);
1066-
Abort();
1058+
ABORT();
10671059
}
10681060
bool from_promise = args[1]->IsTrue();
10691061
errors::TriggerUncaughtException(isolate, exception, message, from_promise);
@@ -1174,7 +1166,7 @@ void TriggerUncaughtException(Isolate* isolate,
11741166
// much we can do, so we just print whatever is useful and crash.
11751167
PrintToStderrAndFlush(
11761168
FormatCaughtException(isolate, context, error, message));
1177-
Abort();
1169+
ABORT();
11781170
}
11791171

11801172
// Invoke process._fatalException() to give user a chance to handle it.

‎src/node_errors.h‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,14 @@ void AppendExceptionLine(Environment* env,
1919
v8::Local<v8::Message> message,
2020
enum ErrorHandlingMode mode);
2121

22+
// This function calls backtrace, it should have not be marked as [[noreturn]].
23+
// But it is a public API, removing the attribute can break.
24+
// Prefer UNREACHABLE() internally instead, it doesn't need manually set
25+
// location.
2226
[[noreturn]] void OnFatalError(const char* location, const char* message);
23-
[[noreturn]] void OOMErrorHandler(const char* location,
24-
const v8::OOMDetails& details);
27+
// This function calls backtrace, do not mark as [[noreturn]]. Read more in the
28+
// ABORT macro.
29+
void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
2530

2631
// Helpers to construct errors similar to the ones provided by
2732
// lib/internal/errors.js.

‎src/node_process_methods.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ Mutex umask_mutex;
6464
#define NANOS_PER_SEC 1000000000
6565

6666
static void Abort(const FunctionCallbackInfo<Value>& args) {
67-
Abort();
67+
ABORT();
6868
}
6969

7070
// For internal testing only, not exposed to userland.

‎src/node_watchdog.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms, bool* timed_out)
4545
int rc;
4646
rc = uv_loop_init(&loop_);
4747
if (rc != 0) {
48-
OnFatalError("node::Watchdog::Watchdog()", "Failed to initialize uv loop.");
48+
UNREACHABLE("Failed to initialize uv loop.");
4949
}
5050

5151
rc = uv_async_init(&loop_, &async_, [](uv_async_t* signal) {

‎src/util.h‎

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,9 @@ struct AssertionInfo {
113113
const char* message;
114114
const char* function;
115115
};
116-
[[noreturn]] void NODE_EXTERN_PRIVATE Assert(const AssertionInfo& info);
117-
[[noreturn]] void NODE_EXTERN_PRIVATE Abort();
116+
117+
// This indirectly calls backtrace so it can not be marked as [[noreturn]].
118+
void NODE_EXTERN_PRIVATE Assert(const AssertionInfo& info);
118119
void DumpNativeBacktrace(FILE* fp);
119120
void DumpJavaScriptBacktrace(FILE* fp);
120121

@@ -125,16 +126,32 @@ void DumpJavaScriptBacktrace(FILE* fp);
125126
#define ABORT_NO_BACKTRACE() abort()
126127
#endif
127128

128-
#define ABORT() node::Abort()
129+
// Caller of this macro must not be marked as [[noreturn]]. Printing of
130+
// backtraces may not work correctly in [[noreturn]] functions because
131+
// when generating code for them the compiler can choose not to
132+
// maintain the frame pointers or link registers that are necessary for
133+
// correct backtracing.
134+
// `ABORT` must be a macro and not a [[noreturn]] function to make sure the
135+
// backtrace is correct.
136+
#define ABORT() \
137+
do { \
138+
node::DumpNativeBacktrace(stderr); \
139+
node::DumpJavaScriptBacktrace(stderr); \
140+
fflush(stderr); \
141+
ABORT_NO_BACKTRACE(); \
142+
} while (0)
129143

130-
#define ERROR_AND_ABORT(expr) \
131-
do { \
132-
/* Make sure that this struct does not end up in inline code, but */ \
133-
/* rather in a read-only data section when modifying this code. */ \
134-
static const node::AssertionInfo args = { \
135-
__FILE__ ":" STRINGIFY(__LINE__), #expr, PRETTY_FUNCTION_NAME \
136-
}; \
137-
node::Assert(args); \
144+
#define ERROR_AND_ABORT(expr) \
145+
do { \
146+
/* Make sure that this struct does not end up in inline code, but */ \
147+
/* rather in a read-only data section when modifying this code. */ \
148+
static const node::AssertionInfo args = { \
149+
__FILE__ ":" STRINGIFY(__LINE__), #expr, PRETTY_FUNCTION_NAME}; \
150+
node::Assert(args); \
151+
/* `node::Assert` doesn't return. Add an [[noreturn]] abort() here to */ \
152+
/* make the compiler happy about no return value in the caller */ \
153+
/* function when calling ERROR_AND_ABORT. */ \
154+
ABORT_NO_BACKTRACE(); \
138155
} while (0)
139156

140157
#ifdef __GNUC__

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL