From patchwork Mon Mar 2 11:54:10 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 38367 Received: (qmail 13203 invoked by alias); 2 Mar 2020 11:54:21 -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 13148 invoked by uid 89); 2 Mar 2020 11:54:21 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=feels, arrive, INF, Further X-HELO: mail-wm1-f50.google.com Received: from mail-wm1-f50.google.com (HELO mail-wm1-f50.google.com) (209.85.128.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Mar 2020 11:54:17 +0000 Received: by mail-wm1-f50.google.com with SMTP id z12so10819048wmi.4 for ; Mon, 02 Mar 2020 03:54:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :in-reply-to:references; bh=UKgv9Dzmq+AW1iwtdG0FrdNevdZ0zhP+KmgdtN2+2os=; b=RVNqzSPiI6k/mfLEa6bn4MycW5XMY19cnkOQc7kkb3+9lz7FtcclbYj1aepiEc2NQV kyAtc3ZjRVv0+FTgiazxS7sJmrtXDILpZGjZVGnMdsmsFYeRzY5fiGKNg5aBgDI3VR8H ThKPf0fhJV8hKCRpDH5LERH2aF7JQO9qp0RNZqVuSYBiKDn6cMR05Z6d/dsEEAn0mAX2 y3nsdrKT0m1yFBcWACcQHr9uVgH6OO1/guUKhsQHA80yoTDWk50V/kzuvCHsgkZjnCwU E1bx7POOsGzBx/iPgkyIo8Wi0TdgHIefLDYJEgonzvboyGPUuklyjDw46L4007qWqzOc TkCg== Return-Path: Received: from localhost ([212.69.42.53]) by smtp.gmail.com with ESMTPSA id r28sm29241896wra.16.2020.03.02.03.54.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 02 Mar 2020 03:54:14 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Pedro Alves , Andrew Burgess Subject: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet Date: Mon, 2 Mar 2020 11:54:10 +0000 Message-Id: In-Reply-To: References: In-Reply-To: References: <137d09cf-9f97-fa5e-19a0-71231a3f760a@redhat.com> X-IsSubscribed: yes With this commit: commit 5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2 Date: Fri Jan 10 20:06:08 2020 +0000 Multi-target support There was a regression in GDB's support for older aspects of the remote protocol. Specifically, when a target sends the 'S' stop reply packet (which doesn't include a thread-id) then GDB has to figure out which thread actually stopped. Before the above commit GDB figured this out by using inferior_ptid in process_stop_reply, which contained the ptid of the current process/thread. This would be fine for single threaded inferiors (which is the only place using an S packet makes sense), but in the general case, relying on inferior_ptid for processing a stop is wrong - there's no reason to believe that what was GDB's current thread will be the same thread that just stopped in the inferior. With the above commit the inferior_ptid now has the value null_ptid inside process_stop_reply, this can be seen in do_target_wait, where we call switch_to_inferior_no_thread before calling do_target_wait_1. The problem this causes can be seen in the new test that runs gdbserver using the flag --disable-packet=T, and causes GDB to throw this assertion: inferior.c:279: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed. A similar problem was fixed in this commit: commit 3cada74087687907311b52781354ff551e10a0ed Date: Thu Jan 11 00:23:04 2018 +0000 Fix backwards compatibility with old GDBservers (PR remote/22597) However, this commit deals with the case where the T packet doesn't include a thread-id, not the S packet case. This commit solves the problem providing a thread-id at the GDB side if the remote target doesn't provide one. The thread-id provided comes from remote_state::general_thread, however, though this does work, I don't think it is the ideal solution. The remote_state tracks two threads, the continue_thread and the general_thread, these are updated when GDB asks the remote target to switch threads. The general_thread is set before performing things like register or memory accesses, and the continue_thread is set before things like continue or step commands. Further, the general_thread is updated after a target stops to reference the thread that stopped. The first thing to note from the above description is that we have a cycle of dependency, when a T packet arrives without a thread-id we fill in the thread-id from the general_thread data. The thread-id from the stop event is then used to set the general_thread. This in itself feels a little weird. The second question is why use the general_thread at all? You'd think given how they are originally set that the continue thread would be a better choice. The problem with this is that the continue_thread, if the user just does "continue", will be set to the minus_one_ptid, in the remote protocol this means all threads. When the stop arrives with no thread-id and we use continue_thread we end up with a very similar assertion to before because we now end up trying to lookup a thread using the minus_one_ptid. By contrast, once GDB has connected to a remote target the general_thread will be set to a valid thread-id, after which, if the inferior is single threaded, and stop events arrive without a thread-id, everything works fine. There is one slight weirdness with the above behaviour though. When GDB first connects to the remote target inferior_ptid is null_ptid, however, upon connecting we query the remote for its threads. As the thread information arrives GDB adds the threads to its internal database, and this process involves setting inferior_ptid to the id of each new thread in turn. Once we know about all the threads we wait for a stop event from the remote target to indicate that GDB is now in control of the inferior. The problem is that after adding the new threads we don't reset inferior_ptid, and the code path we use to wait for the inferior for this first wait also doesn't reset inferior_ptid, it just turns out that during the initial connection inferior_ptid is not null_ptid. This is lucky, because during the initial connection the general_thread variable _is_ set to null_ptid. So, during the initial connection, if the first stop event is missing a thread-id then we "provide" a thead-id from general_thread. This turns out to also be null_ptid meaning no thread-id is known, and then during ::process_stop_reply we fill in the missing thread-id using inferior_ptid. This was all discussed on the mailing list here: https://sourceware.org/ml/gdb-patches/2020-02/msg01011.html My proposal for a fix then is: 1. Move the call to switch_to_inferior_no_thread into do_target_wait_1, this means that in call cases where we are waiting for an inferior the inferior_ptid will be set to null_ptid. This is good as no wait code should rely on inferior_ptid. 2. Remove the use of general_thread from the 'T' packet processing. The general_thread read here was only ever correct by chance, and we shouldn't be using it this way. 3. Remove use of inferior_ptid from ::process_stop_event as this is wrong, and will always be null_ptid now anyway. 4. When a stop_even has null_ptid due to a lack of thread-id (either from a T packet or an S packet) then pick the first non exited thread in the inferior and use that. This will be fine for single threaded inferiors. A multi-threaded inferior really should be using T packets with a thread-id, so we give a warning if the inferior is multi-threaded, and we are still missing a thread-id. 5. Extend the existing test that covered the T packet with missing thread-id to also cover the S packet. gdb/ChangeLog: * remote.c (remote_target::remote_parse_stop_reply): Don't use the general_thread if the stop reply is missing a thread-id. (remote_target::process_stop_reply): Use the first non-exited thread if the target didn't pass a thread-id. * infrun.c (do_target_wait): Move call to switch_to_inferior_no_thread to .... (do_target_wait_1): ... here. gdb/testsuite/ChangeLog: * gdb.server/stop-reply-no-thread.exp: Add test where T packet is disabled. --- gdb/ChangeLog | 10 +++ gdb/infrun.c | 8 ++- gdb/remote.c | 43 ++++++++---- gdb/testsuite/ChangeLog | 5 ++ gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 80 ++++++++++++++--------- 5 files changed, 101 insertions(+), 45 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index d9a6f733519..43199b17b05 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3456,6 +3456,12 @@ do_target_wait_1 (inferior *inf, ptid_t ptid, ptid_t event_ptid; struct thread_info *tp; + /* We know that we are looking for an event in inferior INF, but we don't + know which thread the event might come from. As such we want to make + sure that INFERIOR_PTID is reset so that non of the wait code relies + on it - doing so is always a mistake. */ + switch_to_inferior_no_thread (inf); + /* First check if there is a resumed thread with a wait status pending. */ if (ptid == minus_one_ptid || ptid.is_pid ()) @@ -3651,8 +3657,6 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs, int options) auto do_wait = [&] (inferior *inf) { - switch_to_inferior_no_thread (inf); - ecs->ptid = do_target_wait_1 (inf, wait_ptid, &ecs->ws, options); ecs->target = inf->process_target (); return (ecs->ws.kind != TARGET_WAITKIND_IGNORE); diff --git a/gdb/remote.c b/gdb/remote.c index 4a70ab3fb0d..9b73faf9a34 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7402,18 +7402,14 @@ Packet: '%s'\n"), reported expedited registers. */ if (event->ptid == null_ptid) { + /* If there is no thread-id information then leave + the event->ptid as null_ptid. Later in + process_stop_reply we will pick a suitable + thread. */ const char *thr = strstr (p1 + 1, ";thread:"); if (thr != NULL) event->ptid = read_ptid (thr + strlen (";thread:"), NULL); - else - { - /* Either the current thread hasn't changed, - or the inferior is not multi-threaded. - The event must be for the thread we last - set as (or learned as being) current. */ - event->ptid = event->rs->general_thread; - } } if (rsa == NULL) @@ -7668,10 +7664,35 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply, *status = stop_reply->ws; ptid = stop_reply->ptid; - /* If no thread/process was reported by the stub, assume the current - inferior. */ + /* If no thread/process was reported by the stub then use the first + non-exited thread in the current target. */ if (ptid == null_ptid) - ptid = inferior_ptid; + { + for (thread_info *thr : all_non_exited_threads (this)) + { + if (ptid != null_ptid) + { + static bool warned = false; + + if (!warned) + { + /* If you are seeing this warning then the remote target + has multiple threads and either sent an 'S' stop + packet, or a 'T' stop packet without a thread-id. In + both of these cases GDB is unable to know which thread + just stopped and is now having to guess. The correct + action is to fix the remote target to send the correct + packet (a 'T' packet and include a thread-id). */ + warning (_("multi-threaded target stopped without sending " + "a thread-id, using first non-exited thread")); + warned = true; + } + break; + } + ptid = thr->ptid; + } + gdb_assert (ptid != null_ptid); + } if (status->kind != TARGET_WAITKIND_EXITED && status->kind != TARGET_WAITKIND_SIGNALLED diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp index 45407bc31de..ffc1c27dcb4 100644 --- a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp +++ b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp @@ -32,43 +32,59 @@ if [prepare_for_testing "failed to prepare" $testfile $srcfile] { return -1 } -# Make sure we're disconnected, in case we're testing with an -# extended-remote board, therefore already connected. -gdb_test "disconnect" ".*" +# Run the tests with different features of GDBserver disabled. +proc run_test { disable_feature } { + global binfile gdb_prompt decimal -# Start GDBserver, with ";thread:NNN" in T stop replies disabled, -# emulating old gdbservers when debugging single-threaded programs. -set res [gdbserver_start "--disable-packet=Tthread" $binfile] -set gdbserver_protocol [lindex $res 0] -set gdbserver_gdbport [lindex $res 1] + clean_restart ${binfile} -# Disable XML-based thread listing, and multi-process extensions. -gdb_test_no_output "set remote threads-packet off" -gdb_test_no_output "set remote multiprocess-feature-packet off" + # Make sure we're disconnected, in case we're testing with an + # extended-remote board, therefore already connected. + gdb_test "disconnect" ".*" -set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport] -if ![gdb_assert {$res == 0} "connect"] { - return -} + set res [gdbserver_start "--disable-packet=${disable_feature}" $binfile] + set gdbserver_protocol [lindex $res 0] + set gdbserver_gdbport [lindex $res 1] -# There should be only one thread listed. -set test "info threads" -gdb_test_multiple $test $test { - -re "2 Thread.*$gdb_prompt $" { - fail $test - } - -re "has terminated.*$gdb_prompt $" { - fail $test + # Disable XML-based thread listing, and multi-process extensions. + gdb_test_no_output "set remote threads-packet off" + gdb_test_no_output "set remote multiprocess-feature-packet off" + + set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport] + if ![gdb_assert {$res == 0} "connect"] { + return } - -re "\\\* 1\[\t \]*Thread\[^\r\n\]*\r\n$gdb_prompt $" { - pass $test + + # There should be only one thread listed. + set test "info threads" + gdb_test_multiple $test $test { + -re "2 Thread.*$gdb_prompt $" { + fail $test + } + -re "has terminated.*$gdb_prompt $" { + fail $test + } + -re "\\\* 1\[\t \]*Thread\[^\r\n\]*\r\n$gdb_prompt $" { + pass $test + } } -} -gdb_breakpoint "main" + gdb_breakpoint "main" -# Bad GDB behaved like this: -# (gdb) c -# Cannot execute this command without a live selected thread. -# (gdb) -gdb_test "c" "Breakpoint $decimal, main.*" "continue to main" + # Bad GDB behaved like this: + # (gdb) c + # Cannot execute this command without a live selected thread. + # (gdb) + gdb_test "c" "Breakpoint $decimal, main.*" "continue to main" +} + +# Disable different features within gdbserver: +# +# Tthread: Start GDBserver, with ";thread:NNN" in T stop replies disabled, +# emulating old gdbservers when debugging single-threaded programs. +# +# T: Start GDBserver with the entire 'T' stop reply packet disabled, +# GDBserver will instead send the 'S' stop reply. +foreach_with_prefix to_disable { Tthread T } { + run_test $to_disable +}