Bug 153463 – [mips] don't save to a callee saved register too early

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Guillaume Emont]

If we save $gp to $s4 in pichdr, then in some cases, we were overwriting $s4 before LLInt's pushCalleeSaves() is called (as pichdr is at the very beginning of a function).

[Guillaume Emont]

Created attachment 269812 [details]
Patch

[Konstantin Tokarev]

Tested on my device, works fine and fixes lots of test crashes.

[Julien Brianceau]

Comment on attachment 269812 [details]
Patch

I just took a look at the generated code in LLIntAssembly.h and this looks wrong to me.
I mean, if this patch fixes crashes, there is a better way to do the fix as this patch is actually removing the $gp save/restore mechanism.

[Guillaume Emont]

(In reply to comment #3)
> Comment on attachment 269812 [details]
> Patch
> 
> I just took a look at the generated code in LLIntAssembly.h and this looks
> wrong to me.
> I mean, if this patch fixes crashes, there is a better way to do the fix as
> this patch is actually removing the $gp save/restore mechanism.

I might be missing a need for the $gp save/restore mechanism, but my understanding so far is that it is only needed when making a function call (jal/jalr), and I think that there are no other cases where $gp would get overwritten (LLInt itself does not seem to touch $gp apart from that mechanism and pichdr through cpload). Under that assumption, I don't think we need to save $gp at the beginning of each function, but rather only when we are calling another PIC function (would it be from LLInt or somewhere else), which is what this patch does. And indeed it makes sense to save it in a callee saved register, since if a function will modify it it would take care of saving it.

[Julien Brianceau]

Argh, I forgot that gnu as reorders MIPS opcodes, and I thought that the "move $gp, $s4"
instruction in the following code of LLIntAssembly.h was executed in the delay slot:

        ...
        move $s4, $gp
        jalr $t9
        move $gp, $s4
        ...

Whereas after compilation, it is reordered like this by the assembler:

        ...
        jalr $t9
        move $s4, $gp
        move $gp, $s4
        ...


I'll try to look at your patch more carefully if I have time, meanwhile I remove my r-

[Julien Brianceau]

Comment on attachment 269812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269812&action=review

> Source/JavaScriptCore/offlineasm/mips.rb:562
> +        preList << Instruction.new(operand.codeOrigin, "move", [MIPS_GP_REG, MIPS_GPSAVE_REG])

I'm not a huge fan of this. Instead, can you add something like ' emit "move $s4, $gp" ' at the end of the pushCalleeSaves macro ?
And also take the opportunity to set CalleeSaveRegisterCount to 1 for MIPS and just save/restore $s4 register instead of the $s0-$s4 range.

It seems to work fine on my mips board and slightly reduces the number of generated opcodes in LLInt.

[Guillaume Emont]

Created attachment 270073 [details]
Patch

New version of the patch that saves  to  in pushCalleeSaves() and also only push/pop

[Guillaume Emont]

(In reply to comment #7)
> Created attachment 270073 [details]
> Patch
> 
> New version of the patch that saves  to  in pushCalleeSaves() and also only
> push/pop

"saves to $s4". Got bitten by shell interpretation, I'll use single quotes next time ;).

[Julien Brianceau]

Comment on attachment 270073 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270073&action=review

Thanks that's great. Please just address my comment.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:433
>  elsif SH4 or MIPS

Let the sh4 port have its 5 registers :)

[Guillaume Emont]

Created attachment 270156 [details]
Patch

New version of the patch that doesn't change CalleeSaveRegisterCount for SH4

[Julien Brianceau]

Nice fix, LGTM

[Michael Saboff]

Comment on attachment 270156 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 270156 [details]
Patch

Clearing flags on attachment: 270156

Committed r195926: <http://trac.webkit.org/changeset/195926>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.