Closed Bug 1558538 Opened 1 year ago Closed 1 year ago

BigInt-to-Number conversion is rather borken

Categories

(Core :: JavaScript Engine, task, P1)

Product:
Core
Component:
JavaScript Engine
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- disabled
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(2 files)

Bug 1558538 - BigInt-to-Number conversion is rather borken. r=wingo
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review
Bug 1558538 - Add tests of converting BigInt around and exceeding Number.MAX_VALUE to Number. r=wingo
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review
No description provided.
Priority: -- → P1
Summary: BigInt-to-Number conversion is rather borken → BigInt-to-Number conversion is rather broken

I meant what I typed.

Wow is the current algorithm incredibroken. I think I have a patch for it locally. I also am in the middle of writing a heckton of tests for its relatively considerable wrinkles.

Summary: BigInt-to-Number conversion is rather broken → BigInt-to-Number conversion is rather borken
Blocks: js-bigint
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/autoland/rev/45210f041ea7
BigInt-to-Number conversion is rather borken.  r=wingo

Backed out changeset 45210f041ea7 for causing bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/265a8451194c0a558fde24db7bb44982ad4a8c8e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=45210f041ea71be2f52d7e7c66cf9cb1fb8659c7

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=252460834&repo=autoland&lineNumber=5747

[task 2019-06-19T15:37:25.978Z] 15:37:25 ERROR - /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2803:29: error: use of undeclared identifier 'CountOfBitsInDigitBelowExtraBit'; did you mean 'countOfBitsInDigitBelowExtraBit'?

Failures seem to be caused by BigIntType.cpp

Flags: needinfo?(jwalden)
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/autoland/rev/382f21591267
BigInt-to-Number conversion is rather borken.  r=wingo

Whoops, thinko with the original push. La la la...

Flags: needinfo?(jwalden)
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/js/src/Unified_cpp_js_src11.cpp:47:0:
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2789:5: error: multi-line comment [-Werror=comment]
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -       //  /                    \
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -       ^
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2792:5: error: multi-line comment [-Werror=comment]
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -       //     /                 \
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -       ^
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2817:7: error: multi-line comment [-Werror=comment]
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -         //     /                  \
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -         ^
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2821:7: error: multi-line comment [-Werror=comment]
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -         //     /      \ /         \
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -         ^
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2841:9: error: multi-line comment [-Werror=comment]
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -           //     /               \
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -           ^
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2845:9: error: multi-line comment [-Werror=comment]
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -           //     /   \ /         \
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -           ^
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2871:9: error: multi-line comment [-Werror=comment]
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -           //     /                           \
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -           ^
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  /builds/worker/workspace/build/src/js/src/vm/BigIntType.cpp:2875:9: error: multi-line comment [-Werror=comment]
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -           //     /   \ /         \ /         \
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -           ^
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -  cc1plus: all warnings being treated as errors
[task 2019-06-19T17:19:53.282Z] 17:19:53     INFO -  /builds/worker/workspace/build/src/config/rules.mk:810: recipe for target 'Unified_cpp_js_src11.o' failed
[task 2019-06-19T17:19:53.282Z] 17:19:53    ERROR -  make[4]: *** [Unified_cpp_js_src11.o] Error 1

C++ is trash.

Flags: needinfo?(jwalden)
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/autoland/rev/5b9a3de04646
BigInt-to-Number conversion is rather borken.  r=wingo

In the process of writing up a backport approval request, I got paranoid about values just slightly bigger than Number.MAX_VALUE, so I wrote a bunch of tests for it. The expected behavior comports entirely with the patch as written and pushed here -- it pretty much is just a natural extension of the way you would want to implement rounding for smaller values -- so the added tests require no algorithm changes. But I think I -- we! -- probably will feel more comfortable about this change, with these additional tests landed (and backported) as well.

Keywords: leave-open
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/autoland/rev/30bd16cacdb6
Add tests of converting BigInt around and exceeding Number.MAX_VALUE to Number.  r=wingo

Comment on attachment 9071538 [details]
Bug 1558538 - BigInt-to-Number conversion is rather borken. r=wingo

Beta/Release Uplift Approval Request

  • User impact if declined: BigInt-to-Number conversion, as performed by e.g. var bi = /* this is an arbitrary number, not one demonstrating this bug */ 38139734862323n; var asNumber = Number(bi);, will produce wrongly-rounded values if the big integer is big enough and is situated between representable IEEE-754 double values in the wrong way -- about half the time, in fact.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Big integer code involves a lot of manual bit-shifting and can be tricky to get right. The old algorithm was simple but laughably, unbelievably wrong. The new algorithm is fairly simple to explain, but actually coding it up is a little bit perilous. The complexity is irreducible.

I've written a bunch of tests (in both revs here) that try to carefully exercise all this code to verify its correctness. The tests should exercise all the various corner cases of this for 32-bit and 64-bit (because the algorithms are somewhat different in the two cases). They're as much as I can do to verify this.

I don't think BigInt is shippable in 68 with the current very-broken rounding behavior. It's really wrong, it's pretty visibly wrong if you do any conversions of numbers outside the range of integral precision in IEEE-754 double precision, and getting basic math operations wrong is really not an option for this. It's a Pentium F00F bug sort of thing. So if we choose not to take this patch, IMO BigInt must slip from 68 to 69 and we'll have to flip the BigInt pref back for 68.

  • String changes made/needed:
Attachment #9071538 - Flags: approval-mozilla-beta?

Comment on attachment 9072899 [details]
Bug 1558538 - Add tests of converting BigInt around and exceeding Number.MAX_VALUE to Number. r=wingo

Beta/Release Uplift Approval Request

  • User impact if declined: This is purely test additions, zero risk to this. If we're taking a patch to code, we should take this second patch too to have all the relevant tests performed.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just tests in this patch.
  • String changes made/needed:
Attachment #9072899 - Flags: approval-mozilla-beta?
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
status-firefox69: affected → fixed
Target Milestone: --- → mozilla69

Comment on attachment 9071538 [details]
Bug 1558538 - BigInt-to-Number conversion is rather borken. r=wingo

JS bigint fix for 68.0b13. This clearly only affects bigint so given comment 14 we might as well take this in any case.

Attachment #9071538 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9072899 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I guess the other question is, given a bug like this slipped through until now, might there be others that mean we'd better disable it rather than ship to the next ESR branch?

Flags: needinfo?(jwalden)

(In reply to Julien Cristau [:jcristau] from comment #18)

I guess the other question is, given a bug like this slipped through until now, might there be others that mean we'd better disable it rather than ship to the next ESR branch?

The issue here was in a single function, pretty clearly. It isn't indicative of any sort of systematic issue in the overall idea -- just one portion of it. I believe we've thought through this algorithm pretty carefully (now at multiple points in time), we have tests for the particular rounding quirks of direct concern here, and when rounding isn't needed the algorithm ends up being simpler (and a bug in the simpler part would be visible in many more places, whereas rounding problems only appear if you use really-big numbers like the fresh testing does).

I also think it would probably be unwise to provide big integer support in normal releases for so much additional time before they appear in an ESR. We're likely to strand ESR even harder than normal if we do this, I think -- and anyone who uses big integers is not going to give any care to ESR for sure.

Flags: needinfo?(jwalden)
You need to log in before you can comment on or make changes to this bug.