1394286 - mips performance segment fault

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[qiaopengcheng]

Attachment: fixed segment fault

User Agent: Mozilla/5.0 (X11; Linux mips64-loongson; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20170825111631

Steps to reproduce:

1.   firefox menu ---> Developer ---> Performance ,
or   by hotkey  "Shift  +  F5",

2. selecting  "Performance"

3. Start  Recording Performance.


Actual results:


Program received signal SIGSEGV, Segmentation fault.


Expected results:

good running.

Comment 1

[qiaopengcheng]

on mips64 platform, this file,

xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_mips64.S


has been modified for fixing the bug.

original code will generate a segment fault if interruption happens after line70(REG_L    sp, 0(sp)     # get orig sp back), the information saved on stack  will be discarded after interruption handler return.

Comment 2

[qiaopengcheng]

Comment on attachment 8901639 [details] [diff] [review]
fixed segment fault

>diff --git a/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_mips64.S b/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_mips64.S
>index eef34de..d2c5595 100644
>--- a/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_mips64.S
>+++ b/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_mips64.S
>@@ -67,10 +67,10 @@ NESTED(_NS_InvokeByIndex, FRAMESZ, ra)
>     jal      invoke_copy_to_stack
> 
>     REG_L    t3, 8(sp)     # get previous a0
>-    REG_L    sp, 0(sp)     # get orig sp back
>+    REG_L    s0, 0(sp)     # get orig sp back and save away our stack pointer
> 
>-    REG_L    a0, A0OFF(sp) # a0 - that
>-    REG_L    a1, A1OFF(sp) # a1 - methodIndex
>+    REG_L    a0, A0OFF(s0) # a0 - that
>+    REG_L    a1, A1OFF(s0) # a1 - methodIndex
> 
>     # t1 = methodIndex * pow(2, PTRLOG)
>     # (use shift instead of mult)
>@@ -105,13 +105,12 @@ NESTED(_NS_InvokeByIndex, FRAMESZ, ra)
>     l.d      $f18, 40(t1)
>     l.d      $f19, 48(t1)
> 
>-    # save away our stack pointer and create
>-    # the stack pointer for the function
>-    move     s0, sp
>+    # create the stack pointer for the function
>     move     sp, t3
> 
>     jalr     t9
> 
>+    ## restore stack pointer.
>     move     sp, s0
> 
>     RESTORE_GP64

Comment 3

[qiaopengcheng]

Attachment: performance.patch

add the new patch file.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8902501 [details] [diff] [review]
performance.patch

I am not a proper reviewer for that, sorry!

Comment 5

[qiaopengcheng]

(In reply to Sylvestre Ledru [:sylvestre] from comment #4)
> Comment on attachment 8902501 [details] [diff] [review]
> performance.patch
> 
> I am not a proper reviewer for that, sorry!

Ok, Thank you.

Comment 6

[Petr Sumbera]

Comment on attachment 8902501 [details] [diff] [review]
performance.patch

Review of attachment 8902501 [details] [diff] [review]:
-----------------------------------------------------------------

I also don't think I'm the best reviewer as I don't really know MIPS assembler. Plus I have never done review before.

>+    ## restore stack pointer.

One '#' should be enough here.

Hg commit message should also include bug number:

Bug 1394286 - mips64: fix error correction about stackpointer within function _NS_InvokeByIndex

With that fixed I would be ok with it.

Comment 7

[qiaopengcheng]

(In reply to Petr Sumbera from comment #6)
> Comment on attachment 8902501 [details] [diff] [review]
> performance.patch
> 
> Review of attachment 8902501 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I also don't think I'm the best reviewer as I don't really know MIPS
> assembler. Plus I have never done review before.
> 
> >+    ## restore stack pointer.
> 
> One '#' should be enough here.
> 
> Hg commit message should also include bug number:
> 
> Bug 1394286 - mips64: fix error correction about stackpointer within
> function _NS_InvokeByIndex
> 
> With that fixed I would be ok with it.

Ok, Thanks a lot.
Now, I'm porting the performance/profiler module on mips64 platform.
Next time, I'll rename patch by the formate.

Comment 8

[James Cowgill]

On the technical MIPS side, I think this patch is correct.

At the moment the "REG_L sp, 0(sp)" will load the original stack pointer from before all the argument areas are allocated. If we then take a signal, the signal handler will clobber the argument areas. When the signal handler returns, we later on load values from these areas which are now filled with garbage. Since we need the original stack pointer to end up in s0 later on, it makes sense to just load this value into s0 in the first place. I don't think there's a particular reason it needs to be loaded into sp.

Comment 9

[qiaopengcheng]

(In reply to James Cowgill from comment #8)
> On the technical MIPS side, I think this patch is correct.
> 
> At the moment the "REG_L sp, 0(sp)" will load the original stack pointer
> from before all the argument areas are allocated. If we then take a signal,
> the signal handler will clobber the argument areas. When the signal handler
> returns, we later on load values from these areas which are now filled with
> garbage. Since we need the original stack pointer to end up in s0 later on,
> it makes sense to just load this value into s0 in the first place. I don't
> think there's a particular reason it needs to be loaded into sp.


Yes.
Now, I'm porting the performance/profiler module on mips64 platform.
But always meeting "Program received signal SIGSEGV, Segmentation fault.".
After fixed this bug, performance module is OK.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This needs review from an XPCOM peer before it can land.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8902501 [details] [diff] [review]
performance.patch

Review of attachment 8902501 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this!  You can tell it's been a while since anybody tried things on a MIPS64 platform. :)

This patch is OK.

Comment 12

[Nathan Froyd [:froydnj]]

No try run needed to check this in, since this is NPOTB.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f28ce958895f
mips64: fix error correction about stackpointer within function _NS_InvokeByIndex. r=froydnj

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f28ce958895f