Bug 119433

Summary: REGRESSION: ARM Still crashes after change set r153612
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Blocker CC: allan.jensen, barraclough, cgarcia, commit-queue, fpizlo, ggaren, hausmann, jbriance, loki, mark.lam, mhahnenberg, msaboff, oliver, ossy, rgabor, xinchao.peng, zan, zherczeg
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: All   
Bug Depends on: 119140, 119569    
Bug Blocks: 108645    
Attachments:
Description Flags
Patch to set callFrameRegister to value returned by cti_vm_throw_slowpath
none
ctiVMThrowTrampolineSlowpath fix for ARM and ARMv7 architectures. none

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.