Summary: | REGRESSION(r157164): v8-v6/v8-raytrace.js crashes on arm and sh4 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Brianceau <jbriance> | ||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | fpizlo, ggaren, kkristof, mark.lam, msaboff, oliver, ossy, rgabor, szledan, yannick.poirier, zherczeg | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 108645 | ||||||||||
Attachments: |
|
Description
Julien Brianceau
2013-10-24 10:07:37 PDT
Another information about this: - when DFG is disabled using JSC_useDFGJIT=false, the crash is still there (for both architectures) - when JIT is disabled using JSC_useJIT=false, the test runs fine without crash (for both architectures) Here is the backtrace leading to this code generation on my ARM platform: #0 0x000969f4 in JSC::virtualForThunkGenerator (vm=0xa18710, kind=JSC::CodeForCall) at /webkit/Source/JavaScriptCore/jit/ThunkGenerators.cpp:198 #1 0x00096dd8 in JSC::virtualCallThunkGenerator (vm=0xa18710) at /webkit/Source/JavaScriptCore/jit/ThunkGenerators.cpp:267 #2 0x000902dc in JSC::JITThunks::ctiStub (this=0xa23be0, vm=0xa18710, generator=0x96db0 <JSC::virtualCallThunkGenerator(JSC::VM*)>) at /webkit/Source/JavaScriptCore/jit/JITThunks.cpp:71 #3 0x0007dbc4 in JSC::VM::getCTIStub (this=0xa18710, generator=0x96db0 <JSC::virtualCallThunkGenerator(JSC::VM*)>) at /webkit/Source/JavaScriptCore/runtime/VM.h:326 #4 0x0035a734 in JSC::linkSlowFor (repatchBuffer=..., vm=0xa18710, callLinkInfo=..., kind=JSC::CodeForCall) at /webkit/Source/JavaScriptCore/jit/Repatch.cpp:1219 #5 0x0035ab34 in JSC::linkSlowFor (exec=0x3531bd48, callLinkInfo=..., kind=JSC::CodeForCall) at /webkit/Source/JavaScriptCore/jit/Repatch.cpp:1263 #6 0x00345f00 in JSC::operationLinkClosureCall (execCallee=0x3531bd48) at /webkit/Source/JavaScriptCore/jit/JITOperations.cpp:658 #7 0x34d655d4 in ?? () #8 0x34d655d4 in ?? () First analyzis: it seems that there's a mixup betweeen JSInterfaceJIT::regT0 and GPRInfo::nonArgGPR0. The issue is not seen on X86 and X86_64, because on these architectures we have regT0 == nonArgGPR0 == X86Registers::eax (In reply to comment #3) > The issue is not seen on X86 and X86_64, because on these architectures we have regT0 == nonArgGPR0 == X86Registers::eax I mean *this would explain why* the issue is not seen on X86 and X86_64. Created attachment 215267 [details] run-layout-jsc results for arm r157163 Created attachment 215268 [details] run-layout-jsc results for arm r157164 Here are the run-layout-jsc results for r157163 and r157164. The delta is 20 new crashes between r157163 and r157164 for ARM_TRADITIONAL: js/array-proto-func-property-getter-except js/comparison-operators-greater js/comparison-operators js/comparison-operators-less js/date-set-to-nan js/dfg-float32array js/dfg-float64array js/dfg-inline-unused-this js/dfg-inline-unused-this-method-check js/dfg-int16array js/dfg-int32array js/dfg-int32array-overflow-values js/dfg-int8array js/dfg-intrinsic-unused-this js/dfg-intrinsic-unused-this-method-check js/dfg-uint16array js/dfg-uint32array js/dfg-uint32array-overflow-values js/dfg-uint8array js/dfg-uint8clampedarray Created attachment 215444 [details]
Use regTx instead of nonArgGPRx in virtualForThunkGenerator function
This patch fixes many crashes on arm and sh4 architectures.
Comment on attachment 215444 [details]
Use regTx instead of nonArgGPRx in virtualForThunkGenerator function
I think this is wrong. The point of using nonArgGPR is that we want to use a register that isn't an argument. regT0 may be an argument register on some platforms. I think that the correct change is to make sure that when the JITs make calls, they use nonArgGPR for the callee.
(In reply to comment #6) > Created an attachment (id=215268) [details] > run-layout-jsc results for arm r157164 > > Here are the run-layout-jsc results for r157163 and r157164. > > The delta is 20 new crashes between r157163 and r157164 for ARM_TRADITIONAL: > js/array-proto-func-property-getter-except > js/comparison-operators-greater > js/comparison-operators > js/comparison-operators-less > js/date-set-to-nan > js/dfg-float32array > js/dfg-float64array > js/dfg-inline-unused-this > js/dfg-inline-unused-this-method-check > js/dfg-int16array > js/dfg-int32array > js/dfg-int32array-overflow-values > js/dfg-int8array > js/dfg-intrinsic-unused-this > js/dfg-intrinsic-unused-this-method-check > js/dfg-uint16array > js/dfg-uint32array > js/dfg-uint32array-overflow-values > js/dfg-uint8array > js/dfg-uint8clampedarray I'm curious, is your methodology for making these changes seriously just that you keep trying stuff until tests pass? (In reply to comment #9) > (In reply to comment #6) > > Created an attachment (id=215268) [details] [details] > > run-layout-jsc results for arm r157164 > > > > Here are the run-layout-jsc results for r157163 and r157164. > > > > The delta is 20 new crashes between r157163 and r157164 for ARM_TRADITIONAL: > > js/array-proto-func-property-getter-except > > js/comparison-operators-greater > > js/comparison-operators > > js/comparison-operators-less > > js/date-set-to-nan > > js/dfg-float32array > > js/dfg-float64array > > js/dfg-inline-unused-this > > js/dfg-inline-unused-this-method-check > > js/dfg-int16array > > js/dfg-int32array > > js/dfg-int32array-overflow-values > > js/dfg-int8array > > js/dfg-intrinsic-unused-this > > js/dfg-intrinsic-unused-this-method-check > > js/dfg-uint16array > > js/dfg-uint32array > > js/dfg-uint32array-overflow-values > > js/dfg-uint8array > > js/dfg-uint8clampedarray > > I'm curious, is your methodology for making these changes seriously just that you keep trying stuff until tests pass? Reason why I ask is that quite clearly, the DFG is using nonArgGPR0 for the callee. I just found that out by looking for "Call" in the DFGSpeculativeJIT64.cpp and DFGSpeculativeJIT32_64.cpp files. That led me to emitCall(), where it's clear that we're moving the callee into nonArgGPR0 and not regT0. Hence this code will break the DFG. (In reply to comment #10) > (In reply to comment #9) > > I'm curious, is your methodology for making these changes seriously just that you keep trying stuff until tests pass? No, I thought that my previous comments showed that I did a little analyzis before. > Reason why I ask is that quite clearly, the DFG is using nonArgGPR0 for the callee. I just found that out by looking for "Call" in the DFGSpeculativeJIT64.cpp and DFGSpeculativeJIT32_64.cpp files. That led me to emitCall(), where it's clear that we're moving the callee into nonArgGPR0 and not regT0. Digging into the JavaScriptCore engine is just a part of my job, and I have still to learn on how it works. I don't pretend to be an expert of this engine, I'm just trying to help. > Hence this code will break the DFG. I didn't realize this, but fortunately reviews are made for this, right? This regression has been fixed with Filip's changeset r158315 (http://trac.webkit.org/changeset/158315) Thanks a lot! |