Re: [PATCH v4 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit

Pedro Alves
2023-01-30T18:57:58Z

On 2023-01-12 1:56 a.m., Kevin Buettner wrote:
> When a GDB process receives the SIGTERM signal, handle_sigterm() in
> event-top.c is called. The global variable 'sync_quit_force_run' is
> set by this signal handler. It does some other things too, but the
> setting of this global is the important bit for the SIGTERM part of
> this discussion.
>
> GDB will periodically check to see whether a Ctrl-C or SIGTERM has
> been received. This is performed via use of the QUIT macro in
> GDB's code. QUIT is defined to invoke maybe_quit(), which will be
> periodically called during any lengthy operation. This is supposed to
> ensure that the user won't have to wait too long for a Ctrl-C or
> SIGTERM to be acted upon.
>
> When a Ctrl-C / SIGINT is received, quit_handler() will decide whether
> to pass the SIGINT onto the inferior or to call quit() which causes
> gdb_exception_quit to be thrown. This exception (usually) propagates
> to the top level. Control is then returned to the top level event
> loop.
>
> At the moment, SIGTERM is handled very differently. Instead of
> throwing an exception, quit_force() is called. This does eventually
> cause GDB to exit(), but prior to that happening, the inferiors
> are killed or detached and other target related cleanup occurs.
> As shown in this discussion between Pedro Alves and myself...
>
> https://sourceware.org/pipermail/gdb-patches/2021-July/180802.html
> https://sourceware.org/pipermail/gdb-patches/2021-July/180902.html
> https://sourceware.org/pipermail/gdb-patches/2021-July/180903.html
>
> ...we found that it is possible for inferior_ptid and current_thread_
> to get out of sync. When that happens, the "current_thread_ != nullptr"
> assertion in inferior_thread() can fail resulting in a GDB internal
> error.
>
> Pedro recommended that we "let the normal quit exception propagate all
> the way to the top level, and then have the top level call quit_force
> if sync_quit_force_run is set." However, after the v2 series for this
> patch set, we tweaked that idea by introducing a new exception for
> handling SIGTERM.
>
> This commit implements the obvious part of Pedro's suggestion:
> Instead of calling quit_force from quit(), throw_forced_quit() is now
> called instead. This causes the new exception 'gdb_exception_forced_quit'
> to be thrown.
>
> At the top level, I changed catch_command_errors() and captured_main()
> to catch gdb_exception_forced_quit and then call quit_force() from the
> catch block. I also changed start_event_loop() to also catch
> gdb_exception_forced_quit; while we could also call quit_force() from
> that catch block, it's sufficient to simply rethrow the exception
> since it'll be caught by the newly added code in captured_main().
>
> Making these changes fixed the failure / regression that I was seeing
> for gdb.base/gdb-sigterm.exp when run on a machine with glibc-2.34.
> However, there are many other paths back to the top level which this
> test case does not test. I did an audit of all of the try / catch
> code in GDB in which calls in the try-block might (eventually) call
> QUIT. I found many cases where gdb_exception_quit and the new
> gdb_exception_forced_quit will be swallowed. (When using GDB, have
> you ever hit Ctrl-C and not have it do anything; if so, it could be
> due to a swallowed gdb_exception_quit in one of the cases I've
> identified.) The rest of the patches in this series deal with this
> concern.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=26761
> ---
> gdb/main.c | 12 ++++++++++++
> gdb/utils.c | 2 +-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/main.c b/gdb/main.c
> index c04d37a45f9..323d99321c7 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -410,6 +410,10 @@ start_event_loop ()
> {
> result = gdb_do_one_event ();
> }
> + catch (const gdb_exception_forced_quit &ex)
> + {
> + throw;
> + }
> catch (const gdb_exception &ex)
> {
> exception_print (gdb_stderr, ex);
> @@ -518,6 +522,10 @@ catch_command_errors (catch_command_errors_const_ftype command,
> if (do_bp_actions)
> bpstat_do_actions ();
> }
> + catch (const gdb_exception_forced_quit &e)
> + {
> + quit_force (NULL, 0);
> + }
> catch (const gdb_exception &e)
> {
> return handle_command_errors (e);
> @@ -1309,6 +1317,10 @@ captured_main (void *data)
> {
> captured_command_loop ();
> }
> + catch (const gdb_exception_forced_quit &ex)
> + {
> + quit_force (NULL, 0);
> + }

Tabs vs spaces above.

Otherwise this LGTM.

Pedro Alves

> catch (const gdb_exception &ex)
> {
> exception_print (gdb_stderr, ex);
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 734c5bf7f70..0e126cf103b 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -673,7 +673,7 @@ quit (void)
> if (sync_quit_force_run)
> {
> sync_quit_force_run = 0;
> - quit_force (NULL, 0);
> + throw_forced_quit ("SIGTERM");
> }
>
> #ifdef __MSDOS__
>