Reopened. I'll hopefully upload a patch later today.
https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc#newcode1073 src/platform-linux.cc:1073: if (signal_handler_installed_ && old_signal_handler_.sa_sigaction) On 2012/12/13 00:00:19, danno wrote:
Go ahead and re-open this one, since I'd like to make sure that all of
the
knowledge how to fix this including Markus' comments are in one place.
After
what he wrote, however, I feel completely unqualified to review this
patch. :-) Don't worry, that's exactly why I added Markus to the code review so he could tell me everything I'm doing wrong :) Just need you to doublecheck we're not doing anything stupid with regards to V8, since we're both foreigners here.
On 2012/12/12 22:28:37, willchan wrote: > old_signal_handler_.sa_sigaction is 0x1 in the crash. Apparently
this is the
> magic value SIG_IGN or something. I'm adding markus@ to the code
review so he
> can educate me on what I'm actually supposed to be doing here to
make V8's
> SIGPROF handler and TCMalloc's SIGPROF handler play nice. Then I'll
fix this.
> > danno/v8-dev: Should I reopen this issue and upload new patches
here? Or
should > I open a new issue?
https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc#newcode1074 src/platform-linux.cc:1074: old_signal_handler_.sa_sigaction(signal, info, context); On 2012/12/12 22:58:17, Markus (顧孟勤) wrote:
If you want to do this right, things get a lot more complicated. In
practice,
you can probably skip some of the more complicated corner-cases and
you'll be
OK.
Yeah, I'm going to skip almost all the corner cases. Make things fail loudly when people start doing unexpected things, but otherwise I'll just try to make sure Crankshaft and google-perftools play nice.
You ideally should check the signal mask first. If the signal was
originally
masked, then no matter what you do, it would never be raised.
Of course, inside of the signal handler, this is all too late. The
signal was
obviously unmasked, because otherwise you would never be inside your
own signal
handler. It might in fact be masked right now, though, as that happens
unless
you set the SA_NODEFER flag.
Once you know that signal delivery was in fact expected, you now have
to
determine what the signal disposition is. For that, you should check
whether
SA_SIGINFO was set in sa_flags.
If so, then your code is correct. You can just call the function in sa_sigaction.
If it wasn't set though, you are looking at a signal that behaves as
if it had
been set with the old-school signal(2) function. This means a) you
have to look
at sa_handler rather than at sa_sigaction, and b) you have to also be
prepared
to handle SIG_IGN and SIG_DFL.
These are magic constants and you can just compare them directly
against the
value of sa_handler.
If it is SIG_IGN, things are easy. You don't need to do anything. If
it is
SIG_DFL, things are a little more complicated. You now need to check
what the
default signal disposition is; the man page for signal(7) tells you.
In the case of SIGPROF, it terminates the program. You could just call
_exit()
(please note the underscore!). But the exit code won't be quite right.
If you
need the correct exit code, make sure that your SIGPROF signal is
masked (as it
would be unless you set SA_NODEFER) and then raise SIGPROF again. This
will
result in the expect exit code.
Only, there is another complication. I don't believe that SIGPROF is a synchronous signal, and signal masks are a per-thread property. So,
SIGPROF is
most likely unmasked in another thread and the kernel could very well
decide to
deliver it there. Rather than calling "raise(SIGPROF)", you might want
to do
"tgkill(-1, gettid(), SIGPROF)". I don't know whether we currently
expose
"gettid()"; if we don't, then use "syscall(__NR_gettid)".
Now that you know how to handle SIG_IGN and SIG_DFL, the remaining
question is
what to do if sa_handler points to an actual function.
This is the most difficult problem to solve, as the two different
types of
signal handlers expect completely different stack frames (at least on
some
architectures). You can (often) convert one into the other; if I
recall
correctly, a sigaction-style stack frame is technically a superset of
a
sighandler-style stack frame. This is all highly specific to the
target platform
though. And it is of course Linux specific; POSIX doesn't allow for
any of what
you are doing here.
But I suspect we are running into some diminishing returns. As you
mentioned
earlier, it is hopefully safe to assume that both handlers (if present
at all)
are sigaction-style handlers.
I'll leave it up to you to decide what to do if this last assumption
doesn't
hold. You could fail with a CHECK() or at least a DCHECK(). Or you
could simply
decide to do what we did before and ignore the old handler.
You could also do crazy things with repointing the signal handler and
then
rethrowing the signal; POSIX would be happier if you did that. But
again, that's
probably more trouble than what it's worth. And I don't even want to
know what
the associated unittest is going to look like (you do plan on testing
this,
don't you?).
Talking about CHECK() and DCHECK(). Neither one of them is technically
safe to
be called at this time and could quite conceivably result in a dead
lock or
worse. Hopefully, it's a rare case and we are OK with very rare
inexplicable
bugs; but you might still want to document this.
And just for completeness sake, there is one last bit of information.
This is
probably not something that you'll run into though.
Chaining of signal handlers interposes an additional stack frame. This
break any
call to sigreturn(), and it could also break other low-level code.
Fortunately,
you are unlikely to see that in any signal handler that is written in
a
high-level language. But if you wanted to be really precise, you would
pop your
own stack frame prior to chaining.
https://codereview.chromium.org/11195045/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
