backward-cpp icon indicating copy to clipboard operation
backward-cpp copied to clipboard

`backward::SignalHandling` `crash_status` not resetting to `crash_status::running`

Open sbond75 opened this issue 8 months ago • 1 comments

On Windows at least, there appears to be an edge case that only shows up when not linking against backward.cpp's singleton instance of backward::SignalHandling. Instead, in my code I was creating and destroying SignalHandling instances as needed. As a result, the reporter_thread_ exits shortly after starting, since crashed() returns crash_status::ending by the time that thread starts back up again in the new SignalHandling instance. Do you intend to support re-creating SignalHandling instances? If so, I have a potential fix below. It works in my testing, but only if at most one SignalHandling instance exists at a time (this doesn't make sense to support anyway).

diff --git a/backward.hpp b/backward.hpp
index 670aa45..632b372 100644
--- a/backward.hpp
+++ b/backward.hpp
@@ -4311,28 +4311,38 @@ private:
 class SignalHandling {
 public:
   SignalHandling(const std::vector<int> & = std::vector<int>())
-      : reporter_thread_([]() {
-          /* We handle crashes in a utility thread:
-            backward structures and some Windows functions called here
-            need stack space, which we do not have when we encounter a
-            stack overflow.
-            To support reporting stack traces during a stack overflow,
-            we create a utility thread at startup, which waits until a
-            crash happens or the program exits normally. */
-
-          {
-            std::unique_lock<std::mutex> lk(mtx());
-            cv().wait(lk, [] { return crashed() != crash_status::running; });
-          }
-          if (crashed() == crash_status::crashed) {
-            handle_stacktrace(skip_recs());
-          }
-          {
-            std::unique_lock<std::mutex> lk(mtx());
-            crashed() = crash_status::ending;
-          }
-          cv().notify_one();
-        }) {
+  {
+    // Reset crash status
+    {
+	  std::unique_lock<std::mutex> lk(mtx());
+	  crashed() = crash_status::running;
+    }
+
+    reporter_thread_ = std::thread([]() {
+	  /* We handle crashes in a utility thread:
+		backward structures and some Windows functions called here
+		need stack space, which we do not have when we encounter a
+		stack overflow.
+		To support reporting stack traces during a stack overflow,
+		we create a utility thread at startup, which waits until a
+		crash happens or the program exits normally. */
+
+	  crash_status status;
+	  {
+		std::unique_lock<std::mutex> lk(mtx());
+		cv().wait(lk, [] { return crashed() != crash_status::running; });
+		status = crashed();
+	  }
+	  if (status == crash_status::crashed) {
+		handle_stacktrace(skip_recs());
+	  }
+	  {
+		std::unique_lock<std::mutex> lk(mtx());
+		crashed() = crash_status::ending;
+	  }
+	  cv().notify_one();
+	});
+
     SetUnhandledExceptionFilter(crash_handler);
 
     signal(SIGABRT, signal_handler);

sbond75 avatar May 09 '25 18:05 sbond75

Without much thoughts, I think this can be considered a bug? I don't think SignalHandling should inherently be impossible to cleanly destroy.

The code originally didn't bother using a thread. One reason what that std::thread didn't exist and so there wasn't an easy way to get portable threads.

I a way I made mistake accepting the code with std::thread because this breaks backward-cpp compatibility with C++98. On the other hand it might be time to accept that C++11 should be the new lowest version to support. I keep telling people to avoid breaking C++98 in pull requests, but now I realize the code probably doesn't even compile on it anymore.

And yes this also shows that I have not ran nor tested the code in a long time. I merely comment and accept pull requests. And I don't have any of the hardware backward-cpp can run on that people have contributed to over the years.

With this context, it hopefully helps you decide if this is indeed a bug fix. I will gladly merge a pull request. Maybe add a short comment in the code and API doc in the readme about the requirement of SignalHandling to never be instantiated more than once at a time.

(btw your diff is quite hard to read, I assume because of some indentation change) On Fri, May 9, 2025, 11:32 sbond75 @.***> wrote:

sbond75 created an issue (bombela/backward-cpp#351) https://github.com/bombela/backward-cpp/issues/351

On Windows at least, there appears to be an edge case that only shows up when not linking against backward.cpp's singleton instance of backward::SignalHandling. Instead, in my code I was creating and destroying SignalHandling instances as needed. As a result, the reporter_thread_ exits shortly after starting, since crashed() returns crash_status::ending by the time that thread starts back up again in the new SignalHandling instance. Do you intend to support re-creating SignalHandling instances? If so, I have a potential fix below. It works in my testing, but only if at most one SignalHandling instance exists at a time (this doesn't make sense to support anyway).

diff --git a/backward.hpp b/backward.hpp index 670aa45..632b372 100644--- a/backward.hpp+++ b/backward.hpp@@ -4311,28 +4311,38 @@ private: class SignalHandling { public: SignalHandling(const std::vector & = std::vector())- : reporter_thread_( {- /* We handle crashes in a utility thread:- backward structures and some Windows functions called here- need stack space, which we do not have when we encounter a- stack overflow.- To support reporting stack traces during a stack overflow,- we create a utility thread at startup, which waits until a- crash happens or the program exits normally. /-- {- std::unique_lockstd::mutex lk(mtx());- cv().wait(lk, [] { return crashed() != crash_status::running; });- }- if (crashed() == crash_status::crashed) {- handle_stacktrace(skip_recs());- }- {- std::unique_lockstd::mutex lk(mtx());- crashed() = crash_status::ending;- }- cv().notify_one();- }) {+ {+ // Reset crash status+ {+ std::unique_lockstd::mutex lk(mtx());+ crashed() = crash_status::running;+ }++ reporter_thread_ = std::thread( {+ / We handle crashes in a utility thread:+ backward structures and some Windows functions called here+ need stack space, which we do not have when we encounter a+ stack overflow.+ To support reporting stack traces during a stack overflow,+ we create a utility thread at startup, which waits until a+ crash happens or the program exits normally. */++ crash_status status;+ {+ std::unique_lockstd::mutex lk(mtx());+ cv().wait(lk, [] { return crashed() != crash_status::running; });+ status = crashed();+ }+ if (status == crash_status::crashed) {+ handle_stacktrace(skip_recs());+ }+ {+ std::unique_lockstd::mutex lk(mtx());+ crashed() = crash_status::ending;+ }+ cv().notify_one();+ });+ SetUnhandledExceptionFilter(crash_handler);

 signal(SIGABRT, signal_handler);

— Reply to this email directly, view it on GitHub https://github.com/bombela/backward-cpp/issues/351, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUZDCS63BM3AYZC545UML25TYDHAVCNFSM6AAAAAB4ZWETXGVHI2DSMVQWIX3LMV43ASLTON2WKOZTGA2TEOJQHEYTEMY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

bombela avatar May 09 '25 20:05 bombela