Bug 149900 - Disable tail calls because it is breaking some sites.
Summary: Disable tail calls because it is breaking some sites.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-07 14:59 PDT by Mark Lam
Modified: 2015-10-13 00:37 PDT (History)
5 users (show)

See Also:


Attachments
the patch. (1.30 KB, patch)
2015-10-07 15:05 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
follow up patch. (2.13 KB, patch)
2015-10-07 16:31 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
follow up 2: need to suppress JSC tail call tests (4.36 KB, patch)
2015-10-07 17:31 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-10-07 14:59:55 PDT
This is until we fix whatever the breakage is.
Comment 1 Mark Lam 2015-10-07 15:05:31 PDT
Created attachment 262647 [details]
the patch.
Comment 2 Saam Barati 2015-10-07 15:06:17 PDT
Comment on attachment 262647 [details]
the patch.

r=me
Comment 3 Mark Lam 2015-10-07 15:10:28 PDT
Thanks.  Landed in r190692: <http://trac.webkit.org/r190692>.
Comment 4 Mark Lam 2015-10-07 16:30:53 PDT
Need to update expected test results to match this change.
Comment 5 Mark Lam 2015-10-07 16:31:23 PDT
Created attachment 262654 [details]
follow up patch.
Comment 6 Mark Lam 2015-10-07 16:36:58 PDT
Thanks.  Follow up patch landed in r190695: <http://trac.webkit.org/r190695>.
Comment 7 Mark Lam 2015-10-07 17:31:48 PDT
Created attachment 262662 [details]
follow up 2: need to suppress JSC tail call tests

This should be the last follow up (based on all the tests in http://trac.webkit.org/changeset/190289).  This follow up 2 patch has been rubber stamped by Saam.  I will land it as soon as my test run confirms that I've addressed the remaining issue.
Comment 8 Mark Lam 2015-10-07 17:49:25 PDT
JSC tests are now passing.  Follow up 2 landed in r190699: <http://trac.webkit.org/r190699>.
Comment 9 Alexey Proskuryakov 2015-10-07 21:59:37 PDT
Looks like follow up 2 changed http/tests/misc/large-js-program.php from crash to timeout on Windows Release. Could you please add an appropriate expectation or skip?

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fmisc%2Flarge-js-program.php
Comment 10 Alexey Proskuryakov 2015-10-07 22:00:47 PDT
Also, tail call tests apparently need to be disabled: https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/67893/steps/jscore-test/logs/stdio
Comment 11 Mark Lam 2015-10-07 22:10:50 PDT
(In reply to comment #10)
> Also, tail call tests apparently need to be disabled:
> https://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/
> builds/67893/steps/jscore-test/logs/stdio

They are already.  Build 67893 was built with r190692.  The tests were skipped in r190699.
Comment 12 Mark Lam 2015-10-07 22:41:40 PDT
(In reply to comment #9)
> Looks like follow up 2 changed http/tests/misc/large-js-program.php from
> crash to timeout on Windows Release. Could you please add an appropriate
> expectation or skip?
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fmisc%2Flarge-js-program.php

The flakiness dashboard says that the blame list for that result change is https://trac.webkit.org/log/?verbose=on&rev=190700&stop_rev=190698.  Of those revisions, r190699 is for follow up 2.  But r190699 only changes the JSC tests.  It should have no effect on the layout tests.

On the other hand, r190698 is about changing the testing infrastructure for Windows.  It is more likely that r190698 triggered the change in test result.
Comment 13 Zan Dobersek 2015-10-13 00:37:55 PDT
Before they were disabled, I was experiencing crashes due to tail call optimizations on ARM Thumb and Linux. Filed bug #150083 with some information.