Don't throw quit while handling inferior events

This implements what I suggested here:

 https://inbox.sourceware.org/gdb-patches/ab97c553-f406-b094-cdf3-ba031fdea925@palves.net/

Here is the current default quit_handler, a function that ends up
called by the QUIT macro:

 void
 default_quit_handler (void)
 {
   if (check_quit_flag ())
     {
       if (target_terminal::is_ours ())
 	quit ();
       else
 	target_pass_ctrlc ();
      }
 }

As we can see above, when the inferior is running in the foreground,
then a Ctrl-C is translated into a call to target_pass_ctrlc().

The target_terminal::is_ours() case above is there to handle the
scenario where GDB has the terminal, meaning it is handling some
command the user typed, like "list", or "p a + b" or some such.

However, when the inferior is running on the background, say with
"c&", GDB also has the terminal.  Run control handling is now done in
the "background".  The CLI is responsive to user commands.  If users
type Ctrl-C, they're expecting it to interrupt whatever command they
next type in the CLI, which again, could be "list", "p a + b", etc.
It's as if background run control was handled by a separate thread,
and the Ctrl-C is meant to go to the main thread, handling the CLI.

However, when handling an event, inside fetch_inferior_event &
friends, a Ctrl-C _also_ results in a Quit exception, from the same
default_quit_handler function shown above.  This quit aborts run
control handling, breakpoint condition evaluation, etc., and may even
leave run control in an inconsistent state.

The testcase added by this patch illustrates this.  The test program
just loops a number of times calling the "foo" function.

The idea is to set a breakpoint in the "foo" function with a condition
that sends SIGINT to GDB, and then evaluates to false, which results
in the program being re-resumed in the background.  The SIGINT-sending
emulates pressing Ctrl-C just while GDB was evaluating the breakpoint
condition, except, it's more deterministic.

It looks like this:

  (gdb) p $counter = 0
  $1 = 0
  (gdb) b foo if $counter++ == 10 || $_shell("kill -SIGINT `pidof gdb`") != 0
  Breakpoint 2 at 0x555555555131: file gdb.base/bg-exec-sigint-bp-cond.c, line 21.
  (gdb) c&
  Continuing.
  (gdb)

After that background continue, the breakpoint should be hit 10 times,
and we should see 10 "Quit" being printed on the screen.  As if the
user typed Ctrl-C on the prompt a number of times with no inferior
running:

  (gdb)        <<< Ctrl-C
  (gdb) Quit   <<< Ctrl-C
  (gdb) Quit   <<< Ctrl-C
  (gdb)

However, here's what you see instead:

  (gdb) c&
  Continuing.
  (gdb) Quit
  (gdb)

Just one Quit, and nothing else.  If we look at the thread's state, we see:

  (gdb) info threads
    Id   Target Id                                            Frame
  * 1    Thread 0x7ffff7d6f740 (LWP 112192) "bg-exec-sigint-" foo () at gdb.base/bg-exec-sigint-bp-cond.c:21

So the thread stopped, but we didn't report a stop...

Issuing another continue shows the same immediate-and-silent-stop:

  (gdb) c&
  Continuing.
  (gdb) Quit
  (gdb) p $counter
  $2 = 2

As mentioned, since the run control handling, and breakpoint and
watchpoint evaluation, etc. are running in the background from the
perspective of the CLI, when users type Ctrl-C in this situation,
they're thinking of aborting whatever other command they were typing
or running at the prompt, not the run control side, not the previous
"c&" command.

So I think that we should install a custom quit_handler while inside
fetch_inferior_event, where we already disable pagination and other
things for a similar reason.  This custom quit handler does nothing if
GDB has the terminal, and forwards Ctrl-C to the inferior otherwise.

With the patch implementing that, and the same testcase, here's what
you see instead:

 (gdb) p $counter = 0
 $1 = 0
 (gdb) b foo if $counter++ == 10 || $_shell("kill -SIGINT `pidof gdb`") != 0
 Breakpoint 2 at 0x555555555131: file gdb.base/bg-exec-sigint-bp-cond.c, line 21.
 (gdb) c&
 Continuing.
 (gdb) Quit
 (gdb) Quit
 (gdb) Quit
 (gdb) Quit
 (gdb) Quit
 (gdb) Quit
 (gdb) Quit
 (gdb) Quit
 (gdb) Quit
 (gdb) Quit
 (gdb)
 Breakpoint 2, foo () at gdb.base/bg-exec-sigint-bp-cond.c:21
 21        return 0;

Approved-By: Tom Tromey <tom@tromey.com>
Change-Id: I1f10d99496a7d67c94b258e45963e83e439e1778

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e5d2b97f1dd1e0d5e15f4b07693fcc900996a334..0d7e386dc8607c7e798de25e4cff9c62166eef15 100644 (file)
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4105,6 +4105,44 @@ all_uis_on_sync_execution_starting (void)
     }
 }
 
+/* A quit_handler callback installed while we're handling inferior
+   events.  */
+
+static void
+infrun_quit_handler ()
+{
+  if (target_terminal::is_ours ())
+    {
+      /* Do nothing.
+
+        default_quit_handler would throw a quit in this case, but if
+        we're handling an event while we have the terminal, it means
+        the target is running a background execution command, and
+        thus when users press Ctrl-C, they're wanting to interrupt
+        whatever command they were executing in the command line.
+        E.g.:
+
+         (gdb) c&
+         (gdb) foo bar whatever<ctrl-c>
+
+        That Ctrl-C should clear the input line, not interrupt event
+        handling if it happens that the user types Ctrl-C at just the
+        "wrong" time!
+
+        It's as-if background event handling was handled by a
+        separate background thread.
+
+        To be clear, the Ctrl-C is not lost -- it will be processed
+        by the next QUIT call once we're out of fetch_inferior_event
+        again.  */
+    }
+  else
+    {
+      if (check_quit_flag ())
+       target_pass_ctrlc ();
+    }
+}
+
 /* Asynchronous version of wait_for_inferior.  It is called by the
    event loop whenever a change of state is detected on the file
    descriptor corresponding to the target.  It can be called more than
@@ -4133,6 +4171,13 @@ fetch_inferior_event ()
   scoped_restore save_pagination
     = make_scoped_restore (&pagination_enabled, false);
 
+  /* Install a quit handler that does nothing if we have the terminal
+     (meaning the target is running a background execution command),
+     so that Ctrl-C never interrupts GDB before the event is fully
+     handled.  */
+  scoped_restore restore_quit_handler
+    = make_scoped_restore (&quit_handler, infrun_quit_handler);
+
   /* End up with readline processing input, if necessary.  */
   {
     SCOPE_EXIT { reinstall_readline_callback_handler_cleanup (); };

diff --git a/gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.c b/gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.c
new file mode 100644 (file)
index 0000000..b1cf353
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.c
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+foo (void)
+{
+  return 0;
+}
+
+int
+main (void)
+{
+  int i;
+
+  for (i = 0; i < 30; i++)
+    foo ();
+
+  return 0;
+}

diff --git a/gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.exp b/gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.exp
new file mode 100644 (file)
index 0000000..257efb3
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.exp
@@ -0,0 +1,77 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that sending GDB a SIGINT while handling execution control
+# does not interrupt the execution control.
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Run the test.  Sets a breakpoint with a condition that sends a
+# SIGINT to GDB, and ensures that that doesn't make the breakpoint hit
+# cause a premature stop.  This emulates pressing Ctrl-C just while
+# GDB is evaluating the breakpoint condition.
+proc test {} {
+    clean_restart $::binfile
+
+    if {![runto_main]} {
+       return
+    }
+
+    delete_breakpoints
+
+    set gdb_pid [exp_pid -i [board_info host fileid]]
+
+    # Number of times the breakpoint should be hit before stopping.
+    set num_hits 10
+
+    # A counter used in the breakpoint's condition to ensure that it
+    # causes a stop after NUM_HITS hits.
+    gdb_test "p \$hit_count = 0" " = 0" "reset hit counter"
+
+    # Set a breakpoint with a condition that sends a SIGINT to GDB.  This
+    # emulates pressing Ctrl-C just while GDB is evaluating the breakpoint
+    # condition.
+    gdb_test \
+       "break foo if \$hit_count\+\+ == $num_hits || \$_shell(\"kill -SIGINT $gdb_pid\") != 0" \
+       "Breakpoint .*" \
+       "break foo if <condition>"
+
+    # Number of times we've seen GDB print "Quit" followed by the
+    # prompt.  We should see that exactly $NUM_HITS times.
+    set quit_count 0
+
+    gdb_test_multiple "c&" "SIGINT does not interrupt background execution" {
+       -re "^c&\r\nContinuing\\.\r\n$::gdb_prompt " {
+           exp_continue
+       }
+       -re "^Quit\r\n$::gdb_prompt " {
+           incr quit_count
+           verbose -log "quit_count=$quit_count"
+           exp_continue
+       }
+       -re "^\r\nBreakpoint .*return 0;" {
+           gdb_assert {$quit_count == $num_hits} $gdb_test_name
+       }
+       -re ".*Asynchronous execution not supported on this target\..*" {
+           unsupported "$gdb_test_name (asynchronous execution not supported)"
+       }
+    }
+}
+
+test