Closed Bug 840280 Opened 11 years ago Closed 11 years ago

OdinMonkey: signal handler support


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: luke)




(1 file, 2 obsolete files)

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
Blocks: 840282
Attached patch WIP 1 (obsolete) — Splinter Review
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)
Depends on: 841617
Depends on: 841619
Attached patch WIP 2 (obsolete) — Splinter Review
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)
Comment on attachment 714189 [details] [diff] [review]

Oops, I meant f?.
Attachment #714189 - Flags: review?(mh+mozilla) → feedback?(mh+mozilla)
Attached patch WIP 3Splinter Review
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]

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

Attachment #714874 - Flags: feedback+
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.
Landed on the project repo:

Review will happen in bug 840282.
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.