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 11f2154b73e9e7429ac0d8793e69624d79afb211..2b04c3c6d730600da0eab06cfd49dc455105f5ec 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2021-06-22  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+       * 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  <andrew.burgess@embecosm.com>
 
        * NEWS: Mention new target feature name.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fb011fc1e0fc6839e8467626b5e2a8fcebc099a9..0595c6f8cbd49aad4b34f567a950451bd0ccff28 100644 (file)
--- 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 0f5c1527277aa6a731ee62353ce2b8b81ea30655..88f8d96bf70dd9d68dc5e71e9f304d5692a08c92 100644 (file)
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2021-06-22  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+       * 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  <andrew.burgess@embecosm.com>
 
        * 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 80d78f89738198ba996ad0b071080c143dd768f4..655821cc6f902e39cca994535d1cd9c552e134c3 100644 (file)
--- 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
+}