Bug 234399

Summary: [JSC][32bit] Fix undefined behavior causing miscompilation with clang 13 on ARM
Product: WebKit Reporter: Mikhail R. Gadelha <mikhail>
Component: New BugsAssignee: Mikhail R. Gadelha <mikhail>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Mikhail R. Gadelha 2021-12-16 10:27:50 PST 
[JSC][32bit] Fix undefined behavior causing misscompilation with clang 13 on ARM
Comment 1 Mikhail R. Gadelha 2021-12-16 10:35:20 PST 
Created attachment 447369 [details]
Patch
Comment 2 Mikhail R. Gadelha 2021-12-16 10:37:50 PST 
Created attachment 447370 [details]
Patch
Comment 3 Mikhail R. Gadelha 2021-12-16 10:48:17 PST 
Created attachment 447373 [details]
Patch
Comment 4 Yusuke Suzuki 2021-12-16 13:34:03 PST 
Comment on attachment 447373 [details]
Patch

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

> Source/JavaScriptCore/runtime/MathCommon.h:224
>  {
> -    // Note: Strictly speaking this is an undefined behavior.
> -    return static_cast<int32_t>(value) == value;
> +    return canBeInt32(value) && !(!value && std::signbit(value)); // false for -0.0
>  }

This needs to be super fast. I think we should inline canBeInt32's code here, and use !asInt32 instead of !value.
Comment 5 Mikhail R. Gadelha 2021-12-17 05:51:29 PST 
Created attachment 447451 [details]
Patch
Comment 6 Yusuke Suzuki 2021-12-18 21:54:03 PST 
Comment on attachment 447451 [details]
Patch

r=me
Comment 7 EWS 2021-12-18 22:31:50 PST 
Committed r287235 (245395@main): <https://commits.webkit.org/245395@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447451 [details].
Comment 8 Radar WebKit Bug Importer 2021-12-18 22:32:19 PST 
<rdar://problem/86678800>
Comment 9 Darin Adler 2021-12-19 15:50:23 PST 
I prefer std::isfinite for checks like this.
Comment 10 Darin Adler 2021-12-19 15:51:02 PST 
(In reply to Darin Adler from comment #9)
> I prefer std::isfinite for checks like this.

Unless it is slower than calling both std::isnan and std::isinf. And if it is, then they should really fix that!
Comment 11 Mikhail R. Gadelha 2021-12-20 06:25:17 PST 
(In reply to Darin Adler from comment #10)
> (In reply to Darin Adler from comment #9)
> > I prefer std::isfinite for checks like this.
> 
> Unless it is slower than calling both std::isnan and std::isinf. And if it
> is, then they should really fix that!

Humm, I think it's a good idea to benchmark that, we can even compare the two approaches against using fpclassify.