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..2030775ee0a57bdc135870d4aa6fe0a9ba4e8c2d 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);

[Markus (顧孟勤), 2012/12/13 08:49:45]

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.

[Sven Panne, 2012/12/13 09:34:54]

The Google C++ style guide allows both possibilities and does not even give a
preference to one of them. The Chrome style guide (which we don't follow at all
;-) seems to prefer unnamed namespaces, but I don't understand the reasoning
behind it: At least on Linux the end result seems to be the same.

But to make things short: Just leave "static" here, we use it all over the place
in v8.

[Markus (顧孟勤), 2012/12/13 09:56:50]

Ah, sanity prevails. And yes, I agree that the preference in the Chrome style
guide seems somewhat arbitrary :-)

On 2012/12/13 09:34:54, Sven Panne wrote:

+
+
 class SignalSender : public Thread {
  public:
   enum SleepInterval {
@@ -1122,6 +1069,23 @@ class SignalSender : public Thread {
     }
   }
 
+  static void CallOldSignalHandler(int signal, siginfo_t* info, void* context) {
+    // Only invoke the old signal handler if it did not have SIGPROF masked.
+    if (sigismember(&old_signal_handler_.sa_mask, SIGPROF))

[Markus (顧孟勤), 2012/12/13 08:49:45]

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.

+      return;
+    if (!(old_signal_handler_.sa_flags & SA_SIGINFO)) {
+      // It's safe to ignore the old signal handler if it's one of these values.

[Markus (顧孟勤), 2012/12/13 08:49:45]

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.

+      if (old_signal_handler_.sa_handler == SIG_IGN ||
+          old_signal_handler_.sa_handler == SIG_DFL) {
+        return;
+      }
+      // Sorry, we only support other 3-arg signal handlers (sigaction). Crash
+      // loudly if someone tries to use a different signal handler.
+      *(reinterpret_cast<volatile char*>(NULL) + 74) = 0x34;
+    }
+    old_signal_handler_.sa_sigaction(signal, info, context);
+  }
+
   static void AddActiveSampler(Sampler* sampler) {
     ScopedLock lock(mutex_);
     SamplerRegistry::AddActiveSampler(sampler);
@@ -1255,6 +1219,65 @@ struct sigaction SignalSender::old_signal_handler_;
 bool SignalSender::signal_handler_installed_ = false;
 
 
+static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context) {
+  int curr_errno = errno;
+  USE(info);
+  if (signal != SIGPROF) return;

[Markus (顧孟勤), 2012/12/13 08:49:45]

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?

+  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);
+  errno = curr_errno;
+}
+
+
 void OS::SetUp() {
   // Seed the random number generator. We preserve microsecond resolution.
   uint64_t seed = Ticks() ^ (getpid() << 16);