229898 2021-09-03 15:00:44 -0700 [JSC] Make EnumeratorNextUpdateIndexAndMode clobberizing rule precise 2022-02-12 20:31:24 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ysuzuki ysuzuki ayumi_kojima ews-watchlist keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1790392 0 ysuzuki 2021-09-03 15:00:44 -0700 [JSC] Make EnumeratorNextUpdateIndexAndMode clobberizing rule precise 1790395 1 437319 ysuzuki 2021-09-03 15:02:52 -0700 Created attachment 437319 Patch 1790396 2 ysuzuki 2021-09-03 15:02:54 -0700 <rdar://problem/82714439> 1790447 3 437319 mark.lam 2021-09-03 17:38:06 -0700 Comment on attachment 437319 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437319&action=review > Source/JavaScriptCore/dfg/DFGClobberize.h:374 > - if (mode.isSaneChain()) { > + if (mode.isInBounds()) { After this change (and the ones below), all this code is identical to the case for HasIndexedProperty. Why not just make the EnumeratorNextUpdateIndexAndMode case do this instead? case EnumeratorNextUpdateIndexAndMode: { if (node->enumeratorMetadata() == JSPropertyNameEnumerator::OwnStructureMode && graph.varArgChild(node, 0).useKind() == CellUse) { read(JSObject_butterfly); read(NamedProperties); read(JSCell_structureID); return; } if (node->enumeratorMetadata() != JSPropertyNameEnumerator::IndexedMode) { read(JSObject_butterfly); clobberTop(); return; } FALLTHROUGH; } case HasIndexedProperty: { ... 1790449 4 437319 saam 2021-09-03 17:49:34 -0700 Comment on attachment 437319 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437319&action=review >> Source/JavaScriptCore/dfg/DFGClobberize.h:374 >> + if (mode.isInBounds()) { > > After this change (and the ones below), all this code is identical to the case for HasIndexedProperty. Why not just make the EnumeratorNextUpdateIndexAndMode case do this instead? > > case EnumeratorNextUpdateIndexAndMode: { > if (node->enumeratorMetadata() == JSPropertyNameEnumerator::OwnStructureMode && graph.varArgChild(node, 0).useKind() == CellUse) { > read(JSObject_butterfly); > read(NamedProperties); > read(JSCell_structureID); > return; > } > > if (node->enumeratorMetadata() != JSPropertyNameEnumerator::IndexedMode) { > read(JSObject_butterfly); > clobberTop(); > return; > } > > FALLTHROUGH; > } > > case HasIndexedProperty: { > ... Or write a shared helper. 1790452 5 437319 mark.lam 2021-09-03 18:02:24 -0700 Comment on attachment 437319 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437319&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4303 > + if (node->enumeratorMetadata() != JSPropertyNameEnumerator::OwnStructureMode || m_graph.varArgChild(node, 0).useKind() != CellUse) { > + if (node->enumeratorMetadata() != JSPropertyNameEnumerator::IndexedMode) > + clobberWorld(); > + else { > + switch (arrayMode.type()) { > + case Array::Int32: > + case Array::Double: > + case Array::Contiguous: > + case Array::ArrayStorage: { > + if (arrayMode.isInBounds()) > + break; > + FALLTHROUGH; > + } > + default: { > + clobberWorld(); > + break; > + } > + } > + } > + } > setNonCellTypeForNode(node, SpecBytecodeNumber); Can't you also express this in a similar way as the implementation in clobberize() so that it's more obvious that this is meant to match that, and as so that you can re-use the code for HasIndexedProperty either via FALLTHROUGH, or via a helper function? 1790470 6 437319 ysuzuki 2021-09-03 20:10:40 -0700 Comment on attachment 437319 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=437319&action=review >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4303 >> setNonCellTypeForNode(node, SpecBytecodeNumber); > > Can't you also express this in a similar way as the implementation in clobberize() so that it's more obvious that this is meant to match that, and as so that you can re-use the code for HasIndexedProperty either via FALLTHROUGH, or via a helper function? A bit aligned it to the clobberizing's one. >>> Source/JavaScriptCore/dfg/DFGClobberize.h:374 >>> + if (mode.isInBounds()) { >> >> After this change (and the ones below), all this code is identical to the case for HasIndexedProperty. Why not just make the EnumeratorNextUpdateIndexAndMode case do this instead? >> >> case EnumeratorNextUpdateIndexAndMode: { >> if (node->enumeratorMetadata() == JSPropertyNameEnumerator::OwnStructureMode && graph.varArgChild(node, 0).useKind() == CellUse) { >> read(JSObject_butterfly); >> read(NamedProperties); >> read(JSCell_structureID); >> return; >> } >> >> if (node->enumeratorMetadata() != JSPropertyNameEnumerator::IndexedMode) { >> read(JSObject_butterfly); >> clobberTop(); >> return; >> } >> >> FALLTHROUGH; >> } >> >> case HasIndexedProperty: { >> ... > > Or write a shared helper. Merged both clauses into one. 1790471 7 ysuzuki 2021-09-03 20:24:25 -0700 Committed r282042 (241342@main): <https://commits.webkit.org/241342@main> 1840861 8 bfulgham 2022-02-12 20:31:24 -0800 *** Bug 229842 has been marked as a duplicate of this bug. *** 437319 2021-09-03 15:02:52 -0700 2021-09-03 17:48:37 -0700 Patch bug-229898-20210903150251.patch text/plain 6019 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjgyMDE4CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAw NmNiOGZjODg1OGVmZWU5OWU1NjllODYwNDkzZmUxNmNlODU1Mjk3Li4yM2M4OTk2MWE2MWRmMWU1 YzNlMjUxZDk1NjA3YWI1ZjkzNTcwZTEwIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMCBAQAorMjAyMS0wOS0wMyAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29t PgorCisgICAgICAgIFtKU0NdIE1ha2UgRW51bWVyYXRvck5leHRVcGRhdGVJbmRleEFuZE1vZGUg Y2xvYmJlcml6aW5nIHJ1bGUgcHJlY2lzZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjI5ODk4CisgICAgICAgIHJkYXI6Ly84MjcxNDQzOQorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIENsb2JiZXJpemluZyBy dWxlIGFuZCBBSSBkb2VzIG5vdCBtYXRjaCBmb3IgRW51bWVyYXRvck5leHRVcGRhdGVJbmRleEFu ZE1vZGUgbm9kZS4KKyAgICAgICAgV2UgZml4IGJvdGggY2FzZXM6IGlzU2FuZUNoYWluIGlzIG5v dCByZWxhdGVkIHRvIHRoaXMgbm9kZS4gU28gd2Ugc2hvdWxkIHVzZSBpc0luQm91bmRzCisgICAg ICAgIGFzIHdlIGFyZSBkb2luZyBmb3IgSGFzSW5kZXhlZFByb3BlcnR5IG5vZGUuCisKKyAgICAg ICAgKiBkZmcvREZHQWJzdHJhY3RJbnRlcnByZXRlcklubGluZXMuaDoKKyAgICAgICAgKEpTQzo6 REZHOjpBYnN0cmFjdEludGVycHJldGVyPEFic3RyYWN0U3RhdGVUeXBlPjo6ZXhlY3V0ZUVmZmVj dHMpOgorICAgICAgICAqIGRmZy9ERkdDbG9iYmVyaXplLmg6CisgICAgICAgIChKU0M6OkRGRzo6 Y2xvYmJlcml6ZSk6CisKIDIwMjEtMDktMDMgIFl1c3VrZSBTdXp1a2kgIDx5c3V6dWtpQGFwcGxl LmNvbT4KIAogICAgICAgICBbSlNDXSBJbXBsZW1lbnQgVGVtcG9yYWwuVGltZVpvbmUKZGlmZiAt LWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQWJzdHJhY3RJbnRlcnByZXRlcklu bGluZXMuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQWJzdHJhY3RJbnRlcnByZXRl cklubGluZXMuaAppbmRleCAwN2MyZDFmMzVkYjJmNTk0NjFhNDk3NDk1YTNmYTJjNzQ0NmM3N2E3 Li4zMjlmOTliMGYxZGY4YWI1MTFjOTk3Y2U5OThlNzk1NDgwNmRhODY5IDEwMDY0NAotLS0gYS9T b3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0Fic3RyYWN0SW50ZXJwcmV0ZXJJbmxpbmVzLmgK KysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdBYnN0cmFjdEludGVycHJldGVySW5s aW5lcy5oCkBAIC00MjgwLDExICs0MjgwLDI2IEBAIGJvb2wgQWJzdHJhY3RJbnRlcnByZXRlcjxB YnN0cmFjdFN0YXRlVHlwZT46OmV4ZWN1dGVFZmZlY3RzKHVuc2lnbmVkIGNsb2JiZXJMaW1pCiAK ICAgICBjYXNlIEVudW1lcmF0b3JOZXh0VXBkYXRlSW5kZXhBbmRNb2RlOiB7CiAgICAgICAgIEFy cmF5TW9kZSBhcnJheU1vZGUgPSBub2RlLT5hcnJheU1vZGUoKTsKLSAgICAgICAgaWYgKGFycmF5 TW9kZS5pc1NhbmVDaGFpbigpKQotICAgICAgICAgICAgQVNTRVJUKG5vZGUtPmVudW1lcmF0b3JN ZXRhZGF0YSgpID09IEpTUHJvcGVydHlOYW1lRW51bWVyYXRvcjo6SW5kZXhlZE1vZGUpOwotICAg ICAgICBlbHNlIGlmIChub2RlLT5lbnVtZXJhdG9yTWV0YWRhdGEoKSAhPSBKU1Byb3BlcnR5TmFt ZUVudW1lcmF0b3I6Ok93blN0cnVjdHVyZU1vZGUgfHwgbV9ncmFwaC52YXJBcmdDaGlsZChub2Rl LCAwKS51c2VLaW5kKCkgIT0gQ2VsbFVzZSkKLSAgICAgICAgICAgIGNsb2JiZXJXb3JsZCgpOwot CisgICAgICAgIGlmIChub2RlLT5lbnVtZXJhdG9yTWV0YWRhdGEoKSAhPSBKU1Byb3BlcnR5TmFt ZUVudW1lcmF0b3I6Ok93blN0cnVjdHVyZU1vZGUgfHwgbV9ncmFwaC52YXJBcmdDaGlsZChub2Rl LCAwKS51c2VLaW5kKCkgIT0gQ2VsbFVzZSkgeworICAgICAgICAgICAgaWYgKG5vZGUtPmVudW1l cmF0b3JNZXRhZGF0YSgpICE9IEpTUHJvcGVydHlOYW1lRW51bWVyYXRvcjo6SW5kZXhlZE1vZGUp CisgICAgICAgICAgICAgICAgY2xvYmJlcldvcmxkKCk7CisgICAgICAgICAgICBlbHNlIHsKKyAg ICAgICAgICAgICAgICBzd2l0Y2ggKGFycmF5TW9kZS50eXBlKCkpIHsKKyAgICAgICAgICAgICAg ICBjYXNlIEFycmF5OjpJbnQzMjoKKyAgICAgICAgICAgICAgICBjYXNlIEFycmF5OjpEb3VibGU6 CisgICAgICAgICAgICAgICAgY2FzZSBBcnJheTo6Q29udGlndW91czoKKyAgICAgICAgICAgICAg ICBjYXNlIEFycmF5OjpBcnJheVN0b3JhZ2U6IHsKKyAgICAgICAgICAgICAgICAgICAgaWYgKGFy cmF5TW9kZS5pc0luQm91bmRzKCkpCisgICAgICAgICAgICAgICAgICAgICAgICBicmVhazsKKyAg ICAgICAgICAgICAgICAgICAgRkFMTFRIUk9VR0g7CisgICAgICAgICAgICAgICAgfQorICAgICAg ICAgICAgICAgIGRlZmF1bHQ6IHsKKyAgICAgICAgICAgICAgICAgICAgY2xvYmJlcldvcmxkKCk7 CisgICAgICAgICAgICAgICAgICAgIGJyZWFrOworICAgICAgICAgICAgICAgIH0KKyAgICAgICAg ICAgICAgICB9CisgICAgICAgICAgICB9CisgICAgICAgIH0KICAgICAgICAgc2V0Tm9uQ2VsbFR5 cGVGb3JOb2RlKG5vZGUsIFNwZWNCeXRlY29kZU51bWJlcik7CiAgICAgICAgIGJyZWFrOwogICAg IH0KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQ2xvYmJlcml6ZS5o IGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdDbG9iYmVyaXplLmgKaW5kZXggNmYyMDRk MmM1ZGUwYTYxMmY3MzFjOTU3YjQ2MWRjZjMwYzRhMTFhMy4uYjM0NTU5M2VhNGIxNDhhYzY1MGFi NDUxZGNhZjlmNzg4N2VlMzNiNiAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2Rm Zy9ERkdDbG9iYmVyaXplLmgKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdDbG9i YmVyaXplLmgKQEAgLTM3MSw3ICszNzEsNyBAQCB2b2lkIGNsb2JiZXJpemUoR3JhcGgmIGdyYXBo LCBOb2RlKiBub2RlLCBjb25zdCBSZWFkRnVuY3RvciYgcmVhZCwgY29uc3QgV3JpdGVGdQogICAg ICAgICAgICAgcmV0dXJuOwogICAgICAgICB9CiAgICAgICAgIGNhc2UgQXJyYXk6OkludDMyOiB7 Ci0gICAgICAgICAgICBpZiAobW9kZS5pc1NhbmVDaGFpbigpKSB7CisgICAgICAgICAgICBpZiAo bW9kZS5pc0luQm91bmRzKCkpIHsKICAgICAgICAgICAgICAgICByZWFkKEJ1dHRlcmZseV9wdWJs aWNMZW5ndGgpOwogICAgICAgICAgICAgICAgIHJlYWQoSW5kZXhlZEludDMyUHJvcGVydGllcyk7 CiAgICAgICAgICAgICAgICAgZGVmKEhlYXBMb2NhdGlvbihIYXNJbmRleGVkUHJvcGVydHlMb2Ms IEluZGV4ZWRJbnQzMlByb3BlcnRpZXMsIGdyYXBoLnZhckFyZ0NoaWxkKG5vZGUsIDApLCBncmFw aC52YXJBcmdDaGlsZChub2RlLCAxKSksIExhenlOb2RlKG5vZGUpKTsKQEAgLTM4MSw3ICszODEs NyBAQCB2b2lkIGNsb2JiZXJpemUoR3JhcGgmIGdyYXBoLCBOb2RlKiBub2RlLCBjb25zdCBSZWFk RnVuY3RvciYgcmVhZCwgY29uc3QgV3JpdGVGdQogICAgICAgICB9CiAKICAgICAgICAgY2FzZSBB cnJheTo6RG91YmxlOiB7Ci0gICAgICAgICAgICBpZiAobW9kZS5pc1NhbmVDaGFpbigpKSB7Cisg ICAgICAgICAgICBpZiAobW9kZS5pc0luQm91bmRzKCkpIHsKICAgICAgICAgICAgICAgICByZWFk KEJ1dHRlcmZseV9wdWJsaWNMZW5ndGgpOwogICAgICAgICAgICAgICAgIHJlYWQoSW5kZXhlZERv dWJsZVByb3BlcnRpZXMpOwogICAgICAgICAgICAgICAgIGRlZihIZWFwTG9jYXRpb24oSGFzSW5k ZXhlZFByb3BlcnR5TG9jLCBJbmRleGVkRG91YmxlUHJvcGVydGllcywgZ3JhcGgudmFyQXJnQ2hp bGQobm9kZSwgMCksIGdyYXBoLnZhckFyZ0NoaWxkKG5vZGUsIDEpKSwgTGF6eU5vZGUobm9kZSkp OwpAQCAtMzkxLDcgKzM5MSw3IEBAIHZvaWQgY2xvYmJlcml6ZShHcmFwaCYgZ3JhcGgsIE5vZGUq IG5vZGUsIGNvbnN0IFJlYWRGdW5jdG9yJiByZWFkLCBjb25zdCBXcml0ZUZ1CiAgICAgICAgIH0K IAogICAgICAgICBjYXNlIEFycmF5OjpDb250aWd1b3VzOiB7Ci0gICAgICAgICAgICBpZiAobW9k ZS5pc1NhbmVDaGFpbigpKSB7CisgICAgICAgICAgICBpZiAobW9kZS5pc0luQm91bmRzKCkpIHsK ICAgICAgICAgICAgICAgICByZWFkKEJ1dHRlcmZseV9wdWJsaWNMZW5ndGgpOwogICAgICAgICAg ICAgICAgIHJlYWQoSW5kZXhlZENvbnRpZ3VvdXNQcm9wZXJ0aWVzKTsKICAgICAgICAgICAgICAg ICBkZWYoSGVhcExvY2F0aW9uKEhhc0luZGV4ZWRQcm9wZXJ0eUxvYywgSW5kZXhlZENvbnRpZ3Vv dXNQcm9wZXJ0aWVzLCBncmFwaC52YXJBcmdDaGlsZChub2RlLCAwKSwgZ3JhcGgudmFyQXJnQ2hp bGQobm9kZSwgMSkpLCBMYXp5Tm9kZShub2RlKSk7CmRpZmYgLS1naXQgYS9KU1Rlc3RzL0NoYW5n ZUxvZyBiL0pTVGVzdHMvQ2hhbmdlTG9nCmluZGV4IGZhMjdlYjI0YzAxMmI5NTNhY2YxYjY4MDc0 NGQzOWVkNTdiM2EzOGEuLmFhNjVkZGJkYzg2MmMxNzc5ZDkwY2FjNzM2NGEzODFjOWMxMDUwZjYg MTAwNjQ0Ci0tLSBhL0pTVGVzdHMvQ2hhbmdlTG9nCisrKyBiL0pTVGVzdHMvQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTQgQEAKKzIwMjEtMDktMDMgIFl1c3VrZSBTdXp1a2kgIDx5c3V6dWtpQGFwcGxl LmNvbT4KKworICAgICAgICBbSlNDXSBNYWtlIEVudW1lcmF0b3JOZXh0VXBkYXRlSW5kZXhBbmRN b2RlIGNsb2JiZXJpemluZyBydWxlIHByZWNpc2UKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTIyOTg5OAorICAgICAgICByZGFyOi8vODI3MTQ0MzkKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGNvbXBsZXgu eWFtbDoKKyAgICAgICAgKiBjb21wbGV4L2Zvci1pbi1jbG9iYmVyaXplLmpzOiBBZGRlZC4KKwog MjAyMS0wOS0wMyAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29tPgogCiAgICAgICAg IFtKU0NdIEltcGxlbWVudCBUZW1wb3JhbC5UaW1lWm9uZQpkaWZmIC0tZ2l0IGEvSlNUZXN0cy9j b21wbGV4LnlhbWwgYi9KU1Rlc3RzL2NvbXBsZXgueWFtbAppbmRleCA4MWZmNWU2YWY1NjZjOWE4 MjdjMTNhZWEzMmJhNDU2MDhhNjYyOTlkLi42YjQ1NDVmMTAyY2E4MWMzOGZjNjhhYzkyYmQyNWUy NDFiMmZiYmFhIDEwMDY0NAotLS0gYS9KU1Rlc3RzL2NvbXBsZXgueWFtbAorKysgYi9KU1Rlc3Rz L2NvbXBsZXgueWFtbApAQCAtNTIsMyArNTIsNiBAQAogCiAtIHBhdGg6IGNvbXBsZXgvdGVtcG9y YWwtbm93LXRpbWV6b25lLXdpdGgtYnJva2VuLXR6LmpzCiAgIGNtZDogcnVuQ29tcGxleFRlc3Qg W10sIFtdLCAiVFo9VU5ERUZJTkVEIiwgIi0tdXNlRG9sbGFyVk09MSIsICItLXVzZVRlbXBvcmFs PTEiCisKKy0gcGF0aDogY29tcGxleC9mb3ItaW4tY2xvYmJlcml6ZS5qcworICBjbWQ6IHJ1bkNv bXBsZXhUZXN0IFtdLCBbXSwgIiIsICItLWRlc3Ryb3ktdm0iCmRpZmYgLS1naXQgYS9KU1Rlc3Rz L2NvbXBsZXgvZm9yLWluLWNsb2JiZXJpemUuanMgYi9KU1Rlc3RzL2NvbXBsZXgvZm9yLWluLWNs b2JiZXJpemUuanMKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMC4uZWM5MDE5MjBhMTQ1YTEzOGMwYmJhNTE4MmVhNjgxOTM3 ZDMxM2EyYQotLS0gL2Rldi9udWxsCisrKyBiL0pTVGVzdHMvY29tcGxleC9mb3ItaW4tY2xvYmJl cml6ZS5qcwpAQCAtMCwwICsxLDYgQEAKK0FycmF5LnByb3RvdHlwZS5fX3Byb3RvX18gPSB7fTsK K2xldCBhID0gW107Citmb3IgKGxldCBpPTA7IGk8MTAwOyBpKyspIHsKKyAgYS51bnNoaWZ0KHVu ZGVmaW5lZCk7CisgIGZvciAobGV0IHggaW4gYSk7Cit9Cg==