From patchwork Thu Oct 17 22:50:18 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 35114 Received: (qmail 107554 invoked by alias); 17 Oct 2019 22:50:46 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 107373 invoked by uid 89); 17 Oct 2019 22:50:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 17 Oct 2019 22:50:43 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 41795C057F2C for ; Thu, 17 Oct 2019 22:50:42 +0000 (UTC) Received: from localhost.localdomain (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id C69261001DC0 for ; Thu, 17 Oct 2019 22:50:41 +0000 (UTC) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH v2 16/24] Fix reconnecting to a gdbserver already debugging multiple processes, I Date: Thu, 17 Oct 2019 23:50:18 +0100 Message-Id: <20191017225026.30496-17-palves@redhat.com> In-Reply-To: <20191017225026.30496-1-palves@redhat.com> References: <20191017225026.30496-1-palves@redhat.com> 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: yyyy-mm-dd 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: yyyy-mm-dd Pedro Alves * gdb.server/extended-remote-restart.exp (test_reload): Explicitly disconnect before reconnecting. --- gdb/remote.c | 20 ++++++++++++++++++++ gdb/testsuite/gdb.server/extended-remote-restart.exp | 4 +++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/gdb/remote.c b/gdb/remote.c index e33dedae26..bc1054b192 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -2369,6 +2369,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/gdb.server/extended-remote-restart.exp b/gdb/testsuite/gdb.server/extended-remote-restart.exp index 1fa46f2073..c78342c010 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 {