Bug 198770 - [JSC] Polymorphic call stub's slow path should restore callee saves before performing tail call
Summary: [JSC] Polymorphic call stub's slow path should restore callee saves before pe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-11 16:03 PDT by Yusuke Suzuki
Modified: 2019-06-12 13:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (18.52 KB, patch)
2019-06-11 19:42 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.48 KB, patch)
2019-06-12 00:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.48 KB, patch)
2019-06-12 01:03 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-06-11 16:03:13 PDT
When linkPolymorphicCall gives up compiling polymorphic call, we fall back to virtual call. But `linkVirtualCall` does not restore the callee saves before calling the tail call, while polymorphic call does it.
If the caller CodeBlock clobbers the callee saves (e.g. FTL), we forget restoring it, and caller's caller will see garbage in callee saves.
Comment 1 Yusuke Suzuki 2019-06-11 16:49:34 PDT
And polymorphic call stub's slow path seems not restoring it too.
Comment 2 Yusuke Suzuki 2019-06-11 17:29:40 PDT
<rdar://problem/51260313>
Comment 3 Yusuke Suzuki 2019-06-11 19:42:49 PDT
Created attachment 371914 [details]
Patch
Comment 4 Yusuke Suzuki 2019-06-11 23:31:40 PDT
The change of virtual call is not necessary. Only polymorphic call is problematic.
Comment 5 Yusuke Suzuki 2019-06-12 00:19:40 PDT
Created attachment 371934 [details]
Patch
Comment 6 Yusuke Suzuki 2019-06-12 01:03:59 PDT
Created attachment 371935 [details]
Patch
Comment 7 Saam Barati 2019-06-12 01:16:21 PDT
Comment on attachment 371934 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:30
> +        This patch does that skips after restoring callee saves.

“does that skips” => “makes branches to the slow path happen”

> JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js:5
> +        var a = WeakSet.bind();

Maybe also two other tests:
- One where the poly call is in the test itself instead of a built in (in case we change the built in in the future)
- A test with a non cell callee
Comment 8 Saam Barati 2019-06-12 01:16:57 PDT
Comment on attachment 371935 [details]
Patch

r=me with comments from previous patch
Comment 9 Yusuke Suzuki 2019-06-12 13:25:50 PDT
Comment on attachment 371934 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:30
>> +        This patch does that skips after restoring callee saves.
> 
> “does that skips” => “makes branches to the slow path happen”

Fixed.

>> JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js:5
>> +        var a = WeakSet.bind();
> 
> Maybe also two other tests:
> - One where the poly call is in the test itself instead of a built in (in case we change the built in in the future)
> - A test with a non cell callee

Nice, added.
Comment 10 Yusuke Suzuki 2019-06-12 13:31:18 PDT
Committed r246372: <https://trac.webkit.org/changeset/246372>