* [PATCH] Use RAII to set the per-thread SIGSEGV handler
@ 2021-03-09 14:23 Christian Biesinger
2021-03-09 17:00 ` [PATCH v2] " Christian Biesinger
0 siblings, 1 reply; 4+ messages in thread
From: Christian Biesinger @ 2021-03-09 14:23 UTC (permalink / raw)
To: gdb-patches
(If accepted, I would also like to merge this to the gdb 10 branch because this
fixes a build error on cygwin.)
PR threads/27239
This avoids using a thread-local extern variable, which causes link errors
on some platforms, notably Cygwin. But I think this is a better pattern
even outside of working around linker bugs because it encapsulates direct
access to the variable inside the class, instead of having a global extern
variable.
2021-03-09 Christian Biesinger <cbiesinger@google.com>
* cp-support.c: Use scoped_segv_handler_restore.
* event-top.c (thread_local_segv_handler): Made static.
(scoped_segv_handler_restore::scoped_segv_handler_restore):
New function.
(scoped_segv_handler_restore::~scoped_segv_handler_restore): New
function.
* event-top.h (class scoped_segv_handler_restore): New class.
(thread_local_segv_handler): Removed.
---
gdb/cp-support.c | 9 ++++-----
gdb/event-top.c | 18 ++++++++++++++++--
gdb/event-top.h | 19 ++++++++++++++-----
3 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 1abc3e22a72..fb4e4289777 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -1615,11 +1615,10 @@ gdb_demangle (const char *name, int options)
int crash_signal = 0;
#ifdef HAVE_WORKING_FORK
- scoped_restore restore_segv
- = make_scoped_restore (&thread_local_segv_handler,
- catch_demangler_crashes
- ? gdb_demangle_signal_handler
- : nullptr);
+ scoped_segv_handler_restore restore_segv
+ (catch_demangler_crashes
+ ? gdb_demangle_signal_handler
+ : nullptr);
bool core_dump_allowed = gdb_demangle_attempt_core_dump;
SIGJMP_BUF jmp_buf;
diff --git a/gdb/event-top.c b/gdb/event-top.c
index a33f6425140..79cc411fb10 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -850,9 +850,12 @@ gdb_readline_no_editing_callback (gdb_client_data client_data)
}
\f
-/* See event-top.h. */
+/* The SIGSEGV handler for this thread, or NULL if there is none. GDB
+ always installs a global SIGSEGV handler, and then lets threads
+ indicate their interest in handling the signal by setting this
+ thread-local variable. */
-thread_local void (*thread_local_segv_handler) (int);
+static thread_local void (*thread_local_segv_handler) (int);
static void handle_sigsegv (int sig);
@@ -1288,6 +1291,17 @@ gdb_disable_readline (void)
delete_file_handler (ui->input_fd);
}
+scoped_segv_handler_restore::scoped_segv_handler_restore (segv_handler_t new_handler)
+{
+ m_old_handler = thread_local_segv_handler;
+ thread_local_segv_handler = new_handler;
+}
+
+scoped_segv_handler_restore::~scoped_segv_handler_restore()
+{
+ thread_local_segv_handler = m_old_handler;
+}
+
static const char debug_event_loop_off[] = "off";
static const char debug_event_loop_all_except_ui[] = "all-except-ui";
static const char debug_event_loop_all[] = "all";
diff --git a/gdb/event-top.h b/gdb/event-top.h
index c52826eb063..b036f1385c5 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -70,10 +70,19 @@ 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. GDB
- always installs a global SIGSEGV handler, and then lets threads
- indicate their interest in handling the signal by setting this
- thread-local variable. */
-extern thread_local void (*thread_local_segv_handler) (int);
+typedef void (*segv_handler_t) (int);
+
+/* On construction, replaces the current thread's SIGSEGV handler with
+ the provided one. On destruction, restores the handler to the
+ original one. */
+class scoped_segv_handler_restore
+{
+ public:
+ scoped_segv_handler_restore (segv_handler_t new_handler);
+ ~scoped_segv_handler_restore ();
+
+ private:
+ segv_handler_t m_old_handler;
+};
#endif
--
2.30.1.766.gb4fecdf3b7-goog
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v2] Use RAII to set the per-thread SIGSEGV handler
2021-03-09 14:23 [PATCH] Use RAII to set the per-thread SIGSEGV handler Christian Biesinger
@ 2021-03-09 17:00 ` Christian Biesinger
2021-03-12 14:27 ` Tom Tromey
0 siblings, 1 reply; 4+ messages in thread
From: Christian Biesinger @ 2021-03-09 17:00 UTC (permalink / raw)
To: gdb-patches
PR threads/27239
This avoids using a thread-local extern variable, which causes link errors
on some platforms, notably Cygwin. But I think this is a better pattern
even outside of working around linker bugs because it encapsulates direct
access to the variable inside the class, instead of having a global extern
variable.
The cygwin link error is:
cp-support.o: in function `gdb_demangle(char const*, int)':
/home/Christian/binutils-gdb/obj/gdb/../../gdb/cp-support.c:1619:(.text+0x6472): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `TLS init function for thread_local_segv_handler'
/home/Christian/binutils-gdb/obj/gdb/../../gdb/cp-support.c:1619:(.text+0x648b): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `TLS init function for thread_local_segv_handler'
collect2: error: ld returned 1 exit status
2021-03-09 Christian Biesinger <cbiesinger@google.com>
* cp-support.c: Use scoped_segv_handler_restore.
* event-top.c (thread_local_segv_handler): Made static.
(scoped_segv_handler_restore::scoped_segv_handler_restore):
New function.
(scoped_segv_handler_restore::~scoped_segv_handler_restore): New
function.
* event-top.h (class scoped_segv_handler_restore): New class.
(thread_local_segv_handler): Removed.
---
gdb/cp-support.c | 9 ++++-----
gdb/event-top.c | 18 ++++++++++++++++--
gdb/event-top.h | 19 ++++++++++++++-----
3 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 1abc3e22a72..fb4e4289777 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -1615,11 +1615,10 @@ gdb_demangle (const char *name, int options)
int crash_signal = 0;
#ifdef HAVE_WORKING_FORK
- scoped_restore restore_segv
- = make_scoped_restore (&thread_local_segv_handler,
- catch_demangler_crashes
- ? gdb_demangle_signal_handler
- : nullptr);
+ scoped_segv_handler_restore restore_segv
+ (catch_demangler_crashes
+ ? gdb_demangle_signal_handler
+ : nullptr);
bool core_dump_allowed = gdb_demangle_attempt_core_dump;
SIGJMP_BUF jmp_buf;
diff --git a/gdb/event-top.c b/gdb/event-top.c
index a33f6425140..79cc411fb10 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -850,9 +850,12 @@ gdb_readline_no_editing_callback (gdb_client_data client_data)
}
\f
-/* See event-top.h. */
+/* The SIGSEGV handler for this thread, or NULL if there is none. GDB
+ always installs a global SIGSEGV handler, and then lets threads
+ indicate their interest in handling the signal by setting this
+ thread-local variable. */
-thread_local void (*thread_local_segv_handler) (int);
+static thread_local void (*thread_local_segv_handler) (int);
static void handle_sigsegv (int sig);
@@ -1288,6 +1291,17 @@ gdb_disable_readline (void)
delete_file_handler (ui->input_fd);
}
+scoped_segv_handler_restore::scoped_segv_handler_restore (segv_handler_t new_handler)
+{
+ m_old_handler = thread_local_segv_handler;
+ thread_local_segv_handler = new_handler;
+}
+
+scoped_segv_handler_restore::~scoped_segv_handler_restore()
+{
+ thread_local_segv_handler = m_old_handler;
+}
+
static const char debug_event_loop_off[] = "off";
static const char debug_event_loop_all_except_ui[] = "all-except-ui";
static const char debug_event_loop_all[] = "all";
diff --git a/gdb/event-top.h b/gdb/event-top.h
index c52826eb063..b036f1385c5 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -70,10 +70,19 @@ 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. GDB
- always installs a global SIGSEGV handler, and then lets threads
- indicate their interest in handling the signal by setting this
- thread-local variable. */
-extern thread_local void (*thread_local_segv_handler) (int);
+typedef void (*segv_handler_t) (int);
+
+/* On construction, replaces the current thread's SIGSEGV handler with
+ the provided one. On destruction, restores the handler to the
+ original one. */
+class scoped_segv_handler_restore
+{
+ public:
+ scoped_segv_handler_restore (segv_handler_t new_handler);
+ ~scoped_segv_handler_restore ();
+
+ private:
+ segv_handler_t m_old_handler;
+};
#endif
--
2.30.1.766.gb4fecdf3b7-goog
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] Use RAII to set the per-thread SIGSEGV handler
2021-03-09 17:00 ` [PATCH v2] " Christian Biesinger
@ 2021-03-12 14:27 ` Tom Tromey
2021-03-12 17:30 ` Christian Biesinger
0 siblings, 1 reply; 4+ messages in thread
From: Tom Tromey @ 2021-03-12 14:27 UTC (permalink / raw)
To: Christian Biesinger via Gdb-patches
>>>>> "Christian" == Christian Biesinger via Gdb-patches <gdb-patches@sourceware.org> writes:
Christian> PR threads/27239
This should be in the ChangeLog entry.
Christian> +/* The SIGSEGV handler for this thread, or NULL if there is none. GDB
Christian> + always installs a global SIGSEGV handler, and then lets threads
Christian> + indicate their interest in handling the signal by setting this
Christian> + thread-local variable. */
Christian> -thread_local void (*thread_local_segv_handler) (int);
Christian> +static thread_local void (*thread_local_segv_handler) (int);
I think it would be good for the comment to say something about why this
is static, and why scoped_segv_handler_restore is needed.
Otherwise this looks good to me. It's ok with those two changes.
It's fine for the 10.x branch as well.
thank you,
Tom
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] Use RAII to set the per-thread SIGSEGV handler
2021-03-12 14:27 ` Tom Tromey
@ 2021-03-12 17:30 ` Christian Biesinger
0 siblings, 0 replies; 4+ messages in thread
From: Christian Biesinger @ 2021-03-12 17:30 UTC (permalink / raw)
To: Tom Tromey; +Cc: Christian Biesinger via Gdb-patches
On Fri, Mar 12, 2021 at 3:27 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Christian" == Christian Biesinger via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Christian> PR threads/27239
>
> This should be in the ChangeLog entry.
>
> Christian> +/* The SIGSEGV handler for this thread, or NULL if there is none. GDB
> Christian> + always installs a global SIGSEGV handler, and then lets threads
> Christian> + indicate their interest in handling the signal by setting this
> Christian> + thread-local variable. */
>
> Christian> -thread_local void (*thread_local_segv_handler) (int);
> Christian> +static thread_local void (*thread_local_segv_handler) (int);
>
> I think it would be good for the comment to say something about why this
> is static, and why scoped_segv_handler_restore is needed.
>
> Otherwise this looks good to me. It's ok with those two changes.
> It's fine for the 10.x branch as well.
Thanks, I made those changes and pushed to master and gdb-10-branch.
Christian
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-12 17:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-09 14:23 [PATCH] Use RAII to set the per-thread SIGSEGV handler Christian Biesinger
2021-03-09 17:00 ` [PATCH v2] " Christian Biesinger
2021-03-12 14:27 ` Tom Tromey
2021-03-12 17:30 ` Christian Biesinger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).