From: Radoslaw Zarzynski <rzarzyns@redhat.com>
Date: Thu, 23 Sep 2021 14:25:10 +0000 (+0000)
Subject: crimson/common: skip first 4 frames when dumping a backtrace.
X-Git-Tag: v17.1.0~821^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=refs%2Fpull%2F43288%2Fhead;p=ceph.git

crimson/common: skip first 4 frames when dumping a backtrace.

It's all about these items:

```
 0# print_backtrace(std::basic_string_view<char, std::char_traits<char> >) at /home/rzarzynski/ceph1/build/../src/crimson/common/fatal_signal.cc:80
 1# FatalSignal::signaled(int, siginfo_t const&) at /opt/rh/gcc-toolset-9/root/usr/include/c++/9/ostream:570
 2# FatalSignal::install_oneshot_signal_handler<11>()::{lambda(int, siginfo_t*, void*)#1}::_FUN(int, siginfo_t*, void*) at /home/rzarzynski/ceph1/build/../src/crimson/common/fatal_signal.cc:
62
 3# 0x00007F16BBA13B30 in /lib64/libpthread.so.0
```

They are part of our backtrace handling and typically developers
are not interested in them. Let's be consistent with the classical
OSD and hide them.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
---

diff --git a/src/crimson/common/fatal_signal.cc b/src/crimson/common/fatal_signal.cc
index bda460674a8..f2983769de4 100644
--- a/src/crimson/common/fatal_signal.cc
+++ b/src/crimson/common/fatal_signal.cc
@@ -49,18 +49,26 @@ static void reraise_fatal(const int signum)
   ::_exit(1);
 }
 
+[[gnu::noinline]] void FatalSignal::signal_entry(
+  const int signum,
+  siginfo_t* const info,
+  void*)
+{
+  if (static std::atomic_bool handled{false}; handled.exchange(true)) {
+    return;
+  }
+  assert(info);
+  FatalSignal::signaled(signum, *info);
+  reraise_fatal(signum);
+}
+
 template <int SigNum>
 void FatalSignal::install_oneshot_signal_handler()
 {
   struct sigaction sa;
-  sa.sa_sigaction = [](int signum, siginfo_t *info, void *p) {
-    if (static std::atomic_bool handled{false}; handled.exchange(true)) {
-      return;
-    }
-    assert(info);
-    FatalSignal::signaled(signum, *info);
-    reraise_fatal(signum);
-  };
+  // it's a bad idea to use a lambda here. On GCC there are `operator()`
+  // and `_FUN()`. Controlling their inlineability is hard (impossible?).
+  sa.sa_sigaction = signal_entry;
   sigemptyset(&sa.sa_mask);
   sa.sa_flags = SA_SIGINFO | SA_RESTART | SA_NODEFER;
   if constexpr (SigNum == SIGSEGV) {
@@ -71,13 +79,20 @@ void FatalSignal::install_oneshot_signal_handler()
 }
 
 
-static void print_backtrace(std::string_view cause) {
+[[gnu::noinline]] static void print_backtrace(std::string_view cause) {
   std::cerr << cause;
   if (seastar::engine_is_ready()) {
     std::cerr << " on shard " << seastar::this_shard_id();
   }
+  // nobody wants to see things like `FatalSignal::signaled()` or
+  // `print_backtrace()` in our backtraces. `+ 1` is for the extra
+  // frame created by kernel (signal trampoline, it will take care
+  // about e.g. sigreturn(2) calling; see the man page).
+  constexpr std::size_t FRAMES_TO_SKIP = 3 + 1;
   std::cerr << ".\nBacktrace:\n";
-  std::cerr << boost::stacktrace::stacktrace();
+  std::cerr << boost::stacktrace::stacktrace(
+    FRAMES_TO_SKIP,
+    static_cast<std::size_t>(-1)/* max depth same as the default one */);
   std::cerr << std::flush;
   // TODO: dump crash related meta data to $crash_dir
   //       see handle_fatal_signal()
@@ -138,7 +153,8 @@ static void print_proc_maps()
   }
 }
 
-void FatalSignal::signaled(const int signum, const siginfo_t& siginfo)
+[[gnu::noinline]] void FatalSignal::signaled(const int signum,
+                                             const siginfo_t& siginfo)
 {
   switch (signum) {
   case SIGSEGV:
diff --git a/src/crimson/common/fatal_signal.h b/src/crimson/common/fatal_signal.h
index dd366c28143..626017c9308 100644
--- a/src/crimson/common/fatal_signal.h
+++ b/src/crimson/common/fatal_signal.h
@@ -10,6 +10,7 @@ public:
   FatalSignal();
 
 private:
+  static void signal_entry(int signum, siginfo_t* siginfo, void* p);
   static void signaled(int signum, const siginfo_t& siginfo);
 
   template <int... SigNums>