[ARM64] static_cast<int32_t>() in BinaryOpNode::emitBytecode() prevents op_unsigned emission

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

Using GCC 6.3.1 and targeting ARM64 on Linux, the stress/uint32-to-number-overflows-to-uint52.js JSC stress test (in all variations) fails since r223318 landed.

http://trac.webkit.org/browser/webkit/trunk/JSTests/stress/uint32-to-number-overflows-to-uint52.js

The compareToLargeNumber() function in that test is the problematic one:

> function compareToLargeNumber(value)
> {
>     return (value >>> 0) < 4294967294;
> }

Here's the generated bytecode output for this function on x86_64, which matches ARM64 before r223318:

compareToLargeNumber#D5ozQ8:[0x7ff965a890d0->0x7ff965aa8ba0, NoneFunctionCall, 20 (NeverInline)]: 20 m_instructions; 160 bytes; 2 parameter(s); 8 callee register(s); 6 variable(s); scope at loc3
[   0] enter             
[   1] get_scope         loc3
[   3] mov               loc4, loc3
[   6] check_traps       
[   7] urshift           loc6, arg1, Int32: 0(const0)
[  11] unsigned          loc6, loc6
[  14] less              loc6, loc6, Double: 4751297606871678976, 4294967294.000000(const1)
[  18] ret               loc6

Here's the currently generated bytecode on ARM64, missing op_unsigned:

compareToLargeNumber#D5ozQ8:[0x7f809890d0->0x7f809a8ba0, NoneFunctionCall, 17 (NeverInline)]: 17 m_instructions; 136 bytes; 2 parameter(s); 8 callee register(s); 6 variable(s); scope at loc3
[   0] enter             
[   1] get_scope         loc3
[   3] mov               loc4, loc3
[   6] check_traps       
[   7] urshift           loc6, arg1, Int32: 0(const0)
[  11] below             loc6, loc6, Double: 4751297606871678976, 4294967294.000000(const1)
[  15] ret               loc6

-----

Problem can be tracked to the double-to-int32_t conversion in the helper isUInt32() lambda in BinaryOpNode::emitBytecode(). On x86_64, the 4294967294.0 double value is converted to a negative 32-bit of value -2147483648, which prevents the m_shouldToUnsignedResult value for the op_urshift BinaryOpNode to be set to false, which ends up emitting op_unsigned and thus passing the test.

On ARM64, the same double value is converted to 2147483647. The GCC-generated code does this using the plain fcvtzs instruction:

>   6fe208:	940039b2 	bl	70c8d0 <JSC::NumberNode::value() const [clone .isra.109]>
>   6fe20c:	1e780000 	fcvtzs	w0, d0

Result of this is that both left and right sides of the binary op are recognized as an uint32 value, short-cutting op_unsigned and failing the test.

[Zan Dobersek]

Identical problem also appears on ARM Thumb2, with the float value being converted to the same integer value as on ARM64. 

Here's the generated code that performs conversion:

>   2352c8:	ed98 7b06 	vldr	d7, [r8, #24]
> ...
>   2352d2:	eefd 6bc7 	vcvt.s32.f64	s13, d7go

[Yusuke Suzuki]

Created attachment 324106 [details]
Patch

[Zan Dobersek]

Can confirm this fixes issues on all ARM flavors. Thanks! I'll yield the review to JSC devs.

[Zan Dobersek]

Comment on attachment 324106 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1998
> +                if (value.isInt32()) {
> +                    if (value.asInt32() >= 0)

Nit: these two if-statements can be folded into one.

[Yusuke Suzuki]

Comment on attachment 324106 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1998
>> +                    if (value.asInt32() >= 0)
> 
> Nit: these two if-statements can be folded into one.

Nice, fixed.

[Yusuke Suzuki]

Created attachment 324107 [details]
Patch

[Saam Barati]

Comment on attachment 324107 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1997
> +                auto value = jsNumber(static_cast<NumberNode*>(node)->value());
> +                if (value.isInt32() && value.asInt32() >= 0)

I thought the goal was to treat 32-bit signed JSValue representation as unsigned? Am I missing something?

[Yusuke Suzuki]

Comment on attachment 324107 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1997
>> +                if (value.isInt32() && value.asInt32() >= 0)
> 
> I thought the goal was to treat 32-bit signed JSValue representation as unsigned? Am I missing something?

No. The goal of this is returning `UInt32Result::Constant` if the constant integer value is within uint32_t range. (this is `op_unsigned(lhs) < rhsConstantUInt32`'s rhsConstantUInt32 detection.)
In this patch, we allow [0, INT32_MAX] for this case.

[Yusuke Suzuki]

Comment on attachment 324107 [details]
Patch

Thank you

[WebKit Commit Bot]

Comment on attachment 324107 [details]
Patch

Clearing flags on attachment: 324107

Committed r223745: <https://trac.webkit.org/changeset/223745>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568730>