chore: merge train 235 (v0.5.1614) - #10788
Merged
Merged
Conversation
…nd condition (#10777) `expr_numeric_by_construction` required `rec(operand)` for `Pos`, the same as for `Neg` and `BitNot`. That was not a soundness guard, it was a missed proof. Unary `+` is ToNumber, which either completes holding a Number or throws: a BigInt and a Symbol both throw a TypeError, an object goes through ToPrimitive and then ToNumber again, `undefined` is NaN, and NaN is a Number. A throw stores no value, so the store-universe question this fixpoint asks is vacuous on that path — there is no input for which `+x` finishes holding something other than a Number. `Neg` and `BitNot` keep their operand condition, because ToNumeric is BigInt-preserving: `-1n` is `-1n` and `~1n` is `-2n`, neither a Number. The missed proof left the ACCUMULATOR unproven, so its add kept a per-iteration tag test: const v = +o.a; for (…) h += v 20 -> 9 Ir/iteration const v = +a[0]; for (…) h += v 20 -> 9 (Float64Array) which is exactly where `o.a * 1` and `o.a - 0` already sat. node is 7.03 and 7.50 on the same fixtures, bun 4.27 and 4.48, so this closes the perry-versus-perry gap and does not reach parity.
… user bytes cannot forge a boxed value (#10779) A double whose bit pattern falls inside the NaN-box tag window read back as its payload instead of the NaN it is. `Number.isNaN` then reported false, so the one defensive check a program would use agreed the value was fine. It is not a wrong number. 80 of 144 probe patterns diverge on base and 32 of them SIGSEGV: 0x7FF9... reads back with `typeof === "string"` and 0x7FFD... as `[object Object]` — a pointer forged out of user-controlled bytes. Seeded GC stress with FROMSPACE_SCAN_ABORT=1 survives 0 of 10 seeds on base and 10 of 10 here. A field is a conduit, not a source: the only way a tag-band NaN enters is an ArrayBuffer float read, so canonicalising those is sufficient. Perry already enforces the same invariant for Array<number> on the store side (array/header.rs:841); typed arrays are the one class where it has to be on the read. Rejected, with reasons recorded in the PR: moving the tag band (no NaN-free window exists in NaN space, either sign); fixing it at the decode (0x7FFE... is simultaneously a valid int32 box and a valid NaN); JSC's +/-2^49 offset (charges every double rather than only NaNs); canonicalising at the raw-to-boxed boundary (a perry value IS a double, so that boundary is not a syntactic site); and canonicalising only in-band NaNs, which is unsound — a signalling NaN quiets INTO the band, and fneg/fabs move negative payload NaNs in. Float64Array element read 6.04 instr -0.006% every #10777 and #10761 row 0 h += a[k&255] inline tier 28 -> 30 Float32Array inline tier 41 -> 46 No row where perry beats node regresses. NaN payload bits are no longer preserved through a JS number: 101 of 144 cases differ in bits only, nothing semantic. node preserves them; this matches JSC and SpiderMonkey. It is spec-permitted and unavoidable under any sound design.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
2 participants
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.
Merge train 235 — a memory-safety fix, released as v0.5.1614.
Contents
fix(codegen,runtime): canonicalise NaNs read out of anArrayBufferso user bytes cannot forge a boxed value — fixes #10779perf(codegen): unary+proves a Number by construction, with no operand condition#10785 contains the whole perf chain as ancestry; #10731/#10746/#10752 landed as v0.5.1611 and #10766/#10774/#10767/#10776 as v0.5.1613, so only these two commits are unique and only these two were picked.
This is a pointer forged from caller-controlled bytes
A double whose bit pattern falls inside the NaN-box tag window, written into an
ArrayBufferand read back through a float view, decoded as its payload rather than as the NaN it is — andNumber.isNaNthen answeredfalse, so the one defensive check a program would actually write agreed the value was fine.Reproduced on
main(v0.5.1611) before queueing, and re-checked against this train's own pinned artifacts after:0x7FF9_0000_0000_0001number/isNaN=truetypeof=string/falsenumber/true✓0x7FFD_0000_0000_0001number/isNaN=truetypeof=object/falsenumber/true✓0x7FFF_0000_0000_0001number/isNaN=truetypeof=string/falsenumber/true✓0x7FFE_0000_1234_5678number/isNaN=truenumber/falsenumber/true✓typeof === "string"on bytes the caller wrote means the runtime was prepared to read that payload as aStringHeader*. The author measures 80 of 144 probe patterns diverging on base, 32 of them SIGSEGV, and seeded GC stress withPERRY_GC_FROMSPACE_SCAN_ABORT=1surviving 0 of 10 seeds on base against 10 of 10 on the fix. Any path that moves a double out of binary data reaches it: a file, a socket, WASM memory, a GPU buffer, a C or Rust struct.Why the fix goes on the read, and not somewhere cheaper
A field is a conduit, not a source. The only way a tag-band NaN enters the system is an
ArrayBufferfloat read, so canonicalising there is sufficient —numeric_fieldsthen cannot hold one. Perry already enforces the same invariant forArray<number>on the store side (array/header.rs:841); typed arrays are the one class where it has to be on the read.The author's rejected-alternatives table is the part worth keeping, because each entry is a trap a later reader would otherwise walk back into:
0x7FFE…12345678is simultaneously a valid int32 box and a valid NaN; undecidable there0x7FF7… * 1forged aStringHeader*and SIGSEGV'd), andfneg/fabsmove negative payload NaNs inTwo behavioural notes, stated rather than buried
NaN payload bits are no longer preserved through a JS number — 101 of 144 cases differ in bits only, nothing semantic. Node preserves them; this now matches JSC and SpiderMonkey. Spec-permitted, and unavoidable under any sound design, but it is a real difference from the oracle.
Two rows regress, and neither is one where perry beats node:
h += a[k&255]inline tier 28 → 30, and theFloat32Arrayinline tier 41 → 46. The second is +12% on a path that already loses to node (41 against 15.9). TheFloat64Arrayelement read moves −0.006%, and the eleven #10777/#10761 parity rows are flat at 0.PERRY_NANBOX_CANON=0reproduces base exactly, which is the right arm-distinctness control. Per CLAUDE.md's knob kill-policy it now needs a CI arm exercising its off state, or a plan to delete it after one release of soak — flagged on the PR.Two "missing" insertions, both correct absences
The line-level check flagged five lines in #10785, both artefacts of train 233 landing after that branch was cut:
lower_lru_cache_subclass_init(0 occurrences onmain) and agc_runtime_root_holders.jsonentry forcrates/perry-stdlib/src/commander.rs(file absent frommain). Restoring either would have referenced something that no longer exists;gc_runtime_root_holders.pypasses on this tree.Validation
Assembled on
afd77dbe30; both source heads asserted fresh; no attribution trailers. Nine cheap gates,cargo check --workspace --all-targetsunder-D warnings, all five pinned artifacts byte-identical before and after the sweep, six unit suites with an empty failing set, andlintcomplete at 6-of-6 with nothing outside the known-red public-baseline step.Gap sweep at
PERRY_RUN_TIMEOUT=30, nine areas weighted at the fix's blast radius — every read of a double out of binary data — every area asserted live, zero unexplained regressions: