public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Kevin Buettner <kevinb@redhat.com>
To: gdb-patches@sourceware.org
Cc: pedro@palves.net, simark@simark.ca, tdevries@suse.de,
	Kevin Buettner <kevinb@redhat.com>
Subject: [PATCH v4 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit
Date: Wed, 11 Jan 2023 18:56:24 -0700	[thread overview]
Message-ID: <20230112015630.32999-3-kevinb@redhat.com> (raw)
In-Reply-To: <20230112015630.32999-1-kevinb@redhat.com>

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);
+	}
       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__
-- 
2.34.3


  parent reply	other threads:[~2023-01-12  1:57 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-12  1:56 [PATCH v4 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Kevin Buettner
2023-01-12  1:56 ` [PATCH v4 1/8] Introduce gdb_exception_forced_quit Kevin Buettner
2023-01-30 18:56   ` Pedro Alves
2023-01-12  1:56 ` Kevin Buettner [this message]
2023-01-30 18:57   ` [PATCH v4 2/8] Handle gdb SIGTERM by throwing / catching gdb_exception_force_quit Pedro Alves
2023-01-12  1:56 ` [PATCH v4 3/8] Catch gdb_exception_error instead of gdb_exception (in many places) Kevin Buettner
2023-01-30 19:00   ` Pedro Alves
2023-02-16 10:52     ` Pedro Alves
2023-01-12  1:56 ` [PATCH v4 4/8] Python QUIT processing updates Kevin Buettner
2023-01-30 19:01   ` Pedro Alves
2023-02-20  2:52     ` Kevin Buettner
2023-02-23 12:50       ` Pedro Alves
2023-01-12  1:56 ` [PATCH v4 5/8] Guile " Kevin Buettner
2023-01-12  1:56 ` [PATCH v4 6/8] Call quit_force for gdb_exception_forced_quit in safe_execute_command Kevin Buettner
2023-01-30 19:01   ` Pedro Alves
2023-01-12  1:56 ` [PATCH v4 7/8] QUIT processing w/ explicit throw for gdb_exception_forced_quit Kevin Buettner
2023-01-30 19:02   ` Pedro Alves
2023-01-12  1:56 ` [PATCH v4 8/8] Forced quit cases handled by resetting sync_quit_force_run Kevin Buettner
2023-01-30 19:02   ` Pedro Alves
2023-01-12 12:37 ` [PATCH v4 0/8] Fix gdb.base/gdb-sigterm.exp failure/error Tom de Vries
2023-01-27 22:03   ` Kevin Buettner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230112015630.32999-3-kevinb@redhat.com \
    --to=kevinb@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=pedro@palves.net \
    --cc=simark@simark.ca \
    --cc=tdevries@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).