Closed Bug 1250031 Opened 4 years ago Closed 4 years ago

IonMonkey: MIPS: Fix ion/bug1233343.js crash

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: hev, Assigned: hev)

Details

Attachments

(1 file, 1 obsolete file)

Fix ion/bug1233343.js and debug/Debugger-debuggees-28.js crash on MIPS.
Attachment #8721841 - Flags: review?(hv1989)
Comment on attachment 8721841 [details] [diff] [review]
0001-Bug-1250031-IonMonkey-MIPS-Fix-ion-bug1233343.js-cra.patch

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

We need to have the return address on the stack here.
It is required for stack walking, bailing in ion ...

IIUC this change, it will remove the return address from the stack and only keep the descriptor...
Attachment #8721841 - Flags: review?(hv1989)
(In reply to Hannes Verschore [:h4writer] from comment #2)
> Comment on attachment 8721841 [details] [diff] [review]
> 0001-Bug-1250031-IonMonkey-MIPS-Fix-ion-bug1233343.js-cra.patch
> 
> Review of attachment 8721841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We need to have the return address on the stack here.
> It is required for stack walking, bailing in ion ...
> 
> IIUC this change, it will remove the return address from the stack and only
> keep the descriptor...

The MIPS doesn't use JS_USE_LINK_REGISTER, so I think we don't need the return address on stack that similar to x64.
(In reply to Heiher [:hev] from comment #3)
> (In reply to Hannes Verschore [:h4writer] from comment #2)
> > Comment on attachment 8721841 [details] [diff] [review]
> > 0001-Bug-1250031-IonMonkey-MIPS-Fix-ion-bug1233343.js-cra.patch
> > 
> > Review of attachment 8721841 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We need to have the return address on the stack here.
> > It is required for stack walking, bailing in ion ...
> > 
> > IIUC this change, it will remove the return address from the stack and only
> > keep the descriptor...
> 
> The MIPS doesn't use JS_USE_LINK_REGISTER, so I think we don't need the
> return address on stack that similar to x64.

Why does MIPS not set "JS_USE_LINK_REGISTER" ? It uses a link register instead of pushing the return address, right?
(In reply to Hannes Verschore [:h4writer] from comment #4)
> (In reply to Heiher [:hev] from comment #3)
> > (In reply to Hannes Verschore [:h4writer] from comment #2)
> > > Comment on attachment 8721841 [details] [diff] [review]
> > > 0001-Bug-1250031-IonMonkey-MIPS-Fix-ion-bug1233343.js-cra.patch
> > > 
> > > Review of attachment 8721841 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > We need to have the return address on the stack here.
> > > It is required for stack walking, bailing in ion ...
> > > 
> > > IIUC this change, it will remove the return address from the stack and only
> > > keep the descriptor...
> > 
> > The MIPS doesn't use JS_USE_LINK_REGISTER, so I think we don't need the
> > return address on stack that similar to x64.
> 
> Why does MIPS not set "JS_USE_LINK_REGISTER" ? It uses a link register
> instead of pushing the return address, right?

I don't clear this is why, I kept it on mips64 port from mips32. The return address is saved in ra register on linked call in MIPS. I'm sure, currently the MIPS backend doesn't set JS_USE_LINK_REGISTER, and explicit push return address to stack.
(In reply to Hannes Verschore [:h4writer] from comment #4)
> (In reply to Heiher [:hev] from comment #3)
> > (In reply to Hannes Verschore [:h4writer] from comment #2)
> > > Comment on attachment 8721841 [details] [diff] [review]
> > > 0001-Bug-1250031-IonMonkey-MIPS-Fix-ion-bug1233343.js-cra.patch
> > > 
> > > Review of attachment 8721841 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > We need to have the return address on the stack here.
> > > It is required for stack walking, bailing in ion ...
> > > 
> > > IIUC this change, it will remove the return address from the stack and only
> > > keep the descriptor...
> > 
> > The MIPS doesn't use JS_USE_LINK_REGISTER, so I think we don't need the
> > return address on stack that similar to x64.
> 
> Why does MIPS not set "JS_USE_LINK_REGISTER" ? It uses a link register
> instead of pushing the return address, right?

MIPS push return addresses caller-side, in linked call branch delay solt.
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/mips-shared/MacroAssembler-mips-shared.cpp#1250
> void
> MacroAssembler::callAndPushReturnAddress(Register callee)
> {
>     // Push return address during jalr delay slot.
>     subPtr(Imm32(sizeof(intptr_t)), StackPointer);
>     as_jalr(callee);
>     storePtr(ra, Address(StackPointer, 0));
> }
Attachment #8721841 - Flags: review?(hv1989)
Comment on attachment 8721841 [details] [diff] [review]
0001-Bug-1250031-IonMonkey-MIPS-Fix-ion-bug1233343.js-cra.patch

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

I wasn't able to compile the 32bits mips simulator and couldn't test it.

Now I'm pretty sure what the problem is:
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/SharedIC.cpp#735

Just like ARM, MIPS is using the link register (even if the MIPS build is lying by not setting JS_USE_LINK_REGISTER).
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/mips-shared/SharedICHelpers-mips-shared.h#49
=> no call that pushes the link register to the stack!


Now the JS_USE_LINK_REGISTER define, is to know in which case we are.
In x86, x64 this isn't defined, since the return address is always on the stack.
This isn't the case for arm, mips. As a result we try to minimize the usage of the return address on the stack.
Though there are some places we need it on the stack for introspection.
Everywhere we check for 'JS_USE_LINK_REGISTER' it is to know if we need to push the return address.
And I have no idea how MIPS managed to ignore this totally. There so many places where we need to have the return address on the stack.

So in the long term I would suggest to get a patch that defines JS_USE_LINK_REGISTER=1.

In the short term I would also be happy to r+ a patch that would adjust this:
https://dxr.mozilla.org/mozilla-central/source/js/src/jit/SharedIC.cpp#735
and special case to not do this for MIPS.
Attachment #8721841 - Flags: review?(hv1989)
(In reply to Hannes Verschore [:h4writer] from comment #7)
> Comment on attachment 8721841 [details] [diff] [review]
> 0001-Bug-1250031-IonMonkey-MIPS-Fix-ion-bug1233343.js-cra.patch
> 
> Review of attachment 8721841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wasn't able to compile the 32bits mips simulator and couldn't test it.
> 
> Now I'm pretty sure what the problem is:
> https://dxr.mozilla.org/mozilla-central/source/js/src/jit/SharedIC.cpp#735
> 
> Just like ARM, MIPS is using the link register (even if the MIPS build is
> lying by not setting JS_USE_LINK_REGISTER).
> https://dxr.mozilla.org/mozilla-central/source/js/src/jit/mips-shared/
> SharedICHelpers-mips-shared.h#49
> => no call that pushes the link register to the stack!
> 
> 
> Now the JS_USE_LINK_REGISTER define, is to know in which case we are.
> In x86, x64 this isn't defined, since the return address is always on the
> stack.
> This isn't the case for arm, mips. As a result we try to minimize the usage
> of the return address on the stack.
> Though there are some places we need it on the stack for introspection.
> Everywhere we check for 'JS_USE_LINK_REGISTER' it is to know if we need to
> push the return address.
> And I have no idea how MIPS managed to ignore this totally. There so many
> places where we need to have the return address on the stack.
> 
> So in the long term I would suggest to get a patch that defines
> JS_USE_LINK_REGISTER=1.
Thank you. I'll try to do it.

> 
> In the short term I would also be happy to r+ a patch that would adjust this:
> https://dxr.mozilla.org/mozilla-central/source/js/src/jit/SharedIC.cpp#735
> and special case to not do this for MIPS.
Thank you.
Attachment #8721841 - Attachment is obsolete: true
Attachment #8724610 - Flags: review?(hv1989)
Attachment #8724610 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/b1806f5f3b81
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.