testFFI: compare NaN returns semantically in the invoke-thunk differential - #367
Conversation
…ntial The invoke-thunk differential calls each fixture two ways -- through the generated thunk and through a templated reference call -- with identical argument slots, and compared the raw return slots bitwise. Fixtures like ffi_mix_1 return a sum over their arguments, so when two argument NaNs are in play (the f64 sNaN slot and the f32 sNaN slot) the result's NaN payload is the payload of whichever operand is 'first' in an addsd. Addition is commutative, so the compiler may legally flip operand order, and it does so differently for the inlined reference call under -O2 than for the outlined body reached through the thunk. That surfaced as 0x7ff8000020000000 != 0x7ff8000000000001 on x86-64 release only: same delivered arguments, same body, different-but-legal instruction operand order. The identical binary passes under Rosetta 2, which does not emulate x86 NaN-payload propagation -- consistent with a payload artifact rather than a marshalling error. NaN payloads are implementation-defined, so treat two NaN return slots of the same type as equal. Real value mismatches, NaN-vs-number, and infinities still compare bitwise.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 21 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 (1)
WalkthroughChangesFFI Result Validation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, test-only relaxation with a well-documented root cause; the inline nit about zero/one-arg fixtures is non-blocking.
Extended reasoning...
Overview
This PR touches a single function in Source/JavaScriptCore/ffi/tests/testFFI.cpp — the invoke-thunk differential harness. It changes the return-slot comparison from strict bitwise equality to bitwise-first-then-semantic-NaN: if the raw 64-bit slots differ and the return type is Float/Double, both sides are reinterpreted as their FP type and compared with std::isnan. All other types, all non-NaN FP values (including infinities), and NaN-vs-number remain bitwise-strict.
Security risks
None. This is test-harness code in a standalone test binary (testFFI), not shipped in the JSC runtime or reachable from JavaScript. The change only relaxes an assertion; it introduces no new inputs, allocations, or control flow reachable from untrusted data.
Level of scrutiny
Low. Test-only, ~5 lines, mechanically simple, and the PR description gives a precise root cause (compiler-chosen addsd operand order producing different-but-legal NaN payloads between the inlined -O2 reference call and the outlined thunk body) with a corroborating observation (passes under Rosetta 2, which doesn't emulate x86 NaN-payload propagation). I confirmed canonicalizeSlot stores Float in the low 32 bits (raw & 0xffffffffull), so the static_cast<uint32_t> truncation before bit_cast<float> is correct, and Double slots are stored raw so bit_cast<double> on the full 64 bits is correct.
Other factors
The bug hunter flagged one nit: the NaN fallback also covers zero/one-arg FP fixtures (ffi_ret_impure_nan_f64, ffi_ret_nan_f32, ffi_echo_f*) that have no operand-order nondeterminism, so a future thunk regression that quiets an SNaN or drops a payload bit would now pass silently in the differential. That's a real but minor loss of coverage — those fixtures' primary purification-before-boxing check at lines 2077-2090 is untouched — and the suggested sizeof...(Arguments) >= 2 gate is an easy follow-up if desired. Not worth blocking a CI-unblocking fix on.
The operand-order nondeterminism that justifies treating two NaN return slots as equal only exists when two or more argument NaNs are in play. For zero/one-argument fixtures the bitwise check is sound and would catch a future thunk that quiets an SNaN or canonicalizes a payload, so apply the fallback only when sizeof...(Arguments) >= 2.
There was a problem hiding this comment.
LGTM — thanks for gating the fallback on sizeof...(Arguments) >= 2; the zero/one-arg NaN fixtures keep strict bitwise comparison and only the multi-arg sum cases get the semantic-NaN relaxation.
What was reviewed:
- Confirmed
canonicalizeSlotstores Float in the low 32 bits, sobit_cast<float>(static_cast<uint32_t>(slot))recovers the right bits forisnan. - Non-FP returns, NaN-vs-number, and infinities still fall through to the bitwise
actual == expectedcheck. - The
if constexprgate compiles the fallback out entirely for theffi_ret_*_nan_*/ffi_echo_*instantiations.
Extended reasoning...
Overview
Test-only change to Source/JavaScriptCore/ffi/tests/testFFI.cpp (+9/-1) in the differentialCase template. The invoke-thunk differential previously compared the raw 64-bit return slot bitwise against the native reference call. For multi-argument sum fixtures (ffi_mix_*), two NaN inputs make the resulting NaN payload depend on addsd operand order, which the compiler may (and does) choose differently for the inlined reference vs. the outlined thunk-reached body — a legal codegen difference, not a marshalling bug. The fix keeps the bitwise check as the primary comparison but, when it fails and the return type is Float/Double and the fixture has ≥2 arguments, falls back to isnan(actual) && isnan(expected).
Security risks
None. This is a standalone test binary comparing return values; no runtime, JIT, heap, or API code is touched.
Level of scrutiny
Low. Test-only, ~9 lines, unblocks flaky release-build CI lanes. The rationale (IEEE 754 NaN payloads are implementation-defined for arithmetic results) is sound, and the Rosetta 2 observation in the PR description corroborates that this is a payload-propagation artifact rather than a real thunk defect.
Other factors
- My previous review flagged that the original patch also relaxed zero/one-argument NaN fixtures where bitwise comparison was still meaningful. The author addressed this exactly as suggested by wrapping the fallback in
if constexpr (sizeof...(Arguments) >= 2), and the thread is resolved. - Verified the Float path:
canonicalizeSlotmasks Float slots to the low 32 bits (raw & 0xffffffffullat line 1363), sostatic_cast<uint32_t>→bit_cast<float>correctly reconstructs the value for theisnancheck. - Non-NaN mismatches, NaN-vs-finite, and ±∞ still hit
returnsEqual = actual == expectedand fail as before. - Author reports 9519/0 locally after the follow-up commit.
Preview Builds
|
The invoke-thunk differential compared NaN return payloads bitwise; for multi-argument sum fixtures the payload legally depends on addsd operand order, which the compiler flips differently for the inlined reference call versus the outlined body. That failed only on x86-64 release hardware. #367 compares NaN returns semantically for the multi-argument cases (single/zero-argument fixtures keep the strict bitwise check). Verified: the shipped x64 testFFI from this preview reports 9524/0.
The invoke-thunk differential compared NaN return payloads bitwise; for multi-argument sum fixtures the payload legally depends on addsd operand order, which the compiler flips differently for the inlined reference call versus the outlined body. That failed only on x86-64 release hardware. #367 compares NaN returns semantically for the multi-argument cases (single/zero-argument fixtures keep the strict bitwise check). Verified: the shipped x64 testFFI from this preview reports 9524/0.
The invoke-thunk differential compared NaN return payloads bitwise; for multi-argument sum fixtures the payload legally depends on addsd operand order, which the compiler flips differently for the inlined reference call versus the outlined body. That failed only on x86-64 release hardware. #367 compares NaN returns semantically for the multi-argument cases (single/zero-argument fixtures keep the strict bitwise check). Verified: the shipped x64 testFFI from this preview reports 9524/0.
1 participant
The invoke-thunk differential compared raw return slots bitwise. For fixtures whose return is a sum over the arguments (
ffi_mix_1,ffi_mix_8), two argument NaNs make the result's NaN payload depend onaddsdoperand order — which the compiler may legally flip (addition is commutative), and does flip differently for the inlined-O2reference call versus the outlined body reached through the thunk. On x86-64 release this surfaced as0x7ff8000020000000 != 0x7ff8000000000001: identical arguments, identical body, different-but-legal instruction operand order. The same binary passes under Rosetta 2, which does not emulate x86 NaN-payload propagation, consistent with a payload artifact rather than a marshalling error.NaN payloads are implementation-defined, so two NaN return slots of the same type now compare equal; real value mismatches, NaN-vs-number, and infinities still compare bitwise.
Follow-up to #319; unblocks the darwin-14-x64 / linux x64 release testFFI lanes on oven-sh/bun#35246.