[PATCH v2 0/6] glibc-2.34: Fix gdb.base/gdb-sigterm.exp failure/error

[PATCH v2 0/6] glibc-2.34: Fix gdb.base/gdb-sigterm.exp failure/error

[Kevin Buettner]

The first patch in this series is the same as in v1.  Everything else
is new, though the problem, a regression in gdb.base/gdb-sigterm.exp
when run on a machine w/ glibc-2.34, being solved remains the same.

Kevin Buettner (6):
  Handle recursive internal problem in gdb_internal_error_resync
  Handle gdb SIGTERM via normal QUIT processing
  Catch and (re)throw gdb_exception_quit
  Python QUIT processing updates
  Guile QUIT processing updates
  QUIT processing w/ explicit sync_quit_force_run check

 gdb/ada-lang.c                   |  4 ++++
 gdb/breakpoint.c                 | 15 +++++++++++++++
 gdb/exceptions.c                 |  6 ++++++
 gdb/guile/scm-exception.c        |  4 ++++
 gdb/guile/scm-pretty-print.c     |  4 ++++
 gdb/guile/scm-type.c             |  4 ++++
 gdb/guile/scm-value.c            |  4 ++++
 gdb/i386-linux-tdep.c            |  4 ++++
 gdb/infrun.c                     |  4 ++++
 gdb/jit.c                        |  4 ++++
 gdb/mi/mi-main.c                 |  4 ++++
 gdb/objc-lang.c                  |  4 ++++
 gdb/parse.c                      |  4 ++++
 gdb/printcmd.c                   |  4 ++++
 gdb/python/py-finishbreakpoint.c |  6 ++++--
 gdb/python/py-gdb-readline.c     | 12 ++++++++----
 gdb/python/py-symbol.c           |  4 ++++
 gdb/python/py-utils.c            |  4 +++-
 gdb/python/py-value.c            |  4 ++++
 gdb/record-btrace.c              |  4 ++++
 gdb/remote-fileio.c              |  8 +++++++-
 gdb/solib.c                      |  4 ++++
 gdb/sparc64-linux-tdep.c         |  4 ++++
 gdb/testsuite/lib/gdb.exp        |  4 ++++
 gdb/utils.c                      |  5 +----
 25 files changed, 116 insertions(+), 12 deletions(-)

-- 
2.31.1

[PATCH v2 1/6] Handle recursive internal problem in gdb_internal_error_resync

[Kevin Buettner]

I came across this problem when testing gdb.base/gdb-sigterm.exp
on a machine with a pre-release version of glib-2.34 installed:

A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) Recursive internal problem.
FAIL: gdb.base/gdb-sigterm.exp: expect eof #0 (GDB internal error)
Resyncing due to internal error.
ERROR: : spawn id exp11 not open
    while executing
"expect {
-i exp11 -timeout 10
	    -re "Quit this debugging session\\? \\(y or n\\) $" {
		send_gdb "n\n" answer
		incr count
	    }
	    -re "Create..."
    ("uplevel" body line 1)
    invoked from within
"uplevel $body" NONE : spawn id exp11 not open
ERROR: Could not resync from internal error (timeout)
gdb.base/gdb-sigterm.exp: expect eof #0: stepped 9 times
UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes

I don't have a problem with the latter ERROR nor the UNRESOLVED
messages.  However the first ERROR regarding the exp11 spawn id
not being open is not especially useful.

This commit handles the "Recursive internal problem" case, avoiding
the problematic ERROR shown above.

With this commit in place, the log messages look like this instead:

A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) Recursive internal problem.
FAIL: gdb.base/gdb-sigterm.exp: expect eof #15 (GDB internal error)
Resyncing due to internal error.
ERROR: Could not resync from internal error (recursive internal problem)
gdb.base/gdb-sigterm.exp: expect eof #15: stepped 12 times
UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (gdb_internal_error_resync): Handle "Recursive
	internal problem".
