Re: [PATCH v2] PR gdb/30219: Clear sync_quit_force_run in quit_force

[Simon Marchi <simon.marchi@polymtl.ca>]

On 3/24/23 18:07, Kevin Buettner wrote:
> PR 30219 shows an internal error due to a "Bad switch" in
> print_exception() in gdb/exceptions.c.  The switch in question
> contains cases for RETURN_QUIT and RETURN_ERROR, but is missing a case
> for the recently added RETURN_FORCED_QUIT.  This commit adds that case.
> 
> Making the above change allows the errant test case to pass, but does
> not fix the underlying problem, which I'll describe shortly.  Even
> though the addition of a case for RETURN_FORCED_QUIT isn't the actual
> fix, I still think it's important to add this case so that other
> situations which lead to print_exeption() being called won't generate
> that "Bad switch" internal error.
> 
> In order to understand the underlying problem, please examine
> this portion of the backtrace from the bug report:
> 
> 0x5576e4ff5780 print_exception
>         /home/smarchi/src/binutils-gdb/gdb/exceptions.c:100
> 0x5576e4ff5930 exception_print(ui_file*, gdb_exception const&)
>         /home/smarchi/src/binutils-gdb/gdb/exceptions.c:110
> 0x5576e6a896dd quit_force(int*, int)
>         /home/smarchi/src/binutils-gdb/gdb/top.c:1849
> 
> The real problem is in quit_force; here's the try/catch which
> eventually leads to the internal error:
> 
>   /* Get out of tfind mode, and kill or detach all inferiors.  */
>   try
>     {
>       disconnect_tracing ();
>       for (inferior *inf : all_inferiors ())
> 	kill_or_detach (inf, from_tty);
>     }
>   catch (const gdb_exception &ex)
>     {
>       exception_print (gdb_stderr, ex);
>     }
> 
> While running the calls in the try-block, a QUIT check is being
> performed.  This check finds that sync_quit_force_run is (still) set,
> causing a gdb_exception_forced_quit to be thrown.  The exception
> gdb_exception_forced_quit is derived from gdb_exception, causing
> exception_print to be called.  As shown by the backtrace,
> print_exception is then called, leading to the internal error.
> 
> The actual fix, also implemented by this commit, is to clear
> sync_quit_force_run along with the quit flag.  This will allow the
> various cleanup code, called by quit_force, to run without triggering
> a gdb_exception_forced_quit.  (Though, if another SIGTERM is sent to
> the gdb process, these flags will be set again and a QUIT check in the
> cleanup code will detect it and throw the exception.)
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30219
> Reviewed-by: Simon Marchi <simon.marchi@polymtl.ca>
> ---
>  gdb/exceptions.c | 1 +
>  gdb/top.c        | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/gdb/exceptions.c b/gdb/exceptions.c
> index 4ab8dce58de..8b7858578a9 100644
> --- a/gdb/exceptions.c
> +++ b/gdb/exceptions.c
> @@ -90,6 +90,7 @@ print_exception (struct ui_file *file, const struct gdb_exception &e)
>    switch (e.reason)
>      {
>      case RETURN_QUIT:
> +    case RETURN_FORCED_QUIT:
>        annotate_quit ();
>        break;
>      case RETURN_ERROR:
> diff --git a/gdb/top.c b/gdb/top.c
> index aa2a6409d98..81f74f72f61 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -1824,6 +1824,14 @@ quit_force (int *exit_arg, int from_tty)
>  {
>    int exit_code = 0;
>  
> +  /* Clear the quit flag and sync_quit_force_run so that a
> +     gdb_exception_forced_quit isn't inadvertently triggered by a QUIT
> +     check while running the various cleanup/exit code below.  Note
> +     that the call to 'check_quit_flag' clears the quit flag as a side
> +     effect.  */
> +  check_quit_flag ();
> +  sync_quit_force_run = false;
> +
>    /* An optional expression may be used to cause gdb to terminate with the
>       value of that expression.  */
>    if (exit_arg)
> -- 
> 2.40.0
> 

LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon