Bug 198770

Summary: [JSC] Polymorphic call stub's slow path should restore callee saves before performing tail call
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch saam: review+

Yusuke Suzuki
Reported 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.
Attachments
Patch (18.52 KB, patch)
2019-06-11 19:42 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (13.48 KB, patch)
2019-06-12 00:19 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (13.48 KB, patch)
2019-06-12 01:03 PDT, Yusuke Suzuki 		saam: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2019-06-11 16:49:34 PDT
And polymorphic call stub's slow path seems not restoring it too.
Yusuke Suzuki
Comment 2 2019-06-11 17:29:40 PDT
<rdar://problem/51260313>
Yusuke Suzuki
Comment 3 2019-06-11 19:42:49 PDT
Created attachment 371914 [details] Patch
Yusuke Suzuki
Comment 4 2019-06-11 23:31:40 PDT
The change of virtual call is not necessary. Only polymorphic call is problematic.
Yusuke Suzuki
Comment 5 2019-06-12 00:19:40 PDT
Created attachment 371934 [details] Patch
Yusuke Suzuki
Comment 6 2019-06-12 01:03:59 PDT
Created attachment 371935 [details] Patch
Saam Barati
Comment 7 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
Saam Barati
Comment 8 2019-06-12 01:16:57 PDT
Comment on attachment 371935 [details] Patch r=me with comments from previous patch
Yusuke Suzuki
Comment 9 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.
Yusuke Suzuki
Comment 10 2019-06-12 13:31:18 PDT
Committed r246372: <https://trac.webkit.org/changeset/246372>
Note You need to log in before you can comment on or make changes to this bug.