gdb/remote: handle target dying just before a stepi
authorAndrew Burgess <andrew.burgess@embecosm.com>
Fri, 11 Jun 2021 10:30:47 +0000 (11:30 +0100)
committerAndrew Burgess <andrew.burgess@embecosm.com>
Tue, 22 Jun 2021 09:03:02 +0000 (10:03 +0100)
commit80dc83fd0e70f4d522a534bc601df5e05b81d564
tree9b7a8d2a695cfa2860762d293ecad18ac9e4614e
parent46b8b3d6f87c0c65d908e3f6c83532d7286868b9
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.
gdb/ChangeLog
gdb/breakpoint.c
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.server/server-kill.exp