Re: [PATCH 2/2] gdb, remote: fix set_thread () in start_remote ()

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Markus Metzger <markus.t.metzger@intel.com> writes:

> remote_target::start_remote_1 () calls set_continue_thread (minus_one_ptid)
> with the intent to
>
>       /* Let the stub know that we want it to return the thread.  */
>       set_continue_thread (minus_one_ptid);
>
> I interpret it such that it expects a later get_current_thread () to
> return the thread selected by the target:
>
>           /* We have thread information; select the thread the target
>              says should be current.  If we're reconnecting to a
>              multi-threaded program, this will ideally be the thread
>              that last reported an event before GDB disconnected.  */
>           ptid_t curr_thread = get_current_thread (wait_status);

Oh, that makes sense. Thanks for digging into it!

> This results in the packet sequence Hc-1, qC.
>
> Hc simply sets cont_thread:
>
>           else if (cs.own_buf[1] == 'c')
>             cs.cont_thread = thread_id;
>
>           write_ok (cs.own_buf);
>
> and qC returns the general thread.  This doesn't match.

🤦

> It also has some special treatment for null_ptid and minus_one_ptid:
>
>     if (cs.general_thread != null_ptid && cs.general_thread != minus_one_ptid)
>       ptid = cs.general_thread;
>     else
>       {
>         init_thread_iter ();
>         ptid = thread_iter->id;
>       }
>
> Similarly, Hg has some special treatment for null_ptid:
>
>     if (cs.own_buf[1] == 'g')
>       {
>         if (thread_id == null_ptid)
>           {
>             /* GDB is telling us to choose any thread.  Check if
>                the currently selected thread is still valid. If
>                it is not, select the first available.  */
>             thread_info *thread = find_thread_ptid (cs.general_thread);
>             if (thread == NULL)
>               thread = get_first_thread ();
>             thread_id = thread->id;
>           }
>
>         cs.general_thread = thread_id;
>
> The comment at Hg matches the intent of GDB for sending Hc-1.

Indeed!

> Change the set_thread () call in remote_target::start_remote_1 () to
>
>     set_general_thread (any_thread_ptid);
>
> This results in GDB sending Hg0 and gdbserver preserving the currently
> selected thread that is later returned in response to qC.
>
> CC: Thiago Jung Bauermann  <thiago.bauermann@linaro.org>
> ---
>  gdb/remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Great! My only suggestion is to try to make the GDB comments more
consistent:

> diff --git a/gdb/remote.c b/gdb/remote.c
> index 6208a90f94a..9b76578bd80 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5293,7 +5293,7 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
>        target_update_thread_list ();
>  
>        /* Let the stub know that we want it to return the thread.  */

This comment is a bit cryptic. What about something like:

  /* Let the stub know that we want it to return the thread id in the
     qC reply from the get_current_thread call below.  */

> -      set_continue_thread (minus_one_ptid);
> +      set_general_thread (any_thread_ptid);
>  
>        if (thread_count (this) == 0)
>  	{

Also, your explanation implies that the doc comment in
remote_target::set_thread isn't quite right. What about changing it to
something along the following lines?

-/* If PTID is MAGIC_NULL_PTID, don't set any thread.  If PTID is
-   MINUS_ONE_PTID, set the thread to -1, so the stub returns the
-   thread.  If GEN is set, set the general thread, if not, then set
-   the step/continue thread.  */
+/* If PTID is MAGIC_NULL_PTID or ANY_THREAD_PTID, set the thread to 0.
+   If PTID is MINUS_ONE_PTID, set the thread to -1.  If GEN is set, set
+   the general thread, if not, then set the step/continue thread.  If
+   the thread is 0 or -1 and GEN is set, the stub returns the thread in
+   the qC packet reply.  */
 void
 remote_target::set_thread (ptid_t ptid, int gen)
 {
@@ -3267,6 +3268,7 @@ remote_target::set_thread (ptid_t ptid, int gen)
 
   *buf++ = 'H';
   *buf++ = gen ? 'g' : 'c';
+  /* NB: gdbserver interprets 0 and -1 the same way in the H packet.  */
   if (ptid == magic_null_ptid)
     xsnprintf (buf, endbuf - buf, "0");
   else if (ptid == any_thread_ptid)

One final comment: I agree with your conclusions, but unfortunately I'm
not sure of them. The doc comment in remote_target::set_thread gives me
a bit of pause. Because of that I don't think I should add a
Reviewed-by.

Hopefully someone with more experience with the RSP and remote stubs can
weigh in.

-- 
Thiago