Repository navigation
a[i]++ holds its receiver and index across three user-code-capable calls before js_dyn_index_set (#7154 shape) #7628
Description
Activity
Fixed in #7699 — but not the half this issue names, and the difference is measured rather than argued.
The combinator already existed
This issue asked for "an
Argvariant that carries the group's per-operand decision and materialises it at the instant each call is emitted".#7615slice 6 had already built exactly that:RootedGroupis one temp-root scope whose contents are re-readable at any number of caller-chosen points and released once. Both arms use it, and no new primitive arrived with this caller.The operand half is not a live bug
Sabotage arm, run twice. Collapsing the per-use re-reads back to a single one — and, for
Expr::PropertyUpdate, removing the receiver's root entirely — leaves the emitted IR unchanged in the relevant respect.root_reload(#7280) rematerialises the slot load at every use a collection point can reach, and it does so through the handle derivation:%r49.rs4p = load ptr addrspace(1), ptr %r29 ; inserted by root_reload %r49 = ptrtoint ptr addrspace(1) %r49.rs4p to i64 %r50 = and i64 %r49, 281474976710655 call void @js_object_set_field_by_name(i64 %r50, i64 %r53, double %r46)
That is worth recording against #7280's own taxonomy, which lists "a pointer already unboxed to raw
i64" as case (a), the class the pass cannot repair. That entry is about a raw handle a helper RETURNS, not one masked out of a NaN-boxed value the pass has spilled — the latter's chain roots in aload ptr addrspace(1)and is rematerialised whole.The per-use re-reads are kept anyway: they cost nothing (the pass emits them regardless) and they stop the arms depending on a pass carrying a documented side condition ("unless a store to that slot can also run on the way") and a corpus allowlist. But they are documented as belt-and-braces, and their two tests are named as pipeline assertions rather than lowering assertions — a test that cannot fail on the code it appears to cover should not be spelled like a gate.
What was actually repaired: the result
For a BigInt element
js_to_numeric/js_numeric_stephand back a heapBigIntHeader, and whichever value the expression yields —old_numfor postfix,newfor prefix — is live acrossjs_dyn_index_set/js_object_set_field_by_name, i.e. across a user setter, as a bare call result with no slot forroot_reloadto reload from. That is the taxonomy's case (d).RootedGroup::adopt_emittedcloses it, gated onis_provably_not_bigintso a typed-arrayta[i]++keeps the IR it had.Sabotaging that gate (
protectforcedfalse) turnsthe_result_is_rooted_only_when_the_element_may_be_a_bigintred; the typed-array arm is the measured counterfactual, with its returned register produced above the write.The scope note's
Expr::PropertyUpdatetail is included, with the same treatment.Acceptance
test-files/test_gap_7628_index_update_rooted.ts— both fixities, BigInt elements, avalueOfreceiver, the lodashcountByshape (#957), once-only index evaluation — matches node 26.5.1 byte-for-byte. IR-ordering assertions and both sabotage arms are recorded incrates/perry-codegen/src/expr/issue7628_rooting_tests.rs.The two arms moved to
crates/perry-codegen/src/expr/member_update.rs;instance_misc1.rswas four lines under the 2000-line cap.Closed by
9965eb3f6(PR #7699), section "root a[i]++ / o.f++'s result across the write (#7628)" — the member read-modify-write arms now go throughRootedGroup.The interesting part is that the fix is not the one this issue asked for. On re-verification the operand half filed here is not a live bug —
root_reloadalready covered it. The real repair is rooting the result across a possible user setter, which matters for BigInt elements. Two named tests are cited as pipeline assertions.Worth recording as a pattern: this is the second issue today whose stated mechanism was wrong while its symptom was real (#6984 was the other — its bug turned out to be in an unrelated optimization, not in the kill switch it named). Re-deriving the mechanism before fixing is what caught both.
Found while migrating
expr/instance_misc1.rsonto the Layer 1 rooting API(#7615 slice 2, #7627). The operand-to-operand half is fixed there; this is the
half that needs machinery the campaign does not yet have.
The window
Expr::IndexUpdatelowersa[i]++/--a[i]as a read-modify-write over twooperands that are consumed by four different calls, with collection points
between them:
obj_boxandidx_boxare NaN-boxed registers held across all three precedingcalls. If any of them drives an evacuating minor,
js_dyn_index_setwrites intoabandoned from-space memory: the element update silently does not appear on the
object the program keeps. That is #7154's shape, and the same one #7206 fixed on
the computed-read path.
Reproducible shape:
a[i]++wherea's element is an object with avalueOf,or where
ais a Proxy / accessor-bearing object.Why #7627 did not close it
rooting::with_operands_rootedre-reads its group at one point, at the endof the operand list. That is the right shape when a single collection point
separates the group from its consumer, and it is what closed the
receiver-across-the-index window here. It is the wrong shape when the operands
are consumed by different instructions with collection points between them —
re-reading above
js_dyn_index_getputs them straight back in the window theroots exist to close.
The raw API already has the primitive:
RootedOperands::reread_one, added for#7154's dynamic-call lowering, whose doc describes exactly this situation. What
Layer 1 lacks is a combinator that exposes a rooted operand group as something
call_with_rootscan consume per use — i.e. anArgvariant that carries thegroup's per-operand
Root/Reload/Reusedecision and materialises it atthe instant each call is emitted, rather than once.
Per the template's rule (#7617), a combinator arrives with its caller and with
a written argument for why the existing ones cannot serve, not ahead of one. This
issue is that argument; the caller is this arm.
Scope note
Expr::PropertyUpdate's generic tail has the same read-modify-write skeleton(
js_object_get_field_by_name_f64→js_to_numeric→js_numeric_step→js_object_set_field_by_name) over anobj_handleunboxed once above them. Itshould be looked at in the same change; it lives in the same file and was left
alone for the same reason.