Skip to content

fix(dom): mark mantissa truncated on the slow path when an exponent follows - #154

Open
linhongyu510 wants to merge 1 commit into
bytedance:masterfrom
linhongyu510:fix/trunc-long-mantissa-exponent
Open

linhongyu510 wants to merge 1 commit into
bytedance:masterfrom
linhongyu510:fix/trunc-long-mantissa-exponent

Conversation

@linhongyu510

Copy link
Copy Markdown

Summary

parseNumber has a slow path for integers longer than 19 digits: the first 19 digits are kept in man, and every remaining digit increments exp10. The dropped digits were never reflected in trunc.

  • Without an explicit exponent, control reaches the Integer section, which sets trunc = 1 before the fast path — so results are correct.
  • With an explicit exponent (...e0, ...E+7, …), control jumps to double_exp and skips that assignment. trunc stays 0, so ParseFloatingNormalFast receives a truncated significand but treats it as exact, producing a value 1 ULP below the correctly rounded result.

Setting trunc = 1 on the slow path makes the Eisel–Lemire consistency check trigger the fallback to the native parser whenever the truncated mantissa cannot be rounded exactly.

Evidence

Differential audit vs correctly-rounded strtod (Python float() is bit-identical):

  • 3,000,000 legal JSON numbers: 170 mismatches, all exactly 1 ULP low, zero false rejections.
  • After the fix: 0 mismatches, overflow behavior unchanged (539,902 design-consistent kParseErrorInfinity).
  • Direct counter-proof: -32707769411490445312 (no exponent) → correct; -32707769411490445312e0 → 1 ULP low.

Test

Added ParserTest.LongMantissaWithExponent with 7 inputs that reproduced the bug, asserted bit-exact (via EXPECT_EQ on the raw uint64 bits). EXPECT_DOUBLE_EQ would not catch this — it tolerates a 4-ULP difference.

  • Without the fix: all 7 cases fail, each exactly 1 ULP off.
  • With the fix: 300 tests in the suite pass.

Checklist

  • Local build: cmake --build (AppleClang 21, -O3 -DNDEBUG -std=c++17 -arch arm64 -mcpu=apple-m1)
  • Full test suite: 300/300 pass
  • Regression test fails without the fix, passes with it
  • clang-format clean

When parsing a number whose integer part is longer than 19 digits on the
slow path, digits beyond the first 19 are dropped from 'man' and counted
into 'exp10', but 'trunc' was never set. Without an explicit exponent the
code falls through the Integer section which sets 'trunc' before the fast
path, but with an explicit exponent it jumps straight to 'double_exp' and
then to ParseFloatingNormalFast, which requires an exact significand. The
truncated 19-digit mantissa is then treated as exact, producing a result
that is 1 ULP below the correctly rounded value.

Setting 'trunc' on the slow path makes the Eisel-Lemire consistency check
kick in and fall back to the native parser when the truncated mantissa is
not exactly representable, matching correctly-rounded strtod output.

Adds a regression test with bit-exact expectations (EXPECT_DOUBLE_EQ
tolerates a 4-ULP difference and would not catch this).
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.67%. Comparing base (ee76cf2) to head (a16a587).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #154   +/-   ##
=======================================
  Coverage   69.67%   69.67%           
=======================================
  Files          27       27           
  Lines        3706     3707    +1     
  Branches     1189     1189           
=======================================
+ Hits         2582     2583    +1     
  Misses        211      211           
  Partials      913      913           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants