211543 2020-05-06 18:58:33 -0700 Fix ArrayMode nodes after r261260 2020-05-07 11:14:42 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=211531 InRadar P2 Normal --- 1 keith_miller keith_miller ews-watchlist mark.lam msaboff saam tzagallo webkit-bug-importer ysuzuki oldest_to_newest 1649924 0 keith_miller 2020-05-06 18:58:33 -0700 Fix ArrayMode nodes after r261260 1649925 1 398695 keith_miller 2020-05-06 19:00:47 -0700 Created attachment 398695 Patch 1649938 2 398695 ysuzuki 2020-05-06 19:37:43 -0700 Comment on attachment 398695 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398695&action=review r=me with nits > Source/JavaScriptCore/dfg/DFGClobberize.h:162 > + // all nodes with an ArrayMode can clobber top. We make an exception for CheckArray since it has no effects. > + if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode() && node->op() != CheckArray) > return clobberTop(); Can we list up all ArrayMode nodes here not to forget adding some nodes? Like this. if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { switch (node->op()) { case GetIndexedPropertyStorage: ... break; case CheckArray: return clobberTop(); default: DFG_ASSERT(...); } } Do we need to include CheckArrayOrEmpty too? 1649944 3 398695 mark.lam 2020-05-06 20:29:56 -0700 Comment on attachment 398695 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398695&action=review >> Source/JavaScriptCore/dfg/DFGClobberize.h:162 >> return clobberTop(); > > Can we list up all ArrayMode nodes here not to forget adding some nodes? > > Like this. > > if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { > switch (node->op()) { > case GetIndexedPropertyStorage: > ... > break; > case CheckArray: > return clobberTop(); > default: > DFG_ASSERT(...); > } > } > > Do we need to include CheckArrayOrEmpty too? Yusuke, I think you meant the opposite: if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { switch (node->op()) { case GetIndexedPropertyStorage: ... return clobberTop(); case CheckArray: break; default: DFG_ASSERT(...); } } 1649964 4 398695 ysuzuki 2020-05-06 22:18:52 -0700 Comment on attachment 398695 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398695&action=review >>> Source/JavaScriptCore/dfg/DFGClobberize.h:162 >>> return clobberTop(); >> >> Can we list up all ArrayMode nodes here not to forget adding some nodes? >> >> Like this. >> >> if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { >> switch (node->op()) { >> case GetIndexedPropertyStorage: >> ... >> break; >> case CheckArray: >> return clobberTop(); >> default: >> DFG_ASSERT(...); >> } >> } >> >> Do we need to include CheckArrayOrEmpty too? > > Yusuke, I think you meant the opposite: > > if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { > switch (node->op()) { > case GetIndexedPropertyStorage: > ... > return clobberTop(); > case CheckArray: > break; > default: > DFG_ASSERT(...); > } > } Oops, right. 1650138 5 ryanhaddad 2020-05-07 09:20:42 -0700 I reverted r261260 in https://trac.webkit.org/changeset/261293 to address test failures. This should be combined with the original patch and landed at once. 1650218 6 keith_miller 2020-05-07 11:00:25 -0700 Committed r261313: <https://trac.webkit.org/changeset/261313> 1650221 7 webkit-bug-importer 2020-05-07 11:01:30 -0700 <rdar://problem/62983681> 1650233 8 mark.lam 2020-05-07 11:14:42 -0700 <rdar://problem/62838095> 398695 2020-05-06 19:00:47 -0700 2020-05-06 19:37:43 -0700 Patch bug-211543-20200506190046.patch text/plain 2791 keith_miller U3VidmVyc2lvbiBSZXZpc2lvbjogMjYxMjYwCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBh ODEwYWU4NmMzYTFlYTYwNjQxZDQ5YjNkZTE4ODUxZDI5MGQ2MmVlLi40MTNkMGNmOTkxNjgxYmI2 ZmUzN2JmMGFkMjE1OTI5NzliMTI1YjdmIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMSBAQAorMjAyMC0wNS0wNiAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxl LmNvbT4KKworICAgICAgICBGaXggQXJyYXlNb2RlIG5vZGVzIGFmdGVyIHIyNjEyNjAKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxMTU0MworCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEkgYWNjaWRlbnRhbGx5 IHJhbiB0ZXN0cyB3aXRoIGEgcmVsZWFzZSBidWlsZCByYXRoZXIgdGhhbgorICAgICAgICByZWxl YXNlK2Fzc2VydCB3aGVuIHVwbG9hZGluZyByMjYxMjYwLiBUaGlzIHBhdGNoIHNraXBzIHRoZQor ICAgICAgICBDaGVja0FycmF5IG5vZGUgaW4gdGhlIEFycmF5TW9kZSBjbG9iYmVyc1RvcCgpIGxv Z2ljIGJlZm9yZQorICAgICAgICBGaXh1cC4gQW5kIGFsc28gbWFya3MgYSBHZXRBcnJheUxlbmd0 aCBpbiB0aGUgVHlwZWRBcnJheQorICAgICAgICBpbnRyc2luaWNzIGFzIEV4aXRPSy4KKworICAg ICAgICAqIGRmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHA6CisgICAgICAgIChKU0M6OkRGRzo6Qnl0 ZUNvZGVQYXJzZXI6OmhhbmRsZUludHJpbnNpY0dldHRlcik6CisgICAgICAgICogZGZnL0RGR0Ns b2JiZXJpemUuaDoKKyAgICAgICAgKEpTQzo6REZHOjpjbG9iYmVyaXplKToKKwogMjAyMC0wNS0w NiAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxlLmNvbT4KIAogICAgICAgICBERkcg QnlWYWwgbm9kZXMgd2l0aCBBcnJheU1vZGVzIHNob3VsZCBjbG9iYmVyVG9wIHVudGlsIEZpeHVw IHBoYXNlIHJ1bnMuCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5 dGVDb2RlUGFyc2VyLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQnl0ZUNvZGVQ YXJzZXIuY3BwCmluZGV4IDhhZDRiYzA3OWNmODEyNjdhYjVlMTdkMWIyNDhjNDdjMWFlOGNkNmUu LmExZDQzMGY5NTM0NDhkOGU3ZDdlODk5YTE5N2QxNjViYmUxYWY5ZWMgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQnl0ZUNvZGVQYXJzZXIuY3BwCisrKyBiL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQnl0ZUNvZGVQYXJzZXIuY3BwCkBAIC0zNzUwLDYgKzM3 NTAsMTAgQEAgYm9vbCBCeXRlQ29kZVBhcnNlcjo6aGFuZGxlSW50cmluc2ljR2V0dGVyKE9wZXJh bmQgcmVzdWx0LCBTcGVjdWxhdGVkVHlwZSBwcmVkaWMKICAgICAgICAgfSk7CiAKICAgICAgICAg Tm9kZSogbGVuZ3RoTm9kZSA9IGFkZFRvR3JhcGgoR2V0QXJyYXlMZW5ndGgsIE9wSW5mbyhBcnJh eU1vZGUoYXJyYXlUeXBlLCBBcnJheTo6UmVhZCkuYXNXb3JkKCkpLCB0aGlzTm9kZSk7CisgICAg ICAgIC8vIE91ciBBcnJheU1vZGUgc2hvdWxkbid0IGNhdXNlIHVzIHRvIGV4aXQgaGVyZSBzbyB3 ZSBzaG91bGQgYmUgb2sgdG8gZXhpdCB3aXRob3V0IGVmZmVjdHMuCisgICAgICAgIG1fZXhpdE9L ID0gdHJ1ZTsKKyAgICAgICAgYWRkVG9HcmFwaChFeGl0T0spOworCiAKICAgICAgICAgaWYgKCFs b2dTaXplKSB7CiAgICAgICAgICAgICBzZXQocmVzdWx0LCBsZW5ndGhOb2RlKTsKZGlmZiAtLWdp dCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQ2xvYmJlcml6ZS5oIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL2RmZy9ERkdDbG9iYmVyaXplLmgKaW5kZXggZWRkNDMxZDE5ZWUxYmFiNjkx ZjYzOTQ4NzhiNGQxZDczMTQ3MWZjNi4uM2EzMmVjZjYzMzQyMjJlMjJhNWZiYzVlYmEyYzBhYWZl YTk2N2MzYyAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdDbG9iYmVy aXplLmgKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdDbG9iYmVyaXplLmgKQEAg LTE1Nyw4ICsxNTcsOCBAQCB2b2lkIGNsb2JiZXJpemUoR3JhcGgmIGdyYXBoLCBOb2RlKiBub2Rl LCBjb25zdCBSZWFkRnVuY3RvciYgcmVhZCwgY29uc3QgV3JpdGVGdQogICAgIH07CiAKICAgICAv LyBTaW5jZSBGaXh1cCBjYW4gd2lkZW4gb3VyIEFycmF5TW9kZXMgYmFzZWQgb24gcHJvZmlsaW5n IGZyb20gb3RoZXIgbm9kZXMgd2UgcGVzc2ltaXN0aWNhbGx5IGFzc3VtZQotICAgIC8vIGFsbCBu b2RlcyB3aXRoIGFuIEFycmF5TW9kZSBjYW4gY2xvYmJlciB0b3AuCi0gICAgaWYgKGdyYXBoLm1f cGxhblN0YWdlIDwgUGxhblN0YWdlOjpBZnRlckZpeHVwICYmIG5vZGUtPmhhc0FycmF5TW9kZSgp KQorICAgIC8vIGFsbCBub2RlcyB3aXRoIGFuIEFycmF5TW9kZSBjYW4gY2xvYmJlciB0b3AuIFdl IG1ha2UgYW4gZXhjZXB0aW9uIGZvciBDaGVja0FycmF5IHNpbmNlIGl0IGhhcyBubyBlZmZlY3Rz LgorICAgIGlmIChncmFwaC5tX3BsYW5TdGFnZSA8IFBsYW5TdGFnZTo6QWZ0ZXJGaXh1cCAmJiBu b2RlLT5oYXNBcnJheU1vZGUoKSAmJiBub2RlLT5vcCgpICE9IENoZWNrQXJyYXkpCiAgICAgICAg IHJldHVybiBjbG9iYmVyVG9wKCk7CiAgICAgCiAgICAgc3dpdGNoIChub2RlLT5vcCgpKSB7Cg==