RE: [PATCH 1/2] gdbserver, read_ptid: handle '0' and '-1' thread ids

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

Hello Simon, Thiago,

Thanks for your reviews.

>>> The special thread id '-1' means 'all threads'.
>>> The special thread id '0' means 'any thread'.
>>>
>>> Read_ptid () currently returns
>>>
>>>     <current pid>.-1.0
>>>
>>> and
>>>
>>>     <current pid>.0.0
>>>
>>> respectively.
>>>
>>> Change that to minus_one_ptid for '-1' and to null_ptid for '0'.
>
>I can see that there are read_ptid functions both on GDB and
>GDBserver-side.  We can see that they are similar, but they have
>diverged a bit (and they will diverge even more with this patch).  IWBN
>if possible to have just one shared implementation.  One part that
>differs between the implementations is where we get the default pid
>value.  That value would have to be passed in as a parameter.  The nice
>side-effect of that is that it make it easy to unit test this function,
>which we don't do right now.

They are more different than it first appears.  The gdbserver side
accepts pN.-1, for example, while the gdb side doesn't.  The gdb side
doesn't accept -1 at all.

When the string does not start with 'p', both fall back to the
respective inferior on either side, but gdb has special handling for a
not-yet-known process id, whereas the gdbserver side doesn't.


>>>
>>> CC: Thiago Jung Bauermann  <thiago.bauermann@linaro.org>
>>> ---
>>>  gdbserver/remote-utils.cc | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>
>> Nice improvement. Just a minor comment.
>>
>>> diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
>>> index 15f073dd6be..9562cc2e1fb 100644
>>> --- a/gdbserver/remote-utils.cc
>>> +++ b/gdbserver/remote-utils.cc
>>> @@ -566,6 +566,26 @@ read_ptid (const char *buf, const char **obuf)
>>>    const char *p = buf;
>>>    const char *pp;
>>>
>>> +  /* Handle special thread ids.  */
>>> +  if (*p == '0')
>>
>> I think this should also check the character after '0':
>
>Agreed.  I think it would be valid to pass `0A`, to represent tid 10.
>
>Can we instead let the code go through hex_or_minus_one below and handle
>"tid == 0" there?

That is indeed a nice improvement that simplifies this patch to just:

diff --git a/gdbserver/remote-utils.cc b/gdbserver/remote-utils.cc
index 760655994c5..f5d70c546d3 100644
--- a/gdbserver/remote-utils.cc
+++ b/gdbserver/remote-utils.cc
@@ -587,6 +587,13 @@ read_ptid (const char *buf, const char **obuf)
   /* No multi-process.  Just a tid.  */
   ULONGEST tid = hex_or_minus_one (p, &pp);
 
+  /* Handle special thread ids.  */
+  if (tid == (ULONGEST) -1)
+    return minus_one_ptid;
+
+  if (tid == 0)
+    return null_ptid;
+
   /* Since GDB is not sending a process id (multi-process extensions
      are off), then there's only one process.  Default to the first in
      the list.  */

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