Closed
Bug 1135903
Opened 9 years ago
Closed 9 years ago
Odinmonkey: signal-handling OOB cleanups
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: sunfish, Assigned: sunfish)
Details
Attachments
(2 files, 1 obsolete file)
1.30 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
27.50 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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 3•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 4•9 years ago
|
||
The si_code patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f5d952aff5
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
(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
Assignee | ||
Updated•9 years ago
|
Whiteboard: [leave open]
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/130f0523865d
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•