GeezerVison

By the way does it matter if abort is tail called or not? I think as long as the caller is marked as [[noreturn]], the compiler is completely free to just not maintain the frame pointer/link register for backtrace (it seems LLVM is particularly eager to do this which I guess is why we are seeing this showing up more on macOS). So the rule of thumb here is to just not mark [[noreturn]] for any caller of backtrace().

By the way does it matter if abort is tail called or not? I think as long as the caller is marked as [[noreturn]], the compiler is completely free to just not maintain the frame pointer/link register for backtrace (it seems LLVM is particularly eager to do this which I guess is why we are seeing this showing up more on macOS). So the rule of thumb here is to just not mark [[noreturn]] for any caller of backtrace().

I checked different call sites and their generated code, and I found that a tail call to [[noreturn]] is more eager to be optimized. For example, the following snippet would print the correct backtrace when it reaches the abort:

MaybeLocal<String> BuiltinLoader::LoadBuiltinSource(Isolate* isolate,
                                                    const char* id) const {
  auto source = source_.read();
  const auto source_it = source->find(id);
  if (UNLIKELY(source_it == source->end())) {
    fprintf(stderr, "Cannot find native builtin: \"%s\".\n", id);
    node::Abort();
  }
  return source_it->second.ToStringChecked(isolate);
}

But, yes, the rule should be that we should not mark functions as [[noreturn]] for callers of backtrace().

@legendecas Do you have time to address the comments? test-worker-nearheaplimit-deadlock has been haunting the CI for some time, so I'd like to see this landed soon-ish to get some more information out of the flakes.

CI: https://ci.nodejs.org/job/node-test-pull-request/55989/

CI: https://ci.nodejs.org/job/node-test-pull-request/56009/

Landed in 16a5479

Actually I noticed in https://ci.nodejs.org/job/node-test-binary-windows-js-suites/26795/RUN_SUBSET=1,nodes=win2022-COMPILED_BY-vs2022/console that this change is breaking most of the places being converted to use ABORT() without marking themselves as [[noreturn]]. I think the original point about not making [[noreturn]] Abort() should be revisited:

int MyFunction(int value) {
  if (value >= 0) {
    return value;
  }
  // if Abort is not marked as [[noreturn]], compiler 
  // will complain about no return value in this branch.
  Abort();
};

In this case the function should be rewritten like this:

int MyFunction(int value) {
  if (value < 0) {
    Abort();
  }
  return value;
};

Otherwise every caller of the ABORT() macro has to be marked as [[noreturn]] and the chain is infectious, which is probably not practical.

Oh wait, I think I got it the other way around. Callers of ABORT() must not be macro. Though it seems with msvc, the macro is causing some problems to properly backtrace..

@joyeecheung sorry, but the link you provided didn't report a compiler error. Is the link correct?

The issue is in the crashed test-buffer-isascii with incorrect native stack traces, what we originally tried to fix