[binutils-gdb] Don't throw quit while handling inferior events

[binutils-gdb] Don't throw quit while handling inferior events

[Pedro Alves]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0ace6ace1bfc2982f62ec684cdb26de64aec3366

commit 0ace6ace1bfc2982f62ec684cdb26de64aec3366
Author: Pedro Alves <pedro@palves.net>
Date:   Thu Feb 9 13:46:28 2023 +0000

    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:
---
 gdb/infrun.c                                      | 45 +++++++++++++
 gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.c   | 33 ++++++++++
 gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.exp | 77 +++++++++++++++++++++++
 3 files changed, 155 insertions(+)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e5d2b97f1dd..0d7e386dc86 100644
--- 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
index 00000000000..b1cf35361f8
--- /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
index 00000000000..257efb337f9
--- /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