Bug 153463

Created attachment 269812 [details] Patch

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

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.

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

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-

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.

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

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

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

Created attachment 270156 [details] Patch New version of the patch that doesn't change CalleeSaveRegisterCount for SH4

Nice fix, LGTM

Comment on attachment 270156 [details] Patch r=me

Comment on attachment 270156 [details] Patch Clearing flags on attachment: 270156 Committed r195926: <http://trac.webkit.org/changeset/195926>

All reviewed patches have been landed. Closing bug.