From: Andrew Burgess Date: Fri, 11 Jun 2021 10:30:47 +0000 (+0100) Subject: gdb/remote: handle target dying just before a stepi X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=80dc83fd0e70f4d522a534bc601df5e05b81d564;p=binutils-gdb.git gdb/remote: handle target dying just before a stepi I randomly hit a situation where gdbserver crashed immediately before I issued a 'stepi' to GDB, it turns out that this causes GDB itself to crash. What happens is that as part of the stepi we try to insert some breakpoints into the inferior, so from insert_breakpoints we figure out what we want to insert, then, eventually, try to send some packets to the remote to get the breakpoints inserted. It is only at this point that GDB realises that the target has gone away. This causes GDB to then enter this call stack: unpush_and_perror remote_unpush_target generic_mourn_inferior breakpoint_init_inferior delete_breakpoint update_global_location_list So, we realise the target is gone and so delete the breakpoints associated with that target. GDB then throws a TARGET_CLOSE_ERROR from unpush_and_error. This error is caught in insert_breakpoints where we then try to print a nice error saying something like: Cannot insert breakpoint %d: some error text here... To fill in the '%d' we try to read properties of the breakpoint object. Which was deleted due to the delete_breakpoint call above. And so GDB dies... My proposal in this commit is that, should we catch a TARGET_CLOSE_ERROR in insert_breakpoints, then we just rethrow the error. This will cause the main event loop to print something like: Remote connection closed Which I think is fine, I don't think the user will care much which particular breakpoint GDB was operating on when the connection closed, just knowing that the connection closed should be enough I think. I initially added a test to 'gdb.server/server-kill.exp' for this issue, however, my first attempt was not good enough, the test was passing even without my fix. Turns out that the server-kill.exp test actually kills the PID of the inferior, not the PID of the server. This means that gdbserver is actually able to send a packet to GDB saying that the inferior has exited prior to gdbserver itself shutting down. This extra information was enough to prevent the bug I was seeing manifest. So, I have extended server-kill.exp to run all of the tests twice, the first time we still kill the inferior. On the second run we hard kill the gdbserver itself, this prevents the server from sending anything to GDB before it exits. My new test is only expected to fail in this second mode of operation (killing gdbserver itself), and without my fix, that is what I see. gdb/ChangeLog: * breakpoint.c (insert_bp_location): If we catch a TARGET_CLOSE_ERROR just rethrow it, the breakpoints might have been deleted. gdb/testsuite/ChangeLog: * gdb.server/server-kill.exp: Introduce global kill_pid_of, and make use of this in prepare to select which pid we should kill. Run all the tests twice with a different kill_pid_of value. (prepare): Make use of kill_pid_of. (test_stepi): New proc. --- diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 11f2154b73e..2b04c3c6d73 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,9 @@ +2021-06-22 Andrew Burgess + + * breakpoint.c (insert_bp_location): If we catch a + TARGET_CLOSE_ERROR just rethrow it, the breakpoints might have + been deleted. + 2021-06-21 Andrew Burgess * NEWS: Mention new target feature name. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index fb011fc1e0f..0595c6f8cbd 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2698,6 +2698,14 @@ insert_bp_location (struct bp_location *bl, { /* Can't set the breakpoint. */ + /* If the target has closed then it will have deleted any + breakpoints inserted within the target inferior, as a result + any further attempts to interact with the breakpoint objects + is not possible. Just rethrow the error. */ + if (bp_excpt.error == TARGET_CLOSE_ERROR) + throw bp_excpt; + gdb_assert (bl->owner != nullptr); + /* In some cases, we might not be able to insert a breakpoint in a shared library that has already been removed, but we have not yet processed the shlib unload diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 0f5c1527277..88f8d96bf70 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,11 @@ +2021-06-22 Andrew Burgess + + * gdb.server/server-kill.exp: Introduce global kill_pid_of, and + make use of this in prepare to select which pid we should kill. + Run all the tests twice with a different kill_pid_of value. + (prepare): Make use of kill_pid_of. + (test_stepi): New proc. + 2021-06-21 Andrew Burgess * gdb.python/py-frame.exp: Add Frame.level tests. diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp index 80d78f89738..655821cc6f9 100644 --- a/gdb/testsuite/gdb.server/server-kill.exp +++ b/gdb/testsuite/gdb.server/server-kill.exp @@ -30,6 +30,21 @@ if { [build_executable "failed to prepare" ${testfile}] } { return -1 } +# Global control variable used by the proc prepare. Should be set to +# either 'inferior' or 'server'. +# +# In the proc prepare we start gdbserver and extract a pid, which will +# later be killed by calling the proc kill_server. +# +# When KILL_PID_OF is set to 'inferior' then the pid we kill is that +# of the inferior running under gdbserver, when this process dies +# gdbserver itself will exit. +# +# When KILL_PID_OF is set to 'server' then the pid we kill is that of +# the gdbserver itself, this is a much more aggressive strategy and +# exposes different bugs within GDB. +set kill_pid_of "inferior" + # Spawn GDBserver, run to main, extract GDBserver's PID and save it in # the SERVER_PID global. @@ -54,18 +69,22 @@ proc prepare {} { gdbserver_run "" - # Continue past server_pid assignment. - gdb_breakpoint ${srcfile}:[gdb_get_line_number "i = 0;"] - gdb_continue_to_breakpoint "after server_pid assignment" - - # Get the pid of GDBServer. - set test "p server_pid" - set server_pid 0 - gdb_test_multiple $test $test { - -re " = ($decimal)\r\n$gdb_prompt $" { - set server_pid $expect_out(1,string) - pass $test + if { $::kill_pid_of == "inferior" } { + # Continue past server_pid assignment. + gdb_breakpoint ${srcfile}:[gdb_get_line_number "i = 0;"] + gdb_continue_to_breakpoint "after server_pid assignment" + + # Get the pid of GDBServer. + set test "p server_pid" + set server_pid 0 + gdb_test_multiple $test $test { + -re " = ($decimal)\r\n$gdb_prompt $" { + set server_pid $expect_out(1,string) + pass $test + } } + } else { + set server_pid [exp_pid -i $::server_spawn_id] } if {$server_pid == 0} { @@ -132,6 +151,24 @@ proc_with_prefix test_unwind_syms {} { gdb_test "bt" "(Target disconnected|Remote connection closed|Remote communication error).*" } -test_tstatus -test_unwind_nosyms -test_unwind_syms +# Test performing a stepi right after the connection is dropped. + +proc_with_prefix test_stepi {} { + if ![prepare] { + return + } + + kill_server + + gdb_test "stepi" "(Target disconnected|Remote connection closed|Remote communication error).*" +} + +# Run each test twice, see the description of KILL_PID_OF earlier in +# this file for more details. + +foreach_with_prefix kill_pid_of { "inferior" "server" } { + test_tstatus + test_unwind_nosyms + test_unwind_syms + test_stepi +}