Closed Bug 964258 Opened 10 years ago Closed 10 years ago

IonMonkey: Use mprotect for interrupt check on ARM


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: nbp, Assigned: dougc)




(1 file, 3 obsolete files)

This is a follow-up of Bug 864220 comment 37 to stop using the InterruptCheck on IonCode generated for ARM.
The SIGSEGV handling has problems on some ARM devices, so this will need to be a dynamic decision.
Attached patch Base (obsolete) — Splinter Review
Just wanted to give this a try. This patch needs be applied after patches in bug 986673. There is quickly a lot of jit failures with this form:

Assertion failure: jump->is<InstBranchImm>() || jump->is<InstLDR>(), at /home/ben/code/moz/inbound/js/src/jit/arm/Assembler-arm.cpp:523

Not sure how to debug that (gdb doesn't know the "is" method nor any parametrized variants), so just letting my patch here, in case somebody would like to take it on.
Thank you for the reminder. The offsets need converting to their actual offset for the ARM. This bug would appear to stand on its own without the need for the patches from bug 986673. Shall run some more tests.
There was another path that also needed to be enabled. Passes the tbpl jit-tests now.
Attachment #8453071 - Attachment is obsolete: true
Comment on attachment 8453106 [details] [diff] [review]
Use mprotect for interrupt check on ARM.

bhacket: were you happy with the other parts of the ARM support in bug 864220 or does it all need scrutiny?

benj: can you spot any issues from your experience working on bug 986673?

It passes tbpl jit-tests locally, and I'll continue testing locally, but it seems ok so far and here is a try run:
Attachment #8453106 - Flags: review?(bhackett1024)
Attachment #8453106 - Flags: review?(benj)
Comment on attachment 8453106 [details] [diff] [review]
Use mprotect for interrupt check on ARM.

Review of attachment 8453106 [details] [diff] [review]:

Great, looks all good to me! Thanks for doing that.

::: js/src/jit/Ion.cpp
@@ +1042,3 @@
>          CodeLocationJump backedge(code, info.backedge);
> +        CodeLocationLabel loopHeader(code, CodeOffsetLabel(masm.actualOffset(info.loopHeader->offset())));
> +        CodeLocationLabel interruptCheck(code, CodeOffsetLabel(masm.actualOffset(info.interruptCheck->offset())));

style nit: could you use local variables to reduce line width please, for these two lines?
Attachment #8453106 - Flags: review?(benj) → review+
Attachment #8453106 - Flags: review?(bhackett1024) → review+
Address reviewer feedback. Thank you for the quick reviews.

Shall be interesting so see if there are measurable performance improvements on AWFY.
Assignee: nobody → dtc-moz
Attachment #8452986 - Attachment is obsolete: true
Attachment #8453106 - Attachment is obsolete: true
Attachment #8454836 - Flags: review+
Keywords: checkin-needed
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.