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

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

[Kevin Buettner]

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
in addition to introducing annotate_forced_quit() which will print a
suitable annotation when annotations are turned on.

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
---
 gdb/annotate.c   | 7 +++++++
 gdb/annotate.h   | 1 +
 gdb/exceptions.c | 3 +++
 gdb/top.c        | 8 ++++++++
 4 files changed, 19 insertions(+)

diff --git a/gdb/annotate.c b/gdb/annotate.c
index 60fe6ccd5c2..36efce1af7c 100644
--- a/gdb/annotate.c
+++ b/gdb/annotate.c
@@ -282,6 +282,13 @@ annotate_quit (void)
     printf_unfiltered (("\n\032\032quit\n"));
 }
 
+void
+annotate_forced_quit (void)
+{
+  if (annotation_level > 1)
+    printf_unfiltered (("\n\032\032forced-quit\n"));
+}
+
 void
 annotate_error (void)
 {
diff --git a/gdb/annotate.h b/gdb/annotate.h
index 48fdb7a61d9..a2686d6bb51 100644
--- a/gdb/annotate.h
+++ b/gdb/annotate.h
@@ -55,6 +55,7 @@ extern void annotate_field_value (void);
 extern void annotate_field_end (void);
 
 extern void annotate_quit (void);
+extern void annotate_forced_quit (void);
 extern void annotate_error (void);
 extern void annotate_error_begin (void);
 
diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index 4ab8dce58de..e23135f1866 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -96,6 +96,9 @@ print_exception (struct ui_file *file, const struct gdb_exception &e)
       /* Assume that these are all errors.  */
       annotate_error ();
       break;
+    case RETURN_FORCED_QUIT:
+      annotate_forced_quit ();
+      break;
     default:
       internal_error (_("Bad switch."));
     }
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.39.2

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

[Simon Marchi]

On 3/10/23 18:18, 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
> in addition to introducing annotate_forced_quit() which will print a
> suitable annotation when annotations are turned on.
> 
> 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.)

Clearing the quit flag sounds right.  However, I wonder if we should go
further and disable any "quitting" while GDB is in quit_force.  Meaning
that any SIGTERM or SIGINT received while in quit_force would have no
effect.  I don't know what good could be achieved by responding to a
SIGTERM or SIGINT while GDB is already in the process of cleaning up and
exiting.

What you have is fine with me for now though, it's a clear improvement
over the current situation.

> diff --git a/gdb/annotate.c b/gdb/annotate.c
> index 60fe6ccd5c2..36efce1af7c 100644
> --- a/gdb/annotate.c
> +++ b/gdb/annotate.c
> @@ -282,6 +282,13 @@ annotate_quit (void)
>      printf_unfiltered (("\n\032\032quit\n"));
>  }
>  
> +void
> +annotate_forced_quit (void)
> +{
> +  if (annotation_level > 1)
> +    printf_unfiltered (("\n\032\032forced-quit\n"));

Hmm, this in a new annotation?  In theory, this would need to be
documented, to have a NEWS entry, etc.  And then frontends using
annotations would need to be updated to use that.  We don't really want
that, if possible, given annotations are obsolete.

Would it work to display the "quit" annotation (the one printed by
annotate_quit)?  The doc [1] says, for that annotation:

  This annotation occurs right before GDB responds to an interrupt. 

and:

  Quit and error annotations indicate that any annotations which GDB was
  in the middle of may end abruptly. For example, if a
  value-history-begin annotation is followed by a error, one cannot
  expect to receive the matching value-history-end.

I think it also works in the forced quit case.  The execution of
whatever thing was going on is interrupted, just like in the regular
quit case, it just happens that GDB also happens to exit after that.

[1] https://sourceware.org/gdb/onlinedocs/annotate.html#Errors

Simon

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

[Kevin Buettner]

On Thu, 23 Mar 2023 11:26:56 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> > diff --git a/gdb/annotate.c b/gdb/annotate.c
> > index 60fe6ccd5c2..36efce1af7c 100644
> > --- a/gdb/annotate.c
> > +++ b/gdb/annotate.c
> > @@ -282,6 +282,13 @@ annotate_quit (void)
> >      printf_unfiltered (("\n\032\032quit\n"));
> >  }
> >  
> > +void
> > +annotate_forced_quit (void)
> > +{
> > +  if (annotation_level > 1)
> > +    printf_unfiltered (("\n\032\032forced-quit\n"));  
> 
> Hmm, this in a new annotation?  In theory, this would need to be
> documented, to have a NEWS entry, etc.  And then frontends using
> annotations would need to be updated to use that.  We don't really want
> that, if possible, given annotations are obsolete.
> 
> Would it work to display the "quit" annotation (the one printed by
> annotate_quit)?  The doc [1] says, for that annotation:
> 
>   This annotation occurs right before GDB responds to an interrupt. 
> 
> and:
> 
>   Quit and error annotations indicate that any annotations which GDB was
>   in the middle of may end abruptly. For example, if a
>   value-history-begin annotation is followed by a error, one cannot
>   expect to receive the matching value-history-end.
> 
> I think it also works in the forced quit case.  The execution of
> whatever thing was going on is interrupted, just like in the regular
> quit case, it just happens that GDB also happens to exit after that.
> 
> [1] https://sourceware.org/gdb/onlinedocs/annotate.html#Errors

I agree.  I've prepared a v2 patch which I'll send shortly.

Kevin

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

[Kevin Buettner]

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

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

[Simon Marchi]

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