Bug 186022

Summary: [JSC] JSBigInt::digitDiv has undefined behavior which causes test failures
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, ggaren, keith_miller, mark.lam, msaboff, saam, sam, ticaiolima, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=186006
Bug Depends on: 183996    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Yusuke Suzuki
Reported 2018-05-27 10:59:30 PDT
[JSC] JSBigInt::digitDiv has undefined behavior which causes test failures
Attachments
Patch (5.76 KB, patch)
2018-05-27 11:03 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (5.84 KB, patch)
2018-05-27 11:24 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (5.84 KB, patch)
2018-05-27 11:36 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (5.83 KB, patch)
2018-05-27 11:36 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (8.38 KB, patch)
2018-05-27 21:43 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (6.48 KB, patch)
2018-05-27 21:45 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (6.50 KB, patch)
2018-05-27 21:52 PDT, Yusuke Suzuki 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2018-05-27 11:03:57 PDT
Created attachment 341424 [details] Patch
Yusuke Suzuki
Comment 2 2018-05-27 11:05:51 PDT
I don't think we should have perf test right now. We should do that once features are implemented. (we do not have enough DFG and FTL coverage for JSBigInt. in this situation, numbers are not so meaningful.)
Yusuke Suzuki
Comment 3 2018-05-27 11:24:27 PDT
Created attachment 341425 [details] Patch
Yusuke Suzuki
Comment 4 2018-05-27 11:36:14 PDT
Created attachment 341426 [details] Patch
Yusuke Suzuki
Comment 5 2018-05-27 11:36:55 PDT
Created attachment 341427 [details] Patch
Caio Lima
Comment 6 2018-05-27 18:15:51 PDT
Comment on attachment 341427 [details] Patch Nice work! Thank you for this Patch. I think it is ready to go.
Yusuke Suzuki
Comment 7 2018-05-27 21:22:00 PDT
Comment on attachment 341427 [details] Patch I've come up with more efficient and clean code. Updating.
Yusuke Suzuki
Comment 8 2018-05-27 21:43:50 PDT
Created attachment 341443 [details] Patch
Yusuke Suzuki
Comment 9 2018-05-27 21:45:04 PDT
Created attachment 341444 [details] Patch
Yusuke Suzuki
Comment 10 2018-05-27 21:52:50 PDT
Created attachment 341445 [details] Patch
Darin Adler
Comment 11 2018-05-28 17:29:06 PDT
Comment on attachment 341445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341445&action=review > Source/JavaScriptCore/runtime/JSBigInt.cpp:460 > + static constexpr const unsigned shiftMask = digitBits - 1; I don’t think we ever need const in an expression if we already have constexpr. > Source/JavaScriptCore/runtime/JSBigInt.h:123 > + static constexpr const unsigned maxLength = 1024 * 1024 / (sizeof(void*) * bitsPerByte); Ditto.
Yusuke Suzuki
Comment 12 2018-05-28 23:41:45 PDT
Comment on attachment 341445 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341445&action=review Thanks! >> Source/JavaScriptCore/runtime/JSBigInt.cpp:460 >> + static constexpr const unsigned shiftMask = digitBits - 1; > > I don’t think we ever need const in an expression if we already have constexpr. Fixed. >> Source/JavaScriptCore/runtime/JSBigInt.h:123 >> + static constexpr const unsigned maxLength = 1024 * 1024 / (sizeof(void*) * bitsPerByte); > > Ditto. Fixed.
Yusuke Suzuki
Comment 13 2018-05-28 23:43:43 PDT
Committed r232253: <https://trac.webkit.org/changeset/232253>
Radar WebKit Bug Importer
Comment 14 2018-05-28 23:44:17 PDT
<rdar://problem/40607058>
Note You need to log in before you can comment on or make changes to this bug.