Bug 163834

Summary: TryGetById clobberize rules are wrong
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, ews-watchlist, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, oliver, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch. keith_miller: review+

Description Saam Barati 2016-10-21 19:19:57 PDT
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.
Comment 1 Saam Barati 2020-01-31 16:54:36 PST
safe to execute might also be wrong
Comment 2 Yusuke Suzuki 2020-06-15 10:08:09 PDT
@Saam I think this is now fixed, right?
Comment 3 Saam Barati 2020-06-15 11:22:02 PDT
(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;
    }
Comment 4 Saam Barati 2020-06-15 11:22:12 PDT
We should probably just say write(World)
Comment 5 Saam Barati 2020-06-15 11:22:30 PDT
(In reply to Saam Barati from comment #4)
> We should probably just say write(World)

write(Heap)**
Comment 6 Mark Lam 2020-07-20 17:06:56 PDT
<rdar://problem/65625807>
Comment 7 Mark Lam 2020-07-20 17:48:37 PDT
Created attachment 404782 [details]
proposed patch.
Comment 8 Saam Barati 2020-07-20 18:19:16 PDT
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 9 Mark Lam 2020-07-20 18:22:29 PDT
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 10 Keith Miller 2020-07-20 18:25:39 PDT
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 11 Keith Miller 2020-07-20 18:27:26 PDT
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 12 Saam Barati 2020-07-20 19:02:08 PDT
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.
Comment 13 Mark Lam 2020-07-20 22:20:13 PDT
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>.