178379 2017-10-17 00:33:58 -0700 [ARM64] static_cast<int32_t>() in BinaryOpNode::emitBytecode() prevents op_unsigned emission 2017-11-15 13:04:21 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 108645 1 zan ysuzuki buildbot commit-queue fpizlo ggaren keith_miller mark.lam msaboff saam webkit-bug-importer ysuzuki oldest_to_newest 1361164 0 zan 2017-10-17 00:33:58 -0700 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. 1361282 1 zan 2017-10-17 10:10:00 -0700 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 1361614 2 324106 ysuzuki 2017-10-18 02:11:31 -0700 Created attachment 324106 Patch 1361616 3 zan 2017-10-18 02:28:02 -0700 Can confirm this fixes issues on all ARM flavors. Thanks! I'll yield the review to JSC devs. 1361617 4 324106 zan 2017-10-18 02:28:38 -0700 Comment on attachment 324106 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. 1361618 5 324106 ysuzuki 2017-10-18 02:28:56 -0700 Comment on attachment 324106 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. 1361620 6 324107 ysuzuki 2017-10-18 02:31:33 -0700 Created attachment 324107 Patch 1361822 7 324107 saam 2017-10-18 11:50:37 -0700 Comment on attachment 324107 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? 1362081 8 324107 ysuzuki 2017-10-19 01:10:08 -0700 Comment on attachment 324107 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. 1362590 9 324107 ysuzuki 2017-10-20 00:08:12 -0700 Comment on attachment 324107 Patch Thank you 1362596 10 324107 commit-queue 2017-10-20 00:35:19 -0700 Comment on attachment 324107 Patch Clearing flags on attachment: 324107 Committed r223745: <https://trac.webkit.org/changeset/223745> 1362597 11 commit-queue 2017-10-20 00:35:21 -0700 All reviewed patches have been landed. Closing bug. 1372347 12 webkit-bug-importer 2017-11-15 13:04:21 -0800 <rdar://problem/35568730> 324106 2017-10-18 02:11:31 -0700 2017-10-18 02:31:31 -0700 Patch bug-178379-20171018181130.patch text/plain 2115 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjIzNTk2CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA3 MmNjMmM3MDdlMzFmMzlhNTE2NDJiZmE3YTZlMDQ3NTBlYjk5ZjUyLi43ZWI4Y2E2ZDk1ZmIyYzk1 MDE1ZGNhYzE0YTUyZWE5MWZhNzk2NTNiIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxNy0xMC0xOCAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWls LmNvbT4KKworICAgICAgICBbQVJNNjRdIHN0YXRpY19jYXN0PGludDMyX3Q+KCkgaW4gQmluYXJ5 T3BOb2RlOjplbWl0Qnl0ZWNvZGUoKSBwcmV2ZW50cyBvcF91bnNpZ25lZCBlbWlzc2lvbgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTc4Mzc5CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2UgcmV1c2UganNO dW1iZXIncyBjaGVja2luZyBtZWNoYW5pc20gaGVyZSB0byBwcmVjaXNlbHkgY2hlY2sgdGhlIGdl bmVyYXRlZCBudW1iZXIgaXMgd2l0aGluIHVpbnQzMl90CisgICAgICAgIGluIGJ5dGVjb2RlIGNv bXBpbGVyLiBUaGlzIGlzIHJlYXNvbmFibGUgc2luY2UgdGhlIE51bWJlck5vZGUgd2lsbCBnZW5l cmF0ZSB0aGUgZXhhY3QgdGhpcyBKU1ZhbHVlLgorCisgICAgICAgICogYnl0ZWNvbXBpbGVyL05v ZGVzQ29kZWdlbi5jcHA6CisgICAgICAgIChKU0M6OkJpbmFyeU9wTm9kZTo6ZW1pdEJ5dGVjb2Rl KToKKwogMjAxNy0xMC0xOCAgUnlvc3VrZSBOaXdhICA8cm5pd2FAd2Via2l0Lm9yZz4KIAogICAg ICAgICBGaXggbWFjT1MgYW5kIGlPUyBidWlsZHMgYWZ0ZXIgcjIyMzU5NC4KZGlmZiAtLWdpdCBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvTm9kZXNDb2RlZ2VuLmNwcCBiL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvTm9kZXNDb2RlZ2VuLmNwcAppbmRleCAz NTViNGE3MmVlOTQyNjgzNzY1ZTQ1ODU3YjAzZDlkMGNhMzZhYmM5Li5iZmY4MmU1Zjc2ZDVlZDdk OWYzZDAyNDVmOTZiZGY0MmMzNzI3ZjcwIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvYnl0ZWNvbXBpbGVyL05vZGVzQ29kZWdlbi5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL2J5dGVjb21waWxlci9Ob2Rlc0NvZGVnZW4uY3BwCkBAIC0xOTkzLDkgKzE5OTMsMTEgQEAg UmVnaXN0ZXJJRCogQmluYXJ5T3BOb2RlOjplbWl0Qnl0ZWNvZGUoQnl0ZWNvZGVHZW5lcmF0b3Im IGdlbmVyYXRvciwgUmVnaXN0ZXJJRCoKICAgICAgICAgICAgIGlmIChub2RlLT5pc0JpbmFyeU9w Tm9kZSgpICYmIHN0YXRpY19jYXN0PEJpbmFyeU9wTm9kZSo+KG5vZGUpLT5vcGNvZGVJRCgpID09 IG9wX3Vyc2hpZnQpCiAgICAgICAgICAgICAgICAgcmV0dXJuIFVJbnQzMlJlc3VsdDo6VUludDMy OwogICAgICAgICAgICAgaWYgKG5vZGUtPmlzTnVtYmVyKCkgJiYgc3RhdGljX2Nhc3Q8TnVtYmVy Tm9kZSo+KG5vZGUpLT5pc0ludGVnZXJOb2RlKCkpIHsKLSAgICAgICAgICAgICAgICBpbnQzMl90 IHZhbHVlID0gc3RhdGljX2Nhc3Q8aW50MzJfdD4oc3RhdGljX2Nhc3Q8SW50ZWdlck5vZGUqPihu b2RlKS0+dmFsdWUoKSk7Ci0gICAgICAgICAgICAgICAgaWYgKHZhbHVlID49IDApCi0gICAgICAg ICAgICAgICAgICAgIHJldHVybiBVSW50MzJSZXN1bHQ6OkNvbnN0YW50OworICAgICAgICAgICAg ICAgIGF1dG8gdmFsdWUgPSBqc051bWJlcihzdGF0aWNfY2FzdDxOdW1iZXJOb2RlKj4obm9kZSkt PnZhbHVlKCkpOworICAgICAgICAgICAgICAgIGlmICh2YWx1ZS5pc0ludDMyKCkpIHsKKyAgICAg ICAgICAgICAgICAgICAgaWYgKHZhbHVlLmFzSW50MzIoKSA+PSAwKQorICAgICAgICAgICAgICAg ICAgICAgICAgcmV0dXJuIFVJbnQzMlJlc3VsdDo6Q29uc3RhbnQ7CisgICAgICAgICAgICAgICAg fQogICAgICAgICAgICAgfQogICAgICAgICAgICAgcmV0dXJuIHN0ZDo6bnVsbG9wdDsKICAgICAg ICAgfTsK 324107 2017-10-18 02:31:33 -0700 2017-10-20 00:35:19 -0700 Patch bug-178379-20171018183132.patch text/plain 2002 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjIzNTk2CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA3 MmNjMmM3MDdlMzFmMzlhNTE2NDJiZmE3YTZlMDQ3NTBlYjk5ZjUyLi43ZWI4Y2E2ZDk1ZmIyYzk1 MDE1ZGNhYzE0YTUyZWE5MWZhNzk2NTNiIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxNy0xMC0xOCAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWls LmNvbT4KKworICAgICAgICBbQVJNNjRdIHN0YXRpY19jYXN0PGludDMyX3Q+KCkgaW4gQmluYXJ5 T3BOb2RlOjplbWl0Qnl0ZWNvZGUoKSBwcmV2ZW50cyBvcF91bnNpZ25lZCBlbWlzc2lvbgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTc4Mzc5CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgV2UgcmV1c2UganNO dW1iZXIncyBjaGVja2luZyBtZWNoYW5pc20gaGVyZSB0byBwcmVjaXNlbHkgY2hlY2sgdGhlIGdl bmVyYXRlZCBudW1iZXIgaXMgd2l0aGluIHVpbnQzMl90CisgICAgICAgIGluIGJ5dGVjb2RlIGNv bXBpbGVyLiBUaGlzIGlzIHJlYXNvbmFibGUgc2luY2UgdGhlIE51bWJlck5vZGUgd2lsbCBnZW5l cmF0ZSB0aGUgZXhhY3QgdGhpcyBKU1ZhbHVlLgorCisgICAgICAgICogYnl0ZWNvbXBpbGVyL05v ZGVzQ29kZWdlbi5jcHA6CisgICAgICAgIChKU0M6OkJpbmFyeU9wTm9kZTo6ZW1pdEJ5dGVjb2Rl KToKKwogMjAxNy0xMC0xOCAgUnlvc3VrZSBOaXdhICA8cm5pd2FAd2Via2l0Lm9yZz4KIAogICAg ICAgICBGaXggbWFjT1MgYW5kIGlPUyBidWlsZHMgYWZ0ZXIgcjIyMzU5NC4KZGlmZiAtLWdpdCBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvTm9kZXNDb2RlZ2VuLmNwcCBiL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS9ieXRlY29tcGlsZXIvTm9kZXNDb2RlZ2VuLmNwcAppbmRleCAz NTViNGE3MmVlOTQyNjgzNzY1ZTQ1ODU3YjAzZDlkMGNhMzZhYmM5Li41NDRjNmQyMzU4YzNhNGZh M2UzYzRhOTJmZWZkZTZmZjBkN2NlMTE3IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvYnl0ZWNvbXBpbGVyL05vZGVzQ29kZWdlbi5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL2J5dGVjb21waWxlci9Ob2Rlc0NvZGVnZW4uY3BwCkBAIC0xOTkzLDggKzE5OTMsOCBAQCBS ZWdpc3RlcklEKiBCaW5hcnlPcE5vZGU6OmVtaXRCeXRlY29kZShCeXRlY29kZUdlbmVyYXRvciYg Z2VuZXJhdG9yLCBSZWdpc3RlcklEKgogICAgICAgICAgICAgaWYgKG5vZGUtPmlzQmluYXJ5T3BO b2RlKCkgJiYgc3RhdGljX2Nhc3Q8QmluYXJ5T3BOb2RlKj4obm9kZSktPm9wY29kZUlEKCkgPT0g b3BfdXJzaGlmdCkKICAgICAgICAgICAgICAgICByZXR1cm4gVUludDMyUmVzdWx0OjpVSW50MzI7 CiAgICAgICAgICAgICBpZiAobm9kZS0+aXNOdW1iZXIoKSAmJiBzdGF0aWNfY2FzdDxOdW1iZXJO b2RlKj4obm9kZSktPmlzSW50ZWdlck5vZGUoKSkgewotICAgICAgICAgICAgICAgIGludDMyX3Qg dmFsdWUgPSBzdGF0aWNfY2FzdDxpbnQzMl90PihzdGF0aWNfY2FzdDxJbnRlZ2VyTm9kZSo+KG5v ZGUpLT52YWx1ZSgpKTsKLSAgICAgICAgICAgICAgICBpZiAodmFsdWUgPj0gMCkKKyAgICAgICAg ICAgICAgICBhdXRvIHZhbHVlID0ganNOdW1iZXIoc3RhdGljX2Nhc3Q8TnVtYmVyTm9kZSo+KG5v ZGUpLT52YWx1ZSgpKTsKKyAgICAgICAgICAgICAgICBpZiAodmFsdWUuaXNJbnQzMigpICYmIHZh bHVlLmFzSW50MzIoKSA+PSAwKQogICAgICAgICAgICAgICAgICAgICByZXR1cm4gVUludDMyUmVz dWx0OjpDb25zdGFudDsKICAgICAgICAgICAgIH0KICAgICAgICAgICAgIHJldHVybiBzdGQ6Om51 bGxvcHQ7Cg==