From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 52941 invoked by alias); 30 May 2019 13:40:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 52928 invoked by uid 89); 30 May 2019 13:40:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=Say X-HELO: mail-wm1-f67.google.com Received: from mail-wm1-f67.google.com (HELO mail-wm1-f67.google.com) (209.85.128.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 30 May 2019 13:40:42 +0000 Received: by mail-wm1-f67.google.com with SMTP id e19so6019293wme.1 for ; Thu, 30 May 2019 06:40:42 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:4eeb:42ff:feef:f164? ([2001:8a0:f913:f700:4eeb:42ff:feef:f164]) by smtp.gmail.com with ESMTPSA id t4sm3101814wmi.41.2019.05.30.06.40.36 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Thu, 30 May 2019 06:40:37 -0700 (PDT) Subject: Re: [PATCH v3 6/8] Introduce thread-safe way to handle SIGSEGV To: Tom Tromey , gdb-patches@sourceware.org References: <20190529212916.23721-1-tom@tromey.com> <20190529212916.23721-7-tom@tromey.com> From: Pedro Alves Message-ID: Date: Thu, 30 May 2019 13:40:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190529212916.23721-7-tom@tromey.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-05/txt/msg00686.txt.bz2 On 5/29/19 10:29 PM, Tom Tromey wrote: > The gdb demangler installs a SIGSEGV handler in order to protect gdb > from demangler bugs. However, this is not thread-safe, as signal > handlers are global to the process. > > This patch changes gdb to always install a global SIGSEGV handler, and > then lets thread indicate their interest in handling the signal by "lets threads" > setting a thread-local variable. > > This patch then arranges for the demangler code to use this; being > sure to arrange for calls to warning and the like to be done on the > main thread. The design is explained here, but the patch itself doesn't include any comment in that direction. I think future readers of the code would benefit a lot such commentary. > > gdb/ChangeLog > 2019-05-29 Tom Tromey > > * event-top.h (segv_handler): Declare. > * event-top.c (segv_handler): New global. > (handle_sigsegv): New function. > (async_init_signals): Install SIGSEGV handler. > * cp-support.c (gdb_demangle_jmp_buf): Change type. Now > thread-local. > (report_failed_demangle): New function. > (gdb_demangle): Make core_dump_allowed atomic. Remove signal > handler-setting code, instead use segv_handler. Run warning code > on main thread. > --- > gdb/ChangeLog | 13 ++++++ > gdb/cp-support.c | 114 +++++++++++++++++++++++------------------------ > gdb/event-top.c | 37 +++++++++++++++ > gdb/event-top.h | 3 ++ > 4 files changed, 109 insertions(+), 58 deletions(-) > > diff --git a/gdb/cp-support.c b/gdb/cp-support.c > index 4afb79a4ea9..60bbbc23ec3 100644 > --- a/gdb/cp-support.c > +++ b/gdb/cp-support.c > @@ -37,6 +37,9 @@ > #include "common/gdb_setjmp.h" > #include "safe-ctype.h" > #include "common/selftest.h" > +#include > +#include "event-top.h" > +#include "ser-event.h" > > #define d_left(dc) (dc)->u.s_binary.left > #define d_right(dc) (dc)->u.s_binary.right > @@ -1480,7 +1483,7 @@ static int catch_demangler_crashes = 1; > > /* 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. */ > > @@ -1499,7 +1502,43 @@ gdb_demangle_signal_handler (int signo) > gdb_demangle_attempt_core_dump = 0; > } > > - SIGLONGJMP (gdb_demangle_jmp_buf, signo); > + SIGLONGJMP (*gdb_demangle_jmp_buf, signo); > +} > + > +/* A helper for gdb_demangle that reports a demangling failure. */ > + > +static void > +report_failed_demangle (const char *name, int core_dump_allowed, > + int crash_signal) > +{ > + static int error_reported = 0; bool, while at it? > + > + if (!error_reported) > + { > + std::string short_msg > + = string_printf (_("unable to demangle '%s' " > + "(demangler failed with signal %d)"), > + name, crash_signal); > + > + std::string long_msg > + = string_printf ("%s:%d: %s: %s", __FILE__, __LINE__, > + "demangler-warning", short_msg.c_str ()); > + > + target_terminal::scoped_restore_terminal_state term_state; > + target_terminal::ours_for_output (); > + > + begin_line (); > + if (core_dump_allowed) > + fprintf_unfiltered (gdb_stderr, > + _("%s\nAttempting to dump core.\n"), > + long_msg.c_str ()); > + else > + warn_cant_dump_core (long_msg.c_str ()); > + > + demangler_warning (__FILE__, __LINE__, "%s", short_msg.c_str ()); > + > + error_reported = 1; > + } > } > > #endif > @@ -1513,12 +1552,7 @@ gdb_demangle (const char *name, int options) > int crash_signal = 0; > > #ifdef HAVE_WORKING_FORK > -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) > - struct sigaction sa, old_sa; > -#else > - sighandler_t ofunc; > -#endif > - static int core_dump_allowed = -1; > + static std::atomic core_dump_allowed (-1); > > if (core_dump_allowed == -1) > { > @@ -1528,23 +1562,15 @@ gdb_demangle (const char *name, int options) > gdb_demangle_attempt_core_dump = 0; Are accesses to gdb_demangle_attempt_core_dump going to be racy? Maybe not, didn't think that much about it. If they are, I wonder whether it wouldn't be better if this whole can_dump_core check block were moved to the main thread, before threads are spun? Say, to a gdb_demangle_init() function? Not sure, might not be convenient form an abstraction-hiding perspective. > } > > - if (catch_demangler_crashes) > - { > -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) > - sa.sa_handler = gdb_demangle_signal_handler; > - sigemptyset (&sa.sa_mask); > -#ifdef HAVE_SIGALTSTACK > - sa.sa_flags = SA_ONSTACK; > -#else > - sa.sa_flags = 0; > -#endif > - sigaction (SIGSEGV, &sa, &old_sa); > -#else > - ofunc = signal (SIGSEGV, gdb_demangle_signal_handler); > -#endif > + scoped_restore restore_segv > + = make_scoped_restore (&segv_handler, > + catch_demangler_crashes > + ? gdb_demangle_signal_handler > + : nullptr); > > - crash_signal = SIGSETJMP (gdb_demangle_jmp_buf); > - } > + SIGJMP_BUF jmp_buf; > + if (catch_demangler_crashes) > + crash_signal = SIGSETJMP (jmp_buf); > #endif gdb_demangle_jmp_buf is now a pointer, but I'm not seeing anything set it? > > if (crash_signal == 0) > @@ -1553,42 +1579,14 @@ gdb_demangle (const char *name, int options) > #ifdef HAVE_WORKING_FORK > if (catch_demangler_crashes) > { > -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) > - sigaction (SIGSEGV, &old_sa, NULL); > -#else > - signal (SIGSEGV, ofunc); > -#endif > - > if (crash_signal != 0) > { > - static int error_reported = 0; > - > - if (!error_reported) > - { > - std::string short_msg > - = string_printf (_("unable to demangle '%s' " > - "(demangler failed with signal %d)"), > - name, crash_signal); > - > - std::string long_msg > - = string_printf ("%s:%d: %s: %s", __FILE__, __LINE__, > - "demangler-warning", short_msg.c_str ()); > - > - target_terminal::scoped_restore_terminal_state term_state; > - target_terminal::ours_for_output (); > - > - begin_line (); > - if (core_dump_allowed) > - fprintf_unfiltered (gdb_stderr, > - _("%s\nAttempting to dump core.\n"), > - long_msg.c_str ()); > - else > - warn_cant_dump_core (long_msg.c_str ()); > - > - demangler_warning (__FILE__, __LINE__, "%s", short_msg.c_str ()); > - > - error_reported = 1; > - } > + std::string copy = name; I don't think you need this copy, because ... > + run_on_main_thread ([=] () ... you're already copying here, with "[=]". > + { tabs vs spaces. > + report_failed_demangle (copy.c_str (), core_dump_allowed, > + crash_signal); > + }); > > result = NULL; > } > diff --git a/gdb/event-top.c b/gdb/event-top.c > index 93b7d2d28bc..f1466c0c2ba 100644 > --- a/gdb/event-top.c > +++ b/gdb/event-top.c > @@ -849,6 +849,29 @@ gdb_readline_no_editing_callback (gdb_client_data client_data) > } > > > +/* See event-top.h. */ > + > +thread_local void (*segv_handler) (int); I think it'd help readability if this were named something that indicates it's thread-local, like e.g., thr_segv_handler. > + > +/* Handler for SIGSEGV. */ > + > +static void > +handle_sigsegv (int sig) > +{ > + if (segv_handler == nullptr) > + { > + signal (sig, nullptr); > + raise (sig); > + } > + else > + { > + signal (sig, handle_sigsegv); Doesn't this lose the SA_ONSTACK? > + segv_handler (sig); > + } > +} > + > + > + > /* The serial event associated with the QUIT flag. set_quit_flag sets > this, and check_quit_flag clears it. Used by interruptible_select > to be able to do interruptible I/O with no race with the SIGINT > @@ -916,6 +939,20 @@ async_init_signals (void) > sigtstp_token = > create_async_signal_handler (async_sigtstp_handler, NULL); > #endif > + > +#if defined (HAVE_SIGACTION) && defined (SA_RESTART) > + struct sigaction sa, old_sa; > + sa.sa_handler = handle_sigsegv; > + sigemptyset (&sa.sa_mask); > +#ifdef HAVE_SIGALTSTACK > + sa.sa_flags = SA_ONSTACK; > +#else > + sa.sa_flags = 0; > +#endif > + sigaction (SIGSEGV, &sa, &old_sa); > +#else > + signal (SIGSEGV, handle_sigsegv); > +#endif > } > > /* See defs.h. */ > diff --git a/gdb/event-top.h b/gdb/event-top.h > index 4c3e6cb8612..849f40d9192 100644 > --- a/gdb/event-top.h > +++ b/gdb/event-top.h > @@ -70,4 +70,7 @@ extern void gdb_rl_callback_handler_install (const char *prompt); > currently installed. */ > extern void gdb_rl_callback_handler_reinstall (void); > > +/* The SIGSEGV handler for this thread, or NULL if there is none. */ > +extern thread_local void (*segv_handler) (int); > + > #endif > Thanks, Pedro Alves