Repository navigation
FFI: convert Number arguments of i64/u64 parameters modulo 2^64 - #421
Conversation
writeInt64Slot converted a double-encoded Number with the CPU's truncating double -> int64 instruction (FFI::doubleToInt64). For u64 / u64_fast that has no representation for values in [2^63, 2^64): every such Number arrived in the callee as 9223372036854775808 on x86-64 (cvttsd2si's indefinite value) or 9223372036854775807 on arm64 (fcvtzs saturates). NaN and values outside the int64 range likewise got a different answer on each CPU. Use JSC::toInt64 instead: truncate toward zero and keep the low 64 bits of the two's complement result, with NaN and the infinities becoming 0. That is the ToInt32 conversion writeIntegerSlot already applies to the narrower integer types, widened to 64 bits, and it is what the int32 encoding and the BigInt branch of the same function already produce, so a Number now reaches an i64 / u64 parameter (or leaves an i64 / u64 callback) with the same bits however the engine happened to box it and as the BigInt of the same value. The signed and unsigned types share that bit pattern, so the isUnsigned split goes away, and doubleToUInt64 has no remaining user. doubleToInt64 itself stays the pointer conversion, where the IC stub and DFG inline the same instruction. testFFI pins the slot bits for the new cases; the JSTests batteries gain the Numbers that used to be CPU-specific, and the fuzzer's reference model now covers every double edge instead of excluding |d| >= 2^63.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it changes the user-observable semantics of bun:ffi's Number→i64/u64 argument conversion and is coordinated with a companion Bun-side change, a human sign-off on the chosen semantics would still be worthwhile.
What was reviewed:
writeInt64Slotnow usesJSC::toInt64(modular, CPU-independent); confirmedMathCommon.hprovides it and the include was added.- Verified the IC stub and DFG codegen send double-encoded i64/u64 args to the slow path (
operationFFIWriteSlot), so this one change covers every tier as claimed;truncateDoubleToInt64remains only on the pointer path. doubleToUInt64removal — grep confirms no remaining references;<bit>header drop from FFIConversions.h is safe (only user was the removed inline).- Collapsing the
isUnsignedsplit —toBigInt64/toBigUInt64and signed/unsignedtoInt64yield identical bit patterns, so the simplification is sound.
Extended reasoning...
Overview
The PR changes writeInt64Slot in Source/JavaScriptCore/ffi/FFIConversions.cpp to convert double-encoded JS Numbers via JSC::toInt64 (ECMAScript-style modular conversion: truncate toward zero, keep the low 64 bits, NaN/±∞ → 0) instead of FFI::doubleToInt64 (raw cvttsd2si/fcvtzs, whose out-of-range result differs between x86-64 and arm64). It removes the now-redundant signed/unsigned split and the doubleToUInt64 helper, adds the MathCommon.h include, and expands C++ (testFFI.cpp) and JS stress tests with the previously CPU-specific edge values pinned to literal expectations.
Security risks
None identified. The change is a numeric conversion policy inside the FFI argument marshalling path; it does not touch pointer handling, buffer bounds, auth, or anything that could widen an attack surface. The pointer conversion (writePointerSlot, doubleToInt64, inline truncateDoubleToInt64) is explicitly left unchanged.
Level of scrutiny
Moderate-to-high. The code diff itself is small and mechanically clean, and I verified that (a) toInt64 exists in MathCommon.h with the described semantics, (b) both FFIICStub.cpp and FFIDFGCodegen.cpp route double-encoded values for the 64-bit integer types through the slow path into operationFFIWriteSlot → writeInt64Slot, so no JIT tier is left on the old hardware truncation, and (c) doubleToUInt64 has no remaining callers. However, this is a deliberate semantic change to a public bun:ffi conversion (how out-of-range/non-finite Numbers land in i64/u64 parameters), coordinated with a companion Bun repo PR for the cc() path. That is a design decision — modular wrap vs. saturate vs. error — that a maintainer should ratify even though the chosen behavior (matching JSValueToInt64/ToBigInt64 and the existing int32/BigInt branches) is well-argued.
Other factors
- Test coverage is thorough: hardcoded literals in
testFFI.cppand four stress files exercise the host path, IC stub, and DFG/FTL tiers against identical expected values, plus the fuzzer's reference model was updated to the new semantics and its input generator un-restricted. - The
isUnsignedcollapse is correct becauseJSBigInt::toBigInt64andtoBigUInt64produce the same 64-bit pattern (only the interpretation differs), andstatic_cast<uint64_t>(toInt64(d))is exactlytoUInt64(d). - Removing
<bit>fromFFIConversions.his safe — its only user was the deletedstd::bit_castindoubleToUInt64;FFIConversions.cppstill includes<bit>for its own uses. - No prior human reviews or unresolved comments on the PR; only a CodeRabbit rate-limit notice.
Preview Builds
|
2 participants
Problem
u64/u64_fastargument passed as a JS Number in[2^63, 2^64)reaches the callee as9223372036854775808on x86-64 and as9223372036854775807on arm64, whatever the Number was (bun:ffidlopen()/linkSymbols()/CFunction;2 ** 63 + 2 ** 62arrives as2 ** 63). BigInts of the same values are exact.NaNand Numbers outside the int64 range reachi64/u64parameters asINT64_MINon x86-64 and as0/ the saturated value on arm64.writeInt64Slot(ffi/FFIConversions.cpp) converts a double-encoded Number withFFI::doubleToInt64, which iscvttsd2sion x86-64 andfcvtzson arm64. A signed truncating instruction has no result for[2^63, 2^64), and the two CPUs define the out-of-range result differently.doubleToUInt64onlybit_castthe signed result.Fix
writeInt64Slotconverts a double withJSC::toInt64(runtime/MathCommon.h): truncate toward zero, keep the low 64 bits of the two's complement result,NaN/ infinities become0.-1already gave au64all ones) and the BigInt branch usestoBigUInt64(modulo 2^64). It is alsowriteIntegerSlot's ToInt32 conversion for the 8/16/32-bit types, widened to 64 bits, and the pairing JSC's own C API uses (JSValueToInt64/JSValueToUInt64inAPI/JSValueRef.cpp:JSBigInt::toBigInt64for BigInts,JSC::toInt64for Numbers). A Number therefore reaches the callee with the same bits however the engine boxed it and as the BigInt of the same mathematical value, on every CPU.isUnsignedsplit inwriteInt64SlotanddoubleToUInt64(no other users) are removed.doubleToInt64stays as the pointer conversion, whereFFIICStub.cppandFFIDFGCodegen.cppinline the same instruction (truncateDoubleToInt64); pointer semantics are unchanged.writeSlotFromJSValue->writeInt64Slot: the host call path (FFICallHost.cpp),operationFFIWriteSlot(the IC stub and the DFG send every non-int32, non-BigInt value there), and callback returns (FFICallbackThunk.cpp), so one change covers every tier and both directions.>= 2^63, or non-finite change behavior; in-range values, the int32 encoding and BigInts produce the same bits as before.Tests
testFFI.cpp:testConversionspins the slot bits for the new cases (double-encoded-1.0and-1.5intou64,2^63 + 2^62, the largest double below2^64,2^64wrapping to0,NaN/ infinities to0, the same fori64and the_fastvariants) instead of deriving the expectations fromdoubleToInt64; thedoubleToUInt64corpus loop goes with the function.testDoubleToInt64still pins the CPU-specific pointer conversion.JSTests/stress/ffi-types-echo.js,ffi-host-path.js,ffi-tier-differential.js: the batteries gain the Numbers that used to be CPU-specific, so the host path and every JIT tier are checked against the same literals.JSTests/stress/ffi-fuzz-signatures.js: the reference model fori64/u64is now the modular conversion and the generator feeds it every double edge (it previously had to exclude|d| >= 2^63and non-finite values).FFIConversions.cpp,testFFI.cppand the other includers ofFFIConversions.hcompile (-fsyntax-only) against the headers of the current prebuilt; the binaries and the stress files run in this PR's CI and in the Bun bump PR (linked below), whosetest/js/bun/ffitests compare this path againstcc()on the same inputs.Background
bun:ffihas two argument converters:cc()compiles its own C glue with TinyCC (Bun'ssrc/runtime/ffi/FFI.h), everything else goes through this engine FFI. The Bun PR fixes thecc()side of the same conversion (it dropped the sign of double-encoded negatives) to the definition used here.Math.*results), so-1can arrive either way; the two encodings have to convert identically or the callee sees different values for the same program depending on JIT state.