WTF: PreciseSum rounds negative sums to nearest, ties to even (Math.sumPrecise was one ulp off on every exact negative sum) - #458
Conversation
e7353fe to
771322e
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The negative-branch rounding table checks out against the invariants (lower + sub-j chunks contribute strictly less than one guard-bit unit, so each of the four cases in the new comment holds), and the tests are thorough. Because it's a subtle IEEE-754 rounding change in a core numerical primitive, a human look would still be worthwhile.
What was reviewed:
- Traced the four
(ivalue & 3)/(ivalue & 7)cases against the carry-propagation invariant that lower chunks are non-negative and bounded — each case rounds correctly. - Confirmed
(ivalue & 7) == 6correctly encodes "guard bits 10, mantissa odd"; the== 3branch is exclusive soelse ifis right. - Checked the test additions:
-0→-0.0fix,EXPECT_EQ+signbitfor exact comparison, and that the JS stress test's padded/generator variants exercise both accumulator paths.
Extended reasoning...
Overview
The PR fixes the negative-sum rounding branch of XsumSmall::compute() in Source/WTF/wtf/PreciseSum.cpp, which backs Math.sumPrecise. The old code rounded away from zero whenever no lower bits were set, so every exactly representable negative sum came out one ulp too large (e.g. Math.sumPrecise([-1]) → -1.0000000000000002). The fix implements the correct four-way decision from Radford Neal's reference xsum_small_round(): round away only for guard bits 11, or for guard bits 10 on an odd mantissa with no lower bits set. Two test files are updated/added: the TestWebKitAPI C++ test now compares bit-exactly (fixing EXPECT_DOUBLE_EQ's 4-ulp tolerance and the -0 integer-literal bug) and adds 30 rounding cases; a new JS stress test covers 58 cases across both accumulator variants and generator inputs.
Security risks
None. This is pure floating-point arithmetic on already-validated double values with no memory management, parsing, or external input handling in the changed code path.
Level of scrutiny
High — this is IEEE-754 round-to-nearest-ties-to-even logic where correctness depends on a chain of invariants (post-carry-propagation chunk signs, guard-bit assembly, two's-complement negation). I walked through each case: after ivalue = -ivalue, the true magnitude is ivalue minus a non-negative residual strictly less than 0.25 ulp (since lower is masked below the guard bits and lower chunks occupy non-overlapping windows). That makes guard 11 → (0.5, 0.75] ulp above m (round up), 00/01 → ≤ 0.25 ulp (keep m), 10 → (0.25, 0.5] ulp with equality only when the residual is zero (tie → even). The code matches this exactly. The PR description also reports bit-exact agreement with the reference xsum.c on 3.75M fuzz inputs, which is strong independent evidence.
Other factors
The change is small and surgical (one branch, ~10 logic lines), the positive branch is untouched, and test coverage is excellent — the new cases hit every cell of the rounding table on both signs plus the power-of-two-boundary adjustment. The PR description's verification methodology is unusually rigorous. I'm deferring rather than approving only because ties-to-even rounding in a hand-ported numerical algorithm is exactly the kind of code where a second pair of human eyes on the case analysis is cheap insurance, not because I found anything questionable.
771322e to
7051f3b
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is small and the rounding table matches the reference xsum_small_round() case-for-case, but since it's a numerical-correctness change to spec-mandated Math.sumPrecise rounding — and one that also affects upstream WebKit — a human sign-off would still be worthwhile.
What was reviewed:
- Verified the negative-branch rounding table:
(ivalue & 3) == 3and(ivalue & 7) == 6with no lower bits correctly encode "guard bits 11" and "guard bits 10 on odd mantissa, exact tie" respectively; all other cases correctly truncate. - Checked that the positive branch and the power-of-two borrow adjustment above it are untouched.
- Checked the test changes:
-0→-0.0literal fix andEXPECT_DOUBLE_EQ→ exactEXPECT_EQ+ signbit are both correct tightenings; new cases cover each rounding outcome on both signs. - Confirmed the JSTests file's 1000-element padding correctly exercises the
XsumLargepath perMathObject.cpp's threshold.
Extended reasoning...
Overview
This PR fixes a rounding bug in WTF::Xsum::XsumSmall::compute() (Source/WTF/wtf/PreciseSum.cpp), which backs Math.sumPrecise. The negative-sum branch previously rounded away from zero whenever no lower bits were set, causing every exactly-representable negative sum to be off by one ulp (e.g. Math.sumPrecise([-1]) returned -1.0000000000000002). The fix restructures ~15 lines to implement the four-case rounding table from the reference xsum_small_round(): round away from zero only for guard bits 11, or for guard bits 10 on an odd mantissa when it's an exact tie. It also adds 30 new C++ test cases (tightening comparison from 4-ulp EXPECT_DOUBLE_EQ to exact equality plus sign-of-zero) and a new 238-assertion JSTests stress file.
Security risks
None. This is pure floating-point arithmetic with no external input handling, allocation, or control-flow implications beyond the rounding decision.
Level of scrutiny
High. Math.sumPrecise is an ECMAScript-specified function that must return the correctly-rounded sum, so this is a spec-conformance fix. The logic is subtle IEEE 754 round-to-nearest-ties-to-even reasoning over xsum's chunked accumulator representation. The PR description is exceptionally thorough (differential fuzzing against the reference C implementation over 3.75M inputs, cross-checked against exact rational arithmetic), and I independently walked through the four rounding cases and confirmed the bit-mask encoding is right. But the correctness argument hinges on invariants of the carry-propagated chunk representation (lower chunks non-negative, so they subtract from magnitude for negative sums), and the PR notes this bug also exists in upstream WebKit main — both are reasons a human familiar with this port should confirm before merge and consider upstreaming.
Other factors
- No prior reviews or comments on the PR.
- The test-file fix of
-0(integer, i.e.+0.0) to-0.0and the switch to exact comparison are independently valuable — the old harness could not have caught this class of 1-ulp error. - The change is self-contained to one function's else-branch; the positive path and all accumulation code are unchanged, limiting regression surface.
Preview Builds
|
7051f3b to
ffe2633
Compare
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 17 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The negative-branch rounding table now matches xsum_small_round() case for case, and I independently walked through all four guard-bit outcomes against the round-to-nearest-ties-to-even rule — they check out. Because this is a numerical-correctness change to a spec-defined builtin and diverges from upstream WebKit (which still has the bug), a human look would still be worthwhile, if only to decide on upstreaming.
What was reviewed:
- Verified
(ivalue & 7) == 6correctly selects "guard bits 10, odd mantissa" and that theelsemakes the00/01/10-even cases fall through to truncation. - Confirmed the
lower/chunk scan is unchanged in behaviour, only gated more tightly, and that the positive branch is untouched. - Checked the new test expectations (hex-float literals, tie cases at ±MAX_VALUE,
-0→-0.0fix) are self-consistent with IEEE-754 round-to-nearest. - Confirmed the JS stress test's padding scheme (500 cancelling ±2^500 pairs) exercises
XsumLargewithout perturbing the sum.
Extended reasoning...
Overview
The PR changes the negative-sum rounding branch of XsumSmall::compute() in Source/WTF/wtf/PreciseSum.cpp (~10 lines rearranged), adds 30 test cases and tightens the comparison in Tools/TestWebKitAPI/Tests/WTF/PreciseSum.cpp, and adds a new stress test JSTests/stress/math-sum-precise-negative-rounding.js. XsumSmall::compute() is the single rounding step for both accumulator variants behind Math.sumPrecise.
Correctness analysis
I traced the new logic independently. After carry propagation, chunks below the top are non-negative, so for a negative sum the discarded bits reduce the magnitude held in the negated ivalue. With two guard bits below the mantissa, the four cases in the new comment are exactly what round-to-nearest-ties-to-even requires: 11 is > ½ ulp (round away), 00/01 are ≤ ¼ ulp (truncate), 10 is ≤ ½ ulp with equality only when nothing is set below (tie → even). The (ivalue & 7) == 6 test picks out "guard bits 10 and mantissa LSB 1", which is the only tie case where rounding away yields the even result. The old code's second if (!lower) shouldRoundAwayFromZero = true was unconditional on the guard bits, so an exact value (all discarded bits zero) always rounded away — the reported bug.
Security risks
None. This is pure floating-point arithmetic with no external input handling beyond what Math.sumPrecise already accepts; no memory, auth, or injection surface is touched.
Level of scrutiny
Medium-high. The diff is small and mechanical (restoring the reference implementation's decision table), and the PR author verified it bit-for-bit against Radford Neal's xsum.c on 3.75M inputs plus exact rational arithmetic on a 30k sample. But it changes rounding behaviour of a TC39-specified builtin, and the same bug exists in upstream WebKit — a maintainer should be aware this fork is diverging on it and may want to file upstream.
Other factors
Test coverage is thorough: the C++ test now uses exact bit-level comparison (fixing both the 4-ulp EXPECT_DOUBLE_EQ tolerance and the -0-is-an-int literal bug), and the JS stress test covers both accumulator paths via array length and iterable type. No prior reviewer comments to address. I'm deferring rather than approving because numerical rounding in a standard-library builtin is the kind of change a human should sign off on, not because I found anything wrong.
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
ffe2633 to
f423272
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The negative-branch rounding table now matches xsum_small_round() case for case, and I traced each of the four (ivalue & 3) outcomes against the ties-to-even rule with the sign-inverted lower semantics — it checks out. Because this is subtle floating-point rounding logic backing a spec-defined Math.sumPrecise, a human look would still be worthwhile.
What was reviewed:
- The new negative-branch table in
XsumSmall::compute(): verified(ivalue & 7) == 6correctly isolates the odd-mantissa half-ulp tie, and that the00/01/even-10cases all correctly truncate given thatlowersubtracts from magnitude. - Confirmed the
else ifstructure means the old fallthrough (where(ivalue & 3) == 3withlower == 0would set the flag twice, and(ivalue & 3) == 0with no lower bits would wrongly round) is gone. - The test additions:
-0→-0.0fix (integer-0is+0.0),EXPECT_DOUBLE_EQ→EXPECT_EQ+signbit(needed to catch 1-ulp errors), and the new cases cover each rounding outcome on both signs. - Checked the JS stress test's padding scheme reaches the >1000-element
XsumLargepath and the generator variant forcesXsumSmall.
Extended reasoning...
Overview
This PR fixes a rounding bug in WTF::Xsum::XsumSmall::compute() (Source/WTF/wtf/PreciseSum.cpp), which backs Math.sumPrecise. The port of xsum's negative-sum rounding branch had collapsed the original's four-case decision table into two cases, causing every exactly-representable negative sum to be rounded one ulp away from zero (e.g. Math.sumPrecise([-1]) returned -1.0000000000000002). The fix restores the original table: round away from zero only for guard bits 11, or for guard bits 10 on an odd mantissa with no lower bits set. Two test files are updated: the TestWebKitAPI unit test gains 30 targeted cases and switches to exact bit-level comparison, and a new JSTests stress file exercises both accumulator paths through the JS API.
Security risks
None. This is pure numerical computation on double values with no external input handling, memory management, or privilege boundaries. The change narrows the set of inputs that take the round-away path — it cannot introduce new overflow or out-of-bounds behavior relative to the old code (the chunk scan loop is unchanged, just gated more tightly).
Level of scrutiny
Moderate-to-high. The functional change is ~10 lines, but it is subtle IEEE-754 rounding logic in a spec-defined builtin (Math.sumPrecise per the TC39 proposal requires the correctly-rounded sum). I traced the four cases by hand: after ivalue = -ivalue, the two guard bits and the non-negative lower/remaining-chunks (which subtract from the magnitude for negative sums) give: 11 → strictly >½ ulp above m; 00/01 → within ¼ ulp of m; 10 even-m → ≤½ ulp, even wins; 10 odd-m → tie iff lower is empty, then m+1 is even. The (ivalue & 7) == 6 check encodes exactly the last case. This matches the reference xsum.c and the PR's 3.75M-input differential verification against it. The reasoning is sound, but floating-point edge cases in engine builtins are exactly the kind of change a maintainer should sign off on — particularly since this is also an upstream WebKit bug that may want coordinating.
Other factors
The PR is exceptionally well-documented and well-tested: hand-derived test vectors for every rounding outcome (both signs, ties both directions, power-of-two boundaries, subnormal/overflow neighborhoods), exact-equality assertions that would have caught the original bug, coverage of both XsumSmall and XsumLarge via array length and iterable type, and independent verification against exact rational arithmetic. The -0 → -0.0 and EXPECT_DOUBLE_EQ → EXPECT_EQ fixes in the existing test file are correct and necessary collateral improvements. No prior human review comments to address. I'm deferring rather than approving only because the correctness argument, while sound, is non-trivial numerical reasoning rather than a mechanical change.
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
f423272 to
b185619
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The rounding-table rewrite in XsumSmall::compute() checks out against the IEEE 754 round-to-nearest-ties-to-even semantics and the described xsum_small_round() reference, and the test additions are sound. Because this is subtle floating-point rounding logic in a core Math.sumPrecise path (and likely upstreamable to WebKit), a human look would still be worthwhile.
What was reviewed:
- Verified the four-case negative-branch table:
(ivalue & 3) == 3and(ivalue & 7) == 6correctly encode guard bits11and10-with-odd-mantissa; the sign oflower's contribution (subtracting from magnitude) makes the truncate-by-default correct for00/01/10-even. - Spot-checked test expectations (
[-1, -ULP/2],[-(1+ULP), -ULP/2], the-2 + ULP/2power-of-two boundary,-MAX_VALUEoverflow tie) against manual round-to-even arithmetic. - Confirmed the
-0→-0.0andEXPECT_DOUBLE_EQ→EXPECT_EQ+signbitchanges in the C++ test are needed for the test to actually catch 1-ulp errors and zero sign.
Extended reasoning...
Overview
This PR fixes a rounding bug in WTF::Xsum::XsumSmall::compute() (Source/WTF/wtf/PreciseSum.cpp), the final rounding step of the xsum port that backs Math.sumPrecise. The negative-sum branch previously rounded away from zero whenever no bits were set below the mantissa — which is true of every exactly representable negative sum — so Math.sumPrecise([-1]) returned -1.0000000000000002. The fix replaces that with the correct four-case table from the reference xsum.c: round away only for guard bits 11, or for guard bits 10 on an odd mantissa when nothing is set below (the exact-tie case). Two test files are added/tightened: a new JSTests stress test covering both the small and large accumulator paths, and the TestWebKitAPI unit test updated to compare exactly (previously EXPECT_DOUBLE_EQ tolerated 4 ulps and -0 literals were integer zero).
Security risks
None. This is pure numerical computation on already-validated double values with no memory allocation, no external input parsing, and no change to control flow outside the rounding decision. The change narrows the set of cases that take the round-away path — it cannot introduce new overflow or out-of-bounds behavior.
Level of scrutiny
High. This is IEEE 754 rounding logic in a spec-mandated JS builtin (Math.sumPrecise must return the correctly rounded exact sum). The change is small (~15 lines) and the mathematics is verifiable by hand — I walked through each guard-bit case against the fact that post-carry-propagation lower chunks are non-negative and therefore reduce the magnitude of a negative ivalue, which is what makes the negative table asymmetric with the positive one. The (ivalue & 7) == 6 encoding correctly selects bit pattern 110 (guard 10, mantissa LSB 1). The PR description reports bit-exact agreement with Radford Neal's reference xsum.c on 3.75M generated inputs plus rational-arithmetic cross-checks, and the companion Bun PR passed its full CI. Nonetheless, floating-point rounding at ulp boundaries is exactly the kind of code where a second pair of expert eyes is warranted, and this fix is a good candidate for upstreaming to WebKit proper.
Other factors
The test improvements are independently valuable: the old C++ test's -0 literals were integer zero (so +0.0) and EXPECT_DOUBLE_EQ's 4-ulp tolerance would have hidden this class of bug indefinitely. The new stress test exercises both accumulator sizes (via the 1000-element threshold and via generator vs. array) and includes the power-of-two-boundary case where the negative branch takes the e -= 1 re-normalization path. No prior human reviews or outstanding comments on the PR.
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
b185619 to
0609b9b
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The rounding-table fix checks out against the four-case analysis and the reference xsum_small_round(), and the test changes are sound. Because this is subtle IEEE 754 tie-to-even logic in a core WTF numerical routine, a human look would still be worthwhile.
Checked the negative-branch case table: (ivalue & 3) == 3 and (ivalue & 7) == 6 with the lower scan cover exactly the round-away cases; 00/01/10-even correctly fall through to truncate.
Checked the test harness change: -0 → -0.0 fixes real integer-literal bugs, and EXPECT_EQ + signbit is the right strictness for a correctly-rounded result.
Checked the stress test's padded/generator paths exercise XsumLarge and XsumSmall as intended (1000-element threshold in MathObject.cpp, generator path always uses small).
Extended reasoning...
Overview
This PR fixes a rounding bug in XsumSmall::compute() (Source/WTF/wtf/PreciseSum.cpp), the final rounding step of the xsum port used by Math.sumPrecise. The negative-sum branch previously rounded away from zero whenever no bits were set below the two guard bits — which is true of every exactly representable negative sum — so Math.sumPrecise([-1]) returned -1.0000000000000002. The fix restructures the branch to match the reference xsum_small_round() table: round away only for guard bits 11, or for guard bits 10 on an odd mantissa when nothing is set below (an exact tie). It also adds 30 C++ test cases with exact (bit-identical, sign-of-zero-aware) comparison, and a new JS stress test covering both accumulator paths.
Security risks
None. This is pure numerical logic on already-validated double values with no allocation, indexing, or external input handling changes. The chunk-scan loop is bounded by j > 0 exactly as before.
Level of scrutiny
Medium-high. The functional change is only ~10 lines, but it sits in IEEE 754 round-to-nearest-ties-to-even logic where off-by-one-bit errors are easy and hard to spot. I walked through all eight (ivalue & 7) cases against the comment's derivation (lower bits subtract from the magnitude for negative sums, so guard bits 10 are at most a half-ulp, a tie only when lower and all lower chunks are zero) and they match. The PR author reports bit-for-bit agreement with Radford Neal's xsum.c on 3.75M generated inputs, which is strong evidence. Still, a maintainer familiar with this port should confirm the change matches the upstream reference — this is the kind of code where an independent human check is cheap insurance.
Other factors
The test harness changes are independently valuable: the old -0 literals were integer zero (so +0.0), and EXPECT_DOUBLE_EQ's 4-ulp tolerance could not have caught this class of 1-ulp bug. The new stress test is well-constructed (uses Object.is, pads past the 1000-element XsumLarge threshold with cancelling ±2^500 pairs, and exercises the generator path which always uses XsumSmall). The verifier ruled out a concern about testLoopCount — this test is a correctness table, not a JIT-tiering test, so a loop count is not needed. No prior reviewer comments are outstanding.
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
0609b9b to
a16c864
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The negative-branch rounding table now matches the four cases in xsum's xsum_small_round(), and I traced each one against the guard-bit / lower-bit semantics. Because this is subtle IEEE-754 rounding logic in a spec-mandated builtin — and the same bug exists in upstream WebKit, so upstreaming strategy is worth a human decision — a human look would still be worthwhile.
What was reviewed:
- Verified
(ivalue & 7) == 6correctly encodes "extra bits10, mantissa LSB odd", and that the other three cases (11,00/01,10-even) fall through as intended. - Checked the power-of-two-boundary case (extra bits
00, lower > 0) still rounds correctly without entering theivalue *= 2adjustment. - Confirmed the test tightening (
EXPECT_DOUBLE_EQ→EXPECT_EQ+ signbit, and-0→-0.0) is itself correct and would have caught the original bug.
Extended reasoning...
Overview
This PR fixes a rounding bug in WTF::Xsum::XsumSmall::compute() (Source/WTF/wtf/PreciseSum.cpp), the final rounding step behind Math.sumPrecise. The old negative branch checked (ivalue & 3) == 3 and then, independently, rounded away from zero whenever no lower bit was set — so every exactly representable negative sum (all discarded bits zero) was bumped one ulp further from zero (Math.sumPrecise([-1]) → -1.0000000000000002, [-MAX_VALUE] → -Infinity). The fix replaces this with the four-case table from the reference xsum_small_round(): round away only for extra bits 11, or for extra bits 10 on an odd mantissa with no lower bits set (an exact tie to the even neighbour above). The other two files add regression coverage: 30 new C++ test rows with exact (EXPECT_EQ + signbit) comparison, and a 238-assertion JS stress test exercising both the small and large accumulator paths.
I independently walked the case analysis. After negation, ivalue holds the magnitude with two guard bits; lower and the chunks below j are non-negative and subtract from that magnitude, contributing strictly less than one guard-bit position (¼ ulp). So: 11 → magnitude in (m+½, m+¾] ulp → round up; 10 → (m+¼, m+½], a tie only when lower=0, and ties-to-even means round up only if m is odd; 01/00 → within ¼ ulp of m → keep m. The bitmask (ivalue & 7) == 6 is exactly "guard bits 10 and mantissa LSB 1". This matches the diff. I also checked the edge where extra bits are 00 with m a power of two and lower > 0: the true magnitude is 2^k − ε with ε < ¼·ulp(2^k) = ½·ulp(2^(k−1)), so 2^k is still nearest and "keep m" is right without needing the earlier ivalue *= 2 renormalisation.
Security risks
None. Pure numeric computation on already-validated double inputs; no allocation, no untrusted lengths, no pointer arithmetic changed. The only behavioural change is which of two adjacent doubles is returned.
Level of scrutiny
Moderate-to-high. The code change is ~15 lines and mechanically small, but IEEE-754 round-to-nearest-ties-to-even at bit level is exactly the kind of logic where a one-token error is invisible without differential testing. The PR author did that testing (3.75M inputs bit-exact against Neal's reference xsum.c, plus a rational-arithmetic spot check), and the companion Bun PR passed its full CI, which substantially de-risks it. What keeps me from auto-approving is (a) this is spec-mandated correctness for a TC39 proposal, so a human should sign off on the reasoning rather than trust an automated pass, and (b) the PR notes the same bug is in upstream WebKit main — someone should decide whether/how to upstream, which is a maintainer call.
Other factors
The test-file changes are independently valuable: the old suite used EXPECT_DOUBLE_EQ (4-ulp tolerance) and integer -0 literals (which are +0.0), so it could not have caught either this bug or a wrong sign of zero. Both are now fixed. The new JS stress test pads each case past the 1000-element threshold to cover XsumLarge, and re-runs via a generator to cover the XsumSmall iterable path — good coverage of both accumulator strategies through the actual Math.sumPrecise entry point. No outstanding reviewer comments; only bot activity in the timeline.
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
a16c864 to
9767dac
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The rounding-table rewrite in the negative branch of XsumSmall::compute() matches the described xsum_small_round() cases and the test coverage is thorough, but since this is subtle IEEE-754 rounding logic backing a JS built-in, a human look would still be worthwhile.
What was reviewed:
- The four-case negative rounding table (
(ivalue & 3) == 3/(ivalue & 7) == 6with lower-bit scan) against the semantics in the added comment — each case checks out. - Confirmed the positive branch and the post-rounding overflow/exponent-adjust path are untouched.
- The
-0→-0.0andEXPECT_DOUBLE_EQ→EXPECT_EQtest tightening is correct (integer-0was+0.0; the old macro tolerated 4 ulps). - The new JSTests stress file's expected values are consistent with round-to-nearest-ties-to-even for the listed inputs.
Extended reasoning...
Overview
This PR fixes a rounding bug in WTF::Xsum::XsumSmall::compute() (Source/WTF/wtf/PreciseSum.cpp), the final rounding step of the xsum port that backs Math.sumPrecise. The old negative branch rounded away from zero whenever no bits were set below the mantissa, so every exactly representable negative sum came out one ulp too large in magnitude. The fix replaces that with the four-case table from the reference xsum_small_round(): round away for extra bits 11, or for extra bits 10 on an odd mantissa when nothing is set below; otherwise truncate. The other two files add regression coverage — 30 new C++ table entries with exact (not 4-ulp-tolerant) comparison including sign of zero, and a new JSTests stress file exercising both accumulator paths.
Security risks
None. This is pure numerical logic on local integer state; no allocation, no external input handling beyond doubles already validated by the caller, no change to control-flow bounds (the while (j > 0) scan is identical to before, just gated more narrowly).
Level of scrutiny
Moderate-to-high. The functional diff is ~15 lines and localized to one branch of one function, and the PR description documents differential verification against the reference C implementation over 3.75M inputs plus exact rational cross-checks. I traced each of the four cases in the new table against the stated invariant (post-carry-propagation lower chunks are non-negative, so for a negative sum they reduce the magnitude held in ivalue) and they hold. That said, IEEE-754 tie-to-even reasoning around guard bits is exactly the kind of code where an off-by-one in a bitmask is easy to miss and hard to spot without either running the tests or knowing the reference — a human familiar with the xsum port (or willing to diff against xsum.c) should confirm before merge.
Other factors
- No prior human or bot review comments to address; only preview-build and a rate-limited coderabbit notice on the timeline.
- Test changes are strictly additive/tightening; the
-0→-0.0literal fix andEXPECT_EQswap mean the existing zero-sign cases now actually assert what they appeared to. - Companion Bun PR (oven-sh/bun#39455) is reported green on three preview builds of this exact commit, which is good end-to-end signal but not something I can independently verify here.
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
9767dac to
b7be893
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The rounding-table rewrite in XsumSmall::compute() checks out against the case analysis in the new comment, and the added tests cover each branch. Because this is subtle IEEE 754 rounding logic in core WTF code (and an upstream WebKit bug worth upstreaming), a human sign-off would still be worthwhile.
What was reviewed:
- Traced the four-case negative-branch table (
00/01keep,11round away,10even keep,10odd tie-only round away) against the guard-bit/lower-bit semantics — matches round-to-nearest-ties-to-even given that lower chunks subtract from magnitude. - Confirmed
(ivalue & 7) == 6correctly selects "guard bits10on odd mantissa" and is mutually exclusive with the== 3arm. - Checked the test additions: the
-0→-0.0andEXPECT_DOUBLE_EQ→ exact-compare fixes are correct; the JS stress test's XsumLarge padding and generator paths exercise both accumulators.
Extended reasoning...
Overview
This PR fixes a rounding bug in WTF::Xsum::XsumSmall::compute() (Source/WTF/wtf/PreciseSum.cpp) that caused Math.sumPrecise to return every exactly-representable negative sum one ulp too large in magnitude. The change rewrites the negative-sum rounding branch (~15 lines) to match the reference xsum_small_round() in Radford Neal's xsum.c: round away from zero only for guard bits 11, or for guard bits 10 on an odd mantissa when no lower bit is set. It also adds a JS stress test (58 base cases × array/generator × small/large accumulator) and expands the TestWebKitAPI C++ test from 38 to 68 cases with exact bit-level comparison.
Security risks
None. This is pure numeric computation on doubles with no external input handling, allocation, or control-flow implications beyond the rounding decision.
Level of scrutiny
High. This is correctness-critical floating-point rounding logic in a shared WTF library that backs a spec-mandated JS builtin (Math.sumPrecise, TC39 proposal). The logic is subtle: it depends on the invariant that post-carry-propagation lower chunks are non-negative and therefore subtract from a negative sum's magnitude, which inverts the rounding table relative to the positive branch. I walked through all four guard-bit cases and confirmed the new code implements round-to-nearest-ties-to-even correctly under that invariant, and that (ivalue & 7) == 6 is the right predicate for "guard bits 10, mantissa LSB 1". The PR author also reports bit-for-bit agreement with the reference C implementation on 3.75M fuzzed inputs and rational-arithmetic spot checks, which is strong evidence.
Other factors
- The bug also exists in upstream WebKit; a maintainer may want to coordinate upstreaming.
- The test-file fixes (
-0integer literal →-0.0,EXPECT_DOUBLE_EQ4-ulp tolerance →EXPECT_EQ+ signbit) are independently correct and would have masked this bug before. - No prior human reviews or outstanding comments on the PR.
- Preview builds have passed the companion Bun PR's full CI four times per the description.
Given the subtlety of the domain and that this touches upstream-shared WTF code rather than a Bun-specific addition, I'm deferring rather than auto-approving, even though I found the change correct.
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
b7be893 to
5dba048
Compare
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
5dba048 to
754b429
Compare
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
754b429 to
6ab847b
Compare
6ab847b to
5878425
Compare
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
5878425 to
c7b3aab
Compare
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
c7b3aab to
98b0c6c
Compare
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
98b0c6c to
31d19b1
Compare
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
31d19b1 to
443e5b3
Compare
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
XsumSmall::compute() decides how to round the sum from the two bits below the mantissa plus whether any lower bit is set. In the negative branch the port of xsum's xsum_small_round() lost two of the four outcomes: it rounded away from zero whenever no lower bit was set, regardless of the two extra bits, where the original only does so for a tie on an odd mantissa (or when the extra bits are 11). Since an exactly representable sum has no bits set below the mantissa, every exactly representable negative sum came out one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002, Math.sumPrecise([-Number.MAX_VALUE]) was -Infinity), and a negative sum just inside a power of two was rounded onto it. Positive sums and inexact negative sums were unaffected; XsumLarge is rounded through the same code. Restore the original's decision table. The API test now compares results exactly (EXPECT_DOUBLE_EQ tolerates 4 ulps, and its -0 literals were +0) and covers exact negative sums and each rounding case on both sides of zero; the stress test does the same through Math.sumPrecise, for arrays above and below the XsumLarge threshold and for a non-array iterable.
443e5b3 to
668c1f1
Compare
…tive sums to nearest Every exactly representable negative sum came back one ulp too large in magnitude (Math.sumPrecise([-1]) was -1.0000000000000002 and [-Number.MAX_VALUE] was -Infinity): the rounding step of WTF's xsum port rounded a negative sum away from zero whenever no bit below the guard bits was set. oven-sh/WebKit#458 restores xsum's decision table; this pins its preview build and runs the stress test from that PR as a jsc-stress fixture.
Problem
Math.sumPrecisereturns every exactly representable negative sum one ulp too large in magnitude:Math.sumPrecise([-1])is-1.0000000000000002,[-0.5]is-0.5000000000000001,[1, -2]is-1.0000000000000002,[-Number.MAX_VALUE]is-Infinity. A negative sum just inside a power of two is rounded onto it as well ([-2, 2 ** -52]gives-2instead of-1.9999999999999998). Positive sums and inexact negative sums are right. Found by a differential fuzz run against an exact reference (468 of 10,000 random arrays, all in this bucket); the same results come out of upstream WebKit main and of every Bun release withMath.sumPrecise.XsumSmall::compute()inSource/WTF/wtf/PreciseSum.cpp(the port of xsum'sxsum_small_round(), also used byXsumLarge::compute()) decides the rounding from the two bits below the mantissa plus whether any lower bit is set. The original's negative branch has four outcomes: extra bits11round away from zero,00/01truncate,10on an even mantissa truncates,10on an odd mantissa rounds away from zero only if no lower bit is set. The port kept the first and then unconditionally rounded away from zero whenever no lower bit was set. An exactly representable sum has no bits set below the mantissa, so it always took that path.Fix
The negative branch now implements the original's table: round away from zero for extra bits
11, or for extra bits10on an odd mantissa ((ivalue & 7) == 6) when the scan oflowerand the chunks below it finds nothing set; everything else keeps the truncated mantissa. The positive branch is untouched.Correct because, after carry propagation, the chunks below the top one are non-negative, so for a negative sum the discarded bits reduce the magnitude held in
ivalue: extra bits10are a tie only when nothing is set below them, and00/01are within a quarter ulp of the truncated value. This is the reasoning inxsum.c'sxsum_small_round(), which the comment in the diff restates.Tools/TestWebKitAPI/Tests/WTF/PreciseSum.cpp: compares results exactly, sign of zero included (EXPECT_DOUBLE_EQtolerates 4 ulps, so the existing table could not have caught a 1 ulp error, and its-0literals are the integer 0, i.e.+0.0), and adds exact negative sums plus each rounding case on both sides of zero. 68 cases, run throughXsumSmall/XsumLargexadd/addListas before. With the fix all pass; without it 16 expectations per variant fail.JSTests/stress/math-sum-precise-negative-rounding.js: the same throughMath.sumPrecise, as arrays (58 cases, each also padded past the 1000 elementXsumLargethreshold with cancelling pairs, plus three 1001 element arrays) and as generators (alwaysXsumSmall). The 119 expected values were checked against exact rational arithmetic independently of any engine. On the unfixedautobuild-c6cfe90cjscit fails at its first case with the message above (92 of its 238 assertions fail); it runs in about 30 ms there. The existingmath-sum-precise.jsstill passes; its negative cases are all inexact sums, which is why it did not catch this.Verification of the fix itself:
PreciseSum.cppwith this change, compiled against thec6cfe90cprebuilt WTF and linked in place of itsPreciseSum.o, agrees bit for bit with Radford Neal'sxsum.con 3.75 million generated inputs (random bit patterns, small integers at a shared scale, ties and near ties around a random anchor, magnitudes just inside powers of two, the subnormal and overflow neighbourhoods, 1000 to 2500 element arrays, and the negation of every input), 805,856 of which have a negative exactly representable sum, for all fourXsumSmall/XsumLargexadd/addListvariants; a 30,000 input sample of those was also checked against exact rational arithmetic. The unmodified file mismatches on 20% of the same inputs. The TestWebKitAPI file was compiled and run the same way (the fork's CI does not build it).Branch: the single fix commit, re-applied on the fork commit oven-sh/bun pins (
ceb9f90fat the moment) each time that pin moved, so that the preview build stays exactly "Bun's current engine plus this fix"; none of the three files touched here has changed on main or upstream in the meantime. Companion Bun PR, pinning this PR's preview build: Bump WebKit (oven-sh/WebKit#458 preview): Math.sumPrecise rounds negative sums to nearest bun#39455 (earlier previews of this same commit passed every one of its CI jobs six times).Background
Math.sumPrecise(MathObject.cpp) adds the values into aPreciseSum, which is WTF's port of xsum: an exact accumulator made of 67 64-bit chunks, each holding a window of the sum's binary expansion, so the sum is exact and all rounding happens once, incompute().XsumLarge(used for arrays longer than 1000 elements) is a different accumulation strategy that is folded into anXsumSmalland rounded by the same function.compute()first propagates carries, after which the top non-zero chunk carries the sign and every chunk below it is non-negative, then assembles the top 53 bits of the magnitude plus two guard bits intoivalue, keeping the leftover bits of the chunk it stopped in aslowerand the index of that chunk asj. For a positive sum the leftover bits add toivalue, for a negative sum they subtract from it, which is why the two branches have different tables.shouldRoundAwayFromZeroadds one ulp to the magnitude; it is also what turns-MAX_VALUEinto-Infinitywhen applied wrongly.PreciseSum.cppnames the two sources of the port; the original C isxsum_small_round()inxsum.c(gitlab.com/radfordneal/xsum), whose negative branch this change matches case for case.