Summary: | [mips] fix offsets of branches that have to go over a jump | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Guillaume Emont <guijemont> | ||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Guillaume Emont
2016-01-25 16:38:52 PST
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. |