Bug 163834 – TryGetById clobberize rules are wrong

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

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.

[Saam Barati]

safe to execute might also be wrong

[Yusuke Suzuki]

@Saam I think this is now fixed, right?

[Saam Barati]

(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;
    }

[Saam Barati]

We should probably just say write(World)

[Saam Barati]

(In reply to Saam Barati from comment #4)
> We should probably just say write(World)

write(Heap)**

[Mark Lam]

<rdar://problem/65625807>

[Mark Lam]

Created attachment 404782 [details]
proposed patch.

[Saam Barati]

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.

[Mark Lam]

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?

[Keith Miller]

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.

[Keith Miller]

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.

[Saam Barati]

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.

[Mark Lam]

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>.