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

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Thiago,

Thanks for your review.

>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;
>        }
>    }

It's been a while, so to answer your question I reverted the patch to
trigger the error again.  And it no longer reproduces.

I still think something is wrong, here.

>> 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.

Agreed.

>> 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");

I argue that on the GDB side, magic_null_ptid and any_thread_ptid are
treated similarly and that on the gdbserver side minus_one_ptid and
null_ptid are treated similarly.

When resuming, GDB even turns minus_one_ptid into any_thread_ptid.

But I believe the issue lies elsewhere.

>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.

The comment was added in

    79d7f229011     Use ptid_t.tid to store thread ids instead of ptid_t.pid.

probably based on an earlier comment at the startup set_thread() call in

    c906108c214 (tag: gdb-4_18-branchpoint) Initial creation of sourceware repository

    +  /* Let the stub know that we want it to return the thread.  */
    +  set_thread (-1, 0);
    +
    +  inferior_pid = remote_current_thread (inferior_pid);

The set_continue_thread() (or just set_thread() in earlier versions) is followed by
remote_current_thread() (also today), which sends qC to query the current thread.
This could be what is meant by the target returning the thread.

This query, however, uses the general thread, not the continue thread:

  /* Reply the current thread id.  */
  if (strcmp ("qC", own_buf) == 0 && !disable_packet_qC)
    {
      ptid_t ptid;
      require_running_or_return (own_buf);

      if (cs.general_thread != null_ptid && cs.general_thread != minus_one_ptid)
	ptid = cs.general_thread;
      else
	{
	  init_thread_iter ();
	  ptid = thread_iter->id;
	}

This special handling of the general thread was introduced in

    bd99dc85838     Non-stop mode support.

and it had always been the general thread, not the continue thread.  Before,
there was only the else part.

So, if we wanted to let the target tell us its current thread, we should really be
setting the general thread.  And it doesn't matter if we set it to null_ptid or to
minus_one_ptid.

For Hg we also see some special handling of null_ptid and minus_one_ptid,
which got turned into null_ptid earlier:

	  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;


Maybe we could also rely on gdbserver initial state, as we're just starting,
and remove set_thread() in remote_target::start_remote_1().

It further looks like read_ptid() needs special handling for the special
thread-ids -1 and 0.

This needs more discussion.

We'll drop this patch from the series as it turned out to be unrelated.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928