Bug 153464

Summary: [mips] fix offsets of branches that have to go over a jump
Product: WebKit Reporter: Guillaume Emont <guijemont>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, aperez, commit-queue, jbriance, keith_miller, mark.lam, mcatanzaro, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Other   
OS: Linux   
Bug Depends on:    
Bug Blocks: 108664    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Test to reproduce
none
Patch none

Guillaume Emont
Reported 2016-01-25 16:38:52 PST
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).
Attachments
Patch (6.46 KB, patch)
2016-01-25 16:40 PST, Guillaume Emont 		no flags
DetailsFormatted DiffDiff
Patch (2.20 KB, patch)
2016-01-26 14:37 PST, Guillaume Emont 		no flags
DetailsFormatted DiffDiff
Patch (6.46 KB, patch)
2016-01-26 14:39 PST, Guillaume Emont 		no flags
DetailsFormatted DiffDiff
Test to reproduce (655 bytes, application/javascript)
2016-02-08 17:23 PST, Guillaume Emont 		no flags
Details
Patch (6.49 KB, patch)
2017-10-24 12:53 PDT, Guillaume Emont 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Guillaume Emont
Comment 1 2016-01-25 16:40:46 PST
Created attachment 269813 [details] Patch
Konstantin Tokarev
Comment 2 2016-01-25 23:31:00 PST
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.
Guillaume Emont
Comment 3 2016-01-26 11:14:58 PST
(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.
Guillaume Emont
Comment 4 2016-01-26 14:37:49 PST
Created attachment 269926 [details] Patch New patch that does not save cmpTempRegister
Guillaume Emont
Comment 5 2016-01-26 14:38:55 PST
(In reply to comment #4) > Created attachment 269926 [details] > Patch > > New patch that does not save cmpTempRegister Sorry, wrong bug!
Guillaume Emont
Comment 6 2016-01-26 14:39:42 PST
Created attachment 269927 [details] Patch
Konstantin Tokarev
Comment 7 2016-01-26 14:44:36 PST
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
Guillaume Emont
Comment 8 2016-01-26 17:01:02 PST
(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
Guillaume Emont
Comment 9 2016-02-08 17:23:23 PST
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.
Konstantin Tokarev
Comment 10 2016-07-14 09:59:14 PDT
(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.
Guillaume Emont
Comment 11 2017-10-24 12:53:39 PDT
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.
Adrian Perez
Comment 12 2017-10-24 12:57:30 PDT
Comment on attachment 324703 [details] Patch Informal r+ from me. It'll be nice to have this fixed after such a long while \o/
WebKit Commit Bot
Comment 13 2017-10-24 13:36:53 PDT
Comment on attachment 324703 [details] Patch Clearing flags on attachment: 324703 Committed r223916: <https://trac.webkit.org/changeset/223916>
WebKit Commit Bot
Comment 14 2017-10-24 13:36:54 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2017-11-15 13:12:06 PST
<rdar://problem/35568988>
Note You need to log in before you can comment on or make changes to this bug.