chore: merge train 238 (v0.5.1617) - #10794
Merged
Merged
Conversation
… emitted IR LLVM rejects (#10779) `LlBlock::fcmp` rendered its operand type as `double` unconditionally, and the in-process LLVM builder hardcoded `"double"` to match. `canonicalize_lane_f32`, added in the previous commit for `Buffer.readFloatLE`/`readFloatBE`, passes a `float`, so it emitted %r2 = fcmp uno double %r1, %r1 ; %r1 is a float and the module was rejected with '%r1' defined with type 'float' but expected 'double' Every program calling `readFloatLE` or `readFloatBE` failed to COMPILE. This is a hard codegen failure, not a wrong value, and it is a regression introduced by the previous commit rather than a pre-existing defect. `LlInst::FCmp` now carries its operand type. `fcmp()` keeps its `double` signature and delegates to a new `fcmp_ty()`, so no existing call site changes; both the text renderer and the in-process builder read the type from the instruction. One construction site, two `..` patterns. Why nothing caught it: the read-shape harness covered `readDoubleLE` (the f64 helper) and no `readFloat*`, and none of the 153 realsuite/rungs/clisuite programs calls one — the f32 arm of a two-arm guard had no coverage by construction. Three unit tests now pin it, each sabotage-proven: * f32_canonicalisation_compares_as_float_not_double — flipping `F32` back to `DOUBLE` fails it with `left: "double", right: "float"`. * f64_canonicalisation_compares_as_double — so the f32 case cannot be "fixed" by widening both. * both_select_the_canonical_quiet_nan_on_the_nan_arm — pins 0x7FF8000000000000 on the is-NaN arm and the original value on the other. `readFloatLE`/`readFloatBE`/`readDoubleBE` are also added to the read-shape harness; on the pre-fix runtime the two readFloat rows SIGSEGV. The `#[rustfmt::skip]` on the `I::FCmp` match arm keeps the five-field pattern on one line: `dialect/mod.rs` sits against the 2000-line file-size gate.
…rom an ArrayBuffer (#10779) A C function returning a `double` or a `float` is the same class of source as a `Float64Array` lane: raw native bits that become a JS value with no conversion. Witnessed with a real `perry.nativeLibrary` whose staticlib returns `f64::from_bits(0x7FFE_0000_1234_5678)` and `f32::from_bits(0x7FFF_FFFF)`: before: the f64 return prints 305419896 with `Number.isNaN` false, and the f32 return SEGFAULTS — it widens to 0x7FFF_FFFF_E000_0000, a forged StringHeader*, which is then dereferenced. after: both are NaN, and a control function returning 2.5 is unchanged. Three sources closed, at the source rather than at the consumer: * the C `float` return, before its `fpext`; * the C `double` return, but ONLY when the manifest descriptor is `F64`. That arm also serves Perry's own double-based ABI, where the returned double ALREADY IS a NaN box — canonicalising it would destroy every tag it carries. `JsValue` and a missing descriptor are left untouched. * `load_pod_field_native` for `F64`/`F32` fields: a POD record's backing memory is a native struct, written by C, by Rust, or by a previous native store, so its float fields can hold any NaN. The integer field reps cannot be NaN and pay nothing. This one is guarded on the same reasoning as its neighbour but is NOT witnessed — no PerryPod fixture was built. This does NOT change #10777's precondition either way: `expr_numeric_by_construction` has no `Expr::Call` arm, so an FFI return can never make a `numeric_fields` slot. The precondition was already discharged by the ArrayBuffer sources; this closes the hazard on its own account. Cost: zero on every measured row. The per-op table is unchanged from the previous commit — +2 on two Float64Array read shapes that already lose to node, +5 on the Float32Array shape, and +0 on every #10777 and #10761 row, including `h += p[k & 255]` through a typed parameter, which stays at 13 instructions against node's 14.10.
|
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 (9)
✨ 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 238 — a compile break on
main, released as v0.5.1617.Contents
fix(codegen): givefcmpan operand type, and canonicalise NaNs arriving from native code — fixes theBuffer.readFloat*break and closes the second NaN sourceBuffer.readFloatLEhas not compiled since v0.5.1614Merge train 235 landed #10779's NaN canonicaliser.
canonicalize_lane_f32emits anfcmpon anf32lane, butLlBlock::fcmprendered its operand type asdoubleunconditionally (inst.rs:197), and the in-process LLVM builder hardcoded"double"too (dialect/mod.rs:1356). Any module callingBuffer.readFloatLEorreadFloatBEfails to compile — a hard codegen failure, not a wrong value.Reproduced with pinned binaries on a three-line program, and re-checked against this train's own artifacts:
readFloatLE/readFloatBE/readDoubleLE/readDoubleBEreadFloatLEtypeof=number isNaN=trueLlInst::FCmpnow carries its operand type;fcmp()keeps itsdoublesignature and delegates to a newfcmp_ty(), so no existing call site changes.Why nothing caught it — the part worth keeping
The read-shape harness used for #10779 contained
readDoubleLEand noreadFloat*, and none of the 153 programs in the suites calls one. The guard that PR added therefore had zero coverage on one of its two arms — untested by construction rather than by oversight. The harness is now 54 shapes includingreadFloatLE,readFloatBEandreadDoubleBE; on the pre-fix runtime the first two SIGSEGV, which is the witness that should have existed before the original PR was opened.That is the same failure mode #10787's author found in their own work and caught with a sabotage — "my first correctness set was an absence". Here it shipped.
Also: NaNs arriving from native code
The remaining raw-float source. Witnessed with a real
perry.nativeLibraryreturningf64::from_bits(0x7FFE_0000_1234_5678)andf32::from_bits(0x7FFFFFFF): before, thedoublereturn prints305419896withNumber.isNaNfalse and thefloatreturn SIGSEGVs on a forgedStringHeader*; after,NaN,NaN, and2.5for the unchanged control.Closed at the source, with one deliberate asymmetry: the C
doublereturn is canonicalised only when the manifest declaresF64, because that arm also serves perry's own double ABI where the value already is a NaN box and canonicalising would destroy every tag.load_pod_field_nativeis guarded but not witnessed — noPerryPodfixture was built, and the PR says so.Two gates this driver had never run
repsel-censusandsecurity-auditare sweep-tier jobs that gatemain, and nothing in this validation invoked them — which is how #10793 survived four trains. Both now run, and both reported their known failures on the first try rather than passing silently:The landing gate pins each to its tracked issue, so a different failure in either refuses the train rather than reading as "known red".
Separately,
compiler-output-regressionwas reported as covered by my coverage check and was not — the heuristic matched the shared script name while the driver only ran thecensussubcommand. Run manually against this train's artifacts: both suitesrc=0,failed_workloads: []. Now wired into the driver, keyed on the subcommand and asserting the summary line, because that harness has exited 0 having printed nothing when invoked wrongly.Validation
Assembled on
8e9f6f09e8; source head asserted fresh; PR fully represented, zero missing insertions; no attribution trailers. Nine cheap gates,cargo check --workspace --all-targetsunder-D warnings, all five pinned artifacts byte-identical before and after, six unit suites with an empty failing set,lintcomplete at 6-of-6 with nothing outside the known-red public-baseline step.Gap sweep at
PERRY_RUN_TIMEOUT=30, seven areas aimed at every path that moves a float out of bytes, every one asserted live, zero unexplained regressions: