Fix erlang:raise/3 assert with built stacktrace

[bettio]

• Fix assert in stacktrace_exception_class when erlang:raise/3 is called with a built stacktrace and the re-raised exception hits a non-matching catch clause (triggering the compiler-generated OP_RAISE)
• Wrap the built list into a raw 6-tuple in stacktrace_create_raw_mfa
• Route OP_RAW_RAISE through HANDLE_ERROR() so both the NIF path and the raw_raise opcode path go through the wrapping logic
• Add early exit in stacktrace_build for pre-built stacktraces

Fixes #2185

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

[petermm]

https://ampcode.com/threads/T-019d4d69-3ac6-718e-8d7c-1ad32f287c7b

PR Review: Fix erlang:raise/3 assert with built stacktrace

Commit: 949690a — Fix erlang:raise/3 assert with built stacktrace
Author: Davide Bettio
Files changed: 6 (stacktrace.c, opcodesswitch.h, test_raise_built_stacktrace.erl, CMakeLists.txt, test.c, CHANGELOG.md)

---

Summary

When erlang:raise/3 is called with a built stacktrace (list of {M,F,A,Loc} tuples) and the
re-raised exception passes through a try/catch whose clauses do not match, the compiler-generated
catch-all (OP_RAISE) calls stacktrace_exception_class(), which asserts on a 6-tuple. The built
list was stored as-is, never wrapped.

The fix:

1. Changes OP_RAW_RAISE from goto handle_error to HANDLE_ERROR() so stacktrace_create_raw_mfa() is called
2. stacktrace_create_raw_mfa() detects a built stacktrace (non-empty list) and wraps it into a raw 6-tuple
3. stacktrace_build() detects num_frames == 0 and returns the pre-built list directly

---

Verdict: ✅ Approve with suggestions

The core fix is correct and well-reasoned for the interpreter path. GC root handling is safe,
the num_frames == 0 sentinel is sound (normal stacktraces always have num_frames >= 1), and
TUPLE_SIZE(6) allocation matches. The test covers both the raw_raise opcode and NIF paths.

---

Issues Found

🔴 Critical: JIT raw_raise path is NOT fixed

The same bug exists in the JIT path. jit_raw_raise() calls jit_handle_error(ctx, jit_state, 0),
which only calls stacktrace_create_raw() when offset != 0 || term_is_invalid_term(stacktrace).
With a built list and offset=0, normalization is skipped — the list flows through unchanged and
will hit the same assertion in jit_raise() → stacktrace_exception_class().

File: src/libAtomVM/jit.c

