fix: lift VM register-overflow limit on large records and with updates#256
Merged
Conversation
…low cap Expr::Record and Expr::With emitted OP_RECNEW / OP_RECWITH reading field values from consecutive registers a+1..a+n. That required next_reg + n <= 255, so any record literal with more than ~127 fields, or a moderate one preceded by many locals, hit a hard assert! panic at compile time. Same root-cause shape as PR #237 for list literals. Fix follows the OP_LISTNEW/OP_LISTAPPEND split: three new opcodes (OP_RECNEW_EMPTY = 158, OP_RECCOPY = 159, OP_RECSETFIELD = 160) let the compiler fall back to a per-field setter loop when the contiguous-register layout would overflow. The fallback only needs two registers (result + scratch) regardless of field count. Fast path is preserved: small records still emit a single OP_RECNEW or OP_RECWITH. Only oversized literals or with-updates hit the fallback. With-fallback is gated on all_resolved; the unresolved field-name path keeps the existing dispatch. RC safety on OP_RECSETFIELD: only emitted against records just allocated by OP_RECNEW_EMPTY or OP_RECCOPY, so in-place mutation has no aliasing concern. drop_rc on the old slot + clone_rc on the new value mirrors the OP_RECWITH bookkeeping, just split across two bytecodes. For OP_RECNEW_EMPTY, the old slot is always Nil, so drop_rc is a no-op.
JIT and AOT both need to know about OP_RECNEW_EMPTY, OP_RECCOPY, and OP_RECSETFIELD or they'd bail to interpreter on any program with an oversized record literal. The fallback path is cold (only fires on records with >127 fields or under heavy register pressure), so a simple C-ABI helper call suits both backends — no need to mirror the inline-arena bump allocation OP_RECNEW gets on the fast path. jit_recnew_empty, jit_reccopy, and jit_recsetfield mirror the VM dispatch one-for-one. OP_RECSETFIELD returns no result and skips def_var on its A reg: the record's NanVal is unchanged by in-place field mutation, so the prior RECNEW_EMPTY/RECCOPY definition stays current.
18 regression tests pin the fix across tree, VM, and Cranelift: size sweep at 1/16/60/150/220 fields for both record literals and with-updates, plus the 180-leading-locals shape that originally surfaced the bug in agent-written code. Adjacent coverage: - With on a 150-field record preserves untouched fields (exercises OP_RECCOPY's clone_rc bookkeeping). - With does not mutate the original record (the fallback allocates fresh storage; OP_RECSETFIELD only touches the copy). - Record with mixed string/number fields exercises clone_rc across the fallback boundary for heap field values. Two examples (large-record-literal.ilo, large-record-with.ilo) get picked up by tests/examples_engines.rs so the run/out assertions fire on every engine on every CI run.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Expr::Recordemitted a singleOP_RECNEWthat reads field values from consecutive registersa+1..a+n_fields, andExpr::Withdid the same withOP_RECWITHfor update values. That requirednext_reg + n <= 255, so any record literal with more than ~127 fields, or a moderate one preceded by many locals, hit a hardassert!panic at compile time on the VM and Cranelift backends. Tree-walker was unaffected.Same root-cause shape as PR #237 for list literals, different fix shape: records have fixed cardinality (no RECAPPEND analogue), so the fallback is three new opcodes that let the compiler build a record via OP_RECNEW_EMPTY then an in-place per-field setter loop. From the persona view, big-record literals and big
withupdates just work — no need to chunk via intermediate types or restructure data flow. Every retry that used to burn on this panic is one less token tax.Repro
Before, on
vmandcranelift:Same shape for
withonce the update count crosses the same threshold.After: returns
140cleanly on tree, VM, and Cranelift. Tested up to 220 fields. 180 leading locals + 60-field literal works too.What's in the diff
Three commits:
fix: fall back to per-field record emission to lift VM register-overflow cap — three new opcodes (
OP_RECNEW_EMPTY = 161,OP_RECCOPY = 162,OP_RECSETFIELD = 163). Fast path keeps the existingOP_RECNEW/OP_RECWITHsingle-instruction emission when items fit contiguously (pre_reg + 2n < 255). Fallback emitsOP_RECNEW_EMPTY a, type_idorOP_RECCOPY a, bthen a loop ofOP_RECSETFIELD a, scratch, idx, resettingnext_regbetween updates so scratch slots are reused. The fallback needs only two registers regardless of field count. With-fallback is gated onall_resolvedfield indices; the unresolved name path keeps the existing dispatch.RC safety:
OP_RECSETFIELDis only emitted by the compiler against records just allocated byOP_RECNEW_EMPTYorOP_RECCOPYin the same expression, so in-place mutation has no aliasing concern. The drop_rc on the old slot + clone_rc on the new value mirrors the existing OP_RECWITH bookkeeping, just split across two bytecodes. For OP_RECNEW_EMPTY the old slot is always Nil, so drop_rc is a no-op.cranelift: dispatch arms for the new record-fallback opcodes — JIT and AOT both need the three new opcodes or they'd bail to interpreter on any program with an oversized record. The fallback path is cold (only fires under heavy register pressure), so a simple C-ABI helper call suits both backends — no need to mirror the inline-arena bump allocation that
OP_RECNEWgets on the fast path.jit_recnew_empty,jit_reccopy, andjit_recsetfieldmirror the VM dispatch one-for-one.test: cross-engine coverage for large records and oversized with — 18 regression tests pin the fix across tree, VM, and Cranelift. Size sweep at 1/16/60/150/220 fields for both record literals and with-updates, plus the 180-leading-locals shape that originally surfaced the bug. Adjacent coverage exercises
OP_RECCOPY's clone_rc bookkeeping (string fields, untouched-fields preservation, no-mutation-of-original). Two examples picked up bytests/examples_engines.rs.Test plan
cargo test --release --features cranelift— full suite green (4526 passed, 0 failed)tests/regression_record_register_overflow.rs— 18 tests, all enginestests/examples_engines.rsexercisesexamples/large-record-literal.iloandexamples/large-record-with.ilocargo clippy --release --features cranelift --tests -- -D warnings)cargo fmt --checkcleanFollow-ups
withon records with unresolved field names (i.e. dynamic record types) still hits the original assert. The fallback is gated onall_resolvedbecause the per-field setter needs a static index. A name-keyed setfield variant could lift this, but it's an extreme edge case (>127 updates on a record whose type the verifier didn't pin) and not worth the opcode budget today.alloc_regoverflow when cumulative inner-reg allocation exceeds 255 — same family, different surface. Not addressed here.