181570 2018-01-11 18:33:52 -0800 REGRESSION(226788): AppStore Crashed @ JavaScriptCore: JSC::MacroAssemblerARM64::pushToSaveImmediateWithoutTouchingRegisters 2018-01-17 18:05:36 -0800 1 1 1 Unclassified WebKit JavaScriptCore Other Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 msaboff msaboff commit-queue ews-watchlist keith_miller mark.lam saam webkit-bug-importer oldest_to_newest 1388617 0 msaboff 2018-01-11 18:33:52 -0800 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. 1388618 1 msaboff 2018-01-11 18:34:08 -0800 <rdar://problem/36451128> 1388619 2 331153 msaboff 2018-01-11 18:38:29 -0800 Created attachment 331153 Patch 1388620 3 331153 saam 2018-01-11 18:43:17 -0800 Comment on attachment 331153 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? 1388621 4 331153 keith_miller 2018-01-11 18:44:42 -0800 Comment on attachment 331153 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. 1388622 5 msaboff 2018-01-11 18:48:03 -0800 (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. 1388623 6 msaboff 2018-01-11 18:49:14 -0800 (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. 1388628 7 331156 msaboff 2018-01-11 19:02:14 -0800 Created attachment 331156 Updated Patch 1388629 8 331156 keith_miller 2018-01-11 19:03:16 -0800 Comment on attachment 331156 Updated Patch r=me. 1388645 9 331156 saam 2018-01-11 19:23:42 -0800 Comment on attachment 331156 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 1388649 10 331156 commit-queue 2018-01-11 19:30:45 -0800 Comment on attachment 331156 Updated Patch Clearing flags on attachment: 331156 Committed r226840: <https://trac.webkit.org/changeset/226840> 1388650 11 commit-queue 2018-01-11 19:30:46 -0800 All reviewed patches have been landed. Closing bug. 1388676 12 msaboff 2018-01-11 20:26:46 -0800 (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. 1388690 13 saam 2018-01-11 21:08:01 -0800 (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? 1390320 14 331156 saam 2018-01-17 18:05:36 -0800 Comment on attachment 331156 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. 331153 2018-01-11 18:38:29 -0800 2018-01-11 19:02:14 -0800 Patch 181570.patch text/plain 2532 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjI2ODM3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBA CisyMDE4LTAxLTExICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIFJFR1JFU1NJT04oMjI2Nzg4KTogQXBwU3RvcmUgQ3Jhc2hlZCBAIEphdmFTY3JpcHRDb3Jl OiBKU0M6Ok1hY3JvQXNzZW1ibGVyQVJNNjQ6OnB1c2hUb1NhdmVJbW1lZGlhdGVXaXRob3V0VG91 Y2hpbmdSZWdpc3RlcnMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTE4MTU3MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIFJldmVydGluZyB0aGVzZSBmdW5jdGlvbnMgdG8gdXNlIGRhdGFUZW1wUmVnaXN0ZXIg YW5kIG1lbW9yeVRlbXBSZWdpc3RlciBhcyB0aGV5IG1heQorICAgICAgICBiZSBjYWxsZWQgZnJv bSBjb2RlIHRoYXQgZG9lc24ndCB1c2UgY2FjaGVkIHJlZ2lzdGVycy4gIEFkZGVkIGNvbW1lbnRz IGluIGVhY2ggY2FzZQorICAgICAgICB3aHkgdGhpcyBpcyBzYWZlLgorCisKKyAgICAgICAgKiBh c3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk02NC5oOgorICAgICAgICAoSlNDOjpNYWNyb0Fzc2Vt YmxlckFSTTY0OjphYm9ydFdpdGhSZWFzb24pOgorICAgICAgICAoSlNDOjpNYWNyb0Fzc2VtYmxl ckFSTTY0OjpwdXNoVG9TYXZlSW1tZWRpYXRlV2l0aG91dFRvdWNoaW5nUmVnaXN0ZXJzKToKKwog MjAxOC0wMS0xMSAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAgICAgIFJl bmFtZSBNYXJrZWRBbGxvY2F0b3IgdG8gQmxvY2tEaXJlY3RvcnkgYW5kIEFsbG9jYXRvckF0dHJp YnV0ZXMgdG8gQ2VsbEF0dHJpYnV0ZXMKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3Nl bWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk02NC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2Ny aXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk02NC5oCShyZXZpc2lvbiAyMjY4MzYp CisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNNjQu aAkod29ya2luZyBjb3B5KQpAQCAtMTEwNSwxMyArMTEwNSwxNSBAQCBwdWJsaWM6CiAKICAgICB2 b2lkIGFib3J0V2l0aFJlYXNvbihBYm9ydFJlYXNvbiByZWFzb24pCiAgICAgewotICAgICAgICBt b3ZlKFRydXN0ZWRJbW0zMihyZWFzb24pLCBnZXRDYWNoZWREYXRhVGVtcFJlZ2lzdGVySURBbmRJ bnZhbGlkYXRlKCkpOworICAgICAgICAvLyBJdCBpcyBzYWZlIHRvIHVzZSBkYXRhVGVtcFJlZ2lz dGVyIGRpcmVjdGx5IHNpbmNlIHRoaXMgaXMgYSBjcmFzaGluZyBKSVQgQXNzZXJ0LgorICAgICAg ICBtb3ZlKFRydXN0ZWRJbW0zMihyZWFzb24pLCBkYXRhVGVtcFJlZ2lzdGVyKTsKICAgICAgICAg YnJlYWtwb2ludCgpOwogICAgIH0KIAogICAgIHZvaWQgYWJvcnRXaXRoUmVhc29uKEFib3J0UmVh c29uIHJlYXNvbiwgaW50cHRyX3QgbWlzYykKICAgICB7Ci0gICAgICAgIG1vdmUoVHJ1c3RlZElt bTY0KG1pc2MpLCBnZXRDYWNoZWRNZW1vcnlUZW1wUmVnaXN0ZXJJREFuZEludmFsaWRhdGUoKSk7 CisgICAgICAgIC8vIEl0IGlzIHNhZmUgdG8gdXNlIG1lbW9yeVRlbXBSZWdpc3RlciBkaXJlY3Rs eSBzaW5jZSB0aGlzIGlzIGEgY3Jhc2hpbmcgSklUIEFzc2VydC4KKyAgICAgICAgbW92ZShUcnVz dGVkSW1tNjQobWlzYyksIG1lbW9yeVRlbXBSZWdpc3Rlcik7CiAgICAgICAgIGFib3J0V2l0aFJl YXNvbihyZWFzb24pOwogICAgIH0KIApAQCAtMjIwMSw3ICsyMjAzLDkgQEAgcHVibGljOgogICAg IAogICAgIHZvaWQgcHVzaFRvU2F2ZUltbWVkaWF0ZVdpdGhvdXRUb3VjaGluZ1JlZ2lzdGVycyhU cnVzdGVkSW1tMzIgaW1tKQogICAgIHsKLSAgICAgICAgUmVnaXN0ZXJJRCByZWcgPSBnZXRDYWNo ZWREYXRhVGVtcFJlZ2lzdGVySURBbmRJbnZhbGlkYXRlKCk7CisgICAgICAgIC8vIFdlIG11c3Qg dXNlIGRhdGFUZW1wUmVnaXN0ZXIgZGlyZWN0bHkgaGVyZSwgYmVjYXVzZSB0aGlzIGlzIGNhbGxl ZCBmcm9tIHRoZSBGVEwgZXhpdCBjb2RlCisgICAgICAgIC8vIHdoZXJlIHdlIGRvbid0IGhhdmUg c2NyYXRjaCByZWdpc3RlcnMgKG1fYWxsb3dTY3JhdGNoUmVnaXN0ZXIgaXMgZmFsc2UpCisgICAg ICAgIFJlZ2lzdGVySUQgcmVnID0gZGF0YVRlbXBSZWdpc3RlcjsKICAgICAgICAgcHVzaFBhaXIo cmVnLCByZWcpOwogICAgICAgICBtb3ZlKGltbSwgcmVnKTsKICAgICAgICAgc3RvcmU2NChyZWcs IHN0YWNrUG9pbnRlclJlZ2lzdGVyKTsK 331156 2018-01-11 19:02:14 -0800 2018-01-11 19:30:45 -0800 Updated Patch 181570-2.patch text/plain 2580 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjI2ODM3KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBA CisyMDE4LTAxLTExICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIFJFR1JFU1NJT04oMjI2Nzg4KTogQXBwU3RvcmUgQ3Jhc2hlZCBAIEphdmFTY3JpcHRDb3Jl OiBKU0M6Ok1hY3JvQXNzZW1ibGVyQVJNNjQ6OnB1c2hUb1NhdmVJbW1lZGlhdGVXaXRob3V0VG91 Y2hpbmdSZWdpc3RlcnMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTE4MTU3MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgICogYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNNjQuaDoKKyAgICAgICAgKEpTQzo6 TWFjcm9Bc3NlbWJsZXJBUk02NDo6YWJvcnRXaXRoUmVhc29uKToKKyAgICAgICAgUmV2ZXJ0aW5n IHRoZXNlIGZ1bmN0aW9ucyB0byB1c2UgZGF0YVRlbXBSZWdpc3RlciBhbmQgbWVtb3J5VGVtcFJl Z2lzdGVyIGFzIHRoZXkgYXJlCisgICAgICAgIEpJVCByZWxlYXNlIGFzc2VydHMgdGhhdCB3aWxs IGNyYXNoIHRoZSBwcm9ncmFtLgorCisgICAgICAgIChKU0M6Ok1hY3JvQXNzZW1ibGVyQVJNNjQ6 OnB1c2hUb1NhdmVJbW1lZGlhdGVXaXRob3V0VG91Y2hpbmdSZWdpc3RlcnMpOgorICAgICAgICBD aGFuZ2VkIHRoaXMgc28gdGhhdCBpdCBpbnZhbGlkYXRlcyBhbnkgY2FjaGVkIGRhdGFUbXBSZWdp c3RlciBjb250ZW50cyBpZiB0ZW1wIHJlZ2lzdGVyCisgICAgICAgIGNhY2hpbmcgaXMgZW5hYmxl ZC4KKwogMjAxOC0wMS0xMSAgRmlsaXAgUGl6bG8gIDxmcGl6bG9AYXBwbGUuY29tPgogCiAgICAg ICAgIFJlbmFtZSBNYXJrZWRBbGxvY2F0b3IgdG8gQmxvY2tEaXJlY3RvcnkgYW5kIEFsbG9jYXRv ckF0dHJpYnV0ZXMgdG8gQ2VsbEF0dHJpYnV0ZXMKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29y ZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk02NC5oCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9K YXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk02NC5oCShyZXZpc2lvbiAy MjY4MzYpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVy QVJNNjQuaAkod29ya2luZyBjb3B5KQpAQCAtMTEwNSwxMyArMTEwNSwxNSBAQCBwdWJsaWM6CiAK ICAgICB2b2lkIGFib3J0V2l0aFJlYXNvbihBYm9ydFJlYXNvbiByZWFzb24pCiAgICAgewotICAg ICAgICBtb3ZlKFRydXN0ZWRJbW0zMihyZWFzb24pLCBnZXRDYWNoZWREYXRhVGVtcFJlZ2lzdGVy SURBbmRJbnZhbGlkYXRlKCkpOworICAgICAgICAvLyBJdCBpcyBzYWZlIHRvIHVzZSBkYXRhVGVt cFJlZ2lzdGVyIGRpcmVjdGx5IHNpbmNlIHRoaXMgaXMgYSBjcmFzaGluZyBKSVQgQXNzZXJ0Lgor ICAgICAgICBtb3ZlKFRydXN0ZWRJbW0zMihyZWFzb24pLCBkYXRhVGVtcFJlZ2lzdGVyKTsKICAg ICAgICAgYnJlYWtwb2ludCgpOwogICAgIH0KIAogICAgIHZvaWQgYWJvcnRXaXRoUmVhc29uKEFi b3J0UmVhc29uIHJlYXNvbiwgaW50cHRyX3QgbWlzYykKICAgICB7Ci0gICAgICAgIG1vdmUoVHJ1 c3RlZEltbTY0KG1pc2MpLCBnZXRDYWNoZWRNZW1vcnlUZW1wUmVnaXN0ZXJJREFuZEludmFsaWRh dGUoKSk7CisgICAgICAgIC8vIEl0IGlzIHNhZmUgdG8gdXNlIG1lbW9yeVRlbXBSZWdpc3RlciBk aXJlY3RseSBzaW5jZSB0aGlzIGlzIGEgY3Jhc2hpbmcgSklUIEFzc2VydC4KKyAgICAgICAgbW92 ZShUcnVzdGVkSW1tNjQobWlzYyksIG1lbW9yeVRlbXBSZWdpc3Rlcik7CiAgICAgICAgIGFib3J0 V2l0aFJlYXNvbihyZWFzb24pOwogICAgIH0KIApAQCAtMjIwMSw3ICsyMjAzLDggQEAgcHVibGlj OgogICAgIAogICAgIHZvaWQgcHVzaFRvU2F2ZUltbWVkaWF0ZVdpdGhvdXRUb3VjaGluZ1JlZ2lz dGVycyhUcnVzdGVkSW1tMzIgaW1tKQogICAgIHsKLSAgICAgICAgUmVnaXN0ZXJJRCByZWcgPSBn ZXRDYWNoZWREYXRhVGVtcFJlZ2lzdGVySURBbmRJbnZhbGlkYXRlKCk7CisgICAgICAgIC8vIFdl IGludmFsaWRhdGUgYW55IGNhY2hlZCB2YWx1ZXMgaW4gZGF0YVRlbXBSZWdpc3RlciBpZiB0ZW1w IHJlZ2lzdGVyIGNhY2hpbmcgaXMgZW5hYmxlZC4KKyAgICAgICAgUmVnaXN0ZXJJRCByZWcgPSBt X2FsbG93U2NyYXRjaFJlZ2lzdGVyID8gZ2V0Q2FjaGVkRGF0YVRlbXBSZWdpc3RlcklEQW5kSW52 YWxpZGF0ZSgpIDogZGF0YVRlbXBSZWdpc3RlcjsKICAgICAgICAgcHVzaFBhaXIocmVnLCByZWcp OwogICAgICAgICBtb3ZlKGltbSwgcmVnKTsKICAgICAgICAgc3RvcmU2NChyZWcsIHN0YWNrUG9p bnRlclJlZ2lzdGVyKTsK