Bug 210869

Summary: BigInt32 parsing should be precise
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
rmorisset: review+
patch for landing
none
patch for landing
none
patch for landing
none
patch for landing none

Description Saam Barati 2020-04-22 12:09:04 PDT
...
Comment 1 Saam Barati 2020-04-22 12:36:10 PDT
Created attachment 397236 [details]
patch
Comment 2 Mark Lam 2020-04-22 12:46:00 PDT
Comment on attachment 397236 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397236&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        if the value could be an int32. This patch makes the algorithm precise on

I think predictable is a better description here than precise.
Comment 3 Robin Morisset 2020-04-22 12:51:58 PDT
Comment on attachment 397236 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397236&action=review

Thanks for improving this. I have a couple nits and questions below:

> Source/JavaScriptCore/jsc.cpp:2331
> +    return JSValue::encode(jsBoolean(!!jsDynamicCast<JSBigInt*>(globalObject->vm(), callFrame->argument(0))));

Is there a reason for using this weird !!jsDynamicCast<JSBigInt*> instead of JSValue::isHeapBigInt ?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1876
> +    auto limitWorks = [&] (double radix, double lengthLimit) {

I don't think you need to pass lengthLimit as an argument, it could just be lengthLimitForBigInt32 which is captured.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1906
> +    // radix ** lengthLimitForBigInt32 <= INT32_MAX

Why is this the opposite from the BigInt32 case?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1908
> +    auto limitWorks = [&] (double radix, double lengthLimit) {

same comment as before: no need to pass lengthLimit explicitly.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1967
> +                int64_t maybeResult = digit.unsafeGet();

maybe an assert here that going from uint64_t to int64_t does not overflow? I see no way that it could, but an assert would be reassuring if we ever modify any of that code again.
Comment 4 Saam Barati 2020-04-22 12:55:10 PDT
Comment on attachment 397236 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397236&action=review

>> Source/JavaScriptCore/ChangeLog:9
>> +        if the value could be an int32. This patch makes the algorithm precise on
> 
> I think predictable is a better description here than precise.

The old algorithm was also predictable, it's bounds were just worse.

>> Source/JavaScriptCore/jsc.cpp:2331
>> +    return JSValue::encode(jsBoolean(!!jsDynamicCast<JSBigInt*>(globalObject->vm(), callFrame->argument(0))));
> 
> Is there a reason for using this weird !!jsDynamicCast<JSBigInt*> instead of JSValue::isHeapBigInt ?

nope. I just didn't know it existed.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:1876
>> +    auto limitWorks = [&] (double radix, double lengthLimit) {
> 
> I don't think you need to pass lengthLimit as an argument, it could just be lengthLimitForBigInt32 which is captured.

ok

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:1906
>> +    // radix ** lengthLimitForBigInt32 <= INT32_MAX
> 
> Why is this the opposite from the BigInt32 case?

Because Digit is 32-bit unsigned here, and I don't want to consider what happens below in static_cast<Digit>(uint64_t value)

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:1908
>> +    auto limitWorks = [&] (double radix, double lengthLimit) {
> 
> same comment as before: no need to pass lengthLimit explicitly.

ok

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:1967
>> +                int64_t maybeResult = digit.unsafeGet();
> 
> maybe an assert here that going from uint64_t to int64_t does not overflow? I see no way that it could, but an assert would be reassuring if we ever modify any of that code again.

good idea
Comment 5 Saam Barati 2020-04-22 13:06:07 PDT
Created attachment 397242 [details]
patch
Comment 6 Robin Morisset 2020-04-22 13:27:44 PDT
Comment on attachment 397242 [details]
patch

r=me
Comment 7 Saam Barati 2020-04-22 13:40:18 PDT
Created attachment 397249 [details]
patch for landing
Comment 8 Saam Barati 2020-04-22 13:40:43 PDT
Comment on attachment 397249 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=397249&action=review

> Source/JavaScriptCore/jsc.cpp:2331
> +    return JSValue::encode(jsBoolean(!!jsDynamicCast<JSBigInt*>(globalObject->vm(), callFrame->argument(0))));

oops, forgot about this. Will fix.
Comment 9 Saam Barati 2020-04-22 13:42:29 PDT
Created attachment 397250 [details]
patch for landing
Comment 10 Saam Barati 2020-04-22 13:43:12 PDT
Created attachment 397251 [details]
patch for landing
Comment 11 Saam Barati 2020-04-22 13:44:43 PDT
Created attachment 397252 [details]
patch for landing
Comment 12 EWS 2020-04-22 19:18:50 PDT
Committed r260550: <https://trac.webkit.org/changeset/260550>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397252 [details].
Comment 13 Radar WebKit Bug Importer 2020-04-22 19:19:13 PDT
<rdar://problem/62227321>