Bug 211030

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

Description Yusuke Suzuki 2020-04-25 14:27:28 PDT
...
Comment 1 Yusuke Suzuki 2020-04-25 15:01:46 PDT
Created attachment 397588 [details]
Patch
Comment 2 Darin Adler 2020-04-25 18:06:57 PDT
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?
Comment 3 Yusuke Suzuki 2020-04-25 18:54:21 PDT
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.
Comment 4 Yusuke Suzuki 2020-04-25 18:58:19 PDT
Created attachment 397604 [details]
Patch for landing
Comment 5 Yusuke Suzuki 2020-04-25 19:07:03 PDT
Created attachment 397605 [details]
Patch for landing
Comment 6 EWS 2020-04-25 21:44:26 PDT
Committed r260720: <https://trac.webkit.org/changeset/260720>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397605 [details].
Comment 7 Radar WebKit Bug Importer 2020-04-25 21:45:26 PDT
<rdar://problem/62380341>