[JSC] Handle BigInt32 INT32_MIN shift amount

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 397588 [details]
Patch

[Darin Adler]

Comment on attachment 397588 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] Handle BitInt32 INT32_MIN shift amount

typo: BigInt32

> Source/JavaScriptCore/runtime/Operations.h:776
> +            if (rightInt32 == INT32_MIN) {
> +                // Shift-amount is 0x80000000. For right-shift, shift-amount is reduced to 31.
> +                if (!isLeft)
> +                    return jsBigInt32(leftInt32 >> 31);
> +                // Left-shift with 0x80000000 produces too large BigInt, and throws a RangeError.
> +                // But when leftInt32 is zero, we should return zero.
> +                if (!leftInt32)
> +                    return jsBigInt32(0);
> +                throwRangeError(globalObject, scope, "BigInt generated from this operation is too big"_s);
> +                return { };
> +            }
>              rightInt32 = -rightInt32;

Would this simpler implementation still gives us the correct result?

    if (rightInt32 == INT32_MIN)
        rightInt32 = INT32_MAX; // Shifts one less than requested, but makes no observable difference.
    else
        rightInt32 = -rightInt32;

Would you consider it if it does?

[Yusuke Suzuki]

Comment on attachment 397588 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:3
>> +        [JSC] Handle BitInt32 INT32_MIN shift amount
> 
> typo: BigInt32

Ooops, fixed.

>> Source/JavaScriptCore/runtime/Operations.h:776
>>              rightInt32 = -rightInt32;
> 
> Would this simpler implementation still gives us the correct result?
> 
>     if (rightInt32 == INT32_MIN)
>         rightInt32 = INT32_MAX; // Shifts one less than requested, but makes no observable difference.
>     else
>         rightInt32 = -rightInt32;
> 
> Would you consider it if it does?

Good point! Right, sounds simpler. Fixed.

[Yusuke Suzuki]

Created attachment 397604 [details]
Patch for landing

[Yusuke Suzuki]

Created attachment 397605 [details]
Patch for landing

[EWS]

Committed r260720: <https://trac.webkit.org/changeset/260720>

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

[Radar WebKit Bug Importer]

<rdar://problem/62380341>