Bug 124447 - Crash in virtualForThunkGenerator generated code on ARM64
Summary: Crash in virtualForThunkGenerator generated code on ARM64
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-15 21:44 PST by Michael Saboff
Modified: 2013-11-20 15:16 PST (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 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.
Comment 1 Michael Saboff 2013-11-15 21:51:58 PST
Created attachment 217115 [details]
Patch
Comment 2 Filip Pizlo 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?
Comment 3 Michael Saboff 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.
Comment 4 Michael Saboff 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.
Comment 5 Andy Estes 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>
Comment 6 Michael Saboff 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.
Comment 7 Geoffrey Garen 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.
Comment 8 Andy Estes 2013-11-16 16:10:09 PST
Created attachment 217141 [details]
diff against iOS merge tree
Comment 9 Michael Saboff 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.
Comment 10 Andy Estes 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)
Comment 11 Michael Saboff 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)
Comment 12 Michael Saboff 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.
Comment 13 Geoffrey Garen 2013-11-18 08:53:13 PST
Comment on attachment 217195 [details]
Updated ChangeLog

r=me
Comment 14 Michael Saboff 2013-11-18 09:54:54 PST
Committed r159427: <http://trac.webkit.org/changeset/159427>
Comment 15 Roger Fong 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