Bug 176981 - Arity fixup during inlining should do a 2 phase commit so it properly recovers the frame in case of exit
Summary: Arity fixup during inlining should do a 2 phase commit so it properly recover...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
: 176989 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-09-14 23:07 PDT by Saam Barati
Modified: 2017-09-27 12:27 PDT (History)
15 users (show)

See Also:


Attachments
patch (10.82 KB, patch)
2017-09-15 01:52 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (10.82 KB, patch)
2017-09-15 01:56 PDT, Saam Barati
ysuzuki: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.49 MB, application/zip)
2017-09-15 03:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.42 MB, application/zip)
2017-09-15 03:30 PDT, Build Bot
no flags Details
patch for landing (10.73 KB, patch)
2017-09-15 12:24 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-09-14 23:07:48 PDT
Yusuke convinced me we really don't want to exit during arity fixup. What he wrote:

[arg3][arg2][arg1][arg0]
[fix ][fix ][arg3][arg2][arg1][arg0]

In this case, when moving arg2, it writes arg2 to arg0's place. At that time, area is like,
[arg3][arg2][arg1][arg2][arg1][arg0]
So, in the middle of arity fixup, the region may be clobbered like,
[arg3][arg2][arg1][arg2]
If re-execute arity-fixup again, the stack becomes,
[fix ][fix ][arg3][arg2][arg1][arg2]
Comment 1 Saam Barati 2017-09-15 01:06:56 PDT
*** Bug 176989 has been marked as a duplicate of this bug. ***
Comment 2 Saam Barati 2017-09-15 01:52:29 PDT
Created attachment 320880 [details]
patch
Comment 3 Build Bot 2017-09-15 01:53:46 PDT
Attachment 320880 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:24:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Saam Barati 2017-09-15 01:56:31 PDT
Created attachment 320881 [details]
patch

remove tab character
Comment 5 Yusuke Suzuki 2017-09-15 02:06:25 PDT
Comment on attachment 320881 [details]
patch

r=me, this implementation is ideal one!
Comment 6 Build Bot 2017-09-15 02:39:24 PDT
Comment on attachment 320881 [details]
patch

Attachment 320881 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4556129

New failing tests:
stress/arrowfunction-lexical-bind-supercall-2.js.dfg-eager-no-cjit-validate
stress/promise-finally.js.ftl-eager-no-cjit
stress/arrowfunction-lexical-bind-supercall-2.js.ftl-eager-no-cjit-b3o1
stress/arrowfunction-lexical-bind-supercall-2.js.no-cjit-validate-phases
stress/arrowfunction-lexical-bind-supercall-2.js.ftl-no-cjit-no-put-stack-validate
stress/arrowfunction-lexical-bind-supercall-2.js.ftl-no-cjit-validate-sampling-profiler
stress/arrowfunction-lexical-bind-supercall-2.js.ftl-eager-no-cjit
Comment 7 Build Bot 2017-09-15 03:08:58 PDT
Comment on attachment 320881 [details]
patch

Attachment 320881 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4556374

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.dedicatedworker.html
Comment 8 Build Bot 2017-09-15 03:08:59 PDT
Created attachment 320885 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 9 Yusuke Suzuki 2017-09-15 03:26:46 PDT
Comment on attachment 320881 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1578
>          calleeVariable->mergeShouldNeverUnbox(true);

Is this ok?
I think we need to consider LoadVarargs case too. But this does not become problem, because LoadVarargs case does not have arityFixupCount (isVarargs() case, we respect to function->parameterCount() for var args stack allocation.)
Comment 10 Build Bot 2017-09-15 03:30:52 PDT
Comment on attachment 320881 [details]
patch

Attachment 320881 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4556505

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.dedicatedworker.html
Comment 11 Build Bot 2017-09-15 03:30:54 PDT
Created attachment 320886 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Saam Barati 2017-09-15 12:24:15 PDT
Created attachment 320943 [details]
patch for landing
Comment 13 Saam Barati 2017-09-15 12:30:07 PDT
The fix here is to have semantic origin be the caller's frame, and forExit to be op_enter of callee.
Comment 14 WebKit Commit Bot 2017-09-15 16:27:59 PDT
Comment on attachment 320943 [details]
patch for landing

Clearing flags on attachment: 320943

Committed r222115: <http://trac.webkit.org/changeset/222115>
Comment 15 WebKit Commit Bot 2017-09-15 16:28:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2017-09-27 12:27:29 PDT
<rdar://problem/34693313>