Bug 212645 - SpeculativeJIT::compileDateGet()'s slow path does not need an exception check.
Summary: SpeculativeJIT::compileDateGet()'s slow path does not need an exception check.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-02 10:48 PDT by Mark Lam
Modified: 2020-06-04 07:42 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (8.57 KB, patch)
2020-06-02 11:05 PDT, Mark Lam
saam: review+
Details | Formatted Diff | Diff
proposed patch. (40.81 KB, patch)
2020-06-03 23:14 PDT, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-06-02 10:48:39 PDT
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.
Comment 1 Mark Lam 2020-06-02 11:05:00 PDT
Created attachment 400842 [details]
proposed patch.
Comment 2 Mark Lam 2020-06-03 22:42:35 PDT
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.
Comment 3 Mark Lam 2020-06-03 23:14:42 PDT
Created attachment 401003 [details]
proposed patch.
Comment 4 Yusuke Suzuki 2020-06-03 23:54:16 PDT
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.)
Comment 5 Yusuke Suzuki 2020-06-03 23:56:21 PDT
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.
Comment 6 Mark Lam 2020-06-04 07:33:58 PDT
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.
Comment 7 Mark Lam 2020-06-04 07:41:34 PDT
Thanks for the review.  Landed in r262535: <http://trac.webkit.org/r262535>.
Comment 8 Radar WebKit Bug Importer 2020-06-04 07:42:17 PDT
<rdar://problem/63978017>