Bug 212193 – [JSC] Fix 32bit JSBigInt with INT32_MAX < x <= UINT32_MAX

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Fix 32bit JSBigInt with INT32_MAX < x <= UINT32_MAX

[Yusuke Suzuki]

Created attachment 399945 [details]
Patch

[Mark Lam]

Comment on attachment 399945 [details]
Patch

r=me

[EWS]

Committed r262012: <https://trac.webkit.org/changeset/262012>

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

[Radar WebKit Bug Importer]

<rdar://problem/63498967>

[Saam Barati]

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?

[Mark Lam]

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?

[Saam Barati]

(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?

[Saam Barati]

(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

[Yusuke Suzuki]

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