BigInt-to-Number conversion is rather borken
Categories
(Core :: JavaScript Engine, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox67 | --- | disabled |
firefox68 | --- | fixed |
firefox69 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(2 files)
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
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.
Assignee | ||
Comment 2•3 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/autoland/rev/45210f041ea7 BigInt-to-Number conversion is rather borken. r=wingo
Comment 4•3 years ago
|
||
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
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/autoland/rev/382f21591267 BigInt-to-Number conversion is rather borken. r=wingo
Assignee | ||
Comment 6•3 years ago
|
||
Whoops, thinko with the original push. La la la...
Comment 7•3 years ago
|
||
Backed out changeset 382f21591267 (bug 1558538) for Build bustage. CLOSED TREE
Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=252469595&repo=autoland&lineNumber=5078
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=382f21591267f9563a043c24db8a7e9936ea0bb0
Backout:
https://hg.mozilla.org/integration/autoland/rev/4948b0bacf30949db7175783bc9f84151a6d64b0
Assignee | ||
Comment 8•3 years ago
|
||
[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.
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/autoland/rev/5b9a3de04646 BigInt-to-Number conversion is rather borken. r=wingo
Assignee | ||
Comment 10•3 years ago
|
||
Assignee | ||
Comment 11•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Comment 12•3 years ago
|
||
bugherder |
Comment 13•3 years ago
|
||
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
Assignee | ||
Comment 14•3 years ago
|
||
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:
Assignee | ||
Comment 15•3 years ago
|
||
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:
Comment 16•3 years ago
|
||
bugherder |
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 17•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 18•3 years ago
|
||
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?
Comment 19•3 years ago
|
||
bugherderuplift |
Assignee | ||
Comment 20•3 years ago
|
||
(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.
Description
•