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 <[email protected]>

Author: legendecas

src/api/environment.cc

@@ -734,7 +734,7 @@ Maybe<bool> InitializeContextRuntime(Local<Context> context) {
     }
   } else if (per_process::cli_options->disable_proto != "") {
     // Validated in ProcessGlobalArgs
-    OnFatalError("InitializeContextRuntime()", "invalid --disable-proto mode");
+    UNREACHABLE("invalid --disable-proto mode");
   }
 
   return Just(true);

src/inspector_agent.cc

@@ -36,9 +36,6 @@
 namespace node {
 namespace inspector {
 namespace {
-
-using node::OnFatalError;
-
 using v8::Context;
 using v8::Function;
 using v8::HandleScope;
@@ -917,8 +914,7 @@ void Agent::ToggleAsyncHook(Isolate* isolate, Local<Function> fn) {
   USE(fn->Call(context, Undefined(isolate), 0, nullptr));
   if (try_catch.HasCaught() && !try_catch.HasTerminated()) {
     PrintCaughtException(isolate, context, try_catch);
-    OnFatalError("\nnode::inspector::Agent::ToggleAsyncHook",
-                 "Cannot toggle Inspector's AsyncHook, please report this.");
+    UNREACHABLE("Cannot toggle Inspector's AsyncHook, please report this.");
   }
 }
 

src/node_errors.cc

@@ -376,14 +376,7 @@ void AppendExceptionLine(Environment* env,
             .FromMaybe(false));
 }
 
-[[noreturn]] void Abort() {
-  DumpNativeBacktrace(stderr);
-  DumpJavaScriptBacktrace(stderr);
-  fflush(stderr);
-  ABORT_NO_BACKTRACE();
-}
-
-[[noreturn]] void Assert(const AssertionInfo& info) {
+void Assert(const AssertionInfo& info) {
   std::string name = GetHumanReadableProcessName();
 
   fprintf(stderr,
@@ -396,15 +389,15 @@ void AppendExceptionLine(Environment* env,
           info.message);
 
   fflush(stderr);
-  Abort();
+  ABORT();
 }
 
 enum class EnhanceFatalException { kEnhance, kDontEnhance };
 
 /**
  * Report the exception to the inspector, then print it to stderr.
  * This should only be used when the Node.js instance is about to exit
- * (i.e. this should be followed by a env->Exit() or an Abort()).
+ * (i.e. this should be followed by a env->Exit() or an ABORT()).
  *
  * Use enhance_stack = EnhanceFatalException::kDontEnhance
  * when it's unsafe to call into JavaScript.
@@ -576,8 +569,7 @@ static void ReportFatalException(Environment* env,
   ABORT();
 }
 
-[[noreturn]] void OOMErrorHandler(const char* location,
-                                  const v8::OOMDetails& details) {
+void OOMErrorHandler(const char* location, const v8::OOMDetails& details) {
   // We should never recover from this handler so once it's true it's always
   // true.
   is_in_oom.store(true);
@@ -1063,7 +1055,7 @@ static void TriggerUncaughtException(const FunctionCallbackInfo<Value>& args) {
   if (env != nullptr && env->abort_on_uncaught_exception()) {
     ReportFatalException(
         env, exception, message, EnhanceFatalException::kEnhance);
-    Abort();
+    ABORT();
   }
   bool from_promise = args[1]->IsTrue();
   errors::TriggerUncaughtException(isolate, exception, message, from_promise);
@@ -1174,7 +1166,7 @@ void TriggerUncaughtException(Isolate* isolate,
     // much we can do, so we just print whatever is useful and crash.
     PrintToStderrAndFlush(
         FormatCaughtException(isolate, context, error, message));
-    Abort();
+    ABORT();
   }
 
   // Invoke process._fatalException() to give user a chance to handle it.

src/node_errors.h

@@ -19,9 +19,14 @@ void AppendExceptionLine(Environment* env,
                          v8::Local<v8::Message> message,
                          enum ErrorHandlingMode mode);
 
+// This function calls backtrace, it should have not be marked as [[noreturn]].
+// But it is a public API, removing the attribute can break.
+// Prefer UNREACHABLE() internally instead, it doesn't need manually set
+// location.
 [[noreturn]] void OnFatalError(const char* location, const char* message);
-[[noreturn]] void OOMErrorHandler(const char* location,
-                                  const v8::OOMDetails& details);
+// This function calls backtrace, do not mark as [[noreturn]]. Read more in the
+// ABORT macro.
+void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
 
 // Helpers to construct errors similar to the ones provided by
 // lib/internal/errors.js.

src/node_process_methods.cc

@@ -64,7 +64,7 @@ Mutex umask_mutex;
 #define NANOS_PER_SEC 1000000000
 
 static void Abort(const FunctionCallbackInfo<Value>& args) {
-  Abort();
+  ABORT();
 }
 
 // For internal testing only, not exposed to userland.

src/node_watchdog.cc

@@ -45,7 +45,7 @@ Watchdog::Watchdog(v8::Isolate* isolate, uint64_t ms, bool* timed_out)
   int rc;
   rc = uv_loop_init(&loop_);
   if (rc != 0) {
-    OnFatalError("node::Watchdog::Watchdog()", "Failed to initialize uv loop.");
+    UNREACHABLE("Failed to initialize uv loop.");
   }
 
   rc = uv_async_init(&loop_, &async_, [](uv_async_t* signal) {

src/util.h

@@ -113,8 +113,9 @@ struct AssertionInfo {
   const char* message;
   const char* function;
 };
-[[noreturn]] void NODE_EXTERN_PRIVATE Assert(const AssertionInfo& info);
-[[noreturn]] void NODE_EXTERN_PRIVATE Abort();
+
+// This indirectly calls backtrace so it can not be marked as [[noreturn]].
+void NODE_EXTERN_PRIVATE Assert(const AssertionInfo& info);
 void DumpNativeBacktrace(FILE* fp);
 void DumpJavaScriptBacktrace(FILE* fp);
 
@@ -125,16 +126,32 @@ void DumpJavaScriptBacktrace(FILE* fp);
 #define ABORT_NO_BACKTRACE() abort()
 #endif
 
-#define ABORT() node::Abort()
+// Caller of this macro must not be marked as [[noreturn]]. Printing of
+// backtraces may not work correctly in [[noreturn]] functions because
+// when generating code for them the compiler can choose not to
+// maintain the frame pointers or link registers that are necessary for
+// correct backtracing.
+// `ABORT` must be a macro and not a [[noreturn]] function to make sure the
+// backtrace is correct.
+#define ABORT()                                                                \
+  do {                                                                         \
+    node::DumpNativeBacktrace(stderr);                                         \
+    node::DumpJavaScriptBacktrace(stderr);                                     \
+    fflush(stderr);                                                            \
+    ABORT_NO_BACKTRACE();                                                      \
+  } while (0)
 
-#define ERROR_AND_ABORT(expr)                                                 \
-  do {                                                                        \
-    /* Make sure that this struct does not end up in inline code, but      */ \
-    /* rather in a read-only data section when modifying this code.        */ \
-    static const node::AssertionInfo args = {                                 \
-      __FILE__ ":" STRINGIFY(__LINE__), #expr, PRETTY_FUNCTION_NAME           \
-    };                                                                        \
-    node::Assert(args);                                                       \
+#define ERROR_AND_ABORT(expr)                                                  \
+  do {                                                                         \
+    /* Make sure that this struct does not end up in inline code, but      */  \
+    /* rather in a read-only data section when modifying this code.        */  \
+    static const node::AssertionInfo args = {                                  \
+        __FILE__ ":" STRINGIFY(__LINE__), #expr, PRETTY_FUNCTION_NAME};        \
+    node::Assert(args);                                                        \
+    /* `node::Assert` doesn't return. Add an [[noreturn]] abort() here to  */  \
+    /* make the compiler happy about no return value in the caller         */  \
+    /* function when calling ERROR_AND_ABORT.                              */  \
+    ABORT_NO_BACKTRACE();                                                      \
   } while (0)
 
 #ifdef __GNUC__