From 621a533ec28dbbf8634211c684456b4bbc1bc0fb Mon Sep 17 00:00:00 2001
From: Yabin Cui <yabinc@google.com>
Date: Mon, 15 Jun 2015 16:17:20 -0700
Subject: [PATCH] Simpleperf: add signal handler for SIGCHLD, SIGINT, SIGTERM.

And rely on signal to decide when to stop in record/stat command.

Bug: 19483574
Change-Id: I564cb61e74ea81cffe4de5b050482b3250ed9a06
---
 simpleperf/cmd_record.cpp    | 19 ++++++++------
 simpleperf/cmd_stat.cpp      | 16 +++++++++++-
 simpleperf/utils.h           | 20 +++++++++++++++
 simpleperf/workload.cpp      | 35 ++++++++++---------------
 simpleperf/workload.h        |  5 +---
 simpleperf/workload_test.cpp | 61 ++++++++++++++++++++++++++++++++++++--------
 6 files changed, 110 insertions(+), 46 deletions(-)

diff --git a/simpleperf/cmd_record.cpp b/simpleperf/cmd_record.cpp
index 83fa593a..fe4beac9 100644
--- a/simpleperf/cmd_record.cpp
+++ b/simpleperf/cmd_record.cpp
@@ -31,6 +31,7 @@
 #include "read_elf.h"
 #include "record.h"
 #include "record_file.h"
+#include "utils.h"
 #include "workload.h"
 
 static std::string default_measured_event_type = "cpu-cycles";
@@ -44,6 +45,11 @@ static std::unordered_map<std::string, uint64_t> branch_sampling_type_map = {
     {"ind_call", PERF_SAMPLE_BRANCH_IND_CALL},
 };
 
+static volatile bool signaled;
+static void signal_handler(int) {
+  signaled = true;
+}
+
 class RecordCommand : public Command {
  public:
   RecordCommand()
@@ -78,12 +84,9 @@ class RecordCommand : public Command {
         measured_event_type_(nullptr),
         perf_mmap_pages_(256),
         record_filename_("perf.data") {
-    // We need signal SIGCHLD to break poll().
-    saved_sigchild_handler_ = signal(SIGCHLD, [](int) {});
-  }
-
-  ~RecordCommand() {
-    signal(SIGCHLD, saved_sigchild_handler_);
+    signaled = false;
+    signal_handler_register_.reset(
+        new SignalHandlerRegister({SIGCHLD, SIGINT, SIGTERM}, signal_handler));
   }
 
   bool Run(const std::vector<std::string>& args);
@@ -115,7 +118,7 @@ class RecordCommand : public Command {
   std::string record_filename_;
   std::unique_ptr<RecordFileWriter> record_file_writer_;
 
-  sighandler_t saved_sigchild_handler_;
+  std::unique_ptr<SignalHandlerRegister> signal_handler_register_;
 };
 
 bool RecordCommand::Run(const std::vector<std::string>& args) {
@@ -193,7 +196,7 @@ bool RecordCommand::Run(const std::vector<std::string>& args) {
     if (!event_selection_set_.ReadMmapEventData(callback)) {
       return false;
     }
-    if (workload->IsFinished()) {
+    if (signaled) {
       break;
     }
     poll(&pollfds[0], pollfds.size(), -1);
diff --git a/simpleperf/cmd_stat.cpp b/simpleperf/cmd_stat.cpp
index de3b032a..b319eb1e 100644
--- a/simpleperf/cmd_stat.cpp
+++ b/simpleperf/cmd_stat.cpp
@@ -15,6 +15,7 @@
  */
 
 #include <inttypes.h>
+#include <signal.h>
 #include <stdio.h>
 #include <chrono>
 #include <string>
@@ -28,6 +29,7 @@
 #include "event_selection_set.h"
 #include "event_type.h"
 #include "perf_event.h"
+#include "utils.h"
 #include "workload.h"
 
 static std::vector<std::string> default_measured_event_types{
@@ -36,6 +38,11 @@ static std::vector<std::string> default_measured_event_types{
     "task-clock",   "context-switches",        "page-faults",
 };
 
+static volatile bool signaled;
+static void signal_handler(int) {
+  signaled = true;
+}
+
 class StatCommand : public Command {
  public:
   StatCommand()
@@ -49,6 +56,9 @@ class StatCommand : public Command {
                 "    --verbose    Show result in verbose mode.\n"),
         verbose_mode_(false),
         system_wide_collection_(false) {
+    signaled = false;
+    signal_handler_register_.reset(
+        new SignalHandlerRegister({SIGCHLD, SIGINT, SIGTERM}, signal_handler));
   }
 
   bool Run(const std::vector<std::string>& args);
@@ -63,6 +73,8 @@ class StatCommand : public Command {
   EventSelectionSet event_selection_set_;
   bool verbose_mode_;
   bool system_wide_collection_;
+
+  std::unique_ptr<SignalHandlerRegister> signal_handler_register_;
 };
 
 bool StatCommand::Run(const std::vector<std::string>& args) {
@@ -113,7 +125,9 @@ bool StatCommand::Run(const std::vector<std::string>& args) {
   if (!workload->Start()) {
     return false;
   }
-  workload->WaitFinish();
+  while (!signaled) {
+    sleep(1);
+  }
   auto end_time = std::chrono::steady_clock::now();
 
   // 6. Read and print counters.
diff --git a/simpleperf/utils.h b/simpleperf/utils.h
index 268e5167..f54e698e 100644
--- a/simpleperf/utils.h
+++ b/simpleperf/utils.h
@@ -17,6 +17,7 @@
 #ifndef SIMPLE_PERF_UTILS_H_
 #define SIMPLE_PERF_UTILS_H_
 
+#include <signal.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -52,6 +53,25 @@ class LineReader {
   size_t bufsize_;
 };
 
+class SignalHandlerRegister {
+ public:
+  SignalHandlerRegister(const std::vector<int>& signums, void (*handler)(int)) {
+    for (auto& sig : signums) {
+      sighandler_t old_handler = signal(sig, handler);
+      saved_signal_handlers_.push_back(std::make_pair(sig, old_handler));
+    }
+  }
+
+  ~SignalHandlerRegister() {
+    for (auto& pair : saved_signal_handlers_) {
+      signal(pair.first, pair.second);
+    }
+  }
+
+ private:
+  std::vector<std::pair<int, sighandler_t>> saved_signal_handlers_;
+};
+
 void PrintIndented(size_t indent, const char* fmt, ...);
 
 bool IsPowerOfTwo(uint64_t value);
diff --git a/simpleperf/workload.cpp b/simpleperf/workload.cpp
index 2364b1c4..9138afa5 100644
--- a/simpleperf/workload.cpp
+++ b/simpleperf/workload.cpp
@@ -32,8 +32,11 @@ std::unique_ptr<Workload> Workload::CreateWorkload(const std::vector<std::string
 }
 
 Workload::~Workload() {
-  if (work_pid_ != -1 && work_state_ != NotYetCreateNewProcess && work_state_ != Finished) {
-    kill(work_pid_, SIGKILL);
+  if (work_pid_ != -1 && work_state_ != NotYetCreateNewProcess) {
+    if (!Workload::WaitChildProcess(false)) {
+      kill(work_pid_, SIGKILL);
+      Workload::WaitChildProcess(true);
+    }
   }
   if (start_signal_fd_ != -1) {
     close(start_signal_fd_);
@@ -129,31 +132,19 @@ bool Workload::Start() {
   return true;
 }
 
-bool Workload::IsFinished() {
-  if (work_state_ == Started) {
-    WaitChildProcess(true);
-  }
-  return work_state_ == Finished;
-}
-
-void Workload::WaitFinish() {
-  CHECK(work_state_ == Started || work_state_ == Finished);
-  if (work_state_ == Started) {
-    WaitChildProcess(false);
-  }
-}
-
-void Workload::WaitChildProcess(bool no_hang) {
+bool Workload::WaitChildProcess(bool wait_forever) {
+  bool finished = false;
   int status;
-  pid_t result = TEMP_FAILURE_RETRY(waitpid(work_pid_, &status, (no_hang ? WNOHANG : 0)));
+  pid_t result = TEMP_FAILURE_RETRY(waitpid(work_pid_, &status, (wait_forever ? 0 : WNOHANG)));
   if (result == work_pid_) {
-    work_state_ = Finished;
+    finished = true;
     if (WIFSIGNALED(status)) {
-      LOG(ERROR) << "work process was terminated by signal " << strsignal(WTERMSIG(status));
+      LOG(WARNING) << "child process was terminated by signal " << strsignal(WTERMSIG(status));
     } else if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
-      LOG(ERROR) << "work process exited with exit code " << WEXITSTATUS(status);
+      LOG(WARNING) << "child process exited with exit code " << WEXITSTATUS(status);
     }
   } else if (result == -1) {
-    PLOG(FATAL) << "waitpid() failed";
+    PLOG(ERROR) << "waitpid() failed";
   }
+  return finished;
 }
diff --git a/simpleperf/workload.h b/simpleperf/workload.h
index 9e2f0db6..4bb0ee5b 100644
--- a/simpleperf/workload.h
+++ b/simpleperf/workload.h
@@ -30,7 +30,6 @@ class Workload {
     NotYetCreateNewProcess,
     NotYetStartNewProcess,
     Started,
-    Finished,
   };
 
  public:
@@ -39,8 +38,6 @@ class Workload {
   ~Workload();
 
   bool Start();
-  bool IsFinished();
-  void WaitFinish();
   pid_t GetPid() {
     return work_pid_;
   }
@@ -55,7 +52,7 @@ class Workload {
   }
 
   bool CreateNewProcess();
-  void WaitChildProcess(bool no_hang);
+  bool WaitChildProcess(bool wait_forever);
 
   WorkState work_state_;
   std::vector<std::string> args_;
diff --git a/simpleperf/workload_test.cpp b/simpleperf/workload_test.cpp
index 0cc67b80..ada3969f 100644
--- a/simpleperf/workload_test.cpp
+++ b/simpleperf/workload_test.cpp
@@ -16,24 +16,24 @@
 
 #include <gtest/gtest.h>
 
+#include <signal.h>
+#include <utils.h>
 #include <workload.h>
 
-#include <chrono>
-
-using namespace std::chrono;
+static volatile bool signaled;
+static void signal_handler(int) {
+  signaled = true;
+}
 
-TEST(workload, smoke) {
+TEST(workload, success) {
+  signaled = false;
+  SignalHandlerRegister signal_handler_register({SIGCHLD}, signal_handler);
   auto workload = Workload::CreateWorkload({"sleep", "1"});
   ASSERT_TRUE(workload != nullptr);
-  ASSERT_FALSE(workload->IsFinished());
   ASSERT_TRUE(workload->GetPid() != 0);
-  auto start_time = steady_clock::now();
   ASSERT_TRUE(workload->Start());
-  ASSERT_FALSE(workload->IsFinished());
-  workload->WaitFinish();
-  ASSERT_TRUE(workload->IsFinished());
-  auto end_time = steady_clock::now();
-  ASSERT_TRUE(end_time >= start_time + seconds(1));
+  while (!signaled) {
+  }
 }
 
 TEST(workload, execvp_failure) {
@@ -41,3 +41,42 @@ TEST(workload, execvp_failure) {
   ASSERT_TRUE(workload != nullptr);
   ASSERT_FALSE(workload->Start());
 }
+
+static void run_signaled_workload() {
+  {
+    signaled = false;
+    SignalHandlerRegister signal_handler_register({SIGCHLD}, signal_handler);
+    auto workload = Workload::CreateWorkload({"sleep", "10"});
+    ASSERT_TRUE(workload != nullptr);
+    ASSERT_TRUE(workload->Start());
+    ASSERT_EQ(0, kill(workload->GetPid(), SIGSEGV));
+    while (!signaled) {
+    }
+  }
+  // Make sure all destructors are called before exit().
+  exit(0);
+}
+
+TEST(workload, signaled_warning) {
+  ASSERT_EXIT(run_signaled_workload(), testing::ExitedWithCode(0),
+              "child process was terminated by signal");
+}
+
+static void run_exit_nonzero_workload() {
+  {
+    signaled = false;
+    SignalHandlerRegister signal_handler_register({SIGCHLD}, signal_handler);
+    auto workload = Workload::CreateWorkload({"ls", "nonexistdir"});
+    ASSERT_TRUE(workload != nullptr);
+    ASSERT_TRUE(workload->Start());
+    while (!signaled) {
+    }
+  }
+  // Make sure all destructors are called before exit().
+  exit(0);
+}
+
+TEST(workload, exit_nonzero_warning) {
+  ASSERT_EXIT(run_exit_nonzero_workload(), testing::ExitedWithCode(0),
+              "child process exited with exit code");
+}
-- 
2.11.0