// Line 288-294 — jit_handle_error skips normalization for built lists
static Context *jit_handle_error(Context *ctx, JITState *jit_state, int offset)
{
    if (offset || term_is_invalid_term(ctx->exception_stacktrace)) {
        ctx->exception_stacktrace
            = stacktrace_create_raw(ctx, jit_state->module, offset);
    }
    // ← A built list (from raise/3) passes through here unchanged

// Line 415-422 — jit_raw_raise passes offset=0, built list survives
static Context *jit_raw_raise(Context *ctx, JITState *jit_state)
{
    context_set_exception_class(ctx, ctx->x[0]);
    ctx->exception_reason = ctx->x[1];
    ctx->exception_stacktrace = ctx->x[2];
    return jit_handle_error(ctx, jit_state, 0);
}

Suggested fix:

 static Context *jit_handle_error(Context *ctx, JITState *jit_state, int offset)
 {
-    if (offset || term_is_invalid_term(ctx->exception_stacktrace)) {
+    if (offset || term_is_invalid_term(ctx->exception_stacktrace)
+            || term_is_nonempty_list(ctx->exception_stacktrace)) {
         ctx->exception_stacktrace
             = stacktrace_create_raw(ctx, jit_state->module, offset);
     }

Also, jit_raise() at line 409 calls stacktrace_exception_class(stacktrace) directly on its
argument. If a built list reaches this path, it will assert-fail. This would also need the same
wrapping treatment.

---

🟡 Minor: Stale comment in OP_RAW_RAISE

The old comment at opcodesswitch.h:5248-5250 says:

"This is an optimization from the compiler where we don't need to call stacktrace_create_raw
here because the stack trace has already been created and set in x[2]."

This is now misleading — the whole point of the fix is that we do call stacktrace_create_raw
(via HANDLE_ERROR()) to normalize built lists. The comment should be updated.

Suggested fix:

             case OP_RAW_RAISE: {

                 TRACE("raw_raise/0\n");

-                // This is an optimization from the compiler where we don't need to call
-                // stacktrace_create_raw here because the stack trace has already been created
-                // and set in x[2].
+                // The compiler emits raw_raise when re-raising with an already
+                // captured stacktrace. We route through HANDLE_ERROR() so that
+                // stacktrace_create_raw_mfa normalizes any built stacktrace
+                // (list of {M,F,A,Loc} tuples) into the internal raw 6-tuple.
                 term ex_class = x_regs[0];

---

🟡 Minor: Empty stacktrace [] not preserved

erlang:raise(error, badarg, []) is valid in OTP but won't be detected by the
term_is_nonempty_list() check. The empty list will fall through to the normal stacktrace
construction path, producing a new stacktrace instead of preserving [].

This may be acceptable if AtomVM doesn't need full OTP compatibility here, but worth documenting
as a known limitation.

---

🟢 Nitpick: CHANGELOG wording

"causing an assert" reads slightly better as "causing an assertion failure":

-- Fixed `erlang:raise/3` with a built stacktrace causing an assert when the re-raised exception
+- Fixed `erlang:raise/3` with a built stacktrace causing an assertion failure when the re-raised exception
 passes through a non-matching catch clause

---

🟢 Nitpick: Test coverage could be stronger

The tests verify "no crash" but don't assert that the preserved stacktrace is correct. Adding a
check that the caught stacktrace remains a list matching the original would strengthen the test.
Also missing: exit/throw exception classes and raise(error, badarg, []).

---

What's Good

• GC root handling is correct — exception_stacktrace and exception_reason are properly
  rooted through ctx->x[0..1] before memory_ensure_free_with_roots
• NOLINT(term-use-after-gc) for exception_class is safe — exception_class is always an
  atom (immediate value), never a heap term, so GC cannot move it
• num_frames == 0 sentinel is sound — normal stacktrace construction always does
  num_frames++ at line 211, so 0 is a safe sentinel for "pre-built"
• No performance regression — the added branch is only on exception paths
• Test covers both opcode and NIF paths — good coverage of the two entry points

[bettio]

PR Review: Fix erlang:raise/3 assert with built stacktrace

I think I fixed all of them.

[petermm]

looks good, this is suggested for the codeql complaining, no strong preference on my part..

CodeQL Fix: Use-after-GC false positive in stacktrace.c

CodeQL flagged exception_class as potentially used after garbage collection
(memory_ensure_free_with_roots). While safe in practice (atoms are immediates, not heap-allocated),
the fix eliminates the pattern by reading context_exception_class(ctx) directly at point of use.

 term stacktrace_create_raw_mfa(Context *ctx, Module *mod, int current_offset, term module_atom, term function_atom, int arity)
 {
-    term exception_class = context_exception_class(ctx);
-
     if (term_is_list(ctx->exception_stacktrace)) {
         // Already a built stacktrace (possibly empty) from erlang:raise/3
         ...
         term_put_tuple_element(stack_info, 4, built_stacktrace);
-        // NOLINT(term-use-after-gc) exception_class is always an atom
-        term_put_tuple_element(stack_info, 5, exception_class);
+        term_put_tuple_element(stack_info, 5, context_exception_class(ctx));
         return stack_info;
     }

     ...
     ctx->x[live_x_regs - 1] = ctx->exception_reason;
-    // NOLINT(term-use-after-gc) exception_class is always an atom
     if (UNLIKELY(memory_ensure_free_with_roots(ctx, requested_size, live_x_regs, ctx->x, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {
         ...

     ...
     term_put_tuple_element(stack_info, 4, raw_stacktrace);
-    term_put_tuple_element(stack_info, 5, exception_class);
+    term_put_tuple_element(stack_info, 5, context_exception_class(ctx));

     return stack_info;
 }

No behavioral change — context_exception_class() returns an atom (immediate value), so GC never
moves it. The local variable and NOLINT suppressions are simply no longer needed.

[pguyot]

Suggested change

	|| term_is_nonempty_list(ctx->exception_stacktrace)) {
	|| term_is_list(ctx->exception_stacktrace)) {

[pguyot]

We don't log here but below we do:

        fprintf(stderr, "WARNING: Unable to allocate heap space for raw stacktrace\n");

Arguably, this OOM is less likely.

[pguyot]

I think we have a bug here. We should raise. We should exercise this with a test.

[pguyot]

Forgot to remove this with the OTP26+ change

[pguyot]

Can this still happen?

[bettio]

Everything should be fixed now.

[petermm]

1c34917 changes invalid erlang:raise/3 handling to raise error:badarg, but BEAM still returns the atom badarg as a normal value here. That affects both the NIF path and compiler-generated raw_raise, so test_exception_classes and test_raw_raise now fail because the tests were updated to expect an exception instead of a return value. This looks like a BEAM-parity regression rather than a platform-specific bug.`

[pguyot]

My bad. I thought BEAM raised and we had a bug, but it doesn't and we hadn't. Sorry!

As evaluating this function causes the process to terminate, it has no return value unless the arguments are invalid, in which case the function returns the error reason badarg. If you want to be sure not to return, you can call error(erlang:raise(Class, Reason, Stacktrace)) and hope to distinguish exceptions later.

See https://www.erlang.org/doc/apps/erts/erlang.html#raise/3