Bug 210835

Summary: [JSC] NumberConstructor should accept BigInt
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
mark.lam: review+
Patch for landing none

Description Yusuke Suzuki 2020-04-21 18:07:16 PDT 
...
Comment 1 Yusuke Suzuki 2020-04-21 18:07:59 PDT 
This looks like a spec change from the original one.
https://tc39.es/ecma262/#sec-number-constructor-number-value
Comment 2 Yusuke Suzuki 2020-04-21 18:18:52 PDT 
DFG/FTL support is also required since NumberConstructor is supported in DFG.
Maybe, we should introduce CallNumberConstructor DFG node.
Comment 3 Yusuke Suzuki 2020-04-27 01:14:40 PDT 
Created attachment 397656 [details]
Patch
Comment 4 Yusuke Suzuki 2020-04-27 01:19:25 PDT 
Created attachment 397657 [details]
Patch
Comment 5 Yusuke Suzuki 2020-04-27 01:27:18 PDT 
Created attachment 397658 [details]
Patch
Comment 6 Yusuke Suzuki 2020-04-27 01:35:56 PDT 
Created attachment 397660 [details]
Patch
Comment 7 Mark Lam 2020-04-27 11:23:48 PDT 
Comment on attachment 397660 [details]
Patch

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

Some comments while I continue reviewing.  If you haven't already done so, can you also run the added tests against Chrome and Firefox as sanity check that there's no unexpected difference in behavior?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2256
> +    mantissa >>= 12; // (12 = 64 - 52) (double's mantissa bit count).

12 is the non-mantissa bit count, i.e. exponent and sign bits.  52 is the mantissa it count.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2257
> +    int32_t mantissaBitsUnset = shiftAmount - 12;

This equation was unclear to me at first, but after it working it out, I see that it's correct.  Can you put the following in a comment her to explain how we derived this equation?

unsetBits = 64 - setBits - 12 // 12 for non-mantissa bits
    setBits = 64 - (msdLeadingZeros + 1 + bitsNotAvailableDueToDigitSize);  // 1 for hidden mantissa bit.
                = 64 - (msdLeadingZeros + 1 + (64 - digitBits))
                = 64 - shiftAmount
Hence, unsetBits = 64 - (64 - shiftAmount) - 12 = shiftAmount - 12

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2266
> +    if (mantissaBitsUnset >= static_cast<int32_t>(digitBits) && digitIndex > 0) {
> +        digitIndex--;
> +        currentDigit = static_cast<uint64_t>(bigInt->digit(digitIndex));
> +        mantissa |= (currentDigit << (mantissaBitsUnset - digitBits));
> +        mantissaBitsUnset -= digitBits;
> +    }

Can you wrap this in a "if constexpr (digitBits < 64)"?  This code should be a no-op for 64 bit targets.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2292
> +    uint64_t doubleBits = signBit | exponent | mantissa;

Can you add ASSERTs here to confirm that we didn't have a bug that results in any of the 3 components exceeding their bit boundaries?
    ASSERT(doubleBits & (1 << 63) == signBit);
    ASSERT(doubleBits & (0x7ff << 52) == exponent);
    ASSERT(doubleBits & ((1 << 52) - 1) == mantissa);

> Source/JavaScriptCore/runtime/JSBigInt.h:280
> +    static constexpr unsigned doublePhysicalSignificandSize = 52;

You're using a mix of "Significand" and "mantissa" in this patch.  Can you stick with just one?  Maybe call this doublePhysicalMantissaSize instead?

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:100
> +            numeric = JSBigInt::toNumber(numeric);

Can you ASSERT(numeric.isBigInt()) at the top here?
Comment 8 Mark Lam 2020-04-27 11:41:29 PDT 
Comment on attachment 397660 [details]
Patch

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

> JSTests/stress/number-constructor-bigint.js:29
> +// Around Infinity.

Can you also add some cases for mantissa rounding both near infinity (may rounD to infinity) and not near infinity?
Comment 9 Mark Lam 2020-04-27 14:23:47 PDT 
Comment on attachment 397660 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3102
>  #if USE(BIGINT32)

Let's move this #if up around the "if (node->op() == CallNumberConstructor) {".

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3109
>  #endif

Move below } below.
Comment 10 Yusuke Suzuki 2020-04-27 21:26:47 PDT 
Comment on attachment 397660 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3102
>>  #if USE(BIGINT32)
> 
> Let's move this #if up around the "if (node->op() == CallNumberConstructor) {".

Done.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3109
>>  #endif
> 
> Move below } below.

Done

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:2256
>> +    mantissa >>= 12; // (12 = 64 - 52) (double's mantissa bit count).
> 
> 12 is the non-mantissa bit count, i.e. exponent and sign bits.  52 is the mantissa it count.

Fixed.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:2266
>> +    }
> 
> Can you wrap this in a "if constexpr (digitBits < 64)"?  This code should be a no-op for 64 bit targets.

Sounds nice! Fixed.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:2292
>> +    uint64_t doubleBits = signBit | exponent | mantissa;
> 
> Can you add ASSERTs here to confirm that we didn't have a bug that results in any of the 3 components exceeding their bit boundaries?
>     ASSERT(doubleBits & (1 << 63) == signBit);
>     ASSERT(doubleBits & (0x7ff << 52) == exponent);
>     ASSERT(doubleBits & ((1 << 52) - 1) == mantissa);

Sounds nice, fixed.

>> Source/JavaScriptCore/runtime/JSBigInt.h:280
>> +    static constexpr unsigned doublePhysicalSignificandSize = 52;
> 
> You're using a mix of "Significand" and "mantissa" in this patch.  Can you stick with just one?  Maybe call this doublePhysicalMantissaSize instead?

Fixed, nice.

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:100
>> +            numeric = JSBigInt::toNumber(numeric);
> 
> Can you ASSERT(numeric.isBigInt()) at the top here?

Sounds good. Fixed.

>> JSTests/stress/number-constructor-bigint.js:29
>> +// Around Infinity.
> 
> Can you also add some cases for mantissa rounding both near infinity (may rounD to infinity) and not near infinity?

near infinity thing is already added (`shouldBe(Number(0x3fffffffffffffn << 969n), 8.98846567431158e+307);`), I'll add non-near infinity case.
Comment 11 Yusuke Suzuki 2020-04-27 21:27:30 PDT 
Comment on attachment 397660 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:2257
>> +    int32_t mantissaBitsUnset = shiftAmount - 12;
> 
> This equation was unclear to me at first, but after it working it out, I see that it's correct.  Can you put the following in a comment her to explain how we derived this equation?
> 
> unsetBits = 64 - setBits - 12 // 12 for non-mantissa bits
>     setBits = 64 - (msdLeadingZeros + 1 + bitsNotAvailableDueToDigitSize);  // 1 for hidden mantissa bit.
>                 = 64 - (msdLeadingZeros + 1 + (64 - digitBits))
>                 = 64 - shiftAmount
> Hence, unsetBits = 64 - (64 - shiftAmount) - 12 = shiftAmount - 12

Added, thanks!
Comment 12 Yusuke Suzuki 2020-04-27 22:04:23 PDT 
Created attachment 397805 [details]
Patch for landing
Comment 13 Yusuke Suzuki 2020-04-28 11:04:58 PDT 
Committed r260834: <https://trac.webkit.org/changeset/260834>
Comment 14 Radar WebKit Bug Importer 2020-04-28 11:05:16 PDT 
<rdar://problem/62531125>