Bug 181570 – REGRESSION(226788): AppStore Crashed @ JavaScriptCore: JSC::MacroAssemblerARM64::pushToSaveImmediateWithoutTouchingRegisters

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

[Michael Saboff]

Backtrace

- Crashing App -
AppStore

- Crash Information -
Exception Type:  EXC_BREAKPOINT (SIGTRAP)
Exception Codes: 0x0000000000000001, 0x0000000102388d90
Termination Signal: Trace/BPT trap: 5
Termination Reason: Namespace SIGNAL, Code 0x5
Terminating Process: exc handler [0]
Triggered by Thread:  24

Backtrace:
Thread 24 name:  WTF::AutomaticThread
Thread 24 Crashed:
0   JavaScriptCore                	0x0000000102388d90 JSC::MacroAssemblerARM64::pushToSaveImmediateWithoutTouchingRegisters(JSC::AbstractMacroAssembler<JSC::ARM64Assembler>::TrustedImm32) + 200
1   JavaScriptCore                	0x0000000102386ab0 JSC::FTL::OSRExitHandle::emitExitThunk(JSC::FTL::State&, JSC::CCallHelpers&) + 88


The change in change set r226788, changed pushToSaveImmediateWithoutTouchingRegisters() to use getCachedDataTempRegisterIDAndInvalidate() instead of dataTempRegister.  That doesn't work here in the FTL code because there aren't any cached registers and so we hit the RELEASE_ASSERT() at the top of getCachedDataTempRegisterIDAndInvalidate().  Reverting pushToSaveImmediateWithoutTouchingRegisters() to use dataTempRegister with a comment why it has to be that way.

[Michael Saboff]

<rdar://problem/36451128>

[Michael Saboff]

Created attachment 331153 [details]
Patch

[Saam Barati]

Comment on attachment 331153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331153&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2208
> +        // We must use dataTempRegister directly here, because this is called from the FTL exit code
> +        // where we don't have scratch registers (m_allowScratchRegister is false)
> +        RegisterID reg = dataTempRegister;

How does this solve our problem? Isn't that saying it's invalid to use this?

[Keith Miller]

Comment on attachment 331153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331153&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        Reverting these functions to use dataTempRegister and memoryTempRegister as they may
> +        be called from code that doesn't use cached registers.  Added comments in each case
> +        why this is safe.

It seems like the actual fix is to make the callers of those functions set m_allowScratchRegister = true.

[Michael Saboff]

(In reply to Saam Barati from comment #3)
> Comment on attachment 331153 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331153&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2208
> > +        // We must use dataTempRegister directly here, because this is called from the FTL exit code
> > +        // where we don't have scratch registers (m_allowScratchRegister is false)
> > +        RegisterID reg = dataTempRegister;
> 
> How does this solve our problem? Isn't that saying it's invalid to use this?

That flag says that scratch registers are cached and need to be managed.  It is set false when the calling code is managing registers itself.

[Michael Saboff]

(In reply to Keith Miller from comment #4)
> Comment on attachment 331153 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331153&action=review
> 
> > Source/JavaScriptCore/ChangeLog:10
> > +        Reverting these functions to use dataTempRegister and memoryTempRegister as they may
> > +        be called from code that doesn't use cached registers.  Added comments in each case
> > +        why this is safe.
> 
> It seems like the actual fix is to make the callers of those functions set
> m_allowScratchRegister = true.

If we do that, then we run the risk that a register gets cached and later possible trashed.

[Michael Saboff]

Created attachment 331156 [details]
Updated Patch

[Keith Miller]

Comment on attachment 331156 [details]
Updated Patch

r=me.

[Saam Barati]

Comment on attachment 331156 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331156&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2207
> +        RegisterID reg = m_allowScratchRegister ? getCachedDataTempRegisterIDAndInvalidate() : dataTempRegister;

Why is this correct? Isn’t the assert telling us it’s not safe to use that register? E.g, we’re trashing  it here? 

This code looks footgun-y. If it’s indeed safe to do this in the FTL, why don’t you wrap calls to this with AllowMacroScratchUsage

[WebKit Commit Bot]

Comment on attachment 331156 [details]
Updated Patch

Clearing flags on attachment: 331156

Committed r226840: <https://trac.webkit.org/changeset/226840>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Saboff]

(In reply to Saam Barati from comment #9)
> Comment on attachment 331156 [details]
> Updated Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331156&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2207
> > +        RegisterID reg = m_allowScratchRegister ? getCachedDataTempRegisterIDAndInvalidate() : dataTempRegister;
> 
> Why is this correct? Isn’t the assert telling us it’s not safe to use that
> register? E.g, we’re trashing  it here? 
> 
> This code looks footgun-y. If it’s indeed safe to do this in the FTL, why
> don’t you wrap calls to this with AllowMacroScratchUsage

We don't want to change that we allow scratch registers, we want to handle both cases.

[Saam Barati]

(In reply to Michael Saboff from comment #12)
> (In reply to Saam Barati from comment #9)
> > Comment on attachment 331156 [details]
> > Updated Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=331156&action=review
> > 
> > > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2207
> > > +        RegisterID reg = m_allowScratchRegister ? getCachedDataTempRegisterIDAndInvalidate() : dataTempRegister;
> > 
> > Why is this correct? Isn’t the assert telling us it’s not safe to use that
> > register? E.g, we’re trashing  it here? 
> > 
> > This code looks footgun-y. If it’s indeed safe to do this in the FTL, why
> > don’t you wrap calls to this with AllowMacroScratchUsage
> 
> We don't want to change that we allow scratch registers, we want to handle
> both cases.

But you're unconditionally using the scratch. Am I missing something?

[Saam Barati]

Comment on attachment 331156 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331156&action=review

>>>> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:2207
>>>> +        RegisterID reg = m_allowScratchRegister ? getCachedDataTempRegisterIDAndInvalidate() : dataTempRegister;
>>> 
>>> Why is this correct? Isn’t the assert telling us it’s not safe to use that register? E.g, we’re trashing  it here? 
>>> 
>>> This code looks footgun-y. If it’s indeed safe to do this in the FTL, why don’t you wrap calls to this with AllowMacroScratchUsage
>> 
>> We don't want to change that we allow scratch registers, we want to handle both cases.
> 
> But you're unconditionally using the scratch. Am I missing something?

I lost important context on what this function is doing. It always recovers the value that was in dataTempRegister. Therefore, it should never invalidate it what's in dataTempRegister, since whatever is in there will be recovered.