fix(codegen): Buffer.readFloatLE does not compile on main — give fcmp an operand type (#10779 follow-up) - #10789
proggeramlug wants to merge 3 commits into
Conversation
… emitted IR LLVM rejects (PerryTS#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. Claude-Session: https://claude.ai/code/session_01YaNfLEjMRdhCLtk3SB5MdJ
…rom an ArrayBuffer (PerryTS#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 PerryTS#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 PerryTS#10777 and PerryTS#10761 row, including `h += p[k & 255]` through a typed parameter, which stays at 13 instructions against node's 14.10. Claude-Session: https://claude.ai/code/session_01YaNfLEjMRdhCLtk3SB5MdJ
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughChangesNative floating-point codegen
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 6 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
|
Landed via merge train 238 (#10794) as v0.5.1617 — Your commits are on Verified independently before and after, with pinned binaries rather than from the PR description:
That last row is the arm that had zero coverage, so it is the one I most wanted to see. Your "why nothing caught it" is the most valuable part of the PR and it is quoted in the train body so it reaches the release notes: the #10779 read-shape harness contained Two things I noted rather than changed:
Validation: seven gap areas aimed at every path that moves a float out of bytes ( |
2 participants
Buffer.readFloatLEandreadFloatBEdo not compile onmainright now#10779's NaN canonicaliser landed in merge train 235 (v0.5.1614). The follow-up fix below did not, because I found the break after that PR was already closed. Any module calling
Buffer.readFloatLEorreadFloatBEcurrently fails to compile:This is a hard codegen failure, not a wrong value. The program compiles and prints
1.5on the commit before the canonicaliser.canonicalize_lane_f32(nanbox_inline.rs:63) is reached frombuffer_intrinsic.rs:428on theBuffer.readFloat*path and emits anfcmpon anf32lane — butLlBlock::fcmprenders its operand type asdoubleunconditionally (inst.rs:197), and the in-process LLVM builder hardcodes"double"too (dialect/mod.rs:1356).Fix:
LlInst::FCmpcarries its operand type.fcmp()keeps itsdoublesignature and delegates to a newfcmp_ty(), so no existing call site changes — one construction site and two..patterns. Three regression tests, each sabotage-proven: flippingF32back toDOUBLEfails namingleft: "double", right: "float".Why nothing caught it
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 including
readFloatLE,readFloatBEandreadDoubleBE. On the pre-fix runtime those first two SIGSEGV, which is the witness that should have existed before the original PR was opened.Also included: NaNs arriving from native code
The remaining raw float source.
expr_numeric_by_constructionhas noExpr::Callarm, so an FFI return can never enternumeric_fieldsand #10777's precondition was already discharged — but it is a raw source on its own account, and it is reachable.Witnessed with a real
perry.nativeLibrarywhose staticlib returnsf64::from_bits(0x7FFE_0000_1234_5678)andf32::from_bits(0x7FFFFFFF):doublereturn prints305419896withNumber.isNaNfalse; thefloatreturn SIGSEGVs — the forgedStringHeader*is dereferencedNaN,NaN, and2.5for the unchanged control, plusNaNthrough an object fieldClosed at the source: the C
floatreturn before itsfpext; the Cdoublereturn 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; andload_pod_field_nativefor F64/F32 fields — that last one guarded but not witnessed, as noPerryPodfixture was built.Performance
Every #10777 and #10761 row is unchanged. The one typed-array row where perry beats node —
h += p[k & 255]through a typed parameter — is 13 against node's 14.10, unchanged: a guard that had taken it to 16 was dropped, since every correctness result is identical without it.Rows that do pay (
m_f64_const47→49,m_f64_idx28→30,m_f32_idx41→46) all lose to node by 1.7×–4.5× already, so the cost is invisible there. Making the width-8 canonicalisation cheaper is recorded as its own change with its own measurement.Gates
perry-runtime4074/0 (--test-threads=1),perry-codegen1653/0,perry-hir472/0. 144-probe: 0 crashes, 0 semantic diffs, 101 bits-only. 54 read shapes: 1 diff, the pre-existingFloat64Array.prototype.toString()throw. 27-case cross-module witness: 0 diffs. FFI witness 4/4.cargo fmt --check,check_file_size.sh,gc_runtime_root_holders.py,local_binding_type_audit.pyall clean.manifest_consistencyis red and fails identically on base — pre-existing.Clippy, with a methodology correction worth recording. A whole-workspace run reported a difference; it was an artefact. Clippy only warns on units it actually rebuilds, and
cargo clean -phad left the two arms with different cached sets, so they compiled different target lists. The earlier "clippy identical" claim on the original PR carried the same hazard and merely happened to agree. Since the diff is entirely withinperry-codegen, the comparison that means anything is crate-scoped:cargo clean -p perry-codegenon each arm, thenclippy --release --all-targets— 344 warnings on base, 344 on this branch, tables identical.https://claude.ai/code/session_01YaNfLEjMRdhCLtk3SB5MdJ
Summary by CodeRabbit
Buffer.readFloatLEandBuffer.readFloatBE.