Message ID | 20200131000600.11699-1-andrew.burgess@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 93958 invoked by alias); 31 Jan 2020 00:06:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 93948 invoked by uid 89); 31 Jan 2020 00:06:11 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.2 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=Multitarget, Multi-target, sends, love X-HELO: mail-wr1-f66.google.com Received: from mail-wr1-f66.google.com (HELO mail-wr1-f66.google.com) (209.85.221.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 31 Jan 2020 00:06:09 +0000 Received: by mail-wr1-f66.google.com with SMTP id t2so6454105wrr.1 for <gdb-patches@sourceware.org>; Thu, 30 Jan 2020 16:06:09 -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; bh=enKpISq5acx834bOYd66d+RhMFF4eI83bu2xUvFDs18=; b=UB+TaHf8nKqw+RIHdclCsg5740LhQJqVl6tSimw4R0ny6hiaOqkwKDlF6Ea4q2IQeL VtCfiCxMTNi/cnjFdJYp2VVNstJt/3i3jsDwoN/eHbLb0pQRPPjuJYbeuJkFntQ3So8B dyOKh/0DkahNtQBRIuOQBoPWd21ezIOtC+hTPCAvCFHKORLoJTQ6ywWdFGFGIl9fpmBq EJhau7inHxTslZjb5De1S2KdxcnOfCnXtw9DDoKNRVRWT0MsEWhDsxoZcr1eAEmIcyHP m3/EJXO/zK5Q3mUHzgvTfCAdYwFgCFSH8uat4AcExq4Ra8mRR5DFAQIX2AGgmfvitoDr qHPA== Return-Path: <andrew.burgess@embecosm.com> Received: from localhost (host86-191-239-73.range86-191.btcentralplus.com. [86.191.239.73]) by smtp.gmail.com with ESMTPSA id n12sm8587957wmi.18.2020.01.30.16.06.06 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 30 Jan 2020 16:06:06 -0800 (PST) From: Andrew Burgess <andrew.burgess@embecosm.com> To: gdb-patches@sourceware.org Cc: palves@redhat.com, Andrew Burgess <andrew.burgess@embecosm.com> Subject: [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us Date: Fri, 31 Jan 2020 00:06:00 +0000 Message-Id: <20200131000600.11699-1-andrew.burgess@embecosm.com> X-IsSubscribed: yes |
Commit Message
Andrew Burgess
Jan. 31, 2020, 12:06 a.m. UTC
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. 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 solution I propose in this commit is that, if we don't get a thread id in the stop reply, then we should just ask the target for the current thread. There's no test included with this commit, if anyone has any ideas for how we could test this aspect of the remote protocol (short of writing a new gdbserver) then I'd love to know. It is possible to trigger this bug by attaching GDB to a running GDB, place a breakpoint on remote_parse_stop_reply, and manually change the contents of buf - when we get a 'T' based stop packet, replace it with an 'S' based packet, like this: (gdb) call memset (buf, "S05\0", 4) After this the GDB that is performing the remote debugging will crash with this error: inferior.c:279: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed. gdb/ChangeLog: * remote.c (remote_target::process_stop_reply): Don't use inferior_ptid, instead ask the remote for the current thread. Change-Id: I06f6fe5cc9a5cfc6d7157772aa7582f44c413bd5 --- gdb/ChangeLog | 5 +++++ gdb/remote.c | 18 +++++++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-)
Comments
Any feedback on this patch? I'd like to get this, or something like this merged soon-ish. Thanks, Andrew * Andrew Burgess <andrew.burgess@embecosm.com> [2020-01-31 00:06:00 +0000]: > 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. 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 solution I propose in this commit is that, if we don't get a > thread id in the stop reply, then we should just ask the target for > the current thread. > > There's no test included with this commit, if anyone has any ideas for > how we could test this aspect of the remote protocol (short of writing > a new gdbserver) then I'd love to know. > > It is possible to trigger this bug by attaching GDB to a running GDB, > place a breakpoint on remote_parse_stop_reply, and manually change the > contents of buf - when we get a 'T' based stop packet, replace it with > an 'S' based packet, like this: > > (gdb) call memset (buf, "S05\0", 4) > > After this the GDB that is performing the remote debugging will crash > with this error: > > inferior.c:279: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed. > > gdb/ChangeLog: > > * remote.c (remote_target::process_stop_reply): Don't use > inferior_ptid, instead ask the remote for the current thread. > > Change-Id: I06f6fe5cc9a5cfc6d7157772aa7582f44c413bd5 > --- > gdb/ChangeLog | 5 +++++ > gdb/remote.c | 18 +++++++++++++++--- > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index be2987707ff..94ed57ebf33 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -7668,10 +7668,22 @@ 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 > + thread in the current inferior. */ > if (ptid == null_ptid) > - ptid = inferior_ptid; > + { > + ptid = remote_current_thread (null_ptid); > + if (ptid == null_ptid) > + { > + /* We didn't get a useful answer back from the remote target so > + we need to pick something - we can't just report null_ptid. > + Lets just pick the first thread in GDB's current inferior. */ > + struct thread_info *thread > + = first_thread_of_inferior (current_inferior ()); > + gdb_assert (thread != nullptr); > + ptid = thread->ptid; > + } > + } > > if (status->kind != TARGET_WAITKIND_EXITED > && status->kind != TARGET_WAITKIND_SIGNALLED > -- > 2.14.5 >
On 1/31/20 12:06 AM, Andrew Burgess wrote: > 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. 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 solution I propose in this commit is that, if we don't get a > thread id in the stop reply, then we should just ask the target for > the current thread. I'm really not sure it is worth it to add an extra remote roundtrip for every single-step. If the stub uses the S stop reply, then it must be it doesn't really support multiple threads. Otherwise, even before the multi-target patch, multi-threading support would be broken when the target stopped for a thread different from GDB's current thread (the inferior_ptid). The "Hc" ("set continue thread") + "s/c/S/C" (step/continue) mechanism was broken by design for multi-threading and led to the creation of vCont + T many years ago. So I think that making GDB use the first thread of the target is sufficient and basically restores GDB to its previous behavior. > > There's no test included with this commit, if anyone has any ideas for > how we could test this aspect of the remote protocol (short of writing > a new gdbserver) then I'd love to know. > > It is possible to trigger this bug by attaching GDB to a running GDB, > place a breakpoint on remote_parse_stop_reply, and manually change the > contents of buf - when we get a 'T' based stop packet, replace it with > an 'S' based packet, like this: > > (gdb) call memset (buf, "S05\0", 4) > > After this the GDB that is performing the remote debugging will crash > with this error: > > inferior.c:279: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed. > > gdb/ChangeLog: > > * remote.c (remote_target::process_stop_reply): Don't use > inferior_ptid, instead ask the remote for the current thread. > > Change-Id: I06f6fe5cc9a5cfc6d7157772aa7582f44c413bd5 > --- > gdb/ChangeLog | 5 +++++ > gdb/remote.c | 18 +++++++++++++++--- > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index be2987707ff..94ed57ebf33 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -7668,10 +7668,22 @@ 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 > + thread in the current inferior. */ I suspect this comment was written before you made the code query the remote side? I was more expecting that this comment here would say something like /* If no thread/process was included in the stop reply, then query the remote current thread. */ > if (ptid == null_ptid) > - ptid = inferior_ptid; > + { > + ptid = remote_current_thread (null_ptid); > + if (ptid == null_ptid) > + { > + /* We didn't get a useful answer back from the remote target so > + we need to pick something - we can't just report null_ptid. > + Lets just pick the first thread in GDB's current inferior. */ > + struct thread_info *thread > + = first_thread_of_inferior (current_inferior ()); To be extra pedantic, it should be the first non-exited thread of the target instead of the first thread of the current inferior. Something around: for (thread_info *thr : all_non_exited_threads (this)) { ptid = thr->ptid; break; } gdb_assert (ptid != null_ptid); > + gdb_assert (thread != nullptr); > + ptid = thread->ptid; > + } > + } > Thanks, Pedro Alves
diff --git a/gdb/remote.c b/gdb/remote.c index be2987707ff..94ed57ebf33 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7668,10 +7668,22 @@ 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 + thread in the current inferior. */ if (ptid == null_ptid) - ptid = inferior_ptid; + { + ptid = remote_current_thread (null_ptid); + if (ptid == null_ptid) + { + /* We didn't get a useful answer back from the remote target so + we need to pick something - we can't just report null_ptid. + Lets just pick the first thread in GDB's current inferior. */ + struct thread_info *thread + = first_thread_of_inferior (current_inferior ()); + gdb_assert (thread != nullptr); + ptid = thread->ptid; + } + } if (status->kind != TARGET_WAITKIND_EXITED && status->kind != TARGET_WAITKIND_SIGNALLED