[review v4] Introduce thread-safe way to handle SIGSEGV

["Pedro Alves (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/171
......................................................................


Patch Set 4: Code-Review+2

(2 comments)

LGTM, with the nit below fixed.

| --- gdb/cp-support.c
| +++ gdb/cp-support.c
| @@ -1477,17 +1480,17 @@ static bool catch_demangler_crashes = true;
|  
|  /* Stack context and environment for demangler crash recovery.  */
|  
| -static SIGJMP_BUF gdb_demangle_jmp_buf;
| +static thread_local SIGJMP_BUF *gdb_demangle_jmp_buf;
|  
|  /* If nonzero, attempt to dump core from the signal handler.  */
|  
| -static int gdb_demangle_attempt_core_dump = 1;
| +static std::atomic<bool> gdb_demangle_attempt_core_dump;

PS3, Line 1487:

> What would be really great is to fuzz the demangler and fix all
> the bugs.  Then we could ditch all of this code

One can dream.  :-)

|  
|  /* Signal handler for gdb_demangle.  */
|  
|  static void
|  gdb_demangle_signal_handler (int signo)
|  {
|    if (gdb_demangle_attempt_core_dump)
|      {
|        if (fork () == 0)
| --- gdb/cp-support.c
| +++ gdb/cp-support.c
| @@ -1602,7 +1592,13 @@ #endif
| -
| -	      error_reported = 1;
| -	    }
| -
| -	  result = NULL;
| -	}
| +	  /* If there was a failure, we can't report it here, because
| +	     we might be in a background thread.  Instead, arrange for
| +	     the reporting to happen on the main thread.  */
| +          std::string copy = name;

PS4, Line 1595:

(I thought I had commented on this, but I can't find the comment now.)

This explicit copy is unnecessary/redundant, because you're using [=]
as capture.  As is, you copy the string twice, first into the
"std::string copy" local, and then you copy that again in the lambda's
capture by value.

| +          run_on_main_thread ([=] ()
| +            {
| +              report_failed_demangle (copy.c_str (), core_dump_allowed,
| +                                      crash_signal);
| +            });
| +
| +          result = NULL;
| +        }
|      }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 26 Nov 2019 15:57:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment