Pass the SIGPROF signal on to previously registered signal handler.

src/platform-linux.cc

Index: src/platform-linux.cc
diff --git a/src/platform-linux.cc b/src/platform-linux.cc
index ec48d6305e3de2ea4b51f0571a1c7a83bb0ae619..4217c11c2cfdf1d4e8e3bea9087778484a360402 100644
--- a/src/platform-linux.cc
+++ b/src/platform-linux.cc
@@ -1022,62 +1022,6 @@ static int GetThreadID() {
 }
 
 
-static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context) {
-  USE(info);
-  if (signal != SIGPROF) return;
-  Isolate* isolate = Isolate::UncheckedCurrent();
-  if (isolate == NULL || !isolate->IsInitialized() || !isolate->IsInUse()) {
-    // We require a fully initialized and entered isolate.
-    return;
-  }
-  if (v8::Locker::IsActive() &&
-      !isolate->thread_manager()->IsLockedByCurrentThread()) {
-    return;
-  }
-
-  Sampler* sampler = isolate->logger()->sampler();
-  if (sampler == NULL || !sampler->IsActive()) return;
-
-  TickSample sample_obj;
-  TickSample* sample = CpuProfiler::TickSampleEvent(isolate);
-  if (sample == NULL) sample = &sample_obj;
-
-  // Extracting the sample from the context is extremely machine dependent.
-  ucontext_t* ucontext = reinterpret_cast<ucontext_t*>(context);
-  mcontext_t& mcontext = ucontext->uc_mcontext;
-  sample->state = isolate->current_vm_state();
-#if V8_HOST_ARCH_IA32
-  sample->pc = reinterpret_cast<Address>(mcontext.gregs[REG_EIP]);
-  sample->sp = reinterpret_cast<Address>(mcontext.gregs[REG_ESP]);
-  sample->fp = reinterpret_cast<Address>(mcontext.gregs[REG_EBP]);
-#elif V8_HOST_ARCH_X64
-  sample->pc = reinterpret_cast<Address>(mcontext.gregs[REG_RIP]);
-  sample->sp = reinterpret_cast<Address>(mcontext.gregs[REG_RSP]);
-  sample->fp = reinterpret_cast<Address>(mcontext.gregs[REG_RBP]);
-#elif V8_HOST_ARCH_ARM
-#if defined(__GLIBC__) && !defined(__UCLIBC__) && \
-    (__GLIBC__ < 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ <= 3))
-  // Old GLibc ARM versions used a gregs[] array to access the register
-  // values from mcontext_t.
-  sample->pc = reinterpret_cast<Address>(mcontext.gregs[R15]);
-  sample->sp = reinterpret_cast<Address>(mcontext.gregs[R13]);
-  sample->fp = reinterpret_cast<Address>(mcontext.gregs[R11]);
-#else
-  sample->pc = reinterpret_cast<Address>(mcontext.arm_pc);
-  sample->sp = reinterpret_cast<Address>(mcontext.arm_sp);
-  sample->fp = reinterpret_cast<Address>(mcontext.arm_fp);
-#endif  // defined(__GLIBC__) && !defined(__UCLIBC__) &&
-        // (__GLIBC__ < 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ <= 3))
-#elif V8_HOST_ARCH_MIPS
-  sample->pc = reinterpret_cast<Address>(mcontext.pc);
-  sample->sp = reinterpret_cast<Address>(mcontext.gregs[29]);
-  sample->fp = reinterpret_cast<Address>(mcontext.gregs[30]);
-#endif  // V8_HOST_ARCH_*
-  sampler->SampleStack(sample);
-  sampler->Tick(sample);
-}
-
-
 class Sampler::PlatformData : public Malloced {
  public:
   PlatformData() : vm_tid_(GetThreadID()) {}
@@ -1089,6 +1033,9 @@ class Sampler::PlatformData : public Malloced {
 };
 
 
+static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context);
+
+
 class SignalSender : public Thread {
  public:
   enum SleepInterval {
@@ -1122,6 +1069,11 @@ class SignalSender : public Thread {
     }
   }
 
+  static void CallOldSignalHandler(int signal, siginfo_t* info, void* context) {
+    if (signal_handler_installed_ && old_signal_handler_.sa_sigaction)

[willchan no longer on Chromium, 2012/12/12 22:28:37]

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?

[danno, 2012/12/13 00:00:19]

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. :-)

On 2012/12/12 22:28:37, willchan wrote:

[willchan no longer on Chromium, 2012/12/13 00:15:58]

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.

+      old_signal_handler_.sa_sigaction(signal, info, context);

[Markus (顧孟勤), 2012/12/12 22:58:17]

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.

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.

[willchan no longer on Chromium, 2012/12/13 00:15:58]

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.

+  }
+
   static void AddActiveSampler(Sampler* sampler) {
     ScopedLock lock(mutex_);
     SamplerRegistry::AddActiveSampler(sampler);
@@ -1255,6 +1207,63 @@ struct sigaction SignalSender::old_signal_handler_;
 bool SignalSender::signal_handler_installed_ = false;
 
 
+static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context) {
+  USE(info);
+  if (signal != SIGPROF) return;

[Markus (顧孟勤), 2012/12/12 22:58:17]

This seems somewhat bogus. That really should never happen.

And if it does, then why do decide to skip calling the old signal handler. Isn't
that just papering over bugs.

Maybe a CHECK() or at least a DCHECK() would be more appropriate.

+  SignalSender::CallOldSignalHandler(signal, info, context);
+  Isolate* isolate = Isolate::UncheckedCurrent();
+  if (isolate == NULL || !isolate->IsInitialized() || !isolate->IsInUse()) {
+    // We require a fully initialized and entered isolate.
+    return;
+  }
+  if (v8::Locker::IsActive() &&
+      !isolate->thread_manager()->IsLockedByCurrentThread()) {
+    return;
+  }
+
+  Sampler* sampler = isolate->logger()->sampler();
+  if (sampler == NULL || !sampler->IsActive()) return;
+
+  TickSample sample_obj;
+  TickSample* sample = CpuProfiler::TickSampleEvent(isolate);
+  if (sample == NULL) sample = &sample_obj;
+
+  // Extracting the sample from the context is extremely machine dependent.
+  ucontext_t* ucontext = reinterpret_cast<ucontext_t*>(context);
+  mcontext_t& mcontext = ucontext->uc_mcontext;
+  sample->state = isolate->current_vm_state();
+#if V8_HOST_ARCH_IA32
+  sample->pc = reinterpret_cast<Address>(mcontext.gregs[REG_EIP]);
+  sample->sp = reinterpret_cast<Address>(mcontext.gregs[REG_ESP]);
+  sample->fp = reinterpret_cast<Address>(mcontext.gregs[REG_EBP]);
+#elif V8_HOST_ARCH_X64
+  sample->pc = reinterpret_cast<Address>(mcontext.gregs[REG_RIP]);
+  sample->sp = reinterpret_cast<Address>(mcontext.gregs[REG_RSP]);
+  sample->fp = reinterpret_cast<Address>(mcontext.gregs[REG_RBP]);
+#elif V8_HOST_ARCH_ARM
+#if defined(__GLIBC__) && !defined(__UCLIBC__) && \
+    (__GLIBC__ < 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ <= 3))
+  // Old GLibc ARM versions used a gregs[] array to access the register
+  // values from mcontext_t.
+  sample->pc = reinterpret_cast<Address>(mcontext.gregs[R15]);
+  sample->sp = reinterpret_cast<Address>(mcontext.gregs[R13]);
+  sample->fp = reinterpret_cast<Address>(mcontext.gregs[R11]);
+#else
+  sample->pc = reinterpret_cast<Address>(mcontext.arm_pc);
+  sample->sp = reinterpret_cast<Address>(mcontext.arm_sp);
+  sample->fp = reinterpret_cast<Address>(mcontext.arm_fp);
+#endif  // defined(__GLIBC__) && !defined(__UCLIBC__) &&
+        // (__GLIBC__ < 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ <= 3))
+#elif V8_HOST_ARCH_MIPS
+  sample->pc = reinterpret_cast<Address>(mcontext.pc);
+  sample->sp = reinterpret_cast<Address>(mcontext.gregs[29]);
+  sample->fp = reinterpret_cast<Address>(mcontext.gregs[30]);
+#endif  // V8_HOST_ARCH_*
+  sampler->SampleStack(sample);
+  sampler->Tick(sample);

[Markus (顧孟勤), 2012/12/12 22:58:17]

I don't see any code here that preserves "errno". That is broken. You will
almost certainly clobber this value and cause very subtle bugs.

All of that is ignoring the fact that SIGPROF is an asynchronous signal and I
suspect at least some of what you are calling is not async signal safe ... but I
haven't bothered going over that code with a fine-toothed comb. Maybe, somebody
else already did and it is in fact OK.

In general though, writing signal handlers in C++ is almost impossible to get
right.

+}
+
+
 void OS::SetUp() {
   // Seed the random number generator. We preserve microsecond resolution.
   uint64_t seed = Ticks() ^ (getpid() << 16);