Bug 119433 - REGRESSION: ARM Still crashes after change set r153612
Summary: REGRESSION: ARM Still crashes after change set r153612
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified All
: P1 Blocker
Assignee: Nobody
URL:
Keywords:
Depends on: 119140 119569
Blocks: 108645
  Show dependency treegraph
 
Reported: 2013-08-01 23:55 PDT by Michael Saboff
Modified: 2013-10-31 13:27 PDT (History)
18 users (show)

See Also:


Attachments
Patch to set callFrameRegister to value returned by cti_vm_throw_slowpath (2.02 KB, patch)
2013-08-02 00:04 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
ctiVMThrowTrampolineSlowpath fix for ARM and ARMv7 architectures. (3.00 KB, patch)
2013-08-02 01:22 PDT, Julien Brianceau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2013-08-01 23:55:04 PDT
The fix https://bugs.webkit.org/show_bug.cgi?id=119140 "REGRESSION: Crash beneath cti_vm_throw_slowpath due to invalid CallFrame pointer" didn't completely resolve the crashes on ARM.
Comment 1 Michael Saboff 2013-08-02 00:04:40 PDT
Created attachment 207987 [details]
Patch to set callFrameRegister to value returned by cti_vm_throw_slowpath
Comment 2 WebKit Commit Bot 2013-08-02 00:13:03 PDT
Attachment 207987 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/jit/JITStubsARM.h', u'Source/JavaScriptCore/jit/JITStubsARMv7.h']" exit_code: 1
Source/JavaScriptCore/jit/JITStubsARM.h:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Csaba Osztrogonác 2013-08-02 00:54:53 PDT
I tried this patch, but unfortunately it didn't help.
- ARM traditional results: http://build.webkit.sed.hu/builders/ARMv7%20Linux%20Qt5%20Release%20%28Test%29/builds/9252
- ARM Thumb2 results: http://build.webkit.sed.hu/builders/ARMv7%20Linux%20Qt5%20Release%20%28Test%29/builds/9253

I'll try to collect some debug infos a little bit later.
Comment 4 Julien Brianceau 2013-08-02 01:22:23 PDT
Created attachment 207994 [details]
ctiVMThrowTrampolineSlowpath fix for ARM and ARMv7 architectures.

I think this would work better. This patch is similar to what I've done for sh4 architecture (in http://trac.webkit.org/changeset/153583).
Comment 5 Csaba Osztrogonác 2013-08-02 06:08:43 PDT
(In reply to comment #4)
> Created an attachment (id=207994) [details]
> ctiVMThrowTrampolineSlowpath fix for ARM and ARMv7 architectures.
> 
> I think this would work better. This patch is similar to what I've done for sh4 architecture (in http://trac.webkit.org/changeset/153583).

It seems it works:
- ARM Thumb2: http://build.webkit.sed.hu/builders/ARMv7%20Linux%20Qt5%20Release%20%28Test%29/builds/9254 (still runs ...)
- ARM Traditional: http://build.webkit.sed.hu/builders/ARMv7%20Linux%20Qt5%20Release%20%28Test%29/builds/9255 (will start in an hour ...)
Comment 6 WebKit Commit Bot 2013-08-02 07:58:17 PDT
Comment on attachment 207994 [details]
ctiVMThrowTrampolineSlowpath fix for ARM and ARMv7 architectures.

Clearing flags on attachment: 207994

Committed r153648: <http://trac.webkit.org/changeset/153648>
Comment 7 Geoffrey Garen 2013-08-02 08:03:57 PDT
> Update call frame and do not restore registers from JIT stack frame in ARM and ARMv7
implementations of ctiVMThrowTrampolineSlowpath.

Can you comment on why we should not restore these registers?
Comment 8 Julien Brianceau 2013-08-02 08:49:00 PDT
(In reply to comment #7)
> 
> Can you comment on why we should not restore these registers?

From what I saw with gdb, the handler address retrieved in ctiVMThrowTrampolineSlowpath function in r1 register is likely to be the address of ctiOpThrowNotCaught, where registers are restored from JIT stack frame.

So without this patch, we did the work twice, leading to unexpected values and misplaced stack pointer during the "second restore from JIT stack frame" in ctiOpThrowNotCaught.
Comment 9 Csaba Osztrogonác 2013-10-31 13:27:10 PDT
Comment on attachment 207987 [details]
Patch to set callFrameRegister to value returned by cti_vm_throw_slowpath

The bug is closed and the fix already landed.