Skip to content

Extend cp for more than 255 modules - #2351

Open
pguyot wants to merge 1 commit into
atomvm:release-0.7from
pguyot:w27/extend-cp-for-more-modules
Open

Extend cp for more than 255 modules#2351
pguyot wants to merge 1 commit into
atomvm:release-0.7from
pguyot:w27/extend-cp-for-more-modules

Conversation

@pguyot

@pguyot pguyot commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Continuation of:

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

@pguyot
pguyot force-pushed the w27/extend-cp-for-more-modules branch from a818398 to 23adbb9 Compare July 4, 2026 07:02
@pguyot
pguyot force-pushed the w27/extend-cp-for-more-modules branch from 23adbb9 to c685766 Compare July 23, 2026 05:17
@pguyot
pguyot marked this pull request as ready for review July 23, 2026 05:21
Make the cp a fixed-width 64-bit cp_t that occupies one stack term on 64-bit
and two on 32-bit.

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
@pguyot
pguyot force-pushed the w27/extend-cp-for-more-modules branch from c685766 to f30581b Compare July 25, 2026 17:14
@petermm

petermm commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

AMP:

PR Review: widen continuation pointers beyond 256 modules

Recommendation: request changes

The continuation-pointer migration itself is internally consistent across the interpreter, stack storage, stacktrace scanning, and the reviewed 32-bit JIT backends. However, exception handlers still encode their module identity with the old 8-bit module-index layout. As a result, the stated support for more than 256 modules remains incomplete and can dispatch an exception to the wrong module.

Oracle was consulted to stress-test the review, with emphasis on stack layout, GC traversal, exception flow, 32-bit alignment, and JIT ABI offsets.

Finding 1 — High: catch labels still truncate module indexes above 255

Affected code:

  • src/libAtomVM/term.h:1132-1136,1867-1870
  • src/libAtomVM/opcodesswitch.h:3287-3297,3357-3367
  • libs/jit/src/jit.erl:4213-4218
  • src/libAtomVM/context.c:1321-1344
  • src/libAtomVM/stacktrace.c:188-211,317-339

term_from_catch_label() retains the legacy representation:

return (term) ((module_index << 24) | (label << 6) | TERM_IMMED2_CATCH);

On a 32-bit build, only eight module-index bits fit above bit 23. The generic JIT emits the same module_index << 24 encoding into one target word. On a 64-bit interpreter build, the C expression is still evaluated as 32-bit because module_index is an unsigned int; casting to term only after the shift has already discarded the upper bits.

Trigger: load at least 256 modules, then execute a try/catch handler in a module whose index is greater than 255 and raise an exception inside that protected region.

Impact: context_get_catch_label() decodes the truncated index and calls globalcontext_get_module_by_index() with the wrong module. Error dispatch can therefore jump to the same-numbered label in an unrelated module, use an invalid label for that module, or crash. Stacktrace creation repeats the same truncated lookup.

The new test_many_modules test does exercise continuation pointers from high-index modules because exported_func/1 recursively calls itself, but the fixture contains no exception handler. Therefore the test passes without covering this remaining module-index encoding.

Required fix: redesign catch-label module resolution for 32-bit terms rather than widening only continuation pointers. A practical approach is to store only the catch label in the catch term and derive its module while scanning frames:

  1. Start the scan with the currently executing module.
  2. A catch term belongs to the current frame module.
  3. When crossing a saved CP, load all CP_SIZE_IN_TERMS words and update the frame module with cp_to_module().
  4. Apply the same rule in context_get_catch_label(), both stacktrace passes, crash dumps, and JIT-generated catch terms.
  5. Add a regression fixture that raises and catches inside a dynamically loaded module above index 255, for the emulator and at least one 32-bit JIT target.

A cast before the shift would repair only the 64-bit C path and is not a complete fix:

-    return (term) ((module_index << 24) | (label << 6) | TERM_IMMED2_CATCH);
+    return ((term) module_index << 24) | ((term) label << 6) | TERM_IMMED2_CATCH;

The 32-bit representation still has no room for more than eight module-index bits, so this partial diff should not be merged on its own.

Suggestion 1 — ARMv6-M set_bs adds unnecessary instructions and register pressure

Affected code: libs/jit/src/jit_armv6m.erl:4183-4204

The new comment says ?BS_OFFSET moved to 0x80, outside Thumb's immediate range. Its actual value is 0x6C (108), which remains within the documented maximum of 124. The old direct str therefore still works. The replacement consumes a second temporary register and emits five instructions instead of three on every set_bs path.

Suggested fix:

     Avail = jit_regs:available_regs(Regs0),
     Temp = first_avail(Avail),
-    Temp2 = first_avail(Avail band (bnot reg_bit(Temp))),
     I1 = jit_armv6m_asm:str(TermReg, ?BS),
     I2 = jit_armv6m_asm:movs(Temp, 0),
-    % Since cp grew to two words, ?BS_OFFSET sits at 0x80, beyond the Thumb
-    % str immediate range (max 124). Materialize the address in Temp2 = ctx +
-    % 0x80, then store with a zero offset.
-    {?CTX_REG, BsOffsetOff} = ?BS_OFFSET,
-    I3 = jit_armv6m_asm:movs(Temp2, BsOffsetOff),
-    I4 = jit_armv6m_asm:add(Temp2, ?CTX_REG),
-    I5 = jit_armv6m_asm:str(Temp, {Temp2, 0}),
-    Stream1 = StreamModule:append(
-        Stream0, <<I1/binary, I2/binary, I3/binary, I4/binary, I5/binary>>
-    ),
-    Regs1 = jit_regs:invalidate_reg(jit_regs:invalidate_reg(Regs0, Temp), Temp2),
+    I3 = jit_armv6m_asm:str(Temp, ?BS_OFFSET),
+    Stream1 = StreamModule:append(Stream0, <<I1/binary, I2/binary, I3/binary>>),
+    Regs1 = jit_regs:invalidate_reg(Regs0, Temp),
     State0#state{stream = Stream1, regs = Regs1}.

Suggestion 2 — correct stale WASM32 CP offsets in the ABI comment

Affected code: libs/jit/src/jit_wasm32.erl:137-142

The implementation macros correctly use 0x5C and 0x60, matching the C static assertions, but the adjacent comment says 0x70 and 0x74.

Suggested fix:

 %% ctx->cp is a 64-bit cp_t spanning two words (little-endian): low word at
-%% 0x70 = offset << 2 (?CTX_CP_OFFSET), high word at 0x74 = Module* (?CTX_CP_MODULE_OFFSET).
+%% 0x5C = offset << 2 (?CTX_CP_OFFSET), high word at 0x60 = Module* (?CTX_CP_MODULE_OFFSET).

Verification performed

  • git diff HEAD^ HEAD --check — passed.
  • Built and ran test-structs — passed.
  • Built and ran test-erlang test_many_modules — passed on the local 64-bit arm64 build.
  • The many-modules test emitted existing-style dangling ref-counted-binary warnings during teardown but completed successfully.

The local checks do not exercise a 32-bit runtime or the missing high-index exception case. A small Cortex-M0 compiler probe showed that GCC lowers the packed, 4-byte-aligned Context.cp accesses into two 32-bit loads/stores, so the reviewed packed field is not reported as a blocker; other supported 32-bit toolchains should still be covered by CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants