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.
<rdar://problem/36451128>
Created attachment 331153 [details] Patch
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?
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.
(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.
(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.
Created attachment 331156 [details] Updated Patch
Comment on attachment 331156 [details] Updated Patch r=me.
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
Comment on attachment 331156 [details] Updated Patch Clearing flags on attachment: 331156 Committed r226840: <https://trac.webkit.org/changeset/226840>
All reviewed patches have been landed. Closing bug.
(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.
(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?
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.