192908 2018-12-19 18:15:33 -0800 StrengthReductionPhase ValueAdd rule handles BigInt handleCommutativity is breaking JSC::DFG::Edge::useKind() 2018-12-20 02:28:44 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED DUPLICATE 192723 P2 Normal --- 1 ticaiolima ticaiolima ews-watchlist keith_miller mark.lam msaboff saam ysuzuki oldest_to_newest 1489652 0 ticaiolima 2018-12-19 18:15:33 -0800 Now, there are 35 tests failing into Debug builds: https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/1926/steps/jscore-test/logs/stdio 1489653 1 357768 ticaiolima 2018-12-19 18:19:36 -0800 Created attachment 357768 Patch 1489700 2 357768 mark.lam 2018-12-19 20:26:13 -0800 Comment on attachment 357768 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357768&action=review > Source/JavaScriptCore/ChangeLog:10 > + Current rule is considering that both operands of ValueAdd have nodes, > + but this is not quite true. In such case, we are checking the > + existence of both nodes before using m_node->binaryUseKind. I don't understand: when would ValueAdd ever not have children nodes? Also, please provide a test case. The test will help illustrate the issue. 1489701 3 357768 mark.lam 2018-12-19 20:27:36 -0800 Comment on attachment 357768 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357768&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + existence of both nodes before using m_node->binaryUseKind. > > I don't understand: when would ValueAdd ever not have children nodes? Also, please provide a test case. The test will help illustrate the issue. Oh, I see there are already failing tests. I'm still surprised that ValueAdd has edges that don't have nodes. Perhaps there's something I don't know. 1489767 4 357768 ysuzuki 2018-12-19 23:24:49 -0800 Comment on attachment 357768 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357768&action=review >>> Source/JavaScriptCore/ChangeLog:10 >>> + existence of both nodes before using m_node->binaryUseKind. >> >> I don't understand: when would ValueAdd ever not have children nodes? Also, please provide a test case. The test will help illustrate the issue. > > Oh, I see there are already failing tests. I'm still surprised that ValueAdd has edges that don't have nodes. Perhaps there's something I don't know. Yeah, I don't think the current situation is right. ValueAdd must have two children. If it happens, it's a bug. We should fix that bug instead of checking `child1().node() && child2().node()`. 1489808 5 ticaiolima 2018-12-20 02:28:14 -0800 Thx for the review. (In reply to Yusuke Suzuki from comment #4) > Comment on attachment 357768 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357768&action=review > > >>> Source/JavaScriptCore/ChangeLog:10 > >>> + existence of both nodes before using m_node->binaryUseKind. > >> > >> I don't understand: when would ValueAdd ever not have children nodes? Also, please provide a test case. The test will help illustrate the issue. > > > > Oh, I see there are already failing tests. I'm still surprised that ValueAdd has edges that don't have nodes. Perhaps there's something I don't know. > > Yeah, I don't think the current situation is right. ValueAdd must have two > children. > If it happens, it's a bug. We should fix that bug instead of checking > `child1().node() && child2().node()`. Makes sense. The bug happens when there are 2 constant Strings into ValueAdd and we perform the folding. When this happened we should not execute "m_node->binaryUseKind()". I'm closing this to fix into original Bug https://bugs.webkit.org/show_bug.cgi?id=192723 since we rolled this out. 1489809 6 ticaiolima 2018-12-20 02:28:44 -0800 *** This bug has been marked as a duplicate of bug 192723 *** 357768 2018-12-19 18:19:36 -0800 2018-12-19 23:24:49 -0800 Patch bug-192908-20181220001935.patch text/plain 1682 ticaiolima U3VidmVyc2lvbiBSZXZpc2lvbjogMjM5Mzc3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAw NjFkOTBiZjdlYThlODUyNGYyNmU1ZDI0MTc4ZGI3MTIzZmVkN2JlLi43ZTExYjBmNjBiMmNlZTE5 MzY4ODkwOTY5NmYwOWIxMDAyNDQxZmEwIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxOC0xMi0xOSAgQ2FpbyBMaW1hICA8dGljYWlvbGltYUBnbWFpbC5jb20+ CisKKyAgICAgICAgU3RyZW5ndGhSZWR1Y3Rpb25QaGFzZSBWYWx1ZUFkZCBydWxlIGhhbmRsZXMg QmlnSW50IGhhbmRsZUNvbW11dGF0aXZpdHkgaXMgYnJlYWtpbmcgSlNDOjpERkc6OkVkZ2U6OnVz ZUtpbmQoKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTkyOTA4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg Q3VycmVudCBydWxlIGlzIGNvbnNpZGVyaW5nIHRoYXQgYm90aCBvcGVyYW5kcyBvZiBWYWx1ZUFk ZCBoYXZlIG5vZGVzLAorICAgICAgICBidXQgdGhpcyBpcyBub3QgcXVpdGUgdHJ1ZS4gSW4gc3Vj aCBjYXNlLCB3ZSBhcmUgY2hlY2tpbmcgdGhlCisgICAgICAgIGV4aXN0ZW5jZSBvZiBib3RoIG5v ZGVzIGJlZm9yZSB1c2luZyBtX25vZGUtPmJpbmFyeVVzZUtpbmQuCisKKyAgICAgICAgKiBkZmcv REZHU3RyZW5ndGhSZWR1Y3Rpb25QaGFzZS5jcHA6CisgICAgICAgIChKU0M6OkRGRzo6U3RyZW5n dGhSZWR1Y3Rpb25QaGFzZTo6aGFuZGxlTm9kZSk6CisKIDIwMTgtMTItMTkgIENhaW8gTGltYSAg PHRpY2Fpb2xpbWFAZ21haWwuY29tPgogCiAgICAgICAgIFtCaWdJbnRdIFdlIHNob3VsZCBlbmFi bGUgQ1NFIGludG8gYXJpdGhtZXRpYyBvcGVyYXRpb25zIHRoYXQgc3BlY3VsYXRlIEJpZ0ludFVz ZQpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdTdHJlbmd0aFJlZHVj dGlvblBoYXNlLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHU3RyZW5ndGhSZWR1 Y3Rpb25QaGFzZS5jcHAKaW5kZXggOTVjNmU3MzliNTdjOWM1ZWQ3ODNkMjYyZjRiMjk2ODA3YmM5 ZjdmOS4uYThlMzkxM2QzMDJkMWFmNTNhNzg0NDY2Nzc2ZjRiOTY1NDg1ZjY0MiAxMDA2NDQKLS0t IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdTdHJlbmd0aFJlZHVjdGlvblBoYXNlLmNw cAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR1N0cmVuZ3RoUmVkdWN0aW9uUGhh c2UuY3BwCkBAIC0zNzQsNyArMzc0LDcgQEAgcHJpdmF0ZToKICAgICAgICAgICAgICAgICBtX2No YW5nZWQgPSB0cnVlOwogICAgICAgICAgICAgfQogCi0gICAgICAgICAgICBpZiAobV9ub2RlLT5i aW5hcnlVc2VLaW5kKCkgPT0gQmlnSW50VXNlKQorICAgICAgICAgICAgaWYgKG1fbm9kZS0+Y2hp bGQxKCkubm9kZSgpICYmIG1fbm9kZS0+Y2hpbGQyKCkubm9kZSgpICYmIG1fbm9kZS0+YmluYXJ5 VXNlS2luZCgpID09IEJpZ0ludFVzZSkKICAgICAgICAgICAgICAgICBoYW5kbGVDb21tdXRhdGl2 aXR5KCk7CiAKICAgICAgICAgICAgIGJyZWFrOwo=