Bug 178379

Summary: [ARM64] static_cast<int32_t>() in BinaryOpNode::emitBytecode() prevents op_unsigned emission
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645    
Attachments:
Description Flags
Patch
none
Patch none

Description Zan Dobersek 2017-10-17 00:33:58 PDT
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.
Comment 1 Zan Dobersek 2017-10-17 10:10:00 PDT
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
Comment 2 Yusuke Suzuki 2017-10-18 02:11:31 PDT
Created attachment 324106 [details]
Patch
Comment 3 Zan Dobersek 2017-10-18 02:28:02 PDT
Can confirm this fixes issues on all ARM flavors. Thanks! I'll yield the review to JSC devs.
Comment 4 Zan Dobersek 2017-10-18 02:28:38 PDT
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.
Comment 5 Yusuke Suzuki 2017-10-18 02:28:56 PDT
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.
Comment 6 Yusuke Suzuki 2017-10-18 02:31:33 PDT
Created attachment 324107 [details]
Patch
Comment 7 Saam Barati 2017-10-18 11:50:37 PDT
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?
Comment 8 Yusuke Suzuki 2017-10-19 01:10:08 PDT
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.
Comment 9 Yusuke Suzuki 2017-10-20 00:08:12 PDT
Comment on attachment 324107 [details]
Patch

Thank you
Comment 10 WebKit Commit Bot 2017-10-20 00:35:19 PDT
Comment on attachment 324107 [details]
Patch

Clearing flags on attachment: 324107

Committed r223745: <https://trac.webkit.org/changeset/223745>
Comment 11 WebKit Commit Bot 2017-10-20 00:35:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-11-15 13:04:21 PST
<rdar://problem/35568730>