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.
safe to execute might also be wrong
@Saam I think this is now fixed, right?
(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; }
We should probably just say write(World)
(In reply to Saam Barati from comment #4) > We should probably just say write(World) write(Heap)**
<rdar://problem/65625807>
Created attachment 404782 [details] proposed patch.
Comment on attachment 404782 [details] 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.
Comment on attachment 404782 [details] 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?
Comment on attachment 404782 [details] 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.
Comment on attachment 404782 [details] 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.
Comment on attachment 404782 [details] 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.
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>.