Bug 1394286 (performance)

mips performance segment fault

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: qiaopengcheng-hf, Assigned: qiaopengcheng-hf, Mentored)

Tracking

57 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Posted patch fixed segment fault (obsolete) — Splinter Review
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.
(Assignee)

Comment 1

2 years ago
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.
Mentor: gtatum
Severity: normal → critical
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Component: Untriaged → Developer Tools: Performance Tools (Profiler/Timeline)
Priority: -- → P1
Resolution: --- → INVALID
(Assignee)

Comment 2

2 years ago
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
Attachment #8901639 - Attachment is patch: true
Attachment #8901639 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Updated

2 years ago
Alias: performance
Resolution: INVALID → WORKSFORME
(Assignee)

Updated

2 years ago
Mentor: sledru
(Assignee)

Updated

2 years ago
Mentor: petr.sumbera
(Assignee)

Comment 3

2 years ago
add the new patch file.
Attachment #8902501 - Flags: review?(sledru)
Attachment #8902501 - Flags: review?(petr.sumbera)
Comment on attachment 8902501 [details] [diff] [review]
performance.patch

I am not a proper reviewer for that, sorry!
Attachment #8902501 - Flags: review?(sledru)
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8902501 - Flags: review?(gtatum)

Comment 6

2 years ago
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.
Attachment #8902501 - Flags: review?(petr.sumbera) → review+
(Assignee)

Comment 7

2 years ago
(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

2 years ago
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.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

2 years ago
(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.
This needs review from an XPCOM peer before it can land.
Assignee: nobody → qiaopengcheng-hf
Status: RESOLVED → REOPENED
Component: Developer Tools: Performance Tools (Profiler/Timeline) → XPCOM
Ever confirmed: true
Keywords: checkin-needed
Product: Firefox → Core
Resolution: WORKSFORME → ---
Status: REOPENED → ASSIGNED
Attachment #8901639 - Attachment is obsolete: true
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.
Attachment #8902501 - Flags: review?(gtatum) → review+
No try run needed to check this in, since this is NPOTB.
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f28ce958895f
mips64: fix error correction about stackpointer within function _NS_InvokeByIndex. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f28ce958895f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.