OdinMonkey: signal handler support

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
This bug is for installing SIGSEGV handlers to catch out-of-bounds memory access from Odin-compiled asm.  The important thing is to avoid masking general seg faults (e.g., from breakpad) and generally play well with the rest of Mozilla, which also installs signal handlers.  Fortunately, OdinMonkey puts all the code from a single asm.js module into a single linear segment and maintains a dynamic activation stack of modules so a signal handler can easily check if the current pc is in the innermost module activation's range.  On out-of-bounds, the signal handler can patch in the correct value specified by JS (undefined|0 -> 0, +undefined -> NaN).

Once we have this basic support, there are a bunch of things we can optimize:
 - remove the operation callback check
 - remove the stack overflow check
 - remove the alignment mask for typed array loads by unmasking alignment faults
 - remove the idiv branches for X/0 and INT32_MIN/-1 by handling these signals
 - remove branching in ToInt by unmasking overflow exceptions in cvttsd2si
(Assignee)

Updated

5 years ago
Blocks: 840282
(Assignee)

Comment 1

5 years ago
Created attachment 713679 [details] [diff] [review]
WIP 1

This WIP patch handles SIGSEGV on linxu64.  Still more work to do to move the ArrayBuffer's memory into a new 4GB reserved space.

Mike, do you suppose you could give a quick look at AsmJSSignalHandlers.cpp and tell me if my usage of sigaction() looks reasonable?  I don't think I need a separate stack since my stack use is so minimal.
Attachment #713679 - Flags: feedback?(mh+mozilla)
(Assignee)

Updated

5 years ago
Depends on: 841617
(Assignee)

Updated

5 years ago
Depends on: 841619
(Assignee)

Comment 2

5 years ago
Created attachment 714189 [details] [diff] [review]
WIP 2

This patch works on Linux64 and contains/passes a bunch of tests.  (This is only for out-of-bounds; all the other things (slow script, stack overflow, alignment) will come after the initial landing.)
Attachment #713679 - Attachment is obsolete: true
Attachment #713679 - Flags: feedback?(mh+mozilla)
Attachment #714189 - Flags: review?(mh+mozilla)
(Assignee)

Comment 3

5 years ago
Comment on attachment 714189 [details] [diff] [review]
WIP 2

Oops, I meant f?.
Attachment #714189 - Flags: review?(mh+mozilla) → feedback?(mh+mozilla)
(Assignee)

Comment 4

5 years ago
Created attachment 714874 [details] [diff] [review]
WIP 3

This version should work on linux64 and osx.  The only difference from Linux is the naming of fields of the mcontext_t struct and the fact that access violations are SIGBUS (instead of SIGSEGV).

I'll close out this bug; the other platforms will come with their respective platform bugs.  I've also postponed all the exciting items in the list at the end of comment 0 until after initial landing.
Attachment #714189 - Attachment is obsolete: true
Attachment #714189 - Flags: feedback?(mh+mozilla)
Comment on attachment 714874 [details] [diff] [review]
WIP 3

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

Seems reasonable to me.

::: js/src/ion/AsmJSSignalHandlers.cpp
@@ +255,5 @@
> +    AsmJSActivation *activation = threadData->asmJSActivation;
> +    if (!activation)
> +        return false;
> +
> +#ifdef JS_CPU_X64

#ifdef HAVE_64BIT_OS ?

@@ +290,5 @@
> +
> +    // This signal is not for any asm.js code we expect, so we need to forward
> +    // the signal to the next handler. If there is no next handler (SIG_IGN or
> +    // SIG_DFL), then it's time to crash. To do this, we set the signal back to
> +    // it's previous disposition and return. This will cause the faulting op to

s/it's/its/
Attachment #714874 - Flags: feedback+
(Assignee)

Comment 6

5 years ago
Thanks Mike!  IonMonkey tends to use defined(JS_CPU_X64) throughout as the canonical 64-bit-ness test, so I think I should stick with that.
(Assignee)

Comment 7

5 years ago
Landed on the project repo:
http://hg.mozilla.org/users/lwagner_mozilla.com/odinmonkey/rev/a55682467286

Review will happen in bug 840282.
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.