Bug 234399 – [JSC][32bit] Fix undefined behavior causing miscompilation with clang 13 on ARM

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

[Mikhail R. Gadelha]

[JSC][32bit] Fix undefined behavior causing misscompilation with clang 13 on ARM

[Mikhail R. Gadelha]

Created attachment 447369 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 447370 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 447373 [details]
Patch

[Yusuke Suzuki]

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.

[Mikhail R. Gadelha]

Created attachment 447451 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 447451 [details]
Patch

r=me

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86678800>

[Darin Adler]

I prefer std::isfinite for checks like this.

[Darin Adler]

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

[Mikhail R. Gadelha]

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