The jump() function creates 8 instructions, but the offsets of branches meant to go over them only account for 6. In most cases, this is not an issue as the last two instructions of jump() would be nops, but in the rarer case where the jump destination is in a different 256 MB segment, MIPSAssembler::linkWithOffset() will rewrite the code in a way in which the last 4 instructions would be a 2 instruction load (lui/ori) into $t9, a "j $t9" and then a nop. The wrong offset will mean that the previous branches meant to go over the whole jump will branch to the "j $t9" instruction, which would jump to whatever is currently in $t9 (since lui/ori would not be executed).
Created attachment 269813 [details] Patch
There is a different approach to 256MB jumps proposed in https://bugs.webkit.org/show_bug.cgi?id=125920. We use it in Qt port.
(In reply to comment #2) > There is a different approach to 256MB jumps proposed in > https://bugs.webkit.org/show_bug.cgi?id=125920. We use it in Qt port. Indeed, that patch would fix this bug as well (by removing the 2 nops at the start of branchEqual), but it also does a lot more and has been refused so far, hence my proposing this.
Created attachment 269926 [details] Patch New patch that does not save cmpTempRegister
(In reply to comment #4) > Created attachment 269926 [details] > Patch > > New patch that does not save cmpTempRegister Sorry, wrong bug!
Created attachment 269927 [details] Patch
Comment on attachment 269926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269926&action=review > Source/JavaScriptCore/yarr/YarrJIT.cpp:2585 > + m_assembler.sw(framePointerRegister, stackPointerRegister, 0); Please use pushPair here > Source/JavaScriptCore/yarr/YarrJIT.cpp:2621 > + m_assembler.addiu(stackPointerRegister, stackPointerRegister, 8); And popPair here
(In reply to comment #7) > Comment on attachment 269926 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269926&action=review > > > Source/JavaScriptCore/yarr/YarrJIT.cpp:2585 > > + m_assembler.sw(framePointerRegister, stackPointerRegister, 0); > > Please use pushPair here > > > Source/JavaScriptCore/yarr/YarrJIT.cpp:2621 > > + m_assembler.addiu(stackPointerRegister, stackPointerRegister, 8); > > And popPair here Sorry for the confusion, this patch really belonged to https://bugs.webkit.org/show_bug.cgi?id=153457
Created attachment 270897 [details] Test to reproduce This is a first attempt at a test to reproduce the issue. On my device, this tends to crash, and I could verify that it is when the generated function is overlapping two 256MB segments, though I did not fully analyze the reason for the crash, and if it is indeed our bug (did not really want to step through 32MB of JIT'ed code). I verified that it is overlapping two segments using --dumpDisassembly=true. On mips it does not provide disassembly, and it provides a lot of information, but among it is the address at which the JIT'ed code is.
(In reply to comment #3) > (In reply to comment #2) > > There is a different approach to 256MB jumps proposed in > > https://bugs.webkit.org/show_bug.cgi?id=125920. We use it in Qt port. > > Indeed, that patch would fix this bug as well (by removing the 2 nops at the > start of branchEqual), but it also does a lot more and has been refused so > far, hence my proposing this. Patch from 125920 works fine here in QtWebKit branch based on WebKitGTK 2.12. I think it also has some battle testing in old QtWebKit branch, used in Qt since 5.4 release (2014). Though my device has less than 256MB of RAM available for WebKit, so I cannot code path for boundary cross.
Created attachment 324703 [details] Patch Rebased patch. This is now needed to fix about 1000 test failure on MIPS since changes in r223813 makes this bug much more likely to happen. Submitting this new version for review since I forgot to commit at the time (oops) and this is a bit old.
Comment on attachment 324703 [details] Patch Informal r+ from me. It'll be nice to have this fixed after such a long while \o/
Comment on attachment 324703 [details] Patch Clearing flags on attachment: 324703 Committed r223916: <https://trac.webkit.org/changeset/223916>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35568988>