From 78f2c40a12179d26d3065c09f054b7e751b2732f Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 10 Jan 2020 20:05:53 +0000 Subject: [PATCH] Fix reconnecting to a gdbserver already debugging multiple processes, I The multi-target patch will change the remote target's behavior when: - the current inferior is connected to an extended-remote target. - the current inferior is attached to any process. - some other inferior than than the current one is live. In current master, we get: (gdb) tar extended-remote :9999 A program is being debugged already. Kill it? (y or n) While after multi-target, since each inferior may have its own target connection, we'll get: (gdb) tar extended-remote :9999 Already connected to a remote target. Disconnect? (y or n) That change made gdb.server/extended-remote-restart.exp expose a gdb bug, because it made "target remote", via gdb_reconnect, just disconnect from the previous connection, while in current master that command would kill the inferior before disconnecting. In turn, that would make a multi-target gdb find processes already running under control of gdbserver as soon as it reconnects, while in current master there is never any process around when gdb reconnects, since they'd all been killed prior to disconnection. The bug this exposed is that remote_target::remote_add_inferior was always reusing current_inferior() for the new process, even if the current inferior was already bound to a process. In the testcase's case, when we reconnect, the remote is debugging two processes. So we'd bind the first remote process to the empty current inferior the first time, and then bind the second remote process to the same inferior again, essencially losing track of the first process. That resulted in failed assertions when we look up the inferior for the first process by PID. The fix is to still prefer binding to the current inferior (so that plain "target remote" keeps doing what you'd expect), but not reuse the current inferior if it is already bound to a process. This patch tweaks the test to explicitly disconnect before reconnecting, to avoid GDB killing processes, thus making current GDB behave the same as it will behave when the multi-target work lands. That change alone without the GDB fix exposes the bug like so: (gdb) PASS: gdb.server/extended-remote-restart.exp: kill: 0, follow-child 0: disconnect target extended-remote localhost:2350 Remote debugging using localhost:2350 src/gdb/thread.c:93: internal-error: thread_info* inferior_thread(): Assertion `tp' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) The original bug that the testcase was written for was related to killing, (git 9d4a934ce604 ("gdb: Fix assert for extended-remote target (PR gdb/18050)")), but since the testcase tries reconnecting with both explicitly killing and not explicitly killing, I think we're covering the original bug with this testcase change. gdb/ChangeLog: 2020-01-10 Pedro Alves * remote.c (remote_target::remote_add_inferior): Don't bind a process to the current inferior if the current inferior is already bound to a process. gdb/testsuite/ChangeLog: 2020-01-10 Pedro Alves * gdb.server/extended-remote-restart.exp (test_reload): Explicitly disconnect before reconnecting. --- gdb/ChangeLog | 6 ++++++ gdb/remote.c | 20 +++++++++++++++++++ gdb/testsuite/ChangeLog | 5 +++++ .../gdb.server/extended-remote-restart.exp | 4 +++- 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 78922021681..4163c869afe 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,9 @@ +2020-01-10 Pedro Alves + + * remote.c (remote_target::remote_add_inferior): Don't bind a + process to the current inferior if the current inferior is already + bound to a process. + 2020-01-10 Tankut Baris Aktemur Pedro Alves diff --git a/gdb/remote.c b/gdb/remote.c index ffdeede7af5..751769ea7fc 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -2367,6 +2367,26 @@ remote_target::remote_add_inferior (bool fake_pid_p, int pid, int attached, between program/address spaces. We simply bind the inferior to the program space's address space. */ inf = current_inferior (); + + /* However, if the current inferior is already bound to a + process, find some other empty inferior. */ + if (inf->pid != 0) + { + inf = nullptr; + for (inferior *it : all_inferiors ()) + if (it->pid == 0) + { + inf = it; + break; + } + } + if (inf == nullptr) + { + /* Since all inferiors were already bound to a process, add + a new inferior. */ + inf = add_inferior_with_spaces (); + } + switch_to_inferior_no_thread (inf); inferior_appeared (inf, pid); } diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 52d52d15f6c..0ff75954b1a 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2020-01-10 Pedro Alves + + * gdb.server/extended-remote-restart.exp (test_reload): Explicitly + disconnect before reconnecting. + 2020-01-10 Tankut Baris Aktemur Pedro Alves diff --git a/gdb/testsuite/gdb.server/extended-remote-restart.exp b/gdb/testsuite/gdb.server/extended-remote-restart.exp index b34027348ef..3ce0f0b076a 100644 --- a/gdb/testsuite/gdb.server/extended-remote-restart.exp +++ b/gdb/testsuite/gdb.server/extended-remote-restart.exp @@ -113,7 +113,9 @@ proc test_reload { do_kill_p follow_child_p } { "Check inferior was killed" } - # Reconnect to the target. + # Disconnect, and reconnect to the target. + gdb_test "disconnect" ".*" + if { [gdb_reconnect] == 0 } { pass "reconnect after fork" } else { -- 2.30.2