Pass the SIGPROF signal on to previously registered signal handler.

src/platform-linux.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 1004 matching lines @@
 #if defined(__ANDROID__)
   // Android's C library provides gettid(2).
   return gettid();
 #else
   // Glibc doesn't provide a wrapper for gettid(2).
   return syscall(SYS_gettid);
 #endif
 }
 
 
-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()) {}
 
   int vm_tid() const { return vm_tid_; }
 
  private:
   const int vm_tid_;
 };
 
 
+static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context);
+
+
 class SignalSender : public Thread {
  public:
   enum SleepInterval {
     HALF_INTERVAL,
     FULL_INTERVAL
   };
 
   static const int kSignalSenderStackSize = 64 * KB;
 
   explicit SignalSender(int interval)
@@ skipping 13 matching lines @@
         (sigaction(SIGPROF, &sa, &old_signal_handler_) == 0);
   }
 
   static void RestoreSignalHandler() {
     if (signal_handler_installed_) {
       sigaction(SIGPROF, &old_signal_handler_, 0);
       signal_handler_installed_ = false;
     }
   }
 
+  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);
     if (instance_ == NULL) {
       // Start a thread that will send SIGPROF signal to VM threads,
       // when CPU profiling will be enabled.
       instance_ = new SignalSender(sampler->interval());
       instance_->Start();
     } else {
       ASSERT(instance_->interval_ == sampler->interval());
@@ skipping 113 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(SignalSender);
 };
 
 
 Mutex* SignalSender::mutex_ = NULL;
 SignalSender* SignalSender::instance_ = NULL;
 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);
   srandom(static_cast<unsigned int>(seed));
   limit_mutex = CreateMutex();
 
 #ifdef __arm__
   // When running on ARM hardware check that the EABI used by V8 and
   // by the C code is the same.
   bool hard_float = OS::ArmUsingHardFloat();
@@ skipping 45 matching lines @@
 
 
 void Sampler::Stop() {
   ASSERT(IsActive());
   SignalSender::RemoveActiveSampler(this);
   SetActive(false);
 }
 
 
 } }  // namespace v8::internal