Bug 212645 – SpeculativeJIT::compileDateGet()'s slow path does not need an exception check.

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

[Mark Lam]

This issue was causing a false negative on the doesGC check.  For example, see https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/6382/steps/jscore-test/logs/stdio.

[Mark Lam]

Created attachment 400842 [details]
proposed patch.

[Mark Lam]

Comment on attachment 400842 [details]
proposed patch.

Yusuke pointed out to me that if the exception fuzzer only inserts throws where we emit exception checks.  And we only exception checks usually following something that can throw, and therefore can allocate memory.  Hence, this patch does not actually fix the true underlying issue.

After investigating further, I agree with Yusuke's insight.  I'm obsoleting this patch and will upload a new fix soon.

[Mark Lam]

Created attachment 401003 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 401003 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=401003&action=review

r=me

> Source/JavaScriptCore/ChangeLog:3
> +        SpeculativeJIT::compileDateGet()'s slow path does need a exception check.

/does/does not/

> Source/JavaScriptCore/ChangeLog:33
> +        The patch also proves that all the operation functions cannot throw any exceptions.
> +        Previously, the operations passes a VM& to the Date functions.  The purpose for
> +        doing this is so that the Date functions can work with a few date cache data
> +        structures stored as VM fields.
> +
> +        This patch refactors those VM fields into a VM::DateCache struct, and changed all
> +        those Date functions to take a VM::DateCache& instead of a VM&.  Since the Date
> +        functions no longer take a VM&, this proves that they cannot throw because they
> +        would need a VM& to make a ThrowScope in order to throw.

I think passing VM* is enough to represent that this function never throws an error. Because error requires error objects, error-throwing function requires JSGlobalObject* instead.
So, if the function does not take JSGlobalObject*, we can assume that it will not throw an error (and that's why I didn't pass JSGlobalObject* to these slowpath functions.)

[Yusuke Suzuki]

Comment on attachment 401003 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=401003&action=review

>> Source/JavaScriptCore/ChangeLog:33
>> +        would need a VM& to make a ThrowScope in order to throw.
> 
> I think passing VM* is enough to represent that this function never throws an error. Because error requires error objects, error-throwing function requires JSGlobalObject* instead.
> So, if the function does not take JSGlobalObject*, we can assume that it will not throw an error (and that's why I didn't pass JSGlobalObject* to these slowpath functions.)

When doing ExecState* -> JSGlobalObject* refactoring, I made a lot of operation functions taking only VM* instead of JSGlobalObject* if it does not throw an error.
I think it would be possible that we remove more exception checks.

[Mark Lam]

Comment on attachment 401003 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=401003&action=review

>>> Source/JavaScriptCore/ChangeLog:33
>>> +        would need a VM& to make a ThrowScope in order to throw.
>> 
>> I think passing VM* is enough to represent that this function never throws an error. Because error requires error objects, error-throwing function requires JSGlobalObject* instead.
>> So, if the function does not take JSGlobalObject*, we can assume that it will not throw an error (and that's why I didn't pass JSGlobalObject* to these slowpath functions.)
> 
> When doing ExecState* -> JSGlobalObject* refactoring, I made a lot of operation functions taking only VM* instead of JSGlobalObject* if it does not throw an error.
> I think it would be possible that we remove more exception checks.

I spoke to Yusuke offline.  I agree that the lack of a JSGlobalObject* is sufficient.  However, this refactoring into the VM::DateCache has additional benefits i.e. it also makes it clear that the Date functions are only operating on the DateCache fields and nothing else in the VM.  This helps makes clearer what the Date functions are not doing.  We'll keep the DateCache refactoring, and I'll add a comment about this reasoning in the ChangeLog.

[Mark Lam]

Thanks for the review.  Landed in r262535: <http://trac.webkit.org/r262535>.

[Radar WebKit Bug Importer]

<rdar://problem/63978017>