WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
124447
Crash in virtualForThunkGenerator generated code on ARM64
https://bugs.webkit.org/show_bug.cgi?id=124447
Summary
Crash in virtualForThunkGenerator generated code on ARM64
Michael Saboff
Reported
2013-11-15 21:44:23 PST
The baseline JIT generates slow path call code with the caller is in regT0. The DFG generates call code with the caller in nonArgGPR0. The virtualForThunkGenerator generates code with the caller in nonArgGPR0. For X86 and X86_64, regT0 and nonArgGPR0 are the same CPU register, eax. For other platforms this isn't the case. The same issue exists for JSVALUE32_64 ports as well, where there also is an issue with the callee tag registers being regT1 and nonArgGPR1 in the various locations.
Attachments
Patch
(9.32 KB, patch)
2013-11-15 21:51 PST
,
Michael Saboff
ggaren
: review-
Details
Formatted Diff
Diff
diff against iOS merge tree
(9.76 KB, patch)
2013-11-16 16:10 PST
,
Andy Estes
no flags
Details
Formatted Diff
Diff
Updated ChangeLog
(10.03 KB, patch)
2013-11-18 07:14 PST
,
Michael Saboff
ggaren
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Michael Saboff
Comment 1
2013-11-15 21:51:58 PST
Created
attachment 217115
[details]
Patch
Filip Pizlo
Comment 2
2013-11-15 21:58:37 PST
Comment on
attachment 217115
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=217115&action=review
> Source/JavaScriptCore/jit/GPRInfo.h:358 > - static const unsigned numberOfRegisters = 10; > + static const unsigned numberOfRegisters = 11;
Is this other change intentional?
Michael Saboff
Comment 3
2013-11-15 22:02:48 PST
(In reply to
comment #2
)
> (From update of
attachment 217115
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=217115&action=review
> > > Source/JavaScriptCore/jit/GPRInfo.h:358 > > - static const unsigned numberOfRegisters = 10; > > + static const unsigned numberOfRegisters = 11; > > Is this other change intentional?
Yes. r12 wasn't used. Geoff and I were talking about this and we didn't want any overlap between arg regs or regT0. Being able to add r12 as a temp made that work nicely.
Michael Saboff
Comment 4
2013-11-15 22:03:52 PST
(In reply to
comment #3
)
> (In reply to
comment #2
) > > (From update of
attachment 217115
[details]
[details]) > > View in context:
https://bugs.webkit.org/attachment.cgi?id=217115&action=review
> > > > > Source/JavaScriptCore/jit/GPRInfo.h:358 > > > - static const unsigned numberOfRegisters = 10; > > > + static const unsigned numberOfRegisters = 11; > > > > Is this other change intentional? > > Yes. r12 wasn't used. Geoff and I were talking about this and we didn't want any overlap between arg regs or regT0. Being able to add r12 as a temp made that work nicely.
That is overlap between arg regs or regT0 and the nonARGReg's.
Andy Estes
Comment 5
2013-11-16 01:10:07 PST
This patch seems to have exposed an assertion failure. Possibly this is a new issue exposed by fixing the crash, but since I don't know for sure I'm posting here: 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> ASSERTION FAILED: from.isCell() && from.asCell()->JSCell::inherits(std::remove_pointer<To>::type::info()) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> /Users/estes/Repos/workspaces/ios/OpenSource/Source/JavaScriptCore/runtime/JSCell.h(187) : To JSC::jsCast(JSC::JSValue) [To = JSC::JSScope *] 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 1 0x100894a58 WTFCrash 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 2 0x100269d58 JSC::JSScope* JSC::jsCast<JSC::JSScope*>(JSC::JSValue) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 3 0x100269cd4 JSC::Register::scope() const 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 4 0x100269c34 JSC::ExecState::scope() const 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 5 0x100263e14 JSC::ExecState::vm() const 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 6 0x10058cb0c getHostCallReturnValueWithExecState 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 7 0x1006c95e4 llint_op_call 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 8 0x10055d710 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 9 0x100330b1c JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 10 0x1024bf200 WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 11 0x102cd3a7c WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 12 0x102cd3bbc WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 13 0x102cec16c WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 14 0x10201f124 WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent(WebCore::PendingScript&) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 15 0x10201ef8c WebCore::HTMLScriptRunner::executeParsingBlockingScript() 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 16 0x10201f770 WebCore::HTMLScriptRunner::executeParsingBlockingScripts() 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 17 0x10201f8a0 WebCore::HTMLScriptRunner::executeScriptsWaitingForLoad(WebCore::CachedResource*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 18 0x101f7a58c WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 19 0x101f7a60c non-virtual thunk to WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 20 0x10180b750 WebCore::CachedResource::checkNotify() 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 21 0x10180b854 WebCore::CachedResource::finishLoading(WebCore::ResourceBuffer*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 22 0x1018240a0 WebCore::CachedScript::finishLoading(WebCore::ResourceBuffer*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 23 0x102e3e250 WebCore::SubresourceLoader::didFinishLoading(double) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 24 0x102c85e44 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 25 0x102c7d7c8 WebCore::didFinishLoading(_CFURLConnection*, void const*) 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 26 0x181fd98c0 <redacted> 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 27 0x181fd5b38 <redacted> 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 28 0x182399f7c CFArrayApplyFunction 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 29 0x181f35214 <redacted> 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 30 0x181f35090 <redacted> 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 31 0x181f34ecc <redacted>
Michael Saboff
Comment 6
2013-11-16 08:07:36 PST
(In reply to
comment #5
)
> This patch seems to have exposed an assertion failure. Possibly this is a new issue exposed by fixing the crash, but since I don't know for sure I'm posting here: > > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> ASSERTION FAILED: from.isCell() && from.asCell()->JSCell::inherits(std::remove_pointer<To>::type::info()) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> /Users/estes/Repos/workspaces/ios/OpenSource/Source/JavaScriptCore/runtime/JSCell.h(187) : To JSC::jsCast(JSC::JSValue) [To = JSC::JSScope *] > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 1 0x100894a58 WTFCrash > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 2 0x100269d58 JSC::JSScope* JSC::jsCast<JSC::JSScope*>(JSC::JSValue) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 3 0x100269cd4 JSC::Register::scope() const > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 4 0x100269c34 JSC::ExecState::scope() const > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 5 0x100263e14 JSC::ExecState::vm() const > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 6 0x10058cb0c getHostCallReturnValueWithExecState > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 7 0x1006c95e4 llint_op_call > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 8 0x10055d710 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 9 0x100330b1c JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 10 0x1024bf200 WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 11 0x102cd3a7c WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 12 0x102cd3bbc WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 13 0x102cec16c WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 14 0x10201f124 WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent(WebCore::PendingScript&) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 15 0x10201ef8c WebCore::HTMLScriptRunner::executeParsingBlockingScript() > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 16 0x10201f770 WebCore::HTMLScriptRunner::executeParsingBlockingScripts() > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 17 0x10201f8a0 WebCore::HTMLScriptRunner::executeScriptsWaitingForLoad(WebCore::CachedResource*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 18 0x101f7a58c WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 19 0x101f7a60c non-virtual thunk to WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 20 0x10180b750 WebCore::CachedResource::checkNotify() > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 21 0x10180b854 WebCore::CachedResource::finishLoading(WebCore::ResourceBuffer*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 22 0x1018240a0 WebCore::CachedScript::finishLoading(WebCore::ResourceBuffer*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 23 0x102e3e250 WebCore::SubresourceLoader::didFinishLoading(double) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 24 0x102c85e44 WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 25 0x102c7d7c8 WebCore::didFinishLoading(_CFURLConnection*, void const*) > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 26 0x181fd98c0 <redacted> > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 27 0x181fd5b38 <redacted> > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 28 0x182399f7c CFArrayApplyFunction > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 29 0x181f35214 <redacted> > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 30 0x181f35090 <redacted> > 2013-11-16 01:01:59 -0800 MobileSafari[797] <Notice> 31 0x181f34ecc <redacted>
This looks like the execState passed into getHostCallReturnValueWithExecState() is bad. That execState comes from the inline assembly getHostCallReturnValue() in jit/JITOperations.cpp. Send the ARM64 version of getHostCallReturnValue.
Geoffrey Garen
Comment 7
2013-11-16 09:20:44 PST
Comment on
attachment 217115
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=217115&action=review
r- since we still appear to be crashing in function call code.
> Source/JavaScriptCore/ChangeLog:11 > + Changed nonArgGPR0, nonArgGPR1 and nonArgGPR2 for X86 and X86_64 to not match up with > + regT0-2. Changing these registers will cause a crash on all ports should we have a > + similar problem in the future. Changed the DFG call generating code to use regT0 and > + regT1. Added r12 to X86_64 as a new temp register (regT9) and moved r13 down to regT10.
This says a lot about what you did, but not much about why. For example, you didn't mention that x86_64 overlaps regT0 and nonArgGPR0, but ARM does not, which is why this crash only happens on ARM. You didn't explain why you added r12 (to facilitate non-overlapping registers on x86_64). You also didn't explain why you made the change to regT0/regT1.
Andy Estes
Comment 8
2013-11-16 16:10:09 PST
Created
attachment 217141
[details]
diff against iOS merge tree
Michael Saboff
Comment 9
2013-11-17 16:29:34 PST
(In reply to
comment #8
)
> Created an attachment (id=217141) [details] > diff against iOS merge tree
That all looks fine. What I'd like to see is the contents of the ARM64 section of inline assembly for getHostCallReturnValue() in jit/JITOperations.cpp. I think that is the source of the latest crash and unrelated to this patch.
Andy Estes
Comment 10
2013-11-17 23:27:25 PST
(In reply to
comment #9
)
> (In reply to
comment #8
) > > Created an attachment (id=217141) [details] [details] > > diff against iOS merge tree > > That all looks fine. What I'd like to see is the contents of the ARM64 section of inline assembly for getHostCallReturnValue() in jit/JITOperations.cpp. I think that is the source of the latest crash and unrelated to this patch.
#elif CPU(ARM64) asm ( ".text" "\n" ".align 2" "\n" ".globl " SYMBOL_STRING(getHostCallReturnValue) "\n" HIDE_SYMBOL(getHostCallReturnValue) "\n" SYMBOL_STRING(getHostCallReturnValue) ":" "\n" "ldur x25, [x25, #-32]" "\n" "mov x0, x25" "\n" "b " LOCAL_REFERENCE(getHostCallReturnValueWithExecState) "\n" ); #elif COMPILER(GCC) && CPU(MIPS)
Michael Saboff
Comment 11
2013-11-17 23:55:56 PST
(In reply to
comment #10
)
> (In reply to
comment #9
) > > (In reply to
comment #8
) > > > Created an attachment (id=217141) [details] [details] [details] > > > diff against iOS merge tree > > > > That all looks fine. What I'd like to see is the contents of the ARM64 section of inline assembly for getHostCallReturnValue() in jit/JITOperations.cpp. I think that is the source of the latest crash and unrelated to this patch. > > #elif CPU(ARM64) > asm ( > ".text" "\n" > ".align 2" "\n" > ".globl " SYMBOL_STRING(getHostCallReturnValue) "\n" > HIDE_SYMBOL(getHostCallReturnValue) "\n" > SYMBOL_STRING(getHostCallReturnValue) ":" "\n" > "ldur x25, [x25, #-32]" "\n"
There is the problem. The -32 should be 0. Filed
https://bugs.webkit.org/show_bug.cgi?id=124481
to track.
> "mov x0, x25" "\n" > "b " LOCAL_REFERENCE(getHostCallReturnValueWithExecState) "\n" > ); > > #elif COMPILER(GCC) && CPU(MIPS)
Michael Saboff
Comment 12
2013-11-18 07:14:28 PST
Created
attachment 217195
[details]
Updated ChangeLog Added bug description to ChangeLog and additional comments about regT0/RegT1 as well as the addition of r12 as a temp register. The addition crash is addressed by
https://bugs.webkit.org/show_bug.cgi?id=124481
.
Geoffrey Garen
Comment 13
2013-11-18 08:53:13 PST
Comment on
attachment 217195
[details]
Updated ChangeLog r=me
Michael Saboff
Comment 14
2013-11-18 09:54:54 PST
Committed
r159427
: <
http://trac.webkit.org/changeset/159427
>
Roger Fong
Comment 15
2013-11-20 15:16:06 PST
Some new test failures on Windows: I filed a bug here:
https://bugs.webkit.org/show_bug.cgi?id=124683
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug