Re: [PATCH v2 13/47] gdb, remote, ze: fix "$Hc-1#09...Packet received: E01" during startup

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

Hello,

You know you're looking at an interesting patch when the commit message
is one page long, but it changes only one line. :)

Regarding the subject:

Can you expand a bit on what conditions cause this error to happen?  Is
it broken only for the ze target, or do other targets also get the
error?

Is it returned when, during handling of 'H', find_thread_ptid can't find
the "<PID>.-1" thread? I.e., by this code?

  else
    {
      /* The ptid represents a lwp/tid.  */
      if (find_thread_ptid (thread_id) == NULL)
        {
          write_enn (cs.own_buf);
          break;
        }
    }

Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:

> From: Markus Metzger <markus.t.metzger@intel.com>
>
> When opening a connection, the remote target does
>
>     set_continue_thread (minus_one_ptid);

One interesting aspect of this line is the comment above it:

      /* Let the stub know that we want it to return the thread.  */
      set_continue_thread (minus_one_ptid);

What does it "the stub [...] to return the thread" mean? I tried reading
in gdbserver code what it would do if it understood the Hc packet as
referring to null_ptid or minus_one_ptid, but I couldn't find anything
that could be described as "returning the thread".

And yet it appears to be the purpose of the line above. I'd say we need
someone who understands this aspect to review the patch.

> which results in
>
>     $Hc-1#09
>
> to be send to gdbserver.  In remote-utils.c:read_ptid (), this is read as
>
>     { <current inferior>, -1, 0 }

Nit: I'd say "<current PID>" rather than "<current inferior>".

> and not recognized as minus_one_ptid when handling 'H' in
>
> 	  ptid_t thread_id = read_ptid (&cs.own_buf[2], NULL);
>
> 	  if (thread_id == null_ptid || thread_id == minus_one_ptid)
> 	    thread_id = null_ptid;
>
> Since minus_one_ptid and null_ptid are treated the same way, this is
> probably what we want.

Not sure I understood what "this" means in the sentence. If it means "we
want our Hc packet to be interpreted as thread_id == nullptid in the if
condition above", then that's not what this patch does (sorry if I
misunderstood). As you noted, read_ptid can't return a minus_one_ptid,
but it can't return a null_ptid either.

With the patch applied, read_ptid will return { <current pid>, 0, 0 },
which ptid_t::is_pid () considers a PID and thus the handling of 'H'
will call find_any_thread_of_pid, which I'd guess is why this patch
works.

> In remote_target::remote_resume_with_hc (), we do
>
>       if (ptid == minus_one_ptid)
>         set_continue_thread (any_thread_ptid);
>       else
>         set_continue_thread (ptid);

There's no talk of wanting the stub to "return the thread" here though.

> which amounts to the same thing as set_thread () does

Not exactly. In the code below, there's one more "else if" branch:

>       *buf++ = 'H';
>       *buf++ = gen ? 'g' : 'c';
>       if (ptid == magic_null_ptid)
>         xsnprintf (buf, endbuf - buf, "0");
>       else if (ptid == any_thread_ptid)
>         xsnprintf (buf, endbuf - buf, "0");

  else if (ptid == minus_one_ptid)
    xsnprintf (buf, endbuf - buf, "-1");

So remote_target::set_thread doesn't treat any_thread_ptid and
minus_one_ptid the same way, even though gdbserver apparently does.

Also, the doc comment of set_thread says: "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."

Here's the talk about asking the stub to "return the thread"
again. Frustrating.

> so any_thread_ptid is pretty much the same as null_ptid and as
> minus_one_ptid in this context.

gdbserver appears to think so, but GDB doesn't. I'm confused.

> Use any_thread_ptid to align with remote_target::remote_resume_with_hc ().
> ---
>  gdb/remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 281e616eb83008e75da8a18efaee7001ddbf6d0d..3aaa1614211d617e0d7683eade88f0d00e1bee8b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5284,7 +5284,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.  */
> -      set_continue_thread (minus_one_ptid);
> +      set_continue_thread (any_thread_ptid);
>  
>        if (thread_count (this) == 0)
>  	{

-- 
Thiago