906860 - Allow easily disabling asm.js signal handler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

For debugging, it is a pain to deal with nonfatal segmentation faults. It would be helpful to have an environment variable or command-line flag or something to disable the SEGV optimization.

Comment 1

[Luke Wagner [:luke]]

Yeah, this is annoying.  Note: it's not an issue on Mac (b/c the Mach/Unix interaction works to our advantage) or Windows (because of how vectored exception handlers work).  So, an alternative I've been thinking about for linux is to have a gdb script (auto-installed along w/ the SM pretty-printers) that told gdb not to break on SIGSEGV/SIGBUS if 'continue' would run a handler.

Comment 2

[Mike Hommey [:glandium]]

(In reply to Luke Wagner [:luke] from comment #1)
> that told gdb not to break on SIGSEGV/SIGBUS if 'continue'
> would run a handler.

Note that would stop working if we go on with bug 717538.

Comment 3

[Luke Wagner [:luke]]

(In reply to Mike Hommey [:glandium] from comment #2)
I don't know about breakpad+gdb interactions, but that should be orthogonal with the asm.js signal handlers since the asm.js signal handlers are installed at the thread-level whereas breakpad is at the process/task-level.

Comment 4

[Mike Hommey [:glandium]]

My point is that if you rely on whether 'continue' would run a handler, that's going to fail if breakpad is enabled by default, because there's always going to be a handler.

Comment 5

[Nicolas B. Pierron [:nbp]]

I was wondering, could we use some signal catchpoint[1] to execute a command which will resume the execution when the SEGV is able to continue through the exception handler?

[1] http://sourceware.org/gdb/onlinedocs/gdb/Set-Catchpoints.html#Set-Catchpoints

Comment 6

[Luke Wagner [:luke]]

(In reply to Mike Hommey [:glandium] from comment #4)
Oh, I see.  Well then we add an exception for the breakpad handler :)

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
That looks promising.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: WIP

Here is a solution that adds an auto-loaded gdb script that allows this behavior. It introduces a function called disable-mprotect-segfault (the name is quite long, but heh, everybody uses auto-completion, right? also if somebody has a better name in mind, I'll take it). 

When this function is called, all segfaults with the SEGV_ACCERR error code will be ignored. This error code corresponds to "invalid permissions for mapped object" [1], which is exactly the kind of segfaults we want to dismiss here.

I tried on several scripts and asserts are still correctly caught and stop the execution. I guess that if another program mprotects mapped memory and we call this function, then the handler is going to be muted too.

Not asking for review yet, as it introduces a WARNING during compilation, because there is no pre-processor directives in js-gdb.gdb. I didn't find any other way to do it and I am on my way to #build.

[1] http://man7.org/linux/man-pages/man2/sigaction.2.html

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix without the warning at build

Thanks to gps' help, I correctly fixed the warning at compilation.
Asking review to gps for build system changes and jimb for the gdb parts.

As nbp noticed, if disable-mprotect-segfault is called in the debugger, it will mask other memory protection segfaults, for instance if somebody memallocs memory and then tries to call it (as the user doesn't have the permissions to do it). After some time reading gdb's doc, I didn't find a way to dynamically check whether there is a handler attached to the signal or not, so if anybody knows how to do that (luke?), please take over this patch :)

Comment 9

[Jim Blandy :jimb]

Comment on attachment 793218 [details] [diff] [review]
fix without the warning at build

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

Wow, I didn't know about $_siginfo.

Comment 10

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
Are you able to run C code in the command (to call sigaction)?

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Better fix

(In reply to Luke Wagner [:luke] from comment #10)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> Are you able to run C code in the command (to call sigaction)?

Yes! So we can do everything directly in gdb actually: http://stackoverflow.com/questions/6892252/how-to-get-process-signal-information-in-gdb

Some explanation about the new version:
- regarding the build system: the file actually didn't get copied before; this version just symlinks the js-gdb.gdb script from the sources (glandium said on #build it's perfectly fine).
- the gdb script is now activated by default when running gdb. Maybe we would need to activate it only on Unix platforms?
- when catching SIG_SEGV, gdb will allocate a sigaction struct if there is no already allocated, then call the function sigaction (__GI___sigaction) on it and check whether the associated handler is the AsmJSFaultHandler. If it's the case, it will continue silently, otherwise it will stop as usual. This way, the patch shouldn't break breakpad.
- hookpost-run is automatically run after the command 'run' has finished. This happens either before a SIG_SEGV is caught (so the hook is called before the catch signal commands) or when it terminates. In all cases I've tested (run without issue, run with only asm.js segfault, run with 'regular' segfault), there's no memory corruption.

Asking jimb for review again as the patch is completely different (sorry about that!).

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 793770 [details] [diff] [review]
Better fix

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

Just for the build bits.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f764ef54601

So, this should work most of the cases. However, as it potentially can call malloc every time a signal is caught, there are some chances that the code is already calling malloc, which could cause an unsafe / unknown behaviour (potentially iloop). This can happen when debugging a parallel or a single-threaded shell.

If this case were to happen often, we should find an alternative.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f764ef54601

Comment 15

[Andrew McCreight [:mccr8]]

How do you make use of this if you are debugging the browser on Linux and never build the shell?

Comment 16

[Andrew McCreight [:mccr8]]

Whatever approach it is should also be documented somewhere outside of this bug, maybe including whatever bit of code you hit if you don't work around it, like "Hey, are you hitting this in GDB?  This isn't a real crash.  Do X to work around it." so we're at least somewhat self-documenting.

Comment 17

[Luke Wagner [:luke]]

Unfortunately, the site of the crash is usually some random Ion code or some random C++ location touching Ion code.  One idea (which is what I usually do) is to have a browser pref that dynamically disables the mprotecting that happens in triggerOperationCallback so that it's easy to flip it off.

Comment 18

[Andrew McCreight [:mccr8]]

I filed bug 933807 for fixing the browser case.