Bug 153464 – [mips] fix offsets of branches that have to go over a jump

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Guillaume Emont]

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).

[Guillaume Emont]

Created attachment 269813 [details]
Patch

[Konstantin Tokarev]

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]

(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]

Created attachment 269926 [details]
Patch

New patch that does not save cmpTempRegister

[Guillaume Emont]

(In reply to comment #4)
> Created attachment 269926 [details]
> Patch
> 
> New patch that does not save cmpTempRegister
Sorry, wrong bug!

[Guillaume Emont]

Created attachment 269927 [details]
Patch

[Konstantin Tokarev]

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]

(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]

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]

(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]

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 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 on attachment 324703 [details]
Patch

Clearing flags on attachment: 324703

Committed r223916: <https://trac.webkit.org/changeset/223916>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568988>