Bug 124447 – Crash in virtualForThunkGenerator generated code on ARM64

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

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.

[Michael Saboff]

Created attachment 217115 [details]
Patch

[Filip Pizlo]

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]

(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]

(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]

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]

(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 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]

Created attachment 217141 [details]
diff against iOS merge tree

[Michael Saboff]

(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]

(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]

(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]

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 on attachment 217195 [details]
Updated ChangeLog

r=me

[Michael Saboff]

Committed r159427: <http://trac.webkit.org/changeset/159427>

[Roger Fong]

Some new test failures on Windows:

I filed a bug here: https://bugs.webkit.org/show_bug.cgi?id=124683