Bug 186022 - [JSC] JSBigInt::digitDiv has undefined behavior which causes test failures
Summary: [JSC] JSBigInt::digitDiv has undefined behavior which causes test failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 183996
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-27 10:59 PDT by Yusuke Suzuki
Modified: 2018-05-28 23:44 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.76 KB, patch)
2018-05-27 11:03 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2018-05-27 11:24 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2018-05-27 11:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2018-05-27 11:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (8.38 KB, patch)
2018-05-27 21:43 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2018-05-27 21:45 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2018-05-27 21:52 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-05-27 10:59:30 PDT
[JSC] JSBigInt::digitDiv has undefined behavior which causes test failures
Comment 1 Yusuke Suzuki 2018-05-27 11:03:57 PDT
Created attachment 341424 [details]
Patch
Comment 2 Yusuke Suzuki 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.)
Comment 3 Yusuke Suzuki 2018-05-27 11:24:27 PDT
Created attachment 341425 [details]
Patch
Comment 4 Yusuke Suzuki 2018-05-27 11:36:14 PDT
Created attachment 341426 [details]
Patch
Comment 5 Yusuke Suzuki 2018-05-27 11:36:55 PDT
Created attachment 341427 [details]
Patch
Comment 6 Caio Lima 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.
Comment 7 Yusuke Suzuki 2018-05-27 21:22:00 PDT
Comment on attachment 341427 [details]
Patch

I've come up with more efficient and clean code. Updating.
Comment 8 Yusuke Suzuki 2018-05-27 21:43:50 PDT
Created attachment 341443 [details]
Patch
Comment 9 Yusuke Suzuki 2018-05-27 21:45:04 PDT
Created attachment 341444 [details]
Patch
Comment 10 Yusuke Suzuki 2018-05-27 21:52:50 PDT
Created attachment 341445 [details]
Patch
Comment 11 Darin Adler 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.
Comment 12 Yusuke Suzuki 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.
Comment 13 Yusuke Suzuki 2018-05-28 23:43:43 PDT
Committed r232253: <https://trac.webkit.org/changeset/232253>
Comment 14 Radar WebKit Bug Importer 2018-05-28 23:44:17 PDT
<rdar://problem/40607058>