964258 - IonMonkey: Use mprotect for interrupt check on ARM

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

This is a follow-up of Bug 864220 comment 37 to stop using the InterruptCheck on IonCode generated for ARM.

Comment 1

[Douglas Crosher [:dougc]]

The SIGSEGV handling has problems on some ARM devices, so this will need to be a dynamic decision.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Base

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.

Comment 3

[Douglas Crosher [:dougc]]

Attachment: Use mprotect for interrupt check on ARM.

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.

Comment 4

[Douglas Crosher [:dougc]]

Attachment: Use mprotect for interrupt check on ARM.

There was another path that also needed to be enabled. Passes the tbpl jit-tests now.

Comment 5

[Douglas Crosher [:dougc]]

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

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 7

[Douglas Crosher [:dougc]]

Attachment: Use mprotect for interrupt check on ARM.

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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ecc1aa20f39

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ecc1aa20f39