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, saam, 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

Yusuke Suzuki
Reported 2020-04-25 14:27:28 PDT
...
Attachments
Patch (4.82 KB, patch)
2020-04-25 15:01 PDT, Yusuke Suzuki 		darin: review+
DetailsFormatted DiffDiff
Patch for landing (4.41 KB, patch)
2020-04-25 18:58 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.08 KB, patch)
2020-04-25 19:07 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2020-04-25 15:01:46 PDT
Created attachment 397588 [details] Patch
Darin Adler
Comment 2 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?
Yusuke Suzuki
Comment 3 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.
Yusuke Suzuki
Comment 4 2020-04-25 18:58:19 PDT
Created attachment 397604 [details] Patch for landing
Yusuke Suzuki
Comment 5 2020-04-25 19:07:03 PDT
Created attachment 397605 [details] Patch for landing
EWS
Comment 6 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].
Radar WebKit Bug Importer
Comment 7 2020-04-25 21:45:26 PDT
<rdar://problem/62380341>
Note You need to log in before you can comment on or make changes to this bug.