https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc File src/platform-linux.cc (right):
https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc#newcode1036 src/platform-linux.cc:1036: static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context); Our style guide for some reason prefers that we don't mark symbols as static. Instead, we should put them into an anonymous namespace. I don't have particularly strong feelings either way, so use your own best judgement. https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc#newcode1074 src/platform-linux.cc:1074: if (sigismember(&old_signal_handler_.sa_mask, SIGPROF)) I don't believe this is the correct mask. See below for an explanation. You should use sigprocmask() to store the old signal mask when you use sigaction() to set up your signal handler. And in fact, you really can't rely on the pre-existing signal mask, so you really need to make sure you unmask the signal when you originally set up the signal handler. I believe this is a pre-existing bug in this code. Please note that this is somewhat failure prone, if you already launched threads. Unlike sigaction(), which makes a global change, sigprocmask() changes a per-thread property. You are kind of out of luck, if there are other threads around that also require their signal mask to be adjusted. I don't know enough about V8 to tell you whether this is a problem or not. Background information follows: "old_signal_handler_.sa_mask" is the mask that should be applied each time a SIGPROF is being handled. For example, you could use this feature to prevent SIGALRM from triggering a signal handler while you are already inside of a SIGPROF handler. Once your SIGPROF signal handler returns, the mask will be restored to the value that it had prior to entry into the signal handler. And any delayed signals will then be handled. So, you are not actually losing the pending SIGALRM -- this only ever works for a single signal though; queuing of signal doesn't (typically) happen. This means, sa_mask gives you control over whether you want to nest signal handlers, and if so, which signal handlers can validly be nested. In most use cases, people clear the mask. This results in all nesting of signal handlers to be allowed. The only nesting that is not allowed is a repeat of the same signal (i.e. you won't try to handle another SIGPROF while you are already handling the first one). If you also want to allow nesting of identical signals, then you would have to set the SA_NODEFER flag. But that's often difficult to get right. So, don't do that unless you really need it. https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc#newcode1077 src/platform-linux.cc:1077: // It's safe to ignore the old signal handler if it's one of these values. Your code is arguably the right thing to do ... even though it is technically incorrect. signal(7) tells you that the default disposition for SIGPROF is to terminate the process. In other words, the technically correct solution would require for you to exit if you see that sa_handler used to be SIG_DFL. In practice, this is probably not what you want to do. If nobody ever used profiling, then it is quite likely that sa_handler has been unchanged and is in fact SIG_DFL. And that just didn't cause any problems, because nobody ever raised SIGPROF before we came along. So, yes, ignoring SIG_DFL is the sanest solution. You might just want to add a more verbose comment explaining why it is sane. Maybe, something along the lines: If the old signal disposition was SIG_IGN, then it is safe to ignore the old signal handler. If it is SIG_DFL, then that is pretty strong evidence that nobody other than us is using the profiling infrastructure. It is probably reasonable to assume that in this case we don't need to chain to the old behavior, as that would merely result in the application getting terminated. https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc#newcode1225 src/platform-linux.cc:1225: if (signal != SIGPROF) return; You should probably restore "errno" for each of the possible exit paths of this function. Maybe, for once, it makes sense to use "goto" to jump to a common exit handler at the end of the function? https://codereview.chromium.org/11195045/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
