163834 2016-10-21 19:19:57 -0700 TryGetById clobberize rules are wrong 2020-07-20 22:20:13 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 saam mark.lam benjamin ews-watchlist fpizlo ggaren gskachkov jfbastien keith_miller mark.lam msaboff oliver ticaiolima tzagallo webkit-bug-importer ysuzuki oldest_to_newest 1243226 0 saam 2016-10-21 19:19:57 -0700 It calls getOwnPropertySlot(VMInquiry), which isn't supposed to do user observable effects, but it can definitely do other effects that the compiler needs to be aware of. For example, certain getOwnPropertySlot implementations will materialize properties lazily, so they can allocate and transition structures. It may be safe for now to just claim that TryGetById write(Heap). We may also be able to get away with giving it the same clobberize rules as PureGetById. 1613669 1 saam 2020-01-31 16:54:36 -0800 safe to execute might also be wrong 1662684 2 ysuzuki 2020-06-15 10:08:09 -0700 @Saam I think this is now fixed, right? 1662720 3 saam 2020-06-15 11:22:02 -0700 (In reply to Yusuke Suzuki from comment #2) > @Saam I think this is now fixed, right? No, we still say it doesn't do any writes, even though on VMInquiry, some getOwnPropetySlot may do transitions, etc: case TryGetById: { read(Heap); return; } 1662721 4 saam 2020-06-15 11:22:12 -0700 We should probably just say write(World) 1662722 5 saam 2020-06-15 11:22:30 -0700 (In reply to Saam Barati from comment #4) > We should probably just say write(World) write(Heap)** 1673328 6 mark.lam 2020-07-20 17:06:56 -0700 <rdar://problem/65625807> 1673332 7 404782 mark.lam 2020-07-20 17:48:37 -0700 Created attachment 404782 proposed patch. 1673339 8 404782 saam 2020-07-20 18:19:16 -0700 Comment on attachment 404782 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=404782&action=review > Source/JavaScriptCore/dfg/DFGClobbersExitState.cpp:88 > + case TryGetById: you might want some more explanation here comment-wise. TryGetById does clobber memory potentially. But it's not spec-observable. That's why it's ok that it doesn't clobber exit state. 1673340 9 404782 mark.lam 2020-07-20 18:22:29 -0700 Comment on attachment 404782 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=404782&action=review >> Source/JavaScriptCore/dfg/DFGClobbersExitState.cpp:88 >> + case TryGetById: > > you might want some more explanation here comment-wise. TryGetById does clobber memory potentially. But it's not spec-observable. That's why it's ok that it doesn't clobber exit state. Doesn't the comment immediately below this already say that effectively? 1673341 10 404782 keith_miller 2020-07-20 18:25:39 -0700 Comment on attachment 404782 proposed patch. r=me. Assuming I'm right in my thinking that we shouldn't, correctly, writing to the stack in TryGetById, can you add a comment to the ChangeLog that we are slightly more conservative than necessary? I can't think of a way we can do that off the top of my head today. However, I think the right answer is to just assume some future world where we will ask what our caller is or something. 1673342 11 404782 keith_miller 2020-07-20 18:27:26 -0700 Comment on attachment 404782 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=404782&action=review >>> Source/JavaScriptCore/dfg/DFGClobbersExitState.cpp:88 >>> + case TryGetById: >> >> you might want some more explanation here comment-wise. TryGetById does clobber memory potentially. But it's not spec-observable. That's why it's ok that it doesn't clobber exit state. > > Doesn't the comment immediately below this already say that effectively? Yeah, I don't think you need another comment here. 1673344 12 404782 saam 2020-07-20 19:02:08 -0700 Comment on attachment 404782 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=404782&action=review >>>> Source/JavaScriptCore/dfg/DFGClobbersExitState.cpp:88 >>>> + case TryGetById: >>> >>> you might want some more explanation here comment-wise. TryGetById does clobber memory potentially. But it's not spec-observable. That's why it's ok that it doesn't clobber exit state. >> >> Doesn't the comment immediately below this already say that effectively? > > Yeah, I don't think you need another comment here. but we totally claim to write to an observable heap. It's more nuanced. 1673377 13 mark.lam 2020-07-20 22:20:13 -0700 Thanks for the reviews. I've added some details to the ChangeLog but opted not to add an additional comment in DFGClobbersExitState.cpp. There are nuanced differences but the existing comment is an accurate description about TryGetById as well. Landed in r264643: <http://trac.webkit.org/r264643>. 404782 2020-07-20 17:48:37 -0700 2020-07-20 18:25:39 -0700 proposed patch. bug-163834.patch text/plain 3104 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjY0NjQwKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBA CisyMDIwLTA3LTIwICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBU cnlHZXRCeUlkIGNsb2JiZXJpemUgcnVsZXMgYXJlIHdyb25nLgorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTYzODM0CisgICAgICAgIDxyZGFyOi8vcHJv YmxlbS82NTYyNTgwNz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBUcnlHZXRCeUlkIGNhbiBkbyB0aGUgc2FtZSB0aGluZ3MgR2V0QnlJZCBkb2VzIGku ZS4gcmVpZnkgbGF6eSBwcm9wZXJ0aWVzLCByZWFkCisgICAgICAgIHRoZSBzdGFjaywgZXRjLiAg SGVuY2UsIGl0cyBjbG9iYmVyaXplIHJ1bGUgc2hvdWxkIGJlIGNsb2JiZXJUb3AganVzdCBsaWtl IEdldEJ5SWQuCisKKyAgICAgICAgKiBkZmcvREZHQWJzdHJhY3RJbnRlcnByZXRlcklubGluZXMu aDoKKyAgICAgICAgKEpTQzo6REZHOjpBYnN0cmFjdEludGVycHJldGVyPEFic3RyYWN0U3RhdGVU eXBlPjo6ZXhlY3V0ZUVmZmVjdHMpOgorICAgICAgICAqIGRmZy9ERkdDbG9iYmVyaXplLmg6Cisg ICAgICAgIChKU0M6OkRGRzo6Y2xvYmJlcml6ZSk6CisgICAgICAgICogZGZnL0RGR0Nsb2JiZXJz RXhpdFN0YXRlLmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpjbG9iYmVyc0V4aXRTdGF0ZSk6CisK IDIwMjAtMDctMjAgIFl1c3VrZSBTdXp1a2kgIDx5c3V6dWtpQGFwcGxlLmNvbT4KIAogICAgICAg ICBVbnJldmlld2VkLCBmaXggZHVwbGljYXRlIGZvcndhcmQgZGVjbGFyYXRpb24gaW50cm9kdWNl ZCBieSBtZXJnZSBjb25mbGljdApJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdB YnN0cmFjdEludGVycHJldGVySW5saW5lcy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2Ny aXB0Q29yZS9kZmcvREZHQWJzdHJhY3RJbnRlcnByZXRlcklubGluZXMuaAkocmV2aXNpb24gMjY0 NjA3KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdBYnN0cmFjdEludGVycHJldGVy SW5saW5lcy5oCSh3b3JraW5nIGNvcHkpCkBAIC0zMzM0LDYgKzMzMzQsNyBAQCBib29sIEFic3Ry YWN0SW50ZXJwcmV0ZXI8QWJzdHJhY3RTdGF0ZVR5CiAgICAgY2FzZSBUcnlHZXRCeUlkOgogICAg ICAgICAvLyBGSVhNRTogVGhpcyBzaG91bGQgY29uc3RhbnQgZm9sZCBhdCBsZWFzdCBhcyB3ZWxs IGFzIHRoZSBub3JtYWwgR2V0QnlJZCBjYXNlLgogICAgICAgICAvLyBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU2NDIyCisgICAgICAgIGNsb2JiZXJXb3JsZCgpOwog ICAgICAgICBtYWtlSGVhcFRvcEZvck5vZGUobm9kZSk7CiAgICAgICAgIGJyZWFrOwogCkluZGV4 OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0Nsb2JiZXJpemUuaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0Nsb2JiZXJpemUuaAkocmV2aXNpb24gMjY0 NjA3KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdDbG9iYmVyaXplLmgJKHdvcmtp bmcgY29weSkKQEAgLTY1NSw2ICs2NTUsNyBAQCB2b2lkIGNsb2JiZXJpemUoR3JhcGgmIGdyYXBo LCBOb2RlKiBub2RlCiAgICAgY2FzZSBHZXRCeUlkRGlyZWN0OgogICAgIGNhc2UgR2V0QnlJZERp cmVjdEZsdXNoOgogICAgIGNhc2UgR2V0QnlWYWxXaXRoVGhpczoKKyAgICBjYXNlIFRyeUdldEJ5 SWQ6CiAgICAgY2FzZSBQdXRCeUlkOgogICAgIGNhc2UgUHV0QnlJZFdpdGhUaGlzOgogICAgIGNh c2UgUHV0QnlWYWxXaXRoVGhpczoKQEAgLTEzMDQsMTEgKzEzMDUsNiBAQCB2b2lkIGNsb2JiZXJp emUoR3JhcGgmIGdyYXBoLCBOb2RlKiBub2RlCiAgICAgICAgIHJldHVybjsKICAgICB9CiAKLSAg ICBjYXNlIFRyeUdldEJ5SWQ6IHsKLSAgICAgICAgcmVhZChIZWFwKTsKLSAgICAgICAgcmV0dXJu OwotICAgIH0KLQogICAgIGNhc2UgTXVsdGlHZXRCeU9mZnNldDogewogICAgICAgICByZWFkKEpT Q2VsbF9zdHJ1Y3R1cmVJRCk7CiAgICAgICAgIHJlYWQoSlNPYmplY3RfYnV0dGVyZmx5KTsKSW5k ZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQ2xvYmJlcnNFeGl0U3RhdGUuY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQ2xvYmJlcnNFeGl0U3Rh dGUuY3BwCShyZXZpc2lvbiAyNjQ2MDcpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RG R0Nsb2JiZXJzRXhpdFN0YXRlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtODUsNiArODUsNyBAQCBi b29sIGNsb2JiZXJzRXhpdFN0YXRlKEdyYXBoJiBncmFwaCwgTm9kCiAgICAgY2FzZSBGaWx0ZXJQ dXRCeUlkU3RhdHVzOgogICAgIGNhc2UgRmlsdGVySW5CeUlkU3RhdHVzOgogICAgIGNhc2UgRmls dGVyRGVsZXRlQnlTdGF0dXM6CisgICAgY2FzZSBUcnlHZXRCeUlkOgogICAgICAgICAvLyBUaGVz ZSBkbyBjbG9iYmVyIG1lbW9yeSwgYnV0IG5vdGhpbmcgdGhhdCBpcyBvYnNlcnZhYmxlLiBJdCBt YXkgYmUgbmljZSB0byBzZXBhcmF0ZSB0aGUKICAgICAgICAgLy8gaGVhcHMgaW50byB0aG9zZSB0 aGF0IGFyZSBvYnNlcnZhYmxlIGFuZCB0aG9zZSB0aGF0IGFyZW4ndCwgYnV0IHdlIGRvbid0IGRv IHRoYXQgcmlnaHQgbm93LgogICAgICAgICAvLyBGSVhNRTogaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTE0ODQ0MAo=