Bug 168502 - REGRESSION(r212778): It made 400 tests crash on AArch64 Linux
Summary: REGRESSION(r212778): It made 400 tests crash on AArch64 Linux
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 108645 167737
  Show dependency treegraph
 
Reported: 2017-02-17 02:29 PST by Csaba Osztrogonác
Modified: 2017-03-06 15:07 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.30 KB, patch)
2017-03-06 13:48 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Csaba Osztrogonác 2017-02-19 00:03:56 PST
Haven't you noticed this regression on internal iOS JSC tester bots too?
Comment 2 Filip Pizlo 2017-02-19 07:37:15 PST
Passed tests just fine on my iPad. 

Idea: what happens when you make Linux take the same path as Windows in RegisterStae.h?  Basically make it fall through to the setjmp code.
Comment 3 Filip Pizlo 2017-02-19 14:39:57 PST
Did you get a stack trace?
Comment 4 Csaba Osztrogonác 2017-02-22 01:23:02 PST
r212466 was rolled out and relanded in https://trac.webkit.org/changeset/212778
Now we have only 400 crashes on AArch64 Linux. 

(In reply to comment #2)
> Passed tests just fine on my iPad. 
> 
> Idea: what happens when you make Linux take the same path as Windows in
> RegisterStae.h?  Basically make it fall through to the setjmp code.
Will check soon.

(In reply to comment #3)
> Did you get a stack trace?
Not yet, I'm going to do a debug build in the near future.
Comment 5 Csaba Osztrogonác 2017-02-22 10:02:12 PST
(In reply to comment #2)
> Idea: what happens when you make Linux take the same path as Windows in
> RegisterStae.h?  Basically make it fall through to the setjmp code.

This idea fixed this issue. What does it mean?


(In reply to comment #3)
> Did you get a stack trace?

Unfortunately there are no crashes in debug mode.
Comment 6 Csaba Osztrogonác 2017-03-01 09:07:36 PST
(In reply to comment #5)
> (In reply to comment #2)
> > Idea: what happens when you make Linux take the same path as Windows in
> > RegisterStae.h?  Basically make it fall through to the setjmp code.
> 
> This idea fixed this issue. What does it mean?
> 
> 
> (In reply to comment #3)
> > Did you get a stack trace?
> 
> Unfortunately there are no crashes in debug mode.

AArch64 Linux platform is still broken due to this GC bug.
Any plan to fix it or should we use the setjmp codepath?
Comment 7 Filip Pizlo 2017-03-01 09:53:41 PST
(In reply to comment #5)
> (In reply to comment #2)
> > Idea: what happens when you make Linux take the same path as Windows in
> > RegisterStae.h?  Basically make it fall through to the setjmp code.
> 
> This idea fixed this issue. What does it mean?

It means that this is the fix to this issue. From what I remember setjmp is very fast on Linux so this should be fine. 

> 
> 
> (In reply to comment #3)
> > Did you get a stack trace?
> 
> Unfortunately there are no crashes in debug mode.
Comment 8 Csaba Osztrogonác 2017-03-06 13:48:51 PST
Created attachment 303550 [details]
Patch
Comment 9 Filip Pizlo 2017-03-06 13:56:27 PST
Comment on attachment 303550 [details]
Patch

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

> Source/JavaScriptCore/heap/RegisterState.h:32
> -#if !OS(WINDOWS)
> +#if !OS(WINDOWS) && !(OS(LINUX) && CPU(ARM64))

This is good.

FWIW, it's probably also a bit safer to use setjmp on all Linuxes.

I've written this kind of stuff on Linux before and always found that setjmp was the canonical Linux way of getting register state.

The only reason why we now do it differently is that Darwin's setjmp does some extra state saving that makes it just a tad bit too slow.  I don't think Linux's setjmp has that kind of problem on any architecture.  It would even be fine to change this to OS(DARWIN) rather than !OS(WINDOWS) && !OS(LINUX).

If you want to make any of those changes then lets do it in another patch.
Comment 10 WebKit Commit Bot 2017-03-06 14:48:30 PST
The commit-queue encountered the following flaky tests while processing attachment 303550 [details]:

media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html bug 169158 (authors: graouts@apple.com and ryanhaddad@apple.com)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2017-03-06 14:48:42 PST
The commit-queue encountered the following flaky tests while processing attachment 303550 [details]:

media/modern-media-controls/tracks-support/tracks-support-click-track-in-panel.html bug 169159 (authors: graouts@apple.com and ryanhaddad@apple.com)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2017-03-06 15:07:30 PST
Comment on attachment 303550 [details]
Patch

Clearing flags on attachment: 303550

Committed r213472: <http://trac.webkit.org/changeset/213472>
Comment 13 WebKit Commit Bot 2017-03-06 15:07:36 PST
All reviewed patches have been landed.  Closing bug.