Bug 212193

Summary: [JSC] Fix 32bit JSBigInt with INT32_MAX < x <= UINT32_MAX
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: 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 none

Description Yusuke Suzuki 2020-05-21 01:51:42 PDT
[JSC] Fix 32bit JSBigInt with INT32_MAX < x <= UINT32_MAX
Comment 1 Yusuke Suzuki 2020-05-21 02:02:01 PDT
Created attachment 399945 [details]
Patch
Comment 2 Mark Lam 2020-05-21 09:06:52 PDT
Comment on attachment 399945 [details]
Patch

r=me
Comment 3 EWS 2020-05-21 10:21:19 PDT
Committed r262012: <https://trac.webkit.org/changeset/262012>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399945 [details].
Comment 4 Radar WebKit Bug Importer 2020-05-21 10:22:21 PDT
<rdar://problem/63498967>
Comment 5 Saam Barati 2020-05-21 12:51:43 PDT
Comment on attachment 399945 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.cpp:176
> +        unsignedValue = static_cast<uint64_t>(-(value + 1)) + 1;

why not just use abs?
Comment 6 Mark Lam 2020-05-21 12:54:22 PDT
Comment on attachment 399945 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:176
>> +        unsignedValue = static_cast<uint64_t>(-(value + 1)) + 1;
> 
> why not just use abs?

Would abs guarantee no undefined behavior when value is INT64_MIN?
Comment 7 Saam Barati 2020-05-21 12:54:29 PDT
(In reply to Saam Barati from comment #5)
> Comment on attachment 399945 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399945&action=review
> 
> > Source/JavaScriptCore/runtime/JSBigInt.cpp:176
> > +        unsignedValue = static_cast<uint64_t>(-(value + 1)) + 1;
> 
> why not just use abs?

Maybe we're explicitly avoiding UB if Abs has UB for INT_MIN?
Comment 8 Saam Barati 2020-05-21 12:54:53 PDT
(In reply to Mark Lam from comment #6)
> Comment on attachment 399945 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=399945&action=review
> 
> >> Source/JavaScriptCore/runtime/JSBigInt.cpp:176
> >> +        unsignedValue = static_cast<uint64_t>(-(value + 1)) + 1;
> > 
> > why not just use abs?
> 
> Would abs guarantee no undefined behavior when value is INT64_MIN?

Yep that's the question. Maybe we should invent our own abs if it does have UB
Comment 9 Yusuke Suzuki 2020-05-21 13:39:42 PDT
Comment on attachment 399945 [details]
Patch

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

>>>>> Source/JavaScriptCore/runtime/JSBigInt.cpp:176
>>>>> +        unsignedValue = static_cast<uint64_t>(-(value + 1)) + 1;
>>>> 
>>>> why not just use abs?
>>> 
>>> Would abs guarantee no undefined behavior when value is INT64_MIN?
>> 
>> Maybe we're explicitly avoiding UB if Abs has UB for INT_MIN?
> 
> Yep that's the question. Maybe we should invent our own abs if it does have UB

abs always returns signed types.
https://en.cppreference.com/w/cpp/numeric/math/abs

So, std::abs(value) -> int64_t. And in this case, INT64_MIN will not fit and this is UB[1].

[1]: "Computes the absolute value of an integer number. The behavior is undefined if the result cannot be represented by the return type." from https://en.cppreference.com/w/cpp/numeric/math/abs