Re: [PATCH v2 2/6] Handle gdb SIGTERM via normal QUIT processing

[Pedro Alves <pedro@palves.net>]

Hi Kevin,

On 2021-08-23 12:19 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 indicated 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 ensures 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 (eventually) causes gdb_exception_quit to be thrown.  This
> exception (usually) propagates to the top level.  At that point,
> exception_print() is called to print the exception.  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."
> 
> This commit implements the obvious part of Pedro's suggestion:
> Instead of calling quit_force from quit(), throw_quit() is now called
> instead.  We leave sync_quit_force set for interrogation at a higher
> level.

With you up to here.

> 
> At the top level, I found that the first thing done in each catch
> block is to call exception_print().  Thus I placed a check to
> see whether sync_quit_force is set within exception_print().  When
> it's set, quit_force() will be called.

But here, not so much.  I don't think it's a good idea to do this
within expection_print.  For example, nothing prevents some low level
code say within linux-nat.c to try/catch and print the exception, at a
point again where we're very deep in the target stack and shouldn't assume
much of global state.

> 
> 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.  It seemed likely to me that some catch
> blocks might swallow a QUIT, not allowing it to get to the top level.
> The rest of the patches in this series deal with this concern.