* [PATCH 1/5] Fix swallowed "Quit" when inserting breakpoints
2017-11-06 23:27 [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues Pedro Alves
@ 2017-11-06 23:27 ` Pedro Alves
2017-11-06 23:27 ` [PATCH 2/5] Fix stdin ending up not registered after a Quit Pedro Alves
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-11-06 23:27 UTC (permalink / raw)
To: gdb-patches
If GDB is inserting a breakpoint and you type Ctrl-C at the exact
"right" time, you'll hit a QUIT call in target_read, and the
breakpoint insertion is cancelled. However, the related TRY/CATCH
code in insert_bp_location does:
CATCH (e, RETURN_MASK_ALL)
{
bp_err = e.error;
bp_err_message = e.message;
}
The problem with that is that a RETURN_QUIT exception has e.error ==
0, which means that further below, in the places that check for error
with:
if (bp_err != GDB_NO_ERROR)
because GDB_NO_ERROR == 0, GDB continues as if the breakpoint was
inserted succesfully, and resumes the inferior. Since the breakpoint
wasn't inserted the inferior runs free, out of our control...
Fix this by having insert_bp_location store a copy of the whole
exception instead of just a error/message parts, and then checking
"gdb_exception::reason" instead.
This was exposed by the new gdb.base/bp-cmds-continue-ctrl-c.exp
testcase added later in the series.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* breakpoint.c (insert_bp_location): Replace bp_err and
bp_err_message locals by a gdb_exception local.
---
gdb/breakpoint.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index adc8950..9723227 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2482,8 +2482,7 @@ insert_bp_location (struct bp_location *bl,
int *hw_breakpoint_error,
int *hw_bp_error_explained_already)
{
- enum errors bp_err = GDB_NO_ERROR;
- const char *bp_err_message = NULL;
+ gdb_exception bp_excpt = exception_none;
if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
return 0;
@@ -2592,12 +2591,11 @@ insert_bp_location (struct bp_location *bl,
val = bl->owner->ops->insert_location (bl);
if (val)
- bp_err = GENERIC_ERROR;
+ bp_excpt = gdb_exception {RETURN_ERROR, GENERIC_ERROR};
}
CATCH (e, RETURN_MASK_ALL)
{
- bp_err = e.error;
- bp_err_message = e.message;
+ bp_excpt = e;
}
END_CATCH
}
@@ -2632,16 +2630,16 @@ insert_bp_location (struct bp_location *bl,
val = target_insert_breakpoint (bl->gdbarch,
&bl->overlay_target_info);
if (val)
- bp_err = GENERIC_ERROR;
+ bp_excpt
+ = gdb_exception {RETURN_ERROR, GENERIC_ERROR};
}
CATCH (e, RETURN_MASK_ALL)
{
- bp_err = e.error;
- bp_err_message = e.message;
+ bp_excpt = e;
}
END_CATCH
- if (bp_err != GDB_NO_ERROR)
+ if (bp_excpt.reason != 0)
fprintf_unfiltered (tmp_error_stream,
"Overlay breakpoint %d "
"failed: in ROM?\n",
@@ -2658,12 +2656,11 @@ insert_bp_location (struct bp_location *bl,
val = bl->owner->ops->insert_location (bl);
if (val)
- bp_err = GENERIC_ERROR;
+ bp_excpt = gdb_exception {RETURN_ERROR, GENERIC_ERROR};
}
CATCH (e, RETURN_MASK_ALL)
{
- bp_err = e.error;
- bp_err_message = e.message;
+ bp_excpt = e;
}
END_CATCH
}
@@ -2675,7 +2672,7 @@ insert_bp_location (struct bp_location *bl,
}
}
- if (bp_err != GDB_NO_ERROR)
+ if (bp_excpt.reason != 0)
{
/* Can't set the breakpoint. */
@@ -2687,7 +2684,9 @@ insert_bp_location (struct bp_location *bl,
breakpoint insertion failed (e.g., the remote target
doesn't define error codes), so we must treat generic
errors as memory errors. */
- if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR)
+ if (bp_excpt.reason == RETURN_ERROR
+ && (bp_excpt.error == GENERIC_ERROR
+ || bp_excpt.error == MEMORY_ERROR)
&& bl->loc_type == bp_loc_software_breakpoint
&& (solib_name_from_address (bl->pspace, bl->address)
|| shared_objfile_contains_address_p (bl->pspace,
@@ -2715,16 +2714,18 @@ insert_bp_location (struct bp_location *bl,
if (bl->loc_type == bp_loc_hardware_breakpoint)
{
*hw_breakpoint_error = 1;
- *hw_bp_error_explained_already = bp_err_message != NULL;
+ *hw_bp_error_explained_already = bp_excpt.message != NULL;
fprintf_unfiltered (tmp_error_stream,
"Cannot insert hardware breakpoint %d%s",
- bl->owner->number, bp_err_message ? ":" : ".\n");
- if (bp_err_message != NULL)
- fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_message);
+ bl->owner->number,
+ bp_excpt.message ? ":" : ".\n");
+ if (bp_excpt.message != NULL)
+ fprintf_unfiltered (tmp_error_stream, "%s.\n",
+ bp_excpt.message);
}
else
{
- if (bp_err_message == NULL)
+ if (bp_excpt.message == NULL)
{
std::string message
= memory_error_message (TARGET_XFER_E_IO,
@@ -2740,7 +2741,7 @@ insert_bp_location (struct bp_location *bl,
fprintf_unfiltered (tmp_error_stream,
"Cannot insert breakpoint %d: %s\n",
bl->owner->number,
- bp_err_message);
+ bp_excpt.message);
}
}
return 1;
--
2.5.5
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/5] Fix stdin ending up not registered after a Quit
2017-11-06 23:27 [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues Pedro Alves
2017-11-06 23:27 ` [PATCH 1/5] Fix swallowed "Quit" when inserting breakpoints Pedro Alves
@ 2017-11-06 23:27 ` Pedro Alves
2017-11-06 23:27 ` [PATCH 4/5] Python unwinder sniffer: PyExc_KeyboardInterrupt -> Quit Pedro Alves
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-11-06 23:27 UTC (permalink / raw)
To: gdb-patches
If you press Ctrl-C while GDB is processing breakpoint commands the
TRY/CATCH in inferior_event_handler catches the Quit exception and
prints it, and then if the interpreter was running a foreground
execution command, nothing re-adds stdin back in the event loop,
meaning the debug session ends up busted, because the user can't type
anything...
This was exposed by the new gdb.base/bp-cmds-continue-ctrl-c.exp
testcase added later in the series.
gdb/ChangeLog:
2017-11-03 Pedro Alves <palves@redhat.com>
* inf-loop.c (inferior_event_handler): Don't swallow the exception
if the prompt is blocked.
---
gdb/inf-loop.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/gdb/inf-loop.c b/gdb/inf-loop.c
index bb9fa01..1d573b9 100644
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -73,7 +73,15 @@ inferior_event_handler (enum inferior_event_type event_type,
}
CATCH (e, RETURN_MASK_ALL)
{
- exception_print (gdb_stderr, e);
+ /* If the user was running a foreground execution
+ command, then propagate the error so that the prompt
+ can be reenabled. Otherwise, the user already has
+ the prompt and is typing some unrelated command, so
+ just inform the user and swallow the exception. */
+ if (current_ui->prompt_state == PROMPT_BLOCKED)
+ throw_exception (e);
+ else
+ exception_print (gdb_stderr, e);
}
END_CATCH
}
--
2.5.5
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 4/5] Python unwinder sniffer: PyExc_KeyboardInterrupt -> Quit
2017-11-06 23:27 [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues Pedro Alves
2017-11-06 23:27 ` [PATCH 1/5] Fix swallowed "Quit" when inserting breakpoints Pedro Alves
2017-11-06 23:27 ` [PATCH 2/5] Fix stdin ending up not registered after a Quit Pedro Alves
@ 2017-11-06 23:27 ` Pedro Alves
2017-11-06 23:27 ` [PATCH 5/5] Test breakpoint commands w/ "continue" + Ctrl-C Pedro Alves
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-11-06 23:27 UTC (permalink / raw)
To: gdb-patches
If you happen to press Ctrl-C while GDB is running the Python unwinder
machinery, the Ctrl-C is swallowed by the Python unwinder machinery.
For example, with:
break foo
commands
> c
> end
and
while (1)
foo ();
and then let the inferior hit "foo" repeatedly, sometimes Ctrl-C
results in:
~~~
23 usleep (100);
Breakpoint 2, foo () at gdb.base/bp-cmds-continue-ctrl-c.c:23
23 usleep (100);
^C
Breakpoint 2, Python Exception <class 'KeyboardInterrupt'> <class 'KeyboardInterrupt'>:
foo () at gdb.base/bp-cmds-continue-ctrl-c.c:23
23 usleep (100);
Breakpoint 2, foo () at gdb.base/bp-cmds-continue-ctrl-c.c:23
23 usleep (100);
Breakpoint 2, foo () at gdb.base/bp-cmds-continue-ctrl-c.c:23
23 usleep (100);
~~~
Notice the Python exception above. The interesting thing here is that
GDB continues as if nothing happened, doesn't really stop and give
back control to the user. Instead, the Ctrl-C aborted the Python
unwinder sniffer and GDB moved on to just use another unwinder.
Fix this by translating a PyExc_KeyboardInterrupt back into a Quit
exception once back in GDB.
This was exposed by the new gdb.base/bp-cmds-continue-ctrl-c.exp
testcase added later in the series.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* python/py-unwind.c (pyuw_sniffer): Translate
PyExc_KeyboardInterrupt to a GDB Quit exception.
---
gdb/python/py-unwind.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index acfce7d..b578373 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -539,6 +539,13 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
pyo_pending_frame.get (), NULL));
if (pyo_unwind_info == NULL)
{
+ /* If the unwinder is cancelled due to a Ctrl-C, then propagate
+ the Ctrl-C as a GDB exception instead of swallowing it. */
+ if (PyErr_ExceptionMatches (PyExc_KeyboardInterrupt))
+ {
+ PyErr_Clear ();
+ quit ();
+ }
gdbpy_print_stack ();
return 0;
}
--
2.5.5
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 5/5] Test breakpoint commands w/ "continue" + Ctrl-C
2017-11-06 23:27 [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues Pedro Alves
` (2 preceding siblings ...)
2017-11-06 23:27 ` [PATCH 4/5] Python unwinder sniffer: PyExc_KeyboardInterrupt -> Quit Pedro Alves
@ 2017-11-06 23:27 ` Pedro Alves
2017-11-06 23:27 ` [PATCH 3/5] Don't ever Quit out of resume Pedro Alves
2017-11-16 18:47 ` [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues Pedro Alves
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-11-06 23:27 UTC (permalink / raw)
To: gdb-patches
This adds the testcase that exposed the multiple problems with Ctrl-C
handling fixed by the previous patches, when run against both native
and gdbserver GNU/Linux.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* gdb.base/bp-cmds-continue-ctrl-c.c: New file.
* gdb.base/bp-cmds-continue-ctrl-c.exp: New file.
---
gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c | 35 ++++++
gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp | 136 +++++++++++++++++++++
2 files changed, 171 insertions(+)
create mode 100644 gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c
create mode 100644 gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
diff --git a/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c
new file mode 100644
index 0000000..2ec0b54
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.c
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2017 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/>. */
+
+#include <unistd.h>
+
+static void
+foo (void)
+{
+ usleep (100);
+}
+
+int
+main ()
+{
+ alarm (60);
+
+ while (1)
+ foo ();
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
new file mode 100644
index 0000000..e152459
--- /dev/null
+++ b/gdb/testsuite/gdb.base/bp-cmds-continue-ctrl-c.exp
@@ -0,0 +1,136 @@
+# Copyright 2017 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/>.
+
+# Set a breakpoint with a "continue" command attached, let the
+# inferior hit the breakpoint continuously. Check that we can use ^C
+# to interrupt the command, and that if ^C is pressed while GDB has
+# the terminal (between the stop and the re-resume), the resulting
+# "Quit" doesn't mess up the debug session.
+
+if [target_info exists gdb,nosignals] {
+ verbose "Skipping because of nosignals."
+ continue
+}
+
+# This test requires sending ^C to interrupt the running target.
+if [target_info exists gdb,nointerrupts] {
+ verbose "Skipping because of nointerrupts."
+ return
+}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+ return -1
+}
+
+# See intro.
+
+proc do_test {} {
+ global srcfile binfile
+ global gdb_prompt
+
+ gdb_test "break foo" "Breakpoint .*" "set breakpoint"
+
+ gdb_test \
+ [multi_line_input \
+ {commands} \
+ { c} \
+ {end}] \
+ "" "commands"
+
+ set test "stop with control-c"
+
+ for {set iter 0} {$iter < 20} {incr iter} {
+
+ # Useful for debugging.
+ #send_user "iter: $iter\n"
+
+ # Consume one breakpoint hit (at least), to make sure that the
+ # continue actually continues between attempts, as opposed to
+ # "c" not actually resuming and then Ctrl-C managing to
+ # interrupt anyway.
+ if {[gdb_test_multiple "continue" "$test (continue)" {
+ -re "Continuing.*Breakpoint \[^\r\n\]*\r\n" {
+ }
+ }] != 0} {
+ return
+ }
+
+ set internal_pass "IPASS: $test (iter $iter)"
+
+ # Breakpoint commands run after the target is considered
+ # stopped, and thus run with GDB owning the terminal. That
+ # means that it is expected that a Ctrl-C that arrives between
+ # - GDB reporting the breakpoint hit, and,
+ # - the breakpoint command continuing the target
+ # results in a Quit.
+
+ after 200 {send_gdb "\003"}
+ if {[gdb_test_multiple "" "$test (unexpected)" {
+ -re "Program terminated with signal SIGALRM.*\r\n$gdb_prompt $" {
+ fail "$test (SIGALRM)"
+ return
+ }
+ -re "Program received signal SIGINT.*\r\n$gdb_prompt $" {
+ send_log "$internal_pass (SIGINT)\n"
+ }
+ -re "Quit\r\n$gdb_prompt $" {
+ send_log "$internal_pass (Quit)\n"
+ }
+ -re "Quit\r\n\r\nCommand aborted.\r\n$gdb_prompt $" {
+ send_log "$internal_pass (Command aborted)\n"
+ }
+ -re "Breakpoint \[^\r\n\]*$srcfile" {
+ exp_continue
+ }
+ }] != 0} {
+ break
+ }
+ }
+
+ gdb_assert {$iter == 20} "stop with control-c"
+}
+
+# With native debugging and "run" (with job control), if the inferior
+# is running, the Ctrl-C reaches the inferior directly, not GDB. With
+# native debugging and "attach", or with remote debugging, the Ctrl-C
+# reaches GDB first. So for completeness, try both "run" and
+# "attach".
+
+with_test_prefix "run" {
+ clean_restart $binfile
+
+ if {![runto_main]} {
+ return -1
+ }
+
+ do_test
+}
+
+with_test_prefix "attach" {
+ if {[can_spawn_for_attach]} {
+ clean_restart $binfile
+
+ set test_spawn_id [spawn_wait_for_attach $binfile]
+ set testpid [spawn_id_get_pid $test_spawn_id]
+
+ gdb_test "attach $testpid" "Attaching to.*process $testpid.*" "attach"
+
+ do_test
+
+ kill_wait_spawned_process $test_spawn_id
+ }
+}
--
2.5.5
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 3/5] Don't ever Quit out of resume
2017-11-06 23:27 [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues Pedro Alves
` (3 preceding siblings ...)
2017-11-06 23:27 ` [PATCH 5/5] Test breakpoint commands w/ "continue" + Ctrl-C Pedro Alves
@ 2017-11-06 23:27 ` Pedro Alves
2017-12-09 1:21 ` Maciej W. Rozycki
2017-11-16 18:47 ` [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues Pedro Alves
5 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2017-11-06 23:27 UTC (permalink / raw)
To: gdb-patches
If you have a breakpoint command that re-resumes the target, like:
break foo
commands
> c
> end
and then let the inferior run, hitting the breakpoint, and then press
Ctrl-C at just the right time, between GDB processing the stop at
"foo", and re-resuming the target, you'll hit the QUIT call in
infrun.c:resume.
With this hack, we can reproduce the bad case consistently:
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -31,6 +31,8 @@
#include "top.h"
#include "observer.h"
+bool continue_hack;
+
/* General function to handle events in the inferior. */
void
@@ -64,6 +66,8 @@ inferior_event_handler (enum inferior_event_type event_type,
{
check_frame_language_change ();
+ continue_hack = true;
+
/* Don't propagate breakpoint commands errors. Either we're
stopping or some command resumes the inferior. The user will
be informed. */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d425664..c74b14c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2403,6 +2403,10 @@ resume (enum gdb_signal sig)
gdb_assert (!tp->stop_requested);
gdb_assert (!thread_is_in_step_over_chain (tp));
+ extern bool continue_hack;
+
+ if (continue_hack)
+ set_quit_flag ();
QUIT;
The GDB backtrace looks like this:
(top-gdb) bt
...
#3 0x0000000000612e8b in throw_quit(char const*, ...) (fmt=0xaf84a1 "Quit") at src/gdb/common/common-exceptions.c:408
#4 0x00000000007fc104 in quit() () at src/gdb/utils.c:748
#5 0x00000000006a79d2 in default_quit_handler() () at src/gdb/event-top.c:954
#6 0x00000000007fc134 in maybe_quit() () at src/gdb/utils.c:762
#7 0x00000000006f66a3 in resume(gdb_signal) (sig=GDB_SIGNAL_0) at src/gdb/infrun.c:2406
#8 0x0000000000700c3d in keep_going_pass_signal(execution_control_state*) (ecs=0x7ffcf3744e60) at src/gdb/infrun.c:7793
#9 0x00000000006f5fcd in start_step_over() () at src/gdb/infrun.c:2145
#10 0x00000000006f7b1f in proceed(unsigned long, gdb_signal) (addr=18446744073709551615, siggnal=GDB_SIGNAL_DEFAULT)
at src/gdb/infrun.c:3135
#11 0x00000000006ebdd4 in continue_1(int) (all_threads=0) at src/gdb/infcmd.c:842
#12 0x00000000006ec097 in continue_command(char*, int) (args=0x0, from_tty=0) at src/gdb/infcmd.c:938
#13 0x00000000004b5140 in do_cfunc(cmd_list_element*, char*, int) (c=0x2d18570, args=0x0, from_tty=0)
at src/gdb/cli/cli-decode.c:106
#14 0x00000000004b8219 in cmd_func(cmd_list_element*, char*, int) (cmd=0x2d18570, args=0x0, from_tty=0)
at src/gdb/cli/cli-decode.c:1952
#15 0x00000000007f1532 in execute_command(char*, int) (p=0x7ffcf37452b1 "", from_tty=0) at src/gdb/top.c:608
#16 0x00000000004bd127 in execute_control_command(command_line*) (cmd=0x3a88ef0) at src/gdb/cli/cli-script.c:485
#17 0x00000000005cae0c in bpstat_do_actions_1(bpstat*) (bsp=0x37edcf0) at src/gdb/breakpoint.c:4513
#18 0x00000000005caf67 in bpstat_do_actions() () at src/gdb/breakpoint.c:4563
#19 0x00000000006e8798 in inferior_event_handler(inferior_event_type, void*) (event_type=INF_EXEC_COMPLETE, client_data=0x0)
at src/gdb/inf-loop.c:72
#20 0x00000000006f9447 in fetch_inferior_event(void*) (client_data=0x0) at src/gdb/infrun.c:3970
#21 0x00000000006e870e in inferior_event_handler(inferior_event_type, void*) (event_type=INF_REG_EVENT, client_data=0x0)
at src/gdb/inf-loop.c:43
#22 0x0000000000494d58 in remote_async_serial_handler(serial*, void*) (scb=0x3585ca0, context=0x2cd1b80)
at src/gdb/remote.c:13820
#23 0x000000000044d682 in run_async_handler_and_reschedule(serial*) (scb=0x3585ca0) at src/gdb/ser-base.c:137
#24 0x000000000044d767 in fd_event(int, void*) (error=0, context=0x3585ca0) at src/gdb/ser-base.c:188
#25 0x00000000006a5686 in handle_file_event(file_handler*, int) (file_ptr=0x45997d0, ready_mask=1)
at src/gdb/event-loop.c:733
#26 0x00000000006a5c29 in gdb_wait_for_event(int) (block=1) at src/gdb/event-loop.c:859
#27 0x00000000006a4aa6 in gdb_do_one_event() () at src/gdb/event-loop.c:347
#28 0x00000000006a4ade in start_event_loop() () at src/gdb/event-loop.c:371
and when that happens, you end up with GDB's run control in quite a
messed up state. Something like this:
thread_function1 (arg=0x1) at threads.c:107
107 usleep (SLEEP); /* Loop increment. */
Quit
(gdb) c
Continuing.
** nothing happens, time passes..., press ctrl-c again **
^CQuit
(gdb) info threads
Id Target Id Frame
1 Thread 1462.1462 "threads" (running)
* 2 Thread 1462.1466 "threads" (running)
3 Thread 1462.1465 "function0" (running)
(gdb) c
Cannot execute this command while the selected thread is running.
(gdb)
The first "Quit" above is thrown from within "resume", and cancels run
control while GDB is in the middle of stepping over a breakpoint.
with step_over_info_valid_p() true. The next "c" didn't actually
resume anything, because GDB throught that the step-over was still in
progress. It wasn't, because the thread that was supposed to be
stepping over the breakpoint wasn't actually resumed.
So at this point, we press Ctrl-C again, and this time, the default
quit handler is called directly from the event loop
(event-top.c:default_quit_handler -> quit()), because gdb was left
owning the terminal (because the previous resume was cancelled before
we reach target_resume -> target_terminal::inferior()).
Note that the exception called from within resume ends up calling
normal_stop via resume_cleanups. That's very borked though, because
normal_stop is going to re-handle whatever was the last reported
event, possibly even re-running a hook stop... I think that the only
sane way to safely cancel the run control state machinery is to push
an event via handle_inferior_event like all other events.
The fix here does two things, and either alone would fix the problem
at hand:
#1 - passes the terminal to the inferior earlier, so that any QUIT
call from the point we declare the target as running goes to the
inferior directly, protecting run control from unsafe QUIT calls.
#2 - gets rid of this QUIT call in resume and of its related unsafe
resume_cleanups.
Aboout #2, the comment describing resume says:
/* Resume the inferior, but allow a QUIT. This is useful if the user
wants to interrupt some lengthy single-stepping operation
(for child processes, the SIGINT goes to the inferior, and so
we get a SIGINT random_signal, but for remote debugging and perhaps
other targets, that's not true).
but that's a really old comment that predates a lot of fixes to Ctrl-C
handling throughout both GDB core and the remote target, that made
sure that a Ctrl-C isn't ever lost. In any case, if some target
depended on this, a much better fix would be to make the target return
a SIGINT stop out of target_wait the next time that is called.
This was exposed by the new gdb.base/bp-cmds-continue-ctrl-c.exp
testcase added later in the series.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* infrun.c (resume_cleanups): Delete.
(resume): No longer install a resume_cleanups cleanup nor call
QUIT.
(proceed): Pass the terminal to the inferior.
(keep_going_pass_signal): No longer install a resume_cleanups
cleanup.
---
gdb/infrun.c | 43 ++++++++-----------------------------------
1 file changed, 8 insertions(+), 35 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d425664..a3a4137 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -78,8 +78,6 @@ static void sig_print_info (enum gdb_signal);
static void sig_print_header (void);
-static void resume_cleanups (void *);
-
static int follow_fork (void);
static int follow_fork_inferior (int follow_child, int detach_fork);
@@ -2198,17 +2196,6 @@ infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
}
\f
-/* Resuming. */
-
-/* Things to clean up if we QUIT out of resume (). */
-static void
-resume_cleanups (void *ignore)
-{
- if (!ptid_equal (inferior_ptid, null_ptid))
- delete_single_step_breakpoints (inferior_thread ());
-
- normal_stop ();
-}
static const char schedlock_off[] = "off";
static const char schedlock_on[] = "on";
@@ -2373,17 +2360,12 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig)
target_commit_resume ();
}
-/* Resume the inferior, but allow a QUIT. This is useful if the user
- wants to interrupt some lengthy single-stepping operation
- (for child processes, the SIGINT goes to the inferior, and so
- we get a SIGINT random_signal, but for remote debugging and perhaps
- other targets, that's not true).
+/* Resume the inferior. SIG is the signal to give the inferior
+ (GDB_SIGNAL_0 for none). */
- SIG is the signal to give the inferior (zero for none). */
void
resume (enum gdb_signal sig)
{
- struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
struct regcache *regcache = get_current_regcache ();
struct gdbarch *gdbarch = regcache->arch ();
struct thread_info *tp = inferior_thread ();
@@ -2403,8 +2385,6 @@ resume (enum gdb_signal sig)
gdb_assert (!tp->stop_requested);
gdb_assert (!thread_is_in_step_over_chain (tp));
- QUIT;
-
if (tp->suspend.waitstatus_pending_p)
{
if (debug_infrun)
@@ -2431,7 +2411,6 @@ resume (enum gdb_signal sig)
}
tp->suspend.stop_signal = GDB_SIGNAL_0;
- discard_cleanups (old_cleanups);
if (target_can_async_p ())
target_async (1);
@@ -2543,7 +2522,6 @@ resume (enum gdb_signal sig)
resume_ptid = internal_resume_ptid (user_step);
do_target_resume (resume_ptid, 0, GDB_SIGNAL_0);
- discard_cleanups (old_cleanups);
tp->resumed = 1;
return;
}
@@ -2581,7 +2559,6 @@ resume (enum gdb_signal sig)
"Got placed in step-over queue\n");
tp->control.trap_expected = 0;
- discard_cleanups (old_cleanups);
return;
}
else if (prepared < 0)
@@ -2758,7 +2735,6 @@ resume (enum gdb_signal sig)
do_target_resume (resume_ptid, step, sig);
tp->resumed = 1;
- discard_cleanups (old_cleanups);
}
\f
/* Proceeding. */
@@ -3061,6 +3037,12 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
if (!tp->control.in_infcall)
set_running (resume_ptid, 1);
+ /* Since we've marked the inferior running, give it the terminal. A
+ QUIT/Ctrl-C from here on is forwarded to the target (which can
+ still detect attempts to unblock a stuck connection with repeated
+ Ctrl-C from within target_pass_ctrlc). */
+ target_terminal::inferior ();
+
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog,
"infrun: proceed (addr=%s, signal=%s)\n",
@@ -7663,10 +7645,6 @@ stop_waiting (struct execution_control_state *ecs)
static void
keep_going_pass_signal (struct execution_control_state *ecs)
{
- /* Make sure normal_stop is called if we get a QUIT handled before
- reaching resume. */
- struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
-
gdb_assert (ptid_equal (ecs->event_thread->ptid, inferior_ptid));
gdb_assert (!ecs->event_thread->resumed);
@@ -7688,7 +7666,6 @@ keep_going_pass_signal (struct execution_control_state *ecs)
non-signal event (e.g., a fork); or took a signal which we
are supposed to pass through to the inferior. Simply
continue. */
- discard_cleanups (old_cleanups);
resume (ecs->event_thread->suspend.stop_signal);
}
else if (step_over_info_valid_p ())
@@ -7716,8 +7693,6 @@ keep_going_pass_signal (struct execution_control_state *ecs)
"resume of %s deferred\n",
target_pid_to_str (tp->ptid));
}
-
- discard_cleanups (old_cleanups);
}
else
{
@@ -7781,14 +7756,12 @@ keep_going_pass_signal (struct execution_control_state *ecs)
{
exception_print (gdb_stderr, e);
stop_waiting (ecs);
- discard_cleanups (old_cleanups);
return;
}
END_CATCH
ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
- discard_cleanups (old_cleanups);
resume (ecs->event_thread->suspend.stop_signal);
}
--
2.5.5
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/5] Don't ever Quit out of resume
2017-11-06 23:27 ` [PATCH 3/5] Don't ever Quit out of resume Pedro Alves
@ 2017-12-09 1:21 ` Maciej W. Rozycki
2017-12-11 11:20 ` Pedro Alves
0 siblings, 1 reply; 9+ messages in thread
From: Maciej W. Rozycki @ 2017-12-09 1:21 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Hi Pedro,
On Mon, 6 Nov 2017, Pedro Alves wrote:
> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * infrun.c (resume_cleanups): Delete.
> (resume): No longer install a resume_cleanups cleanup nor call
> QUIT.
> (proceed): Pass the terminal to the inferior.
> (keep_going_pass_signal): No longer install a resume_cleanups
> cleanup.
This change, i.e.:
commit d930703d68ae160ddfe8ebe5fdcf416fb6090e1e
Author: Pedro Alves <palves@redhat.com>
Date: Thu Nov 16 18:44:43 2017 +0000
caused regressions to appear in remote `mips-mti-linux-gnu' target testing
(`x86_64-linux-gnu' host), specifically:
FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: single-step breakpoint is not left behind
FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw on: single-step breakpoint is not left behind
FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted on: auto-hw off: step in ro region (cannot insert hw break)
FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted on: auto-hw off: single-step breakpoint is not left behind
FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted on: auto-hw on: single-step breakpoint is not left behind
and indeed detailed logs indicate a breakpoint is left lingering, e.g.:
(gdb) PASS: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: step in ro region (cannot insert sw break)
maint info breakpoints 0
Num Type Disp Enb Address What
0 sw single-step keep y 0x00400774 in main at [...]/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c:24 inf 1 thread 1
stop only in thread 1
(gdb) FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: single-step breakpoint is not left behind
vs:
(gdb) PASS: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: step in ro region (cannot insert sw break)
maint info breakpoints 0
No breakpoint or watchpoint matching '0'.
(gdb) PASS: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: single-step breakpoint is not left behind
as at commit d930703d68ae^.
Can you please look into it? If I can assist you anyhow, then just let
me know.
Maciej
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/5] Don't ever Quit out of resume
2017-12-09 1:21 ` Maciej W. Rozycki
@ 2017-12-11 11:20 ` Pedro Alves
0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-12-11 11:20 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gdb-patches
On 12/09/2017 01:16 AM, Maciej W. Rozycki wrote:
> Hi Pedro,
>
> On Mon, 6 Nov 2017, Pedro Alves wrote:
>
>> gdb/ChangeLog:
>> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>>
>> * infrun.c (resume_cleanups): Delete.
>> (resume): No longer install a resume_cleanups cleanup nor call
>> QUIT.
>> (proceed): Pass the terminal to the inferior.
>> (keep_going_pass_signal): No longer install a resume_cleanups
>> cleanup.
>
> This change, i.e.:
>
> commit d930703d68ae160ddfe8ebe5fdcf416fb6090e1e
> Author: Pedro Alves <palves@redhat.com>
> Date: Thu Nov 16 18:44:43 2017 +0000
>
> caused regressions to appear in remote `mips-mti-linux-gnu' target testing
> (`x86_64-linux-gnu' host), specifically:
>
> FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: single-step breakpoint is not left behind
> FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw on: single-step breakpoint is not left behind
> FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted on: auto-hw off: step in ro region (cannot insert hw break)
> FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted on: auto-hw off: single-step breakpoint is not left behind
> FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted on: auto-hw on: single-step breakpoint is not left behind
>
> and indeed detailed logs indicate a breakpoint is left lingering, e.g.:
>
> (gdb) PASS: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: step in ro region (cannot insert sw break)
> maint info breakpoints 0
> Num Type Disp Enb Address What
> 0 sw single-step keep y 0x00400774 in main at [...]/gdb/testsuite/gdb.base/breakpoint-in-ro-region.c:24 inf 1 thread 1
> stop only in thread 1
> (gdb) FAIL: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: single-step breakpoint is not left behind
>
> vs:
>
> (gdb) PASS: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: step in ro region (cannot insert sw break)
> maint info breakpoints 0
> No breakpoint or watchpoint matching '0'.
> (gdb) PASS: gdb.base/breakpoint-in-ro-region.exp: always-inserted off: auto-hw off: single-step breakpoint is not left behind
>
> as at commit d930703d68ae^.
>
> Can you please look into it? If I can assist you anyhow, then just let
> me know.
>
Thanks. I filed <https://sourceware.org/bugzilla/show_bug.cgi?id=22583> to
track this.
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues
2017-11-06 23:27 [PATCH 0/5][Resend] Fix multiple Ctrl-C/Quit issues Pedro Alves
` (4 preceding siblings ...)
2017-11-06 23:27 ` [PATCH 3/5] Don't ever Quit out of resume Pedro Alves
@ 2017-11-16 18:47 ` Pedro Alves
5 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2017-11-16 18:47 UTC (permalink / raw)
To: GDB Patches
On 11/06/2017 11:27 PM, Pedro Alves wrote:
> As I mentioned before, I've been messing with GDB's terminal handling
> in context of multi-target. While doing some inflow.c surgery, I had
> the bright idea of writing a test that did:
>
> break foo
> commands
> printf "hello\n"
> continue
>
> and run that against a program that does basically:
>
> while (1)
> foo ();
>
> and then while the inferior is running and hitting that breakpoint,
> hit Ctrl-C, to make sure target_terminal::inferior/ours handling was
> correct and that the user always re-gained control.
>
> Unfortunately, that test hit a number of other, preexisting
> problems... (I never learn... :-P) This series fixes them, and then
> adds a testcase similar to the above. The test passes cleanly with
> the fixes in place, but fails otherwise.
>
> Pedro Alves (5):
> Fix swallowed "Quit" when inserting breakpoints
> Fix stdin ending up not registered after a Quit
> Don't ever Quit out of resume
> Python unwinder sniffer: PyExc_KeyboardInterrupt -> Quit
> Test breakpoint commands w/ "continue" + Ctrl-C
I've pushed these in as well.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 9+ messages in thread