* [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us
@ 2020-01-31 7:35 Andrew Burgess
2020-02-26 18:52 ` Andrew Burgess
2020-02-26 19:28 ` Pedro Alves
0 siblings, 2 replies; 18+ messages in thread
From: Andrew Burgess @ 2020-01-31 7:35 UTC (permalink / raw)
To: gdb-patches; +Cc: palves, Andrew Burgess
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us 2020-01-31 7:35 [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us Andrew Burgess @ 2020-02-26 18:52 ` Andrew Burgess 2020-02-26 19:28 ` Pedro Alves 1 sibling, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2020-02-26 18:52 UTC (permalink / raw) To: gdb-patches; +Cc: palves 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us 2020-01-31 7:35 [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us Andrew Burgess 2020-02-26 18:52 ` Andrew Burgess @ 2020-02-26 19:28 ` Pedro Alves 2020-02-27 16:17 ` [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet Andrew Burgess 1 sibling, 1 reply; 18+ messages in thread From: Pedro Alves @ 2020-02-26 19:28 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet 2020-02-26 19:28 ` Pedro Alves @ 2020-02-27 16:17 ` Andrew Burgess 2020-02-27 19:46 ` Pedro Alves 0 siblings, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2020-02-27 16:17 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves, Andrew Burgess Pedro, Thanks for your feedback. I agree with all your points, this version of the patch: 1. Doesn't query the target for the current thread, but just uses the first non-exited thread. 2. Gives a warning if the target has more than one non-exited thread. 3. Commit message is revised to reflect new implementation. How's this? Thanks, Andrew --- 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 to use the first non exited thread of the inferior. Additionally, GDB will give a warning if a multi-threaded target stops without passing a thread-id. 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): Use the first non-exited thread if the target didn't pass a thread-id. --- 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 4a70ab3fb0d..4ac359ad5d9 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 + non-exited thread in the current inferior. */ if (ptid == null_ptid) - ptid = inferior_ptid; + { + for (thread_info *thr : all_non_exited_threads (this)) + { + if (ptid != null_ptid) + { + warning (_("multi-threaded target stopped without sending " + "a thread-id, using first non-exited thread")); + break; + } + ptid = thr->ptid; + } + gdb_assert (ptid != null_ptid); + } if (status->kind != TARGET_WAITKIND_EXITED && status->kind != TARGET_WAITKIND_SIGNALLED -- 2.14.5 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet 2020-02-27 16:17 ` [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet Andrew Burgess @ 2020-02-27 19:46 ` Pedro Alves 2020-02-28 14:03 ` Andrew Burgess 0 siblings, 1 reply; 18+ messages in thread From: Pedro Alves @ 2020-02-27 19:46 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 2/27/20 4:17 PM, Andrew Burgess wrote: > The solution I propose in this commit is to use the first non exited > thread of the inferior. s/of the inferior/of the target/ > Additionally, GDB will give a warning if a > multi-threaded target stops without passing a thread-id. > > 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. I forgot to mention this earlier, but I think we could test this by using gdbserver's --disable-packet=Tthread option (or --disable-packet=threads to disable vCont as well.). This sets the disable_packet_Tthread global in gdbserver, which makes it skip including the thread in the T stop reply. $ gdbserver --disable-packet Disableable packets: vCont All vCont packets qC Querying the current thread qfThreadInfo Thread listing Tthread Passing the thread specifier in the T stop reply packet threads All of the above I added these options several years ago exactly to exercise support for old non-threaded protocol. ISTR having documented them, ... ... ah, here: https://sourceware.org/ml/gdb-patches/2008-06/msg00501.html doesn't look like that patch was ever merged. :-/ > > - /* 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 inferior. */ s/current inferior/current target/ > if (ptid == null_ptid) > - ptid = inferior_ptid; > + { > + for (thread_info *thr : all_non_exited_threads (this)) > + { > + if (ptid != null_ptid) > + { > + warning (_("multi-threaded target stopped without sending " > + "a thread-id, using first non-exited thread")); I wonder about making this only warn once, to avoid a flood of warnings. Like: if (!warned) { warned = true; warning (....); } We use that same pattern in other places we only want to warn once. E.g., parse_memory_map. Otherwise LGTM. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet 2020-02-27 19:46 ` Pedro Alves @ 2020-02-28 14:03 ` Andrew Burgess 2020-02-28 17:46 ` Pedro Alves 0 siblings, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2020-02-28 14:03 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches * Pedro Alves <palves@redhat.com> [2020-02-27 19:46:12 +0000]: > On 2/27/20 4:17 PM, Andrew Burgess wrote: > > > The solution I propose in this commit is to use the first non exited > > thread of the inferior. > > s/of the inferior/of the target/ > > > Additionally, GDB will give a warning if a > > multi-threaded target stops without passing a thread-id. > > > > 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. > > I forgot to mention this earlier, but I think we could test this by using > gdbserver's --disable-packet=Tthread option (or --disable-packet=threads > to disable vCont as well.). This sets the disable_packet_Tthread global in > gdbserver, which makes it skip including the thread in the T stop reply. > > $ gdbserver --disable-packet > Disableable packets: > vCont All vCont packets > qC Querying the current thread > qfThreadInfo Thread listing > Tthread Passing the thread specifier in the T stop reply packet > threads All of the above > > I added these options several years ago exactly to exercise support > for old non-threaded protocol. ISTR having documented them, ... > ... ah, here: > https://sourceware.org/ml/gdb-patches/2008-06/msg00501.html Thanks, this looks super useful. I thought I'd write a test using this feature but it turns out there's already gdb.server/stop-reply-no-thread.exp which does --disable-packet=Tthreads. So, I run the test (without my patch) and ... it passes. The reason: commit 3cada74087687907311b52781354ff551e10a0ed Author: Pedro Alves <palves@redhat.com> Date: Thu Jan 11 00:23:04 2018 +0000 Fix backwards compatibility with old GDBservers (PR remote/22597) Specifically, this hunk: diff --git a/gdb/remote.c b/gdb/remote.c index 81c772a5451..a1cd9ae1df3 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -6940,7 +6940,13 @@ Packet: '%s'\n"), event->ptid = read_ptid (thr + strlen (";thread:"), NULL); else - event->ptid = magic_null_ptid; + { + /* 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) This code sets the stop_reply::ptid to the general_thread if we have no other thread specified in a 'T' packet. I haven't looked at the gdbserver code yet, but from your description I don't think I can stop gdbserver sending a 'T' and revert back to an 'S'. It seems a little weird that we use general_thread here, I don't see why that would be any more valid than the continue_thread, and you might think that if we set a continue_thread just before we execute the inferior then the stop event is possibly going to be for the continue thread. Still, like you say, the design of the old mechanism is horribly broken anyway, so I suspect there's little point worrying too much about changing this stuff. So, my next thought was maybe I should be doing something similar, instead of "fixing" a missing ptid in process_stop_event, fill in a valid ptid earlier, so I did this: diff --git a/gdb/remote.c b/gdb/remote.c index 4ac359ad5d9..67c5f298ee6 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7492,6 +7492,8 @@ Packet: '%s'\n"), event->ws.value.sig = (enum gdb_signal) sig; else event->ws.value.sig = GDB_SIGNAL_UNKNOWN; + if (event->ptid == null_ptid) + event->ptid = event->rs->general_thread; } break; case 'w': /* Thread exited. */ And this seems to do the trick. However, I'm still worried by the code in ::process_stop_event that says: if (ptid == null_ptid) ptid = inferior_ptid; After all, we know that as part of do_target_wait the inferior_ptid is reset to null_ptid, so this code should be pointless now, right... So I tried deleting it, and then both the stop-reply-no-thread.exp, and my local testing with my minimal gdb stub failed because once again we end up in ::process_stop_event with stop_reply->ptid being null_ptid - which now is coming from general_thread. Out of interest I tried replacing both the uses of general_thread (one in the 'T' packet and one I just added in the 'S' packet processing) with continue_thread, and this didn't do any better, though now the assertion that triggers is complaining that we end up using minus_one_ptid, and this leads to my next thought... I think that general_thread and continue_thread are more representative of what GDB has asked of the inferior, rather than what the inferior is telling GDB (maybe?). Let me explain my thinking: GDB updates the general_thread to a valid (something other than null_ptid, minus_one_ptid, etc) in only two places: (a) _AFTER_ processing a stop reply packet, to make the general_thread match the thread that stopped, this is based on the ptid that was parsed out of the stop reply, or (b) When we send a 'H' packet to the remote, setting either the general 'g' or continue 'c' thread variable, which is done by GDB just before an action. Now relying on (a) to set general_thread so we can read it on the _next_ stop clearly makes no sense - what stopped this time has no relevance to what might stop next time, and similarly, relying on the value of general_thread set via the 'H' packet is clearly just wrong, as GDB must send a 'Hc' (and sets the continue_thread) to pick which thread should continue. I think what I'm arguing is that using general_thread as a fall-back when processing 'T' (and 'S') is wrong, we should instead in these cases deliberately leave the stop event ptid as null_ptid (indicating that the stop didn't name a thread), and then use my patch (with your fixes) to pick a non-exited thread. I include my expanded proposed patch below for your consideration. I've updated the code and ChangeLog in the patch below, but I haven't yet updated the commit message - I want to see how this discussion goes first - but my plan would be to roll some of the above description into the commit message - if you agree with this change. Thanks, Andrew --- From 478f7a25259a8cf2dbdcccb11612a8f586335438 Mon Sep 17 00:00:00 2001 From: Andrew Burgess <andrew.burgess@embecosm.com> Date: Thu, 30 Jan 2020 14:35:40 +0000 Subject: [PATCH] gdb/remote: Restore support for 'S' stop reply packet 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 to use the first non exited thread of the inferior. Additionally, GDB will give a warning if a multi-threaded target stops without passing a thread-id. 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::remote_parse_stop_reply): Don't use the general_thread if the stop reply is missing a thread-id. * remote.c (remote_target::process_stop_reply): Use the first non-exited thread if the target didn't pass a thread-id. --- gdb/ChangeLog | 7 +++++++ gdb/remote.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index 4a70ab3fb0d..9c3e40fef16 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 send 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 -- 2.14.5 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet 2020-02-28 14:03 ` Andrew Burgess @ 2020-02-28 17:46 ` Pedro Alves 2020-03-02 11:54 ` [PATCHv3 2/2] " Andrew Burgess 2020-03-02 11:54 ` [PATCHv3 0/2] " Andrew Burgess 0 siblings, 2 replies; 18+ messages in thread From: Pedro Alves @ 2020-02-28 17:46 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches On 2/28/20 2:02 PM, Andrew Burgess wrote: > * Pedro Alves <palves@redhat.com> [2020-02-27 19:46:12 +0000]: > >> On 2/27/20 4:17 PM, Andrew Burgess wrote: >> >>> The solution I propose in this commit is to use the first non exited >>> thread of the inferior. >> >> s/of the inferior/of the target/ >> >>> Additionally, GDB will give a warning if a >>> multi-threaded target stops without passing a thread-id. >>> >>> 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. >> >> I forgot to mention this earlier, but I think we could test this by using >> gdbserver's --disable-packet=Tthread option (or --disable-packet=threads >> to disable vCont as well.). This sets the disable_packet_Tthread global in >> gdbserver, which makes it skip including the thread in the T stop reply. >> >> $ gdbserver --disable-packet >> Disableable packets: >> vCont All vCont packets >> qC Querying the current thread >> qfThreadInfo Thread listing >> Tthread Passing the thread specifier in the T stop reply packet >> threads All of the above >> >> I added these options several years ago exactly to exercise support >> for old non-threaded protocol. ISTR having documented them, ... >> ... ah, here: >> https://sourceware.org/ml/gdb-patches/2008-06/msg00501.html > > Thanks, this looks super useful. I thought I'd write a test using > this feature but it turns out there's already > gdb.server/stop-reply-no-thread.exp which does > --disable-packet=Tthreads. > > So, I run the test (without my patch) and ... it passes. The reason: > > commit 3cada74087687907311b52781354ff551e10a0ed > Author: Pedro Alves <palves@redhat.com> > Date: Thu Jan 11 00:23:04 2018 +0000 > > Fix backwards compatibility with old GDBservers (PR remote/22597) Ah... I did have a feeling of deja vu... Thanks for digging it out. > > Specifically, this hunk: > > diff --git a/gdb/remote.c b/gdb/remote.c > index 81c772a5451..a1cd9ae1df3 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -6940,7 +6940,13 @@ Packet: '%s'\n"), > event->ptid = read_ptid (thr + strlen (";thread:"), > NULL); > else > - event->ptid = magic_null_ptid; > + { > + /* 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) > > This code sets the stop_reply::ptid to the general_thread if we have > no other thread specified in a 'T' packet. I haven't looked at the > gdbserver code yet, but from your description I don't think I can stop > gdbserver sending a 'T' and revert back to an 'S'. Right, you can't. > > It seems a little weird that we use general_thread here, I don't see > why that would be any more valid than the continue_thread, and you > might think that if we set a continue_thread just before we execute > the inferior then the stop event is possibly going to be for the > continue thread. > > Still, like you say, the design of the old mechanism is horribly > broken anyway, so I suspect there's little point worrying too much > about changing this stuff. > > So, my next thought was maybe I should be doing something similar, > instead of "fixing" a missing ptid in process_stop_event, fill in a > valid ptid earlier, so I did this: > > diff --git a/gdb/remote.c b/gdb/remote.c > index 4ac359ad5d9..67c5f298ee6 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -7492,6 +7492,8 @@ Packet: '%s'\n"), > event->ws.value.sig = (enum gdb_signal) sig; > else > event->ws.value.sig = GDB_SIGNAL_UNKNOWN; > + if (event->ptid == null_ptid) > + event->ptid = event->rs->general_thread; > } > break; > case 'w': /* Thread exited. */ > > And this seems to do the trick. > > However, I'm still worried by the code in ::process_stop_event that > says: > > if (ptid == null_ptid) > ptid = inferior_ptid; > > After all, we know that as part of do_target_wait the inferior_ptid is > reset to null_ptid, so this code should be pointless now, right... Right. Relying on inferior_ptid when processing events is just wrong and should be removed. > > So I tried deleting it, and then both the stop-reply-no-thread.exp, > and my local testing with my minimal gdb stub failed because once > again we end up in ::process_stop_event with stop_reply->ptid being > null_ptid - which now is coming from general_thread. > > Out of interest I tried replacing both the uses of general_thread (one > in the 'T' packet and one I just added in the 'S' packet processing) > with continue_thread, and this didn't do any better, though now the > assertion that triggers is complaining that we end up using > minus_one_ptid, and this leads to my next thought... Right. > > I think that general_thread and continue_thread are more > representative of what GDB has asked of the inferior, rather than what > the inferior is telling GDB (maybe?). Let me explain my thinking: > GDB updates the general_thread to a valid (something other than > null_ptid, minus_one_ptid, etc) in only two places: > > (a) _AFTER_ processing a stop reply packet, to make the > general_thread match the thread that stopped, this is based on the > ptid that was parsed out of the stop reply, or > > (b) When we send a 'H' packet to the remote, setting either the > general 'g' or continue 'c' thread variable, which is done by GDB > just before an action. > > Now relying on (a) to set general_thread so we can read it on the > _next_ stop clearly makes no sense - what stopped this time has no > relevance to what might stop next time, and similarly, relying on the > value of general_thread set via the 'H' packet is clearly just wrong, > as GDB must send a 'Hc' (and sets the continue_thread) to pick which > thread should continue. The theory of the original fix was that if the stub isn't sending a thread id in the stop reply, then there's only one thread in the remote target, so the thread recorded in general_thread is going to be the right thread anyway. As you noticed, continue_thread is going to be minus_one_ptid when we tell the stub to resume all threads, so continue_thread was a better choice. I think. > > I think what I'm arguing is that using general_thread as a fall-back > when processing 'T' (and 'S') is wrong, we should instead in these > cases deliberately leave the stop event ptid as null_ptid (indicating > that the stop didn't name a thread), and then use my patch (with your > fixes) to pick a non-exited thread. That sounds good to me. Seems safer / better than relying on general_thread pointing to a thread, since in some cases it can point to a while process, or to no process/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 send an 'S' stop You mean "send" -> "sent", I think. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-02-28 17:46 ` Pedro Alves @ 2020-03-02 11:54 ` Andrew Burgess 2020-03-02 12:25 ` Pedro Alves 2020-03-09 17:35 ` Tom Tromey 2020-03-02 11:54 ` [PATCHv3 0/2] " Andrew Burgess 1 sibling, 2 replies; 18+ messages in thread From: Andrew Burgess @ 2020-03-02 11:54 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves, Andrew Burgess 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 +} -- 2.14.5 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-03-02 11:54 ` [PATCHv3 2/2] " Andrew Burgess @ 2020-03-02 12:25 ` Pedro Alves 2020-03-02 15:19 ` Andrew Burgess 2020-03-09 17:35 ` Tom Tromey 1 sibling, 1 reply; 18+ messages in thread From: Pedro Alves @ 2020-03-02 12:25 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 3/2/20 11:54 AM, Andrew Burgess wrote: > 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 "in all cases" > 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 "stop_even" -> "stop_event" > 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 "in the inferior" -> "in the target". > inferiors. A multi-threaded inferior really should be using T Instead of "A multi-threaded inferior", we should say "A multi-thread or multi-inferior aware remote server/stub" or something around that. > packets with a thread-id, so we give a warning if the inferior is > multi-threaded, and we are still missing a thread-id. "inferior" -> "target". The "inferior" -> "target" distinction I'm making in these small remarks above matters, because say the remote server is debugging two single-threaded inferiors. We still want to (and do) warn. > > 5. Extend the existing test that covered the T packet with missing > thread-id to also cover the S packet. Excellent. > > 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 "in the inferior INF" -> "in the target of inferior INF". The distinction is important -- target_wait may well return an event for an inferior different from INF. > + 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 "that non of" -> "that none of" ? > + on it - doing so is always a mistake. */ > + switch_to_inferior_no_thread (inf); > + OK with the nits above fixed. Thanks much for doing this! Pedro Alves ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-03-02 12:25 ` Pedro Alves @ 2020-03-02 15:19 ` Andrew Burgess 2020-03-02 19:07 ` Pedro Alves 0 siblings, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2020-03-02 15:19 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Thanks for all the feedback. I pushed the patches with the fixes you suggested. * Pedro Alves <palves@redhat.com> [2020-03-02 12:25:12 +0000]: > On 3/2/20 11:54 AM, Andrew Burgess wrote: > > > 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 > > "in all cases" > > > 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 > > "stop_even" -> "stop_event" > > > 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 > > "in the inferior" -> "in the target". > > > inferiors. A multi-threaded inferior really should be using T > > Instead of > "A multi-threaded inferior", > we should say > "A multi-thread or multi-inferior aware remote server/stub" > or something around that. > > > packets with a thread-id, so we give a warning if the inferior is > > multi-threaded, and we are still missing a thread-id. > > "inferior" -> "target". > > The "inferior" -> "target" distinction I'm making in these > small remarks above matters, because say the remote server > is debugging two single-threaded inferiors. We still want to > (and do) warn. I hadn't really considered this case, however, this raises a follow on question: Before entering the target wait code we call switch_to_inferior_no_thread, partly, as I understand it because having inferior_ptid pointing at a thread leads to invalid code that relies on this thread being _the_ event thread, when really we need to extract the inferior and thread-id from the stop event. So, why do we allow the current_inferior to remain valid while we perform the wait? Instead, why don't we clear both current_inferior and inferior_ptid, and then enter the wait code? Thanks, Andrew > > > > > 5. Extend the existing test that covered the T packet with missing > > thread-id to also cover the S packet. > > Excellent. > > > > > 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 > > "in the inferior INF" -> "in the target of inferior INF". > > The distinction is important -- target_wait may well return an event > for an inferior different from INF. > > > + 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 > > "that non of" -> "that none of" ? > > > + on it - doing so is always a mistake. */ > > + switch_to_inferior_no_thread (inf); > > + > > OK with the nits above fixed. Thanks much for doing this! > > Pedro Alves > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-03-02 15:19 ` Andrew Burgess @ 2020-03-02 19:07 ` Pedro Alves 2020-03-02 19:25 ` Andrew Burgess 0 siblings, 1 reply; 18+ messages in thread From: Pedro Alves @ 2020-03-02 19:07 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches On 3/2/20 3:19 PM, Andrew Burgess wrote: > * Pedro Alves <palves@redhat.com> [2020-03-02 12:25:12 +0000]: >> "inferior" -> "target". >> >> The "inferior" -> "target" distinction I'm making in these >> small remarks above matters, because say the remote server >> is debugging two single-threaded inferiors. We still want to >> (and do) warn. > > I hadn't really considered this case, however, this raises a > follow on question: > > Before entering the target wait code we call > switch_to_inferior_no_thread, partly, as I understand it because > having inferior_ptid pointing at a thread leads to invalid code > that relies on this thread being _the_ event thread, when really we > need to extract the inferior and thread-id from the stop event. The main reason we switch the inferior is that the target stack is a property of the inferior now. Each inferior has its own target stack (see inferior::m_target_stack). Note, target stack, not target. A single target may be used by different inferiors. E.g., when remote debugging, if you're debugging two inferiors under control of the same gdbserver, each inferior will have a pointer to the same remote target instance in its target stack. To call any target method, we must be sure to make the relevant inferior current, because target methods consult the inferior's target stack to know which target to call. When target methods defer to the target beneath, they'll again consult the target stack stored in the current inferior (the "this->beneath ()->foo()" calls in target-delegates.c). So to call the target_wait method to poll events out of some target, you need to switch to some representative inferior that uses the process_stratum target you want to poll. However, a reason we need to switch to all inferiors in turn and not just a subset sufficient to cover all instantiated process_stratum targets, is the strata above process_stratum, like record_stratum. Those targets aren't shared between inferiors, and they generate target events as well. I suspect it may be possible to come up with a cleaner design here, but I haven't been able to come up with one that's as simple as the current one, given all the constraints of the rest of current gdb's design. I didn't try very hard though. Since we're switching the inferior around, the question of -- which thread in the inferior should we pick? -- immediately arises. And the best answer is "none". Switching to no-thread also has the property that it "catches" target backends relying on inferior_ptid to infer things about the next stopping event, when they should not. (Another point that helps with seeing how that's wrong is to consider that inferior_ptid includes no field that would indicate which target instance the ptid came from. So e.g., if you're debugging against two remote stubs, it could well be the case that when you get to target_wait, inferior_ptid points at a thread of the other target, even though it seemingly looks like a valid thread in the current target.) > > So, why do we allow the current_inferior to remain valid while we > perform the wait? Instead, why don't we clear both current_inferior > and inferior_ptid, and then enter the wait code? It's impossible to clear current_inferior with the current design. There must always be a current inferior. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-03-02 19:07 ` Pedro Alves @ 2020-03-02 19:25 ` Andrew Burgess 0 siblings, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2020-03-02 19:25 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches * Pedro Alves <palves@redhat.com> [2020-03-02 19:07:33 +0000]: > On 3/2/20 3:19 PM, Andrew Burgess wrote: > > * Pedro Alves <palves@redhat.com> [2020-03-02 12:25:12 +0000]: > > >> "inferior" -> "target". > >> > >> The "inferior" -> "target" distinction I'm making in these > >> small remarks above matters, because say the remote server > >> is debugging two single-threaded inferiors. We still want to > >> (and do) warn. > > > > I hadn't really considered this case, however, this raises a > > follow on question: > > > > Before entering the target wait code we call > > switch_to_inferior_no_thread, partly, as I understand it because > > having inferior_ptid pointing at a thread leads to invalid code > > that relies on this thread being _the_ event thread, when really we > > need to extract the inferior and thread-id from the stop event. > > The main reason we switch the inferior is that the target stack is a > property of the inferior now. Each inferior has its own target stack > (see inferior::m_target_stack). Note, target stack, not target. > A single target may be used by different inferiors. E.g., > when remote debugging, if you're debugging two inferiors under control > of the same gdbserver, each inferior will have a pointer to the > same remote target instance in its target stack. > > To call any target method, we must be sure to make the relevant > inferior current, because target methods consult the inferior's > target stack to know which target to call. When target methods > defer to the target beneath, they'll again consult the target stack > stored in the current inferior (the "this->beneath ()->foo()" calls > in target-delegates.c). > > So to call the target_wait method to poll events out of some target, > you need to switch to some representative inferior that uses the > process_stratum target you want to poll. > > However, a reason we need to switch to all inferiors in turn and not > just a subset sufficient to cover all instantiated process_stratum > targets, is the strata above process_stratum, like record_stratum. > Those targets aren't shared between inferiors, and they generate > target events as well. > > I suspect it may be possible to come up with a cleaner design here, > but I haven't been able to come up with one that's as simple as the > current one, given all the constraints of the rest of current > gdb's design. I didn't try very hard though. > > Since we're switching the inferior around, the question > of -- which thread in the inferior should we pick? -- immediately > arises. And the best answer is "none". Switching to no-thread also > has the property that it "catches" target backends relying on > inferior_ptid to infer things about the next stopping event, > when they should not. (Another point that helps with seeing how that's > wrong is to consider that inferior_ptid includes no field > that would indicate which target instance the ptid came from. > So e.g., if you're debugging against two remote stubs, it could well > be the case that when you get to target_wait, inferior_ptid points at > a thread of the other target, even though it seemingly looks > like a valid thread in the current target.) > > > > > So, why do we allow the current_inferior to remain valid while we > > perform the wait? Instead, why don't we clear both current_inferior > > and inferior_ptid, and then enter the wait code? > > It's impossible to clear current_inferior with the current design. > There must always be a current inferior. Thanks for the explanation. Andrew ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-03-02 11:54 ` [PATCHv3 2/2] " Andrew Burgess 2020-03-02 12:25 ` Pedro Alves @ 2020-03-09 17:35 ` Tom Tromey 2020-03-10 19:05 ` Andrew Burgess 2020-03-11 16:23 ` Andrew Burgess 1 sibling, 2 replies; 18+ messages in thread From: Tom Tromey @ 2020-03-09 17:35 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Pedro Alves Andrew> There was a regression in GDB's support for older aspects of the Andrew> remote protocol. Specifically, when a target sends the 'S' stop reply Andrew> packet (which doesn't include a thread-id) then GDB has to figure out Andrew> which thread actually stopped. With this patch, I started seeing this warning when running the AdaCore internal test suite using qemu. I investigated a little and the warning comes from this packet: Packet received: W00 warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread It seems to me that the warning must not be correct in this case, though I am not 100% sure, so I thought I would ask. My reason is that this is a whole-process exit (so reporting a thread doesn't make sense); and that in this run, qemu is not reporting that it is multi-process-capable (so reporting a PID doesn't make sense). So, I wonder if process_stop_reply should be modified like (beware, untested patch): diff --git a/gdb/remote.c b/gdb/remote.c index 9b73faf9a34..810df658c54 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7666,7 +7666,7 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply, /* If no thread/process was reported by the stub then use the first non-exited thread in the current target. */ - if (ptid == null_ptid) + if (ptid == null_ptid && stop_reply->stop_reason != TARGET_WAITKIND_EXITED) { for (thread_info *thr : all_non_exited_threads (this)) { Perhaps this also applies to some of the other TARGET_WAITKIND_ values as well? TARGET_WAITKIND_SIGNALLED seems similar. thanks, Tom ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-03-09 17:35 ` Tom Tromey @ 2020-03-10 19:05 ` Andrew Burgess 2020-03-11 16:23 ` Andrew Burgess 1 sibling, 0 replies; 18+ messages in thread From: Andrew Burgess @ 2020-03-10 19:05 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches, Pedro Alves * Tom Tromey <tromey@adacore.com> [2020-03-09 11:35:52 -0600]: > Andrew> There was a regression in GDB's support for older aspects of the > Andrew> remote protocol. Specifically, when a target sends the 'S' stop reply > Andrew> packet (which doesn't include a thread-id) then GDB has to figure out > Andrew> which thread actually stopped. > > With this patch, I started seeing this warning when running the AdaCore > internal test suite using qemu. > > I investigated a little and the warning comes from this packet: > > Packet received: W00 > warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread > > It seems to me that the warning must not be correct in this case, though > I am not 100% sure, so I thought I would ask. > > My reason is that this is a whole-process exit (so reporting a thread > doesn't make sense); and that in this run, qemu is not reporting that it > is multi-process-capable (so reporting a PID doesn't make sense). > > So, I wonder if process_stop_reply should be modified like (beware, > untested patch): > > diff --git a/gdb/remote.c b/gdb/remote.c > index 9b73faf9a34..810df658c54 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -7666,7 +7666,7 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply, > > /* If no thread/process was reported by the stub then use the first > non-exited thread in the current target. */ > - if (ptid == null_ptid) > + if (ptid == null_ptid && stop_reply->stop_reason != TARGET_WAITKIND_EXITED) > { > for (thread_info *thr : all_non_exited_threads (this)) > { > > > Perhaps this also applies to some of the other TARGET_WAITKIND_ values > as well? TARGET_WAITKIND_SIGNALLED seems similar. Sorry for the breakage. Looking through the list I think only W and X stop packets are going to have this problem, which correspond to TARGET_WAITKIND_EXITED and TARGET_WAITKIND_SIGNALLED respectively. I think the patch below will fix your problem, though it would be great if you could give it a try. I've only tested this against the specific issue you're seeing, I still need to clean this up, and add a test, so this is not the final version. Still, thoughts and feedback welcome. My motivation here, is to have ::process_stop_reply return some kind of valid ptid_t, so in this case, for a process wide event, I'm creating something like `ptid_t (pid)` where pid comes from the first thread we find in the target. Then I only give a warning for process wide events when the pids don't match. I'll try to post a cleaned up version with test tomorrow. Thanks, Andrew --- diff --git a/gdb/remote.c b/gdb/remote.c index 9b73faf9a34..4e201d58cd6 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7670,7 +7670,10 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply, { for (thread_info *thr : all_non_exited_threads (this)) { - if (ptid != null_ptid) + if (ptid != null_ptid + && ((!(status->kind == TARGET_WAITKIND_EXITED + || status->kind == TARGET_WAITKIND_SIGNALLED)) + || (ptid.pid () != thr->ptid.pid ()))) { static bool warned = false; @@ -7689,7 +7692,12 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply, } break; } - ptid = thr->ptid; + + if (status->kind == TARGET_WAITKIND_EXITED + || status->kind == TARGET_WAITKIND_SIGNALLED) + ptid = ptid_t (thr->ptid.pid ()); + else + ptid = thr->ptid; } gdb_assert (ptid != null_ptid); } ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-03-09 17:35 ` Tom Tromey 2020-03-10 19:05 ` Andrew Burgess @ 2020-03-11 16:23 ` Andrew Burgess 2020-03-11 18:02 ` Tom Tromey 1 sibling, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2020-03-11 16:23 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches, Pedro Alves * Tom Tromey <tromey@adacore.com> [2020-03-09 11:35:52 -0600]: > Andrew> There was a regression in GDB's support for older aspects of the > Andrew> remote protocol. Specifically, when a target sends the 'S' stop reply > Andrew> packet (which doesn't include a thread-id) then GDB has to figure out > Andrew> which thread actually stopped. > > With this patch, I started seeing this warning when running the AdaCore > internal test suite using qemu. > > I investigated a little and the warning comes from this packet: > > Packet received: W00 > warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread > > It seems to me that the warning must not be correct in this case, though > I am not 100% sure, so I thought I would ask. Below is my propose patch, including tests. Feedback welcome. Thanks, Andrew --- commit 1f8244fd28909286e52458f443607849cbbc412c Author: Andrew Burgess <andrew.burgess@embecosm.com> Date: Wed Mar 11 12:30:13 2020 +0000 gdb: Handle W and X remote packets without giving a warning In this commit: commit 24ed6739b699f329c2c45aedee5f8c7d2f54e493 Date: Thu Jan 30 14:35:40 2020 +0000 gdb/remote: Restore support for 'S' stop reply packet A regression was introduced such that the W and X packets would give a warning in some cases. The warning was: warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread This problem would arise when: 1. The multi-process extensions to the remote protocol were not being used, and 2. The inferior has multiple threads. In this case when the W (or X) packet arrives the ptid of the stop_reply is set to null_ptid, then when we arrive in process_stop_reply GDB spots that we have multiple non-exited theads, but the stop event didn't specify a thread-id. The problem with this is that the W (and X) packets are actually process wide events, they apply to all threads. So not specifying a thread-id is not a problem, in fact, the best these packets allow is for the remote to specify a process-id, not a thread-id. If we look at how the W (and X) packets deal with a specified process-id, then what happens is GDB sets to stop_reply ptid to a value which indicates all threads in the process, this is done by creating a value `ptid_t (pid)`, which sets the pid field of the ptid_t, but leaves the tid field as 0, indicating all threads. So, this commit does the same thing for the case where there is not process-id specified. In process_stop_reply we not distinguish between stop events that apply to all threads, and those that apply to only one. If the stop event applies to only one thread then we treat it as before. If, however, the stop event applies to all threads, then we find the first non-exited thread, and use the pid from this thread to create a `ptid_t (pid)` value. If the target has multiple inferiors, and receives a process wide event without specifying a process-id GDB now gives this warning: warning: multi-inferior target stopped without sending a process-id, using first non-exited inferior gdb/ChangeLog: * remote.c (remote_target::process_stop_reply): Handle events for all threads differently. gdb/testsuite/ChangeLog: * gdb.server/exit-multiple-threads.c: New file. * gdb.server/exit-multiple-threads.exp: New file. diff --git a/gdb/remote.c b/gdb/remote.c index 9b73faf9a34..2c813b74fa1 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7668,28 +7668,54 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply, non-exited thread in the current target. */ if (ptid == null_ptid) { + /* Some stop events apply to all threads in an inferior, while others + only apply to a single thread. */ + bool is_stop_for_all_threads + = (status->kind == TARGET_WAITKIND_EXITED + || status->kind == TARGET_WAITKIND_SIGNALLED); + for (thread_info *thr : all_non_exited_threads (this)) { - if (ptid != null_ptid) + if (ptid != null_ptid + && (!is_stop_for_all_threads + || ptid.pid () != thr->ptid.pid ())) { 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")); + has stopped without specifying a thread-id, but the + target does have multiple threads (or inferiors), and + so GDB is having to guess which thread stopped. + + Examples of what might cause this are the target + sending and 'S' stop packet, or a 'T' stop packet and + not including a thread-id. + + Additionally, the target might send a 'W' or 'X + packet without including a process-id, when the target + has multiple running inferiors. */ + if (is_stop_for_all_threads) + warning (_("multi-inferior target stopped without " + "sending a process-id, using first " + "non-exited inferior")); + else + warning (_("multi-threaded target stopped without " + "sending a thread-id, using first " + "non-exited thread")); warned = true; } break; } - ptid = thr->ptid; + + /* If this is a stop for all threads then don't use a particular + threads ptid, instead create a new ptid where only the pid + field is set. */ + if (is_stop_for_all_threads) + ptid = ptid_t (thr->ptid.pid ()); + else + ptid = thr->ptid; } gdb_assert (ptid != null_ptid); } diff --git a/gdb/testsuite/gdb.server/exit-multiple-threads.c b/gdb/testsuite/gdb.server/exit-multiple-threads.c new file mode 100644 index 00000000000..c78cd7806d0 --- /dev/null +++ b/gdb/testsuite/gdb.server/exit-multiple-threads.c @@ -0,0 +1,202 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2020 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <pthread.h> +#include <sys/types.h> +#include <stdio.h> +#include <unistd.h> +#include <stdlib.h> +#include <pthread.h> + +/* The number of threads to create. */ +int thread_count = 3; + +/* Counter accessed from threads to ensure that all threads have been + started. Is initialised to THREAD_COUNT and each thread decrements it + upon startup. */ +volatile int counter; + +/* Lock guarding COUNTER. */ +pthread_mutex_t counter_mutex = PTHREAD_MUTEX_INITIALIZER; + +/* Is initialised with our pid, GDB will read this. */ +pid_t global_pid; + +/* Just somewhere to put a breakpoint. */ +static void +breakpt () +{ + /* Nothing. */ +} + +/* Thread safe decrement of the COUNTER global. */ +static void +decrement_counter () +{ + if (pthread_mutex_lock (&counter_mutex) != 0) + abort (); + --counter; + if (pthread_mutex_unlock (&counter_mutex) != 0) + abort (); +} + +/* Thread safe read of the COUNTER global. */ +static int +read_counter () +{ + int val; + + if (pthread_mutex_lock (&counter_mutex) != 0) + abort (); + val = counter; + if (pthread_mutex_unlock (&counter_mutex) != 0) + abort (); + + return val; +} + +#if defined DO_EXIT_TEST + +/* Thread entry point. ARG is a pointer to a single integer, the ID for + this thread numbered 1 to THREAD_COUNT (a global). */ +static void * +thread_worker_exiting (void *arg) +{ + int id; + + id = *((int *) arg); + + decrement_counter (); + + if (id != thread_count) + { + int i; + + /* All threads except the last one will wait here while the test is + carried out. Don't wait forever though, just in case the test + goes wrong. */ + for (i = 0; i < 60; ++i) + sleep (1); + } + else + { + /* The last thread waits here until all other threads have been + created. */ + while (read_counter () > 0) + sleep (1); + + /* Hit the breakpoint so GDB can stop. */ + breakpt (); + + /* And exit all threads. */ + exit (0); + } + + return NULL; +} + +#define thread_worker thread_worker_exiting + +#elif defined DO_SIGNAL_TEST + +/* Thread entry point. ARG is a pointer to a single integer, the ID for + this thread numbered 1 to THREAD_COUNT (a global). */ +static void * +thread_worker_signalling (void *arg) +{ + int i, id; + + id = *((int *) arg); + + decrement_counter (); + + if (id == thread_count) + { + /* The last thread waits here until all other threads have been + created. */ + while (read_counter () > 0) + sleep (1); + + /* Hit the breakpoint so GDB can stop. */ + breakpt (); + } + + /* All threads wait here while the testsuite sends us a signal. Don't + block forever though, just in case the test goes wrong. */ + for (i = 0; i < 60; ++i) + sleep (1); + + return NULL; +} + +#define thread_worker thread_worker_signalling + +#else + +#error "Compile with DO_EXIT_TEST or DO_SIGNAL_TEST defined" + +#endif + +struct thread_info +{ + pthread_t thread; + int id; +}; + +int +main () +{ + int i, max = thread_count; + + /* Put the pid somewhere easy for GDB to read. */ + global_pid = getpid (); + + /* Space to hold all of the thread_info objects. */ + struct thread_info *info = malloc (sizeof (struct thread_info) * max); + if (info == NULL) + abort (); + + /* Initialise the counter. Don't do this under lock as we only have the + main thread at this point. */ + counter = thread_count; + + /* Create all of the threads. */ + for (i = 0; i < max; ++i) + { + struct thread_info *thr = &info[i]; + thr->id = i + 1; + if (pthread_create (&thr->thread, NULL, thread_worker, &thr->id) != 0) + abort (); + } + + /* Gather in all of the threads. This never completes, as the + final thread created will exit the process, and all of the other + threads block forever. Still, it gives the main thread something to + do. */ + for (i = 0; i < max; ++i) + { + struct thread_info *thr = &info[i]; + if (pthread_join (thr->thread, NULL) != 0) + abort (); + } + + free (info); + + /* Return non-zero. We should never get here, but if we do make sure we + indicate something has gone wrong. */ + return 1; +} diff --git a/gdb/testsuite/gdb.server/exit-multiple-threads.exp b/gdb/testsuite/gdb.server/exit-multiple-threads.exp new file mode 100644 index 00000000000..aede2f799d9 --- /dev/null +++ b/gdb/testsuite/gdb.server/exit-multiple-threads.exp @@ -0,0 +1,136 @@ +# This testcase is part of GDB, the GNU debugger. +# +# Copyright 2020 Free Software Foundation, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test that GDB can handle receiving the W and X packets for a target +# with multiple threads, but only a single inferior. +# +# Specifically, check GDB handles this case where multi-process +# extensions are turned off. At one point this was causing GDB to +# give a warning when the exit arrived that the remote needed to +# include a thread-id, which was not correct. + +load_lib gdbserver-support.exp + +if { [skip_gdbserver_tests] } { + verbose "skipping gdbserver tests" + return -1 +} + +standard_testfile + +# Start up GDB and GDBserver debugging EXECUTABLE. When +# DISABLE_MULTI_PROCESS is true then disable GDB's remote +# multi-process support, otherwise, leave it enabled. +# +# Places a breakpoint in function 'breakpt' and then continues to the +# breakpoint, at which point it runs 'info threads'. +proc prepare_for_test { executable disable_multi_process } { + clean_restart ${executable} + + # Make sure we're disconnected, in case we're testing with an + # extended-remote board, therefore already connected. + gdb_test "disconnect" ".*" + + # Disable XML-based thread listing, and possible the multi-process + # extensions. + gdb_test_no_output "set remote threads-packet off" + if { $disable_multi_process } { + gdb_test_no_output "set remote multiprocess-feature-packet off" + } + + # Start gdbserver and connect. + set res [gdbserver_start "" $executable] + set gdbserver_protocol [lindex $res 0] + set gdbserver_gdbport [lindex $res 1] + set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport] + if ![gdb_assert {$res == 0} "connect"] { + return + } + + # Run until we hit the breakpt function, then list all threads. + gdb_breakpoint "breakpt" + gdb_continue_to_breakpoint "breakpt" + gdb_test "info threads" ".*" +} + +# Run the tests where the inferior exits normally (the W packet) while +# we have multiple-threads. EXECUTABLE is the binary under test, and +# DISABLE_MULTI_PROCESS indicates if we should disable GDB's remote +# multi-process support. +proc run_exit_test { executable disable_multi_process } { + global decimal + + prepare_for_test ${executable} ${disable_multi_process} + + # Finally, continue until the process exits, ensure we don't see + # any warnings between "Continuing." and the final process has + # exited message. + if { $disable_multi_process } { + set process_pattern "Remote target" + } else { + set process_pattern "process $decimal" + } + gdb_test "continue" \ + [multi_line \ + "Continuing\\." \ + "\\\[Inferior $decimal \\\(${process_pattern}\\\) exited normally\\\]" ] \ + "continue until process exits" +} + +# Run the tests where the inferior exits with a signal (the X packet) +# while we have multiple-threads. EXECUTABLE is the binary under +# test, and DISABLE_MULTI_PROCESS indicates if we should disable GDB's +# remote multi-process support. +proc run_signal_test { executable disable_multi_process } { + global decimal gdb_prompt + + prepare_for_test ${executable} ${disable_multi_process} + + set inf_pid [get_valueof "/d" "global_pid" "unknown"] + gdb_assert ![string eq ${inf_pid} "unknown"] "read the pid" + + # This sets the inferior running again, with all threads going + # into a long delay loop. + send_gdb "continue\n" + + # Send the inferior a signal to kill it. + sleep 1 + remote_exec target "kill -9 ${inf_pid}" + + # Process the output from GDB. + gdb_test_multiple "" "inferior exited with signal" { + -re "Continuing\\.\r\n\r\nProgram terminated with signal SIGKILL, Killed.\r\nThe program no longer exists.\r\n$gdb_prompt $" { + pass $gdb_test_name + } + } +} + +# Run all of the tests. +foreach_with_prefix test { exit signal } { + set def "DO_[string toupper $test]_TEST" + set func "run_${test}_test" + + set executable "$binfile-${test}" + if [prepare_for_testing "failed to prepare" $executable $srcfile \ + [list debug pthreads additional_flags=-D${def}]] { + return -1 + } + + foreach_with_prefix multi_process { 0 1 } { + $func ${executable} ${multi_process} + } +} ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet 2020-03-11 16:23 ` Andrew Burgess @ 2020-03-11 18:02 ` Tom Tromey 0 siblings, 0 replies; 18+ messages in thread From: Tom Tromey @ 2020-03-11 18:02 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, Pedro Alves, gdb-patches Andrew> Below is my propose patch, including tests. Andrew> Feedback welcome. Thanks. It looks good to me, and I can confirm it fixes the problem I was seeing. Tom ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCHv3 0/2] gdb/remote: Restore support for 'S' stop reply packet 2020-02-28 17:46 ` Pedro Alves 2020-03-02 11:54 ` [PATCHv3 2/2] " Andrew Burgess @ 2020-03-02 11:54 ` Andrew Burgess [not found] ` <27befec6c761c43f2e1a9e59403644c198cbd75b.1583149853.git.andrew.burgess@embecosm.com> 1 sibling, 1 reply; 18+ messages in thread From: Andrew Burgess @ 2020-03-02 11:54 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves, Andrew Burgess Pedro, Thanks for your feedback. In this version I have: 1. Added a new patch #1 that adds a debug feature to gdbserver to allow it to send S packets. 2. Used this new feature to add a test for S packets into patch #2. 3. Moved a call to switch_to_inferior_no_thread as part of patch #2 to make sure that inferior_ptid is null_ptid during wait code. 4. Rewritten the commit message for patch #2 to describe what's going on. Thanks, Andrew --- Andrew Burgess (2): gdbserver: Add mechanism to prevent sending T stop packets gdb/remote: Restore support for 'S' stop reply packet 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 ++++++++++++++--------- gdbserver/ChangeLog | 8 +++ gdbserver/remote-utils.cc | 20 ++++++ gdbserver/server.cc | 3 + gdbserver/server.h | 1 + 9 files changed, 133 insertions(+), 45 deletions(-) -- 2.14.5 ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <27befec6c761c43f2e1a9e59403644c198cbd75b.1583149853.git.andrew.burgess@embecosm.com>]
* Re: [PATCHv3 1/2] gdbserver: Add mechanism to prevent sending T stop packets [not found] ` <27befec6c761c43f2e1a9e59403644c198cbd75b.1583149853.git.andrew.burgess@embecosm.com> @ 2020-03-02 12:24 ` Pedro Alves 0 siblings, 0 replies; 18+ messages in thread From: Pedro Alves @ 2020-03-02 12:24 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 3/2/20 11:54 AM, Andrew Burgess wrote: > There is a developer only feature in gdbserver that provides a > command line option --disable-packet that prevents some packets from > being sent, which is used to increase test coverage within GDB. > > This commit extends this mechanism to prevent GDBserver from sending > the T stop reply packets, instead limiting GDBserver to only send the > S stop reply packets. > > The S stop reply packet is part of the older target control mechanism, > which has design flaws that were worked around with the introduction > of the newer target control mechanism, which uses the T stop reply > packet. > > Limiting GDBserver to use S stop packets instead of T stop packets > will, inevitably, mean that GDBserver doesn't function correctly in > many cases involving multiple threads, however, I don't think this is > too important, this is a developer only feature, intended to allow us > to test GDB. > > A new test that makes use of this feature will be added in the next > commit. > > gdbserver/ChangeLog: > > * remote-utils.cc (prepare_resume_reply): Add ability to convert T > reply into an S reply. > * server.cc (disable_packet_T): New global. > (captured_main): Set new global when appropriate. > * server.h (disable_packet_T): Declare. OK. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-03-11 18:02 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 7:35 [PATCH] gdb/remote: Ask target for current thread if it doesn't tell us Andrew Burgess
2020-02-26 18:52 ` Andrew Burgess
2020-02-26 19:28 ` Pedro Alves
2020-02-27 16:17 ` [PATCHv2] gdb/remote: Restore support for 'S' stop reply packet Andrew Burgess
2020-02-27 19:46 ` Pedro Alves
2020-02-28 14:03 ` Andrew Burgess
2020-02-28 17:46 ` Pedro Alves
2020-03-02 11:54 ` [PATCHv3 2/2] " Andrew Burgess
2020-03-02 12:25 ` Pedro Alves
2020-03-02 15:19 ` Andrew Burgess
2020-03-02 19:07 ` Pedro Alves
2020-03-02 19:25 ` Andrew Burgess
2020-03-09 17:35 ` Tom Tromey
2020-03-10 19:05 ` Andrew Burgess
2020-03-11 16:23 ` Andrew Burgess
2020-03-11 18:02 ` Tom Tromey
2020-03-02 11:54 ` [PATCHv3 0/2] " Andrew Burgess
[not found] ` <27befec6c761c43f2e1a9e59403644c198cbd75b.1583149853.git.andrew.burgess@embecosm.com>
2020-03-02 12:24 ` [PATCHv3 1/2] gdbserver: Add mechanism to prevent sending T stop packets Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox