IonMonkey: Use mprotect for interrupt check on ARM

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: nbp, Assigned: dougc)

Tracking

unspecified
mozilla33
ARM
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Posted 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: https://tbpl.mozilla.org/?tree=Try&rev=02e134caffd1
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.

https://tbpl.mozilla.org/?tree=Try&rev=b185982423d5
Assignee: nobody → dtc-moz
Attachment #8452986 - Attachment is obsolete: true
Attachment #8453106 - Attachment is obsolete: true
Attachment #8454836 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ecc1aa20f39
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1041079
No longer depends on: 1041079
Depends on: 1073911
You need to log in before you can comment on or make changes to this bug.