Closed Bug 1135903 Opened 9 years ago Closed 9 years ago

Odinmonkey: signal-handling OOB cleanups

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: sunfish, Assigned: sunfish)

Details

Attachments

(2 files, 1 obsolete file)

While working on bug 986981, I made a few cleanup patches.

The first is to check the si_code code in the siginfo structure, on Unix platforms, so that we don't try to handle SIGSEGV signals originating from sources other than memory faults.
Attachment #8568216 - Flags: review?(luke)
This patch introduces a ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB feature macro, and uses it instead of hardcoding checks for x64 everywhere. This ought to make it easier to extend signal-handling OOB checking to other platforms which can support it. Also, this patch provides a clean way to make the registering of SIGSEGV handlers conditional, so with this patch we no longer register the asm.js SIGSEGV handlers on platforms which don't use them.
Attachment #8568222 - Flags: review?(luke)
Comment on attachment 8568216 [details] [diff] [review]
check-si_code.patch

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

::: js/src/asmjs/AsmJSSignalHandlers.cpp
@@ +1091,5 @@
> +    // mprotected memory. If the signal originates anywhere else, don't try
> +    // to handle it.
> +    MOZ_RELEASE_ASSERT(signum == SIGSEGV);
> +    if (info->si_code != SEGV_ACCERR)
> +        return false;

Nice.  Can we put this at the top of the function?  It's feels pre-condition-y.
Attachment #8568216 - Flags: review?(luke) → review+
Comment on attachment 8568222 [details] [diff] [review]
refactor-signal-handlers-for-oob.patch

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

::: js/src/asmjs/AsmJSModule.h
@@ +25,5 @@
>  
>  #include "jsscript.h"
>  
>  #include "asmjs/AsmJSFrameIterator.h"
> +#include "asmjs/AsmJSSignalHandlers.h" // for ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB

I assume the reason you're putting in this comment is so noone tries to remove the header and everything compiles but is silently broken.  How about instead we move this to somewhere central that we know is included everywhere, like where we define JS_CODEGEN_*?

::: js/src/asmjs/AsmJSSignalHandlers.cpp
@@ +350,5 @@
>       return reinterpret_cast<uint8_t**>(&PC_sig(context));
>  #endif
>  }
>  
> +#if defined(ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB)

I feel like you could wrap almost all of AsmJSSignalHandlers.cpp in this #ifdef instead of more selective parts (I've kinda felt guilty about not doing this for a while).  The only thing that's not #ifdef'd is the Ion part of InterruptRunningJitCode.  Maybe we could move InterruptRunningIonCode to Ion.cpp and make the whole of AsmJSSignalHandlers.cpp be #idef ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB?
Whiteboard: [leave open]
Comment on attachment 8568222 [details] [diff] [review]
refactor-signal-handlers-for-oob.patch

Clearing for now b/c I think something new is brewing.
Attachment #8568222 - Flags: review?(luke)
Something else is brewing, but this refactoring patch is still a useful first step. I moved the macro definitions as requested above.

I didn't yet clean up AsmJSSignalHandlers.cpp as requested above, but nothing in this patch makes that cleanup harder.
Attachment #8568222 - Attachment is obsolete: true
Attachment #8580761 - Flags: review?(luke)
Comment on attachment 8580761 [details] [diff] [review]
asmjs-refactor-signal-handler-oob.patch

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

::: js/src/configure.in
@@ +3150,5 @@
>  elif test "$CPU_ARCH" = "x86_64"; then
>      AC_DEFINE(JS_CODEGEN_X64)
>      JS_CODEGEN_X64=1
> +
> +    dnl Single-handler OOM checking requires large mprotected guard regions, so

s/Single/signal/

::: js/src/jit/MIRGraph.cpp
@@ +46,2 @@
>      usesSignalHandlersForAsmJSOOB_(usesSignalHandlersForAsmJSOOB),
> +#endif

Should the decl in MIRGenerator.h also be #ifdef'd?
Attachment #8580761 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #8)
> Comment on attachment 8580761 [details] [diff] [review]
> asmjs-refactor-signal-handler-oob.patch
> 
> Review of attachment 8580761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/configure.in
> @@ +3150,5 @@
> >  elif test "$CPU_ARCH" = "x86_64"; then
> >      AC_DEFINE(JS_CODEGEN_X64)
> >      JS_CODEGEN_X64=1
> > +
> > +    dnl Single-handler OOM checking requires large mprotected guard regions, so
> 
> s/Single/signal/

Fixed.

> ::: js/src/jit/MIRGraph.cpp
> @@ +46,2 @@
> >      usesSignalHandlersForAsmJSOOB_(usesSignalHandlersForAsmJSOOB),
> > +#endif
> 
> Should the decl in MIRGenerator.h also be #ifdef'd?

It already was.

https://hg.mozilla.org/integration/mozilla-inbound/rev/130f0523865d
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/130f0523865d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.