Bug 210869 – BigInt32 parsing should be precise

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 397236 [details]
patch

[Mark Lam]

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.

[Robin Morisset]

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.

[Saam Barati]

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

[Saam Barati]

Created attachment 397242 [details]
patch

[Robin Morisset]

Comment on attachment 397242 [details]
patch

r=me

[Saam Barati]

Created attachment 397249 [details]
patch for landing

[Saam Barati]

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.

[Saam Barati]

Created attachment 397250 [details]
patch for landing

[Saam Barati]

Created attachment 397251 [details]
patch for landing

[Saam Barati]

Created attachment 397252 [details]
patch for landing

[EWS]

Committed r260550: <https://trac.webkit.org/changeset/260550>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397252 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62227321>