---
 gdb/testsuite/lib/gdb.exp | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 6b6a70a89b0..481a9bc25c4 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -754,6 +754,10 @@ proc gdb_internal_error_resync {} {
     set count 0
     while {$count < 10} {
 	gdb_expect {
+	    -re "Recursive internal problem\\." {
+		perror "Could not resync from internal error (recursive internal problem)"
+		return 0
+	    }
 	    -re "Quit this debugging session\\? \\(y or n\\) $" {
 		send_gdb "n\n" answer
 		incr count
-- 
2.31.1

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

[Kevin Buettner]

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.

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.

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.
---
 gdb/exceptions.c | 6 ++++++
 gdb/utils.c      | 5 +----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index 32db6fe8235..d3b15dd6320 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -108,6 +108,12 @@ print_exception (struct ui_file *file, const struct gdb_exception &e)
 void
 exception_print (struct ui_file *file, const struct gdb_exception &e)
 {
+  if (sync_quit_force_run)
+    {
+      sync_quit_force_run = 0;
+      quit_force (NULL, 0);
+    }
+
   if (e.reason < 0 && e.message != NULL)
     {
       print_flush ();
diff --git a/gdb/utils.c b/gdb/utils.c
index c59c63565eb..971302ced16 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -624,10 +624,7 @@ void
 quit (void)
 {
   if (sync_quit_force_run)
-    {
-      sync_quit_force_run = 0;
-      quit_force (NULL, 0);
-    }
+    throw_quit ("SIGTERM Quit");
 
 #ifdef __MSDOS__
   /* No steenking SIGINT will ever be coming our way when the
-- 
2.31.1

[PATCH v2 3/6] Catch and (re)throw gdb_exception_quit

[Kevin Buettner]

As described in previous commit for this series, I became concerned
that there might be instances where a QUIT (due to either a SIGINT or
SIGTERM) might not cause execution to return to the top level.  In
some instances, it is okay (desired even) to swallow a Ctrl-C / SIGINT,
but I don't think there are any instances where we want an exception
handler to swallow a SIGTERM.  (Allowing this would definitely be a
deviation from the current behavior.)

I looked at all cases where an exception handler catches a
gdb_exception.  Handlers which did NOT need modification where those
which satisifed one or more of the following conditions:

  1) There is no call path to maybe_quit() in the try block.  I used a
     static analysis tool to help make this determination.  In
     instances where the tool didn't provide an answer of "yes, this
     call path can result in maybe_quit() being called", I reviewed it
     by hand.

  2) The catch block contains a throw for conditions that it
     doesn't want to handle; these "not handled" conditions
     must include the quit exception.

  3) The catch block contains a call to exception_print().  An earlier
     patch in this series modified exception_print() to check
     sync_quit_force_run (which indicates that GDB was sent a SIGTERM
     signal), calling quit_force() when set.  quit_force() does
     various cleanups and then causes GDB to exit.  Therefore, catch
     blocks which contain a call to exception_print() will cause GDB
     to terminate when GDB receives a SIGTERM signal.

Any try/catch blocks not meeting the above conditions could
potentially swallow a QUIT exception.  This commit (mostly)
adds a new catch of the following form prior to the gdb_exception
catch:

  catch (const gdb_exception_quit &ex)
    {
      throw;
    }

There is one case - the first hunk in gdb/breakpoint.c - which is
a little different, but I didn't consider it to be different enough
to split it out into its own patch.

Guile and Python QUIT processing updates are split into their own
commits.  There are also two other tricky (but somewhat related) cases
which I've placed in a separate commit.

There is one case that I examined which I could not figure out how to
handle.  It is the scoped_switch_fork_info destructor in
gdb/linux-fork.c.  It looks looks like this:

  ~scoped_switch_fork_info ()
  {
    if (m_oldfp != nullptr)
      {
	/* Switch back to inferior_ptid.  */
	try
	  {
	    remove_breakpoints ();
	    fork_load_infrun_state (m_oldfp);
	    insert_breakpoints ();
	  }
	catch (const gdb_exception &ex)
	  {
	    warning (_("Couldn't restore checkpoint state in %s: %s"),
		     target_pid_to_str (m_oldfp->ptid).c_str (),
		     ex.what ());
	  }
      }
  }

The static analysis tool determined that there is a call path
to maybe_quit():

  "_ZN23scoped_switch_fork_infoD2Ev"
    -> "_Z18remove_breakpointsv"
    -> "_ZL17remove_breakpointP11bp_location"
    -> "_ZL19remove_breakpoint_1P11bp_location16remove_bp_reason"
    -> "_Z26memory_validate_breakpointP7gdbarchP14bp_target_info"
    -> "_Z18target_read_memorymPhl"
    -> "_Z11target_readP10target_ops13target_objectPKcPhml"
    -> "_Z10maybe_quitv";

The catch block simply prints a warning, thus (potentially) swallowing
a QUIT exception.  It's not clear to me whether rethrowing the
exception is safe here.
---
 gdb/ada-lang.c           |  4 ++++
 gdb/breakpoint.c         | 15 +++++++++++++++
 gdb/i386-linux-tdep.c    |  4 ++++
 gdb/infrun.c             |  4 ++++
 gdb/jit.c                |  4 ++++
 gdb/objc-lang.c          |  4 ++++
 gdb/parse.c              |  4 ++++
 gdb/printcmd.c           |  4 ++++
 gdb/record-btrace.c      |  4 ++++
 gdb/solib.c              |  4 ++++
 gdb/sparc64-linux-tdep.c |  4 ++++
 11 files changed, 55 insertions(+)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index b098991612d..f2f6acf0bfe 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -11679,6 +11679,10 @@ should_stop_exception (const struct bp_location *bl)
       stop = value_true (evaluate_expression (ada_loc->excep_cond_expr.get ()));
       value_free_to_mark (mark);
     }
+  catch (const gdb_exception_quit &ex)
+    {
+      throw;
+    }
   catch (const gdb_exception &ex)
     {
       exception_fprintf (gdb_stderr, ex,
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5cc37430e36..6271545073b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2699,6 +2699,9 @@ insert_bp_location (struct bp_location *bl,
 	{
 	  /* Can't set the breakpoint.  */
 
+	  if (bp_excpt.reason == RETURN_QUIT)
+	    throw bp_excpt;
+
 	  /* If the target has closed then it will have deleted any
 	     breakpoints inserted within the target inferior, as a result
 	     any further attempts to interact with the breakpoint objects
@@ -5082,6 +5085,10 @@ bpstat_check_watchpoint (bpstat bs)
 	    {
 	      e = watchpoint_check (bs);
 	    }
+	  catch (const gdb_exception_quit &ex)
+	    {
+	      throw;
+	    }
 	  catch (const gdb_exception &ex)
 	    {
 	      exception_fprintf (gdb_stderr, ex,
@@ -5317,6 +5324,10 @@ bpstat_check_breakpoint_conditions (bpstat bs, thread_info *thread)
 	    {
 	      condition_result = breakpoint_cond_eval (cond);
 	    }
+	  catch (const gdb_exception_quit &ex)
+	    {
+	      throw;
+	    }
 	  catch (const gdb_exception &ex)
 	    {
 	      exception_fprintf (gdb_stderr, ex,
@@ -14385,6 +14396,10 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition,
 	  bpt->enable_state = bp_enabled;
 	  update_watchpoint (w, 1 /* reparse */);
 	}
+      catch (const gdb_exception_quit &ex)
+	{
+	  throw;
+	}
       catch (const gdb_exception &e)
 	{
 	  bpt->enable_state = orig_enable_state;
diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
index c0df916ce6d..73b7b465b78 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -415,6 +415,10 @@ i386_linux_report_signal_info (struct gdbarch *gdbarch, struct ui_out *uiout,
       access
 	= parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr");
     }
+  catch (const gdb_exception_quit &ex)
+    {
+      throw;
+    }
   catch (const gdb_exception &exception)
     {
       return;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8cfd58b267b..b72bb6d9cb1 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -8634,6 +8634,10 @@ normal_stop (void)
 	{
 	  execute_cmd_pre_hook (stop_command);
 	}
+      catch (const gdb_exception_quit &ex)
+	{
+	  throw;
+	}
       catch (const gdb_exception &ex)
 	{
 	  exception_fprintf (gdb_stderr, ex,
diff --git a/gdb/jit.c b/gdb/jit.c
index b3c52c02eee..f29e8390516 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -655,6 +655,10 @@ jit_reader_try_read_symtab (struct jit_code_entry *code_entry,
 			      code_entry->symfile_size))
 	status = 0;
     }
+  catch (const gdb_exception_quit &ex)
+    {
+      throw;
+    }
   catch (const gdb_exception &e)
     {
       status = 0;
diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
index 077ac772f79..e8ed0cf1c43 100644
--- a/gdb/objc-lang.c
+++ b/gdb/objc-lang.c
@@ -1277,6 +1277,10 @@ find_objc_msgcall_submethod (int (*f) (CORE_ADDR, CORE_ADDR *),
       if (f (pc, new_pc) == 0)
 	return 1;
     }
+  catch (const gdb_exception_quit &ex)
+    {
+      throw;
+    }
   catch (const gdb_exception &ex)
     {
       exception_fprintf (gdb_stderr, ex,
diff --git a/gdb/parse.c b/gdb/parse.c
index b2f23eb67bb..632841c9f79 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -509,6 +509,10 @@ parse_exp_in_context (const char **stringptr, CORE_ADDR pc,
     {
       lang->parser (&ps);
     }
+  catch (const gdb_exception_quit &ex)
+    {
+      throw;
+    }
   catch (const gdb_exception &except)
     {
       /* If parsing for completion, allow this to succeed; but if no
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 3cd42f817f5..883492ccc4f 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -2094,6 +2094,10 @@ do_one_display (struct display *d)
 	  d->exp = parse_expression (d->exp_string.c_str (), &tracker);
 	  d->block = tracker.block ();
 	}
+      catch (const gdb_exception_quit &ex)
+	{
+	  throw;
+	}
       catch (const gdb_exception &ex)
 	{
 	  /* Can't re-parse the expression.  Disable this display item.  */
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index bf899ebeb24..1c81f285570 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2953,6 +2953,10 @@ cmd_record_btrace_start (const char *args, int from_tty)
     {
       execute_command ("target record-btrace", from_tty);
     }
+  catch (const gdb_exception_quit &ex)
+    {
+      throw;
+    }
   catch (const gdb_exception &exception)
     {
       record_btrace_conf.format = BTRACE_FORMAT_BTS;
diff --git a/gdb/solib.c b/gdb/solib.c
index 317f7eb485e..fff69de910b 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -736,6 +736,10 @@ update_solib_list (int from_tty)
 	    {
 	      ops->open_symbol_file_object (from_tty);
 	    }
+	  catch (const gdb_exception_quit &ex)
+	    {
+	      throw;
+	    }
 	  catch (const gdb_exception &ex)
 	    {
 	      exception_fprintf (gdb_stderr, ex,
diff --git a/gdb/sparc64-linux-tdep.c b/gdb/sparc64-linux-tdep.c
index ea692ac00ef..cc36c1b79bb 100644
--- a/gdb/sparc64-linux-tdep.c
+++ b/gdb/sparc64-linux-tdep.c
@@ -139,6 +139,10 @@ sparc64_linux_report_signal_info (struct gdbarch *gdbarch, struct ui_out *uiout,
       if (si_code >= SEGV_ACCADI && si_code <= SEGV_ADIPERR)
 	addr = parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr");
     }
+  catch (const gdb_exception_quit &ex)
+    {
+      throw;
+    }
   catch (const gdb_exception &exception)
     {
       return;
-- 
2.31.1

[PATCH v2 4/6] Python QUIT processing updates

[Kevin Buettner]

See the previous patch in this series for the motivation behind these
updates.

This commit contains updates to Python's QUIT handling.  Specifically,
it makes sure that a SIGTERM sent to GDB will be propagated to the top
level.  While there are instances where we want to allow python code
to catch Ctrl-C / SIGINT (and possibly) swallow it, I don't think we
want that behavior for SIGTERM.  Therefore, there are cases where
sync_quit_force_run is checked explicitly.  In other cases, I
was able to defer the decision to GDB_PY_HANDLE_EXCEPTION.  (My goal
was to minimize explicit references to sync_quit_force_run.)
---
 gdb/python/py-finishbreakpoint.c |  6 ++++--
 gdb/python/py-gdb-readline.c     | 12 ++++++++----
 gdb/python/py-symbol.c           |  4 ++++
 gdb/python/py-utils.c            |  4 +++-
 gdb/python/py-value.c            |  4 ++++
 5 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index 1d8373d807e..120e40db6eb 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -270,8 +270,10 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
     }
   catch (const gdb_exception &except)
     {
-      /* Just swallow.  Either the return type or the function value
-	 remain NULL.  */
+      /* Swallow everything except for a SIGTERM sent to GDB.  Either the
+         return type or the function value remain NULL.  */
+      if (sync_quit_force_run)
+	throw;
     }
 
   if (self_bpfinish->return_type == NULL || self_bpfinish->function_value == NULL)
diff --git a/gdb/python/py-gdb-readline.c b/gdb/python/py-gdb-readline.c
index 978c3a8b185..ba1dad931d2 100644
--- a/gdb/python/py-gdb-readline.c
+++ b/gdb/python/py-gdb-readline.c
@@ -45,12 +45,16 @@ gdbpy_readline_wrapper (FILE *sys_stdin, FILE *sys_stdout,
       p = command_line_input (prompt, "python");
     }
   /* Handle errors by raising Python exceptions.  */
+  catch (const gdb_exception_quit &except)
+    {
+      /* Propagate exception for SIGTERM.  */
+      if (sync_quit_force_run)
+        throw;
+      /* Otherwise, return for user interrupt (Ctrl-C).  */
+      return NULL;
+    }
   catch (const gdb_exception &except)
     {
-      /* Detect user interrupt (Ctrl-C).  */
-      if (except.reason == RETURN_QUIT)
-	return NULL;
-
       /* The thread state is nulled during gdbpy_readline_wrapper,
 	 with the original value saved in the following undocumented
 	 variable (see Python's Parser/myreadline.c and
diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
index d44b55ed5a9..89bddefaa1c 100644
--- a/gdb/python/py-symbol.c
+++ b/gdb/python/py-symbol.c
@@ -501,6 +501,10 @@ gdbpy_lookup_static_symbol (PyObject *self, PyObject *args, PyObject *kw)
 	= get_selected_frame (_("No frame selected."));
       block = get_frame_block (selected_frame, NULL);
     }
+  catch (const gdb_exception_quit &except)
+    {
+      GDB_PY_HANDLE_EXCEPTION (except);
+    }
   catch (const gdb_exception &except)
     {
       /* Nothing.  */
diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
index 10c4173efcd..d6d8cb82e96 100644
--- a/gdb/python/py-utils.c
+++ b/gdb/python/py-utils.c
@@ -233,7 +233,9 @@ gdbpy_convert_exception (const struct gdb_exception &exception)
 {
   PyObject *exc_class;
 
-  if (exception.reason == RETURN_QUIT)
+  if (sync_quit_force_run)
+    throw exception;
+  else if (exception.reason == RETURN_QUIT)
     exc_class = PyExc_KeyboardInterrupt;
   else if (exception.error == MEMORY_ERROR)
     exc_class = gdbpy_gdb_memory_error;
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 8df8a15f8d6..cfc7a87d766 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -350,6 +350,10 @@ valpy_get_address (PyObject *self, void *closure)
 	  res_val = value_addr (val_obj->value);
 	  val_obj->address = value_to_value_object (res_val);
 	}
+      catch (const gdb_exception_quit &except)
+	{
+	  GDB_PY_HANDLE_EXCEPTION (except);
+	}
       catch (const gdb_exception &except)
 	{
 	  val_obj->address = Py_None;
-- 
2.31.1

[PATCH v2 5/6] Guile QUIT processing updates

[Kevin Buettner]

This commit contains QUIT processing updates for GDB's Guile support.
As with the Python updates, we want the extension language to be able
to handle Ctrl-C / SIGINT if it wants, but we don't want to allow it
to swallow a SIGTERM.

This commit changes gdbscm_throw_gdb_exception to check for
sync_quit_force_run, throwing a suitable exception when it
is set.  (Note that a gdbscm_gdb_exception must be converted to
a gdb_exception.)

The other changes are straightforward: catch blocks for
gdb_exception_quit are added in a few places.
---
 gdb/guile/scm-exception.c    | 4 ++++
 gdb/guile/scm-pretty-print.c | 4 ++++
 gdb/guile/scm-type.c         | 4 ++++
 gdb/guile/scm-value.c        | 4 ++++
 4 files changed, 16 insertions(+)

diff --git a/gdb/guile/scm-exception.c b/gdb/guile/scm-exception.c
index b62eaebfda6..97ee6a4e53e 100644
--- a/gdb/guile/scm-exception.c
+++ b/gdb/guile/scm-exception.c
@@ -456,6 +456,10 @@ gdbscm_scm_from_gdb_exception (const gdbscm_gdb_exception &exception)
 void
 gdbscm_throw_gdb_exception (gdbscm_gdb_exception exception)
 {
+  /* Throw to higher level for SIGTERM sent to GDB.  */
+  if (sync_quit_force_run)
+    throw_quit ("%s", exception.message);
+
   SCM scm_exception = gdbscm_scm_from_gdb_exception (exception);
   xfree (exception.message);
   gdbscm_throw (scm_exception);
diff --git a/gdb/guile/scm-pretty-print.c b/gdb/guile/scm-pretty-print.c
index 96a3ef584c9..7ac5bff8d6c 100644
--- a/gdb/guile/scm-pretty-print.c
+++ b/gdb/guile/scm-pretty-print.c
@@ -558,6 +558,10 @@ ppscm_pretty_print_one_value (SCM printer, struct value **out_value,
 	    (_("invalid result from pretty-printer to-string"), result);
 	}
     }
+  catch (const gdb_exception_quit &except)
+    {
+      GDBSCM_HANDLE_GDB_EXCEPTION (unpack (except));
+    }
   catch (const gdb_exception &except)
     {
     }
diff --git a/gdb/guile/scm-type.c b/gdb/guile/scm-type.c
index d65102b01c7..564d574beb9 100644
--- a/gdb/guile/scm-type.c
+++ b/gdb/guile/scm-type.c
@@ -111,6 +111,10 @@ tyscm_type_name (struct type *type)
       LA_PRINT_TYPE (type, "", &stb, -1, 0, &type_print_raw_options);
       return std::move (stb.string ());
     }
+  catch (const gdb_exception_quit &except)
+    {
+      GDBSCM_HANDLE_GDB_EXCEPTION (unpack (except));
+    }
   catch (const gdb_exception &except)
     {
       excp = gdbscm_scm_from_gdb_exception (unpack (except));
diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c
index d4d76df0411..d1962e1a562 100644
--- a/gdb/guile/scm-value.c
+++ b/gdb/guile/scm-value.c
@@ -416,6 +416,10 @@ gdbscm_value_address (SCM self)
 	    {
 	      address = vlscm_scm_from_value (value_addr (value));
 	    }
+	  catch (const gdb_exception_quit &except)
+	    {
+	      GDBSCM_HANDLE_GDB_EXCEPTION (unpack (except));
+	    }
 	  catch (const gdb_exception &except)
 	    {
 	    }
-- 
2.31.1

[PATCH v2 6/6] QUIT processing w/ explicit sync_quit_force_run check

[Kevin Buettner]

Aside from the extension language updates, I found two other
cases which required explicit checks of sync_quit_force_run...

1) remote_fileio_request in gdb/remote-fileio.c:

The try block calls do_remote_fileio_request which can (in turn)
call one of the functions in remote_fio_func_map[].  Taking the
first one, remote_fileio_func_open(), we have the following call
path to maybe_quit():

  "_ZL23remote_fileio_func_openP13remote_targetPc"
    -> "_Z18target_read_memorymPhl"
    -> "_Z11target_readP10target_ops13target_objectPKcPhml"
    -> "_Z10maybe_quitv";

Since there is a path to maybe_quit(), we must ensure that the
catch block is not permitted to swallow a QUIT representing a
SIGTERM.

However, for this case, we must take care not to change the way
that Ctrl-C / SIGINT is handled; we want to send a suitable
EINTR reply to the remote target should that happen.  That
being the case, I added an explicit check for sync_quit_force_run;
if set, it rethrows the exception.

2) mi_execute_command in gdb/mi/mi-main.c:

The try block calls captured_mi_execute_command(); there exists
a call path to maybe_quit():

  "_ZL27captured_mi_execute_commandP6ui_outP8mi_parse"
    -> "_ZL14mi_cmd_executeP8mi_parse"
    -> "_Z17get_current_framev"
    -> "_ZL23get_prev_frame_always_1P10frame_info"
    -> "_ZL30frame_register_unwind_locationP10frame_infoiPiP9lval_typePmS1_"
    -> "_Z21frame_register_unwindP10frame_infoiPiS1_P9lval_typePmS1_Ph"
    -> "_Z24value_entirely_availableP5value"
    -> "_Z16value_fetch_lazyP5value"
    -> "_ZL23value_fetch_lazy_memoryP5value"
    -> "_Z17read_value_memoryP5valuelimPhm"
    -> "_Z10maybe_quitv";

That being the case, we can't allow the exception handler (catch block)
to swallow a gdb_exception_quit for SIGTERM.  However, it does seem
reasonable to output the exception via the mi interface so that some
suitable message regarding SIGTERM might be printed; therefore, I
do the check of sync_quit_force_run and the potential throw at the
end of the catch block.

(While doing all of this, it's occurred to me that these checks of the
global could be avoided by introducing a new exception for SIGTERM.
I haven't explored the implications of doing this though.)
---
 gdb/mi/mi-main.c    | 4 ++++
 gdb/remote-fileio.c | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index e293dddc08d..74a02f2b58d 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1962,6 +1962,10 @@ mi_execute_command (const char *cmd, int from_tty)
 	     somewhere.  */
 	  mi_print_exception (command->token, result);
 	  mi_out_rewind (current_uiout);
+
+	  /* Throw to a higher level catch for SIGTERM sent to GDB.  */
+	  if (sync_quit_force_run)
+	    throw;
 	}
 
       bpstat_do_actions ();
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 9765093a723..e4965acaad6 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -1194,7 +1194,13 @@ remote_fileio_request (remote_target *remote, char *buf, int ctrlc_pending_p)
       catch (const gdb_exception &ex)
 	{
 	  if (ex.reason == RETURN_QUIT)
-	    remote_fileio_reply (remote, -1, FILEIO_EINTR);
+	    {
+	      /* Throw to a higher level catch for SIGTERM sent to GDB.  */
+	      if (sync_quit_force_run)
+		throw;
+
+	      remote_fileio_reply (remote, -1, FILEIO_EINTR);
+	    }
 	  else
 	    remote_fileio_reply (remote, -1, FILEIO_EIO);
 	}
-- 
2.31.1

Re: [PATCH v2 0/6] glibc-2.34: Fix gdb.base/gdb-sigterm.exp failure/error

[Kevin Buettner]

Ping.

On Sun, 22 Aug 2021 16:19:54 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> The first patch in this series is the same as in v1.  Everything else
> is new, though the problem, a regression in gdb.base/gdb-sigterm.exp
> when run on a machine w/ glibc-2.34, being solved remains the same.
> 
> Kevin Buettner (6):
>   Handle recursive internal problem in gdb_internal_error_resync
>   Handle gdb SIGTERM via normal QUIT processing
>   Catch and (re)throw gdb_exception_quit
>   Python QUIT processing updates
>   Guile QUIT processing updates
>   QUIT processing w/ explicit sync_quit_force_run check
> 
>  gdb/ada-lang.c                   |  4 ++++
>  gdb/breakpoint.c                 | 15 +++++++++++++++
>  gdb/exceptions.c                 |  6 ++++++
>  gdb/guile/scm-exception.c        |  4 ++++
>  gdb/guile/scm-pretty-print.c     |  4 ++++
>  gdb/guile/scm-type.c             |  4 ++++
>  gdb/guile/scm-value.c            |  4 ++++
>  gdb/i386-linux-tdep.c            |  4 ++++
>  gdb/infrun.c                     |  4 ++++
>  gdb/jit.c                        |  4 ++++
>  gdb/mi/mi-main.c                 |  4 ++++
>  gdb/objc-lang.c                  |  4 ++++
>  gdb/parse.c                      |  4 ++++
>  gdb/printcmd.c                   |  4 ++++
>  gdb/python/py-finishbreakpoint.c |  6 ++++--
>  gdb/python/py-gdb-readline.c     | 12 ++++++++----
>  gdb/python/py-symbol.c           |  4 ++++
>  gdb/python/py-utils.c            |  4 +++-
>  gdb/python/py-value.c            |  4 ++++
>  gdb/record-btrace.c              |  4 ++++
>  gdb/remote-fileio.c              |  8 +++++++-
>  gdb/solib.c                      |  4 ++++
>  gdb/sparc64-linux-tdep.c         |  4 ++++
>  gdb/testsuite/lib/gdb.exp        |  4 ++++
>  gdb/utils.c                      |  5 +----
>  25 files changed, 116 insertions(+), 12 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH v2 1/6] Handle recursive internal problem in gdb_internal_error_resync

[Pedro Alves]

On 2021-08-23 12:19 a.m., Kevin Buettner wrote:
> I came across this problem when testing gdb.base/gdb-sigterm.exp
> on a machine with a pre-release version of glib-2.34 installed:
> 
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) Recursive internal problem.
> FAIL: gdb.base/gdb-sigterm.exp: expect eof #0 (GDB internal error)
> Resyncing due to internal error.
> ERROR: : spawn id exp11 not open
>     while executing
> "expect {
> -i exp11 -timeout 10
> 	    -re "Quit this debugging session\\? \\(y or n\\) $" {
> 		send_gdb "n\n" answer
> 		incr count
> 	    }
> 	    -re "Create..."
>     ("uplevel" body line 1)
>     invoked from within
> "uplevel $body" NONE : spawn id exp11 not open
> ERROR: Could not resync from internal error (timeout)
> gdb.base/gdb-sigterm.exp: expect eof #0: stepped 9 times
> UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
> 
> I don't have a problem with the latter ERROR nor the UNRESOLVED
> messages.  However the first ERROR regarding the exp11 spawn id
> not being open is not especially useful.
> 
> This commit handles the "Recursive internal problem" case, avoiding
> the problematic ERROR shown above.
> 
> With this commit in place, the log messages look like this instead:
> 
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) Recursive internal problem.
> FAIL: gdb.base/gdb-sigterm.exp: expect eof #15 (GDB internal error)
> Resyncing due to internal error.
> ERROR: Could not resync from internal error (recursive internal problem)
> gdb.base/gdb-sigterm.exp: expect eof #15: stepped 12 times
> UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
> 
> gdb/testsuite/ChangeLog:
> 
> 	* lib/gdb.exp (gdb_internal_error_resync): Handle "Recursive
> 	internal problem".

LGTM.

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

[Pedro Alves]

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.

Re: [PATCH v2 3/6] Catch and (re)throw gdb_exception_quit

[Pedro Alves]

On 2021-08-23 12:20 a.m., Kevin Buettner wrote:

> There is one case that I examined which I could not figure out how to
> handle.  It is the scoped_switch_fork_info destructor in
> gdb/linux-fork.c.  It looks looks like this:
> 
>   ~scoped_switch_fork_info ()
>   {
>     if (m_oldfp != nullptr)
>       {
> 	/* Switch back to inferior_ptid.  */
> 	try
> 	  {
> 	    remove_breakpoints ();
> 	    fork_load_infrun_state (m_oldfp);
> 	    insert_breakpoints ();
> 	  }
> 	catch (const gdb_exception &ex)
> 	  {
> 	    warning (_("Couldn't restore checkpoint state in %s: %s"),
> 		     target_pid_to_str (m_oldfp->ptid).c_str (),
> 		     ex.what ());
> 	  }
>       }
>   }
> 
> The static analysis tool determined that there is a call path
> to maybe_quit():
> 
>   "_ZN23scoped_switch_fork_infoD2Ev"
>     -> "_Z18remove_breakpointsv"
>     -> "_ZL17remove_breakpointP11bp_location"
>     -> "_ZL19remove_breakpoint_1P11bp_location16remove_bp_reason"
>     -> "_Z26memory_validate_breakpointP7gdbarchP14bp_target_info"
>     -> "_Z18target_read_memorymPhl"
>     -> "_Z11target_readP10target_ops13target_objectPKcPhml"
>     -> "_Z10maybe_quitv";
> 
> The catch block simply prints a warning, thus (potentially) swallowing
> a QUIT exception.  It's not clear to me whether rethrowing the
> exception is safe here.


No, it's not safe -- you'd crash gdb.  Destructors by default are noexcept, so if you let
an exception escape a destructor the C++ runtime calls std::terminate().

I'm thinking we could instead do:

~foo::foo ()
{
  try
    {
       ... destruction code here ...
    }
  catch (const gdb_exception_quit &ex)
    {
      /* Can't let this escape the dtor, but we also don't want to lose it.
         Set the global quit flag so that we handle it later once we
         get to a QUIT.  */
      set_quit_flag ();
    }
  catch (const gdb_exception &ex)
    {
       warning (....);
    }
}

we could even wrap this in a function that take gdb::function_view to avoid
duplicating the try/catch all over the place.  Like:

void
wrap_dtor (gdb::function_view<void ()> body)
{
  try
    {
       body ();
    }
  catch (const gdb_exception_quit &ex)
    {
      /* Can't handle now.  Set the global flag so that we handle it
         later once we get to a QUIT.  */
      set_quit_flag ();
    }
  catch (const gdb_exception &ex)
    {
       warning (....);
    }
}

~foo::foo ()
{
   wrap_dtor ([] ()
    {
       ... destruction code here ...
    });
}


> ---
>  gdb/ada-lang.c           |  4 ++++
>  gdb/breakpoint.c         | 15 +++++++++++++++
>  gdb/i386-linux-tdep.c    |  4 ++++
>  gdb/infrun.c             |  4 ++++
>  gdb/jit.c                |  4 ++++
>  gdb/objc-lang.c          |  4 ++++
>  gdb/parse.c              |  4 ++++
>  gdb/printcmd.c           |  4 ++++
>  gdb/record-btrace.c      |  4 ++++
>  gdb/solib.c              |  4 ++++
>  gdb/sparc64-linux-tdep.c |  4 ++++
>  11 files changed, 55 insertions(+)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index b098991612d..f2f6acf0bfe 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -11679,6 +11679,10 @@ should_stop_exception (const struct bp_location *bl)
>        stop = value_true (evaluate_expression (ada_loc->excep_cond_expr.get ()));
>        value_free_to_mark (mark);
>      }
> +  catch (const gdb_exception_quit &ex)
> +    {
> +      throw;
> +    }
>    catch (const gdb_exception &ex)

Why not just replace the existing:

    catch (const gdb_exception &ex)

with:

    catch (const gdb_exception_error &ex)

?

>      {
>        exception_fprintf (gdb_stderr, ex,
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 5cc37430e36..6271545073b 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2699,6 +2699,9 @@ insert_bp_location (struct bp_location *bl,
>  	{
>  	  /* Can't set the breakpoint.  */
>  
> +	  if (bp_excpt.reason == RETURN_QUIT)
> +	    throw bp_excpt;
> +
>  	  /* If the target has closed then it will have deleted any
>  	     breakpoints inserted within the target inferior, as a result
>  	     any further attempts to interact with the breakpoint objects
> @@ -5082,6 +5085,10 @@ bpstat_check_watchpoint (bpstat bs)
>  	    {
>  	      e = watchpoint_check (bs);
>  	    }
> +	  catch (const gdb_exception_quit &ex)
> +	    {
> +	      throw;
> +	    }
>  	  catch (const gdb_exception &ex)

Same question here.

>  	    {
>  	      exception_fprintf (gdb_stderr, ex,
> @@ -5317,6 +5324,10 @@ bpstat_check_breakpoint_conditions (bpstat bs, thread_info *thread)
>  	    {
>  	      condition_result = breakpoint_cond_eval (cond);
>  	    }
> +	  catch (const gdb_exception_quit &ex)
> +	    {
> +	      throw;
> +	    }
>  	  catch (const gdb_exception &ex)

Ditto.

>  	    {
>  	      exception_fprintf (gdb_stderr, ex,
> @@ -14385,6 +14396,10 @@ enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition,
>  	  bpt->enable_state = bp_enabled;
>  	  update_watchpoint (w, 1 /* reparse */);
>  	}
> +      catch (const gdb_exception_quit &ex)
> +	{
> +	  throw;
> +	}
>        catch (const gdb_exception &e)

Ditto.  Same for the remainder of the patch.

Re: [PATCH v2 4/6] Python QUIT processing updates

[Pedro Alves]

On 2021-08-23 12:20 a.m., Kevin Buettner via Gdb-patches wrote:
> See the previous patch in this series for the motivation behind these
> updates.
> 
> This commit contains updates to Python's QUIT handling.  Specifically,
> it makes sure that a SIGTERM sent to GDB will be propagated to the top
> level.  While there are instances where we want to allow python code
> to catch Ctrl-C / SIGINT (and possibly) swallow it, I don't think we
> want that behavior for SIGTERM.  Therefore, there are cases where
> sync_quit_force_run is checked explicitly.  In other cases, I
> was able to defer the decision to GDB_PY_HANDLE_EXCEPTION.  (My goal
> was to minimize explicit references to sync_quit_force_run.)

Something to ponder about is whether having a distinct gdb_exception_sigterm
would be cleaner.

> ---
>  gdb/python/py-finishbreakpoint.c |  6 ++++--
>  gdb/python/py-gdb-readline.c     | 12 ++++++++----
>  gdb/python/py-symbol.c           |  4 ++++
>  gdb/python/py-utils.c            |  4 +++-
>  gdb/python/py-value.c            |  4 ++++
>  5 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index 1d8373d807e..120e40db6eb 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -270,8 +270,10 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>      }
>    catch (const gdb_exception &except)
>      {
> -      /* Just swallow.  Either the return type or the function value
> -	 remain NULL.  */
> +      /* Swallow everything except for a SIGTERM sent to GDB.  Either the
> +         return type or the function value remain NULL.  */
> +      if (sync_quit_force_run)
> +	throw;

AFAIK, it is not safe to let C++ exceptions cross the Python runtime, as Python
is written in C.  The issue may be papered over on x86-64 Linux and there
everything is built with -fasynchronous-unwind-tables.  Even if it looks like
it works, Python doesn't see the exception at all so isn't given a chance to
properly cleanup as the exception unwinds the stack.

The same concern applies to the rest of the patch.

Re: [PATCH v2 5/6] Guile QUIT processing updates

[Pedro Alves]

The concern raised in the Python patch applies here just as well -- can't let C++ exceptions cross Guile.

Re: [PATCH v2 6/6] QUIT processing w/ explicit sync_quit_force_run check

[Pedro Alves]

On 2021-08-23 12:20 a.m., Kevin Buettner wrote:
> Aside from the extension language updates, I found two other
> cases which required explicit checks of sync_quit_force_run...
> 
> 1) remote_fileio_request in gdb/remote-fileio.c:
> 
> The try block calls do_remote_fileio_request which can (in turn)
> call one of the functions in remote_fio_func_map[].  Taking the
> first one, remote_fileio_func_open(), we have the following call
> path to maybe_quit():
> 
>   "_ZL23remote_fileio_func_openP13remote_targetPc"
>     -> "_Z18target_read_memorymPhl"
>     -> "_Z11target_readP10target_ops13target_objectPKcPhml"
>     -> "_Z10maybe_quitv";

These would be easier to read if you pass them through c++filt.

$ cat /tmp/foo | c++filt 

  "remote_fileio_func_open(remote_target*, char*)"
    -> "target_read_memory(unsigned long, unsigned char*, long)"
    -> "target_read(target_ops*, target_object, char const*, unsigned char*, unsigned long, long)"
    -> "maybe_quit()";

> 
> Since there is a path to maybe_quit(), we must ensure that the
> catch block is not permitted to swallow a QUIT representing a
> SIGTERM.
> 
> However, for this case, we must take care not to change the way
> that Ctrl-C / SIGINT is handled; we want to send a suitable
> EINTR reply to the remote target should that happen.  That
> being the case, I added an explicit check for sync_quit_force_run;
> if set, it rethrows the exception.

Again makes me wonder whether a separate exception type for sigterm
wouldn't address this whole thing better.

> 
> 2) mi_execute_command in gdb/mi/mi-main.c:
> 
> The try block calls captured_mi_execute_command(); there exists
> a call path to maybe_quit():
> 
>   "_ZL27captured_mi_execute_commandP6ui_outP8mi_parse"
>     -> "_ZL14mi_cmd_executeP8mi_parse"
>     -> "_Z17get_current_framev"
>     -> "_ZL23get_prev_frame_always_1P10frame_info"
>     -> "_ZL30frame_register_unwind_locationP10frame_infoiPiP9lval_typePmS1_"
>     -> "_Z21frame_register_unwindP10frame_infoiPiS1_P9lval_typePmS1_Ph"
>     -> "_Z24value_entirely_availableP5value"
>     -> "_Z16value_fetch_lazyP5value"
>     -> "_ZL23value_fetch_lazy_memoryP5value"
>     -> "_Z17read_value_memoryP5valuelimPhm"
>     -> "_Z10maybe_quitv";
> 
> That being the case, we can't allow the exception handler (catch block)
> to swallow a gdb_exception_quit for SIGTERM.  However, it does seem
> reasonable to output the exception via the mi interface so that some
> suitable message regarding SIGTERM might be printed; therefore, I
> do the check of sync_quit_force_run and the potential throw at the
> end of the catch block.
> 
> (While doing all of this, it's occurred to me that these checks of the
> global could be avoided by introducing a new exception for SIGTERM.
> I haven't explored the implications of doing this though.)

Ah-ha!  Yeah.

If we don't do that, then this patch seems fine.

Re: [PATCH v2 1/6] Handle recursive internal problem in gdb_internal_error_resync

[Kevin Buettner]

I've pushed this commit.

On Mon, 27 Sep 2021 18:38:40 +0100
Pedro Alves <pedro@palves.net> wrote:

> On 2021-08-23 12:19 a.m., Kevin Buettner wrote:
> > I came across this problem when testing gdb.base/gdb-sigterm.exp
> > on a machine with a pre-release version of glib-2.34 installed:
> > 
> > A problem internal to GDB has been detected,
> > further debugging may prove unreliable.
> > Quit this debugging session? (y or n) Recursive internal problem.
> > FAIL: gdb.base/gdb-sigterm.exp: expect eof #0 (GDB internal error)
> > Resyncing due to internal error.
> > ERROR: : spawn id exp11 not open
> >     while executing
> > "expect {
> > -i exp11 -timeout 10
> > 	    -re "Quit this debugging session\\? \\(y or n\\) $" {
> > 		send_gdb "n\n" answer
> > 		incr count
> > 	    }
> > 	    -re "Create..."
> >     ("uplevel" body line 1)
> >     invoked from within
> > "uplevel $body" NONE : spawn id exp11 not open
> > ERROR: Could not resync from internal error (timeout)
> > gdb.base/gdb-sigterm.exp: expect eof #0: stepped 9 times
> > UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
> > 
> > I don't have a problem with the latter ERROR nor the UNRESOLVED
> > messages.  However the first ERROR regarding the exp11 spawn id
> > not being open is not especially useful.
> > 
> > This commit handles the "Recursive internal problem" case, avoiding
> > the problematic ERROR shown above.
> > 
> > With this commit in place, the log messages look like this instead:
> > 
> > A problem internal to GDB has been detected,
> > further debugging may prove unreliable.
> > Quit this debugging session? (y or n) Recursive internal problem.
> > FAIL: gdb.base/gdb-sigterm.exp: expect eof #15 (GDB internal error)
> > Resyncing due to internal error.
> > ERROR: Could not resync from internal error (recursive internal problem)
> > gdb.base/gdb-sigterm.exp: expect eof #15: stepped 12 times
> > UNRESOLVED: gdb.base/gdb-sigterm.exp: 50 SIGTERM passes
> > 
> > gdb/testsuite/ChangeLog:
> > 
> > 	* lib/gdb.exp (gdb_internal_error_resync): Handle "Recursive
> > 	internal problem".  
> 
> LGTM.
>