Repository navigation
[JSC] PutCellButterflySlot forbids the double format for the variable it stores - #792
Conversation
… it stores
The DFG and the FTL inline Array.prototype.sort for at most 16 elements
(ByteCodeParser::handleArraySort). The insertion sort keeps its pivot in a
variable, passes it to the inlined comparator as the first argument, and
stores it back into the scratch with PutCellButterflySlot. That store writes
the value as is, the loop reads the scratch of an Int32 array back as Int32s
with no check, and ArraySortCommit copies the scratch as is into the array.
A comparator that does double arithmetic on its first parameter makes double
voting give that variable the double format. The pivot of an Int32 array then
goes back into the scratch as a boxed double:
function f(v, w, x, y) { return [v, w, x, y].sort((a, b) => (a + 0.5) - (b + 0.5)); }
Once the DFG has compiled f, f(17, 8, 1, 1) returns the elements out of
order, as boxed doubles in an ArrayWithInt32. Element reads give 8,1,1,17,
join() gives "0,0,0,17" and indexOf(8) gives -1. After the first such store
the comparator also gets numbers that are not in the array. FTL code for f
aborts under --validateAbstractInterpreterState=1.
Double voting now merges CantUseDoubleFormat into the variable that a
PutCellButterflySlot stores. Only the double format changes the bits that go
back: a variable that holds an Int32 or a JSValue gives back the value that
came out of the array. The pivot of such a sort is now a JSValue, and the
comparator converts it where it uses it. A sort whose pivot was not in the
double format gets the code it got before.
The inlined sort is upstream's (WebKit#63908), and upstream main
returns the same arrays.
* JSTests/stress/array-sort-inline-int32-double-comparator.js: Added.
* Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:
|
Status: ready for review. How to reproduce (Release function f(v, w, x, y) { return [v, w, x, y].sort((a, b) => (a + 0.5) - (b + 0.5)); }
noInline(f);
var r;
for (var i = 0; i < 20000; i++) r = f(17, 8, 1, 1);
print(r[0] + "," + r[1] + "," + r[2] + "," + r[3], r.join(), r.indexOf(8));
|
![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; because it changes DFG double-format voting (a tier-wide compiler correctness decision) and I could not run the new stress test in this checkout, a human JSC look is still worthwhile.
What was reviewed:
- Confirmed child3 is the stored value for both PutCellButterflySlot sites in handleArraySort; the pivot store at DFGByteCodeParser.cpp:14253 is a GetLocal in a fresh block (no earlier SetLocal of tmpPivot there, so CPS rethreading cannot replace it), and the shift store at :14237 stores a GetCellButterflySlot result, not a local.
- Traced how the pivot gets double votes: the inlined comparator's get() of its first argument resolves to the same GetLocal(tmpPivot) node in-block, so ArithAdd votes land on the pivot's VariableAccessData; merging CantUseDoubleFormat there is checked before tallyVotes in the same round and is idempotent across rounds.
- Checked the voteChildren call: child1 (scratch cell) gets a harmless VoteValue, child2 (ArithAdd) is a no-op in voteNode, matching the previous default-case behavior.
- Unconditional application (no array-mode gate) only costs a per-call DoubleRep in the comparator; it cannot produce wrong results for Double/Contiguous scratches.
Extended reasoning...
The change adds one case to doDoubleVoting in Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp that forbids the double flush format for a local stored by PutCellButterflySlot, plus a new stress test in JSTests/stress; it touches no security-sensitive surface (no auth, injection, or data exposure). The logic is small and consistent with the parser's node shape and the VariableAccessData state machine, and it is only reachable from the inlined Array.prototype.sort expansion. It is not approved outright because it is a JIT type-inference correctness fix whose effect depends on how later phases (fixup, CPS/SSA conversion) consume the flush format, no jsc build exists in this checkout to confirm the test fails before and passes after, and the changed paths fall under the /Source/JavaScriptCore and /JSTests CODEOWNERS entries.
|
Preview build of 252ca22: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe DFG prediction pass now handles ChangesInt32 Array Sort Comparator Prediction
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the primary fix: preventing PutCellButterflySlot from allowing the double format for the stored variable. |
| Description check | |
The description is detailed and directly explains the bug, cause, fix, affected files, reproduction, and verification results. It does not include the required Bugzilla URL, Reviewed by NOBODY marker,… |
Full details: Description check
-
Explanation
The description is detailed and directly explains the bug, cause, fix, affected files, reproduction, and verification results. It does not include the required Bugzilla URL, Reviewed by NOBODY marker, or a template-formatted changed-file list, but the core information is complete.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.
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 @coderabbitai help to get the list of available commands.
|
A second pull request needs this change under it: #806, the cherry-pick of WebKit#76845. With #806 the division in function f(v, w, x, y) { return [v, w, x, y].sort((a, b) => (!(a / 4) | 0) - (!(b / 4) | 0)); }20,000 calls of
With this change under #806 the results are also equal to the interpreter's ( Upstream main has both WebKit#76149 and WebKit#76845 now. The next upstream sync brings the two changes with or without #778 and #806. |
…ned-sort-keeps-element-values
There was a problem hiding this comment.
I reviewed the updated version (merge of main plus the shortened test and comment) and found no bugs; since this is a DFG/FTL correctness fix in the inlined Array#sort path, a maintainer look is still worthwhile.
What was reviewed:
- Confirmed child3 is the value operand of PutCellButterflySlot at both parser sites (DFGByteCodeParser.cpp:14323 and :14339) and the pivot site always emits a fresh GetLocal after clearCaches, so the GetLocal check covers the only case where a DFG variable is stored.
- Confirmed the merge to CantUseDoubleFormat runs inside doDoubleVoting before tallyVotesForShouldUseDoubleFormat in every round, uses find() for unified variables, and the inlined sort only supports Int32/Contiguous (no Double) array modes, so the rule is safe for all inlined shapes.
- The shortened test no longer has the //@ runNoCJIT directive and no longer compares against a noDFG reference; I could not run it here, so whether this final form still fails on a build without the fix is worth a quick check.
Extended reasoning...
The change adds one case to the double-voting switch in Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp that votes VoteValue for PutCellButterflySlot children and forbids the double format for a GetLocal stored as the value, plus a 14-line stress test. It touches no security-sensitive surface; it is a JIT type-format correctness fix shared by the DFG and FTL tiers. The logic is small and I verified the operand position, the voting-round ordering, and the supported array modes against the parser and backend code, but it sits in JSC compiler internals and the simplified test's ability to reproduce the bug could not be confirmed without a build, so a human look is the better call than an automated approve.
Bumps `WEBKIT_VERSION` from `b1e30d08cb14` to `87ee10da087b` (current oven-sh/WebKit `main`). The `autobuild-87ee10da087b63e011bd1ada64d1814a850a4395` release exists. oven-sh/WebKit changes picked up: - [JSC] PutCellButterflySlot forbids the double format for the variable it stores (oven-sh/WebKit#792) Checked locally on a debug build, whose `process.versions.webkit` is `87ee10da087b63e011bd1ada64d1814a850a4395`: `[17, 8, 1, 1]` passed as four arguments to `(v, w, x, y) => [v, w, x, y].sort((a, b) => (a + 0.5) - (b + 0.5))` comes back as `1,1,8,17` in 200,000 of 200,000 calls. Bun 1.4.3 returns a wrong array in 192,141 of them. Everything else is left to CI.
Problem
An inlined
Array#sortof an Int32 array can return its elements out of order, as boxed doubles.8,1,1,17 0,0,0,17 -1ASSERTION FAILED: result.isInt32() || !resultinJSObject::tryGetIndexQuickly1,1,8,17 1,1,8,17 2In Bun 1.4.3 the same function returns a wrong array in 197,490 of 200,000 calls.
Cause
handleArraySortkeeps the pivot of its insertion sort in a DFG variable (tmpPivot,DFGByteCodeParser.cpp:14220) and stores it back withPutCellButterflySlot(:14339), which writes the bits of the JSValue as they are.GetCellButterflySlotis typedSpecInt32Onlywith no check, andArraySortCommitcopies the scratch into the array as is. So every store into the scratch has to be a boxed Int32.GetLocalof the pivot. Double arithmetic on it votes the variable into the double format. Main then hasSetLocal(DoubleRep, tmp2(MC<Number>/FlushedDouble))and aValueRep(DoubleRep)in front of the store, and a boxed double goes into the scratch.Fix
doDoubleVotingmergesCantUseDoubleFormatinto the variable that aPutCellButterflySlotstores. The pivot is thentmp2(MC~<Int32>/FlushedJSValue), and the store takes itsGetLocaldirectly.For a Contiguous array the rule has no effect: the pivot is predicted
SpecBytecodeTop, and only a variable with a number prediction can take the double format.Why this is a JSC bug
jscshell.handleArraySortis line for line the same here and on WebKit main f71518e (2026-10-16). The only difference of the fork in double voting is a case forBufferWrite. No upstream commit after the fork's merge point (23b0720) touchesGetCellButterflySlot,PutCellButterflySlot,ArraySortCompact,ArraySortCommitorhandleArraySort. This was compared by source. No upstream binary was run.USE(BUN_JSC_ADDITIONS)guard.Verification
jscof main b1e30d0 and of this branch on it, Linux x64, Release with and without assertions.array-sort-inline-int32-double-comparator.js, 17 configurationssortin its name (43 files, 667 runs), assertionsbad value: 0,0,0,17 expected: 1,1,8,17. It passes on main inlockdown,no-cjit-validate-phasesandftl-no-cjit-no-inline-validate. Slowest configuration on this branch: 17 ms (Release), 35 ms (assertions).Measurements
Probe. 41 comparators by 8 array lengths (2, 3, 4, 5, 8, 15, 16, 17) give 328 rows. A row is 3,000 sorts of random Int32s. Each result is compared with the result of a copy of the function that has the DFG off. In the 64 rows whose comparator is not consistent, where the order is up to the implementation, the check is that the result has the same elements and that they are Int32s.
--useConcurrentJIT=0and with--useConcurrentJIT=0 --useFTLJIT=0.--useConcurrentJIT=0alone, and with each of--useFTLJIT=0,--validateAbstractInterpreterState=1,--validateGraphAtEachPhase=1 --validateGraph=1,--doubleVoteRatioForDoubleFormat=0,--useFTLJIT=0 --doubleVoteRatioForDoubleFormat=0.01--useConcurrentJIT=0with tier-up thresholds of 10 and 20, and with--doubleVoteRatioForDoubleFormat=0Sorts that are right on main. The only code that tells
CantUseDoubleFormatfromNotUsingDoubleFormatisArgumentPosition, which merges the states of the variables of an argument. The pivot is a private tmp and is in no argument position.The 41 comparators at lengths 4 and 8 give 82 rows per tier. The node lists come from
--dumpDFGDisassembly=1and--dumpFTLDisassembly=1, with addresses and structure names masked, 5 runs per binary, most frequent list compared.ValueRep(DoubleRep)The 18 other FTL rows are all of length 8, where runs of one binary differ from each other too (the most frequent list of main has 2 to 4 of 5 runs). 13 of the 18 have no inlined sort in their FTL code. For the other 5, 14 runs per binary give 3 to 7 distinct lists per binary, and 3 or 4 of them occur in both.
Instructions in user space (
perf stat, millions, least of 3 runs in rotated order) for 3,000,000 sorts of 8 Int32s in reverse order,--useConcurrentJIT=0. "main again" is a copy of the same binary.x - y, DFGx - y, FTLK[x] - K[y], DFGK[x] - K[y], FTLSorts that change.
(x + 0.5) - (y + 0.5)over 8 Int32s in reverse order, where an insertion sort needs 28 comparisons:Main runs fewer instructions because it makes fewer comparisons.
Alternative. One line in the parser,
mergeShouldNeverUnbox(true)on the pivot, also has 0 wrong rows of 328. It changes the DFG code of sorts that are right on main: 6,280.3 forx - y(-0.3%) and 7,456.8 forK[x] - K[y](+0.9%) in the table above. In the FTL it gives 2,382.7 and 3,411.2.