From 0ace6ace1bfc2982f62ec684cdb26de64aec3366 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 9 Feb 2023 13:46:28 +0000 Subject: [PATCH] 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 Change-Id: I1f10d99496a7d67c94b258e45963e83e439e1778 --- gdb/infrun.c | 45 +++++++++++ .../gdb.base/bg-exec-sigint-bp-cond.c | 33 ++++++++ .../gdb.base/bg-exec-sigint-bp-cond.exp | 77 +++++++++++++++++++ 3 files changed, 155 insertions(+) create mode 100644 gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.c create mode 100644 gdb/testsuite/gdb.base/bg-exec-sigint-bp-cond.exp 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 + + 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 . */ + +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 . + +# 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 " + + # 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 -- 2.30.2