Bug 210835 – [JSC] NumberConstructor should accept BigInt

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

This looks like a spec change from the original one.
https://tc39.es/ecma262/#sec-number-constructor-number-value

[Yusuke Suzuki]

DFG/FTL support is also required since NumberConstructor is supported in DFG.
Maybe, we should introduce CallNumberConstructor DFG node.

[Yusuke Suzuki]

Created attachment 397656 [details]
Patch

[Yusuke Suzuki]

Created attachment 397657 [details]
Patch

[Yusuke Suzuki]

Created attachment 397658 [details]
Patch

[Yusuke Suzuki]

Created attachment 397660 [details]
Patch

[Mark Lam]

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?

[Mark Lam]

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?

[Mark Lam]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Created attachment 397805 [details]
Patch for landing

[Yusuke Suzuki]

Committed r260834: <https://trac.webkit.org/changeset/260834>

[Radar WebKit Bug Importer]

<rdar://problem/62531125>