Repository navigation
VM onto the slot API (#1665 arm D B2) - #1707
Conversation
…diate via dict_value_set_num Both fixes for the B2 Ir regression (bench_dmg_shape +38.4% -> +5.5%): - eigs_container_slot returned a heap-tagged slot for a boxed NUM in release builds, so every element read took the slot_to_value/make_num slow path. It now decodes to an immediate in every build (the rehearsal behaviour), which is what the VM stack expects. - dict_set_cached_immediate tested dict_value_get_ref(...)->type == VAL_NUM, but get_ref is NULL for a number by contract, so the DMG register-write in-place fast path never fired. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KC99CmwatKssQYggkBCgwF
…dundant num_guard on normalized slots; computed-goto handlers use EIGS_VIEW_NOCHECK + explicit checks bench_dmg_shape Ir +0.71% vs main (was +38.4% in round 1), bench_perf -2.25%, bench_idxset +1.45%. tests/test_borrow_guard.sh plants a moved view in a fused VM handler and requires the explicit check to abort. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KC99CmwatKssQYggkBCgwF
…st and pin fixes The decode-always fix (an earlier commit on this branch) made every borrowed read present a boxed number as an immediate. That broke ownership before the flip: free_value / GC clear skip number slots, so boxes leaked (33 ASan leaks), and vm_borrow_scan could no longer match a returned boxed-number child (coalesce double free). Reverted: list_slot is storage-faithful again (rehearsal still decodes). The VM read sites that push onto the stack normalise instead: - slot_read_value: boxed number -> immediate (borrowed); - slot_read_owned: the same plus incref of a non-number heap value in one branch (10 read+incref+push sites fused). - jit_helper_iter_next pushes the element as a slot instead of list_get_owned + vm_push (a make_num/free per iteration). Ir vs main: bench_dmg_shape +0.98%, bench_idxset +0.04%, bench_perf -1.46%. ASan core sections 218/218, leak tally 0. - tests/test_borrow_guard.sh: the VM-view plant program raises in every build, so "compiled out" is now detected by comparing against a run without the plant (it misreported a failure on release builds). - num_read allowlist: slot_read_value / slot_read_owned. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KC99CmwatKssQYggkBCgwF
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0ea8820e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| EigsSlot item = list_slot(arg, i); | ||
| if (slot_is_heap(item) && slot_as_ptr(item) == result) { | ||
| val_incref(result); return; |
There was a problem hiding this comment.
When EIGS_SLOT_REHEARSAL is enabled, list_slot() converts a boxed numeric child to an immediate slot, so this heap-only comparison cannot recognize a Value * returned directly by a legacy builtin such as coalesce. The caller then decrefs the argument list without adding a result reference, leaving the returned number at refcount zero or on the numeric freelist before it is pushed; this is a use-after-free in the rehearsal build and will carry into the storage flip. Keep a storage-pointer comparison here until all direct-child-returning builtins use the slot return API, or migrate those builtins atomically.
Useful? React with 👍 / 👎.
| Value *v = slot_as_ptr(s); | ||
| if (v->type == VAL_NUM) return slot_from_num(VAL_NUM_RAW(v)); | ||
| val_incref(v); | ||
| } | ||
| return s; |
There was a problem hiding this comment.
When a list or dictionary contains null, this helper leaves its boxed VAL_NULL as a heap slot instead of restoring the immediate-null representation previously produced by vm_push. That leaks through assignments because obs_slot_observable() treats every heap slot as observable: for example, assigning x is xs[0] with xs is [null] makes trajectory of x report "observed": true, while assigning x is null reports false. Normalize heap VAL_NULL here (and in slot_read_value) so container reads do not change observer results based solely on where null came from.
Useful? React with 👍 / 👎.
3 participants
Part of #1665: batch B2 of the arm D step-2 design.
src/vm.cmoves onto the slot API and joins the migrated-files gate. Storage staysValue**.What changed
vm_borrow_scanmatches only heap slots, and keeps the escaped-view guard.slot_read_value/slot_read_ownedturn a boxed number into an immediate, and the fused version also increfs any other heap value in one branch.list_slotitself stays faithful to storage before the flip, sofree_value, the GC clear and the borrow scan still see and release the boxes.dict_value_rewrite_numrestores DMG's in-place register-write fast path.EIGS_VIEW_NOCHECKwith an expliciteigs_view_checkon every exit edge, because GCC skipscleanupon a computed-goto exit. clang shows noindirect_goto_in_protected_scope.tests/test_borrow_guard.shplants a moved view in a fused VM handler, and the explicit check aborts.How it got here
list_slotheap-tagged boxed numbers, so every stack push tookslot_to_value/make_num; DMG's in-place fast path testedget_ref(...)->type == VAL_NUM, which is never truecoalescedouble free)list_slot, with normalising stack readsEvery round was measured with
cg_diffat function level.Gates (run locally)
jit-smokejit_diffdetect_leaks=1[99d]failure belowRehearsal suite failures:
[99d]failure below;coalesce:builtins.cisn't migrated yet. Under rehearsal, the borrow scan can't match the box the legacy API returns. B3 (arm-d-B3) movescoalescetolist_get_return;[70d]when_liveRSS: pre-existing on main under rehearsal, same 123 MB. Filed below.Pre-existing on main and filed separately:
[99d]the binary-fingerprint self-test fails on main too;when_liveRSS growth.🤖 Generated with Claude Code
https://claude.ai/code/session_01KC99CmwatKssQYggkBCgwF