Re: [PATCH v3 6/8] gdb/remote: Parse tdesc field in stop reply and threads list XML

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

Luis Machado <luis.machado@arm.com> writes:

> On 1/30/23 04:45, Thiago Jung Bauermann via Gdb-patches wrote:
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -80,6 +80,7 @@
>>   #include <unordered_map>
>>   #include "async-event.h"
>>   #include "gdbsupport/selftest.h"
>> +#include "xml-tdesc.h"
>>     /* The remote target.  */
>>   @@ -238,6 +239,16 @@ class remote_state
>>     /* Get the remote arch state for GDBARCH.  */
>>     struct remote_arch_state *get_remote_arch_state (struct gdbarch *gdbarch);
>>   +  /* Add new ID to the target description list.  The corresponding XML will be
>> +     requested soon.  */
>
> Will it be requested soon or can gdb just ignore it if the user
> doesn't switch to that thread?

In this patch it would be requested soon, right after the threads list
XML and the stop reply packet were parsed.

But in my local branch that will become v4 I implemented Andrew's
suggestion of getting the target descriptions on demand in
remote_state::get_tdesc so now GDB may indeed ignore it.

> If gdb can ignore it, then it might be nice to mention it here that
> gdb can chose to request it at any point in time, but may opt not to
> do it at all.

As a consequence of Andrew's suggestion, the add_tdesc_id method isn't
necessary anymore, so this comment isn't present anymore.

>> +  void add_tdesc_id (ULONGEST id);
>> +
>> +  /* Get the target description corresponding to remote protocol ID.  */
>
> s/remote protocol/remote target description?

I meant that in the sense of “ID that is used in the remote protocol”,
but I agree it's more confusing than helpful. I changed it to:

/* Get the target description corresponding to the given remote target
   description ID.  */

WDYT?

>> @@ -3814,6 +3844,13 @@ start_thread (struct gdb_xml_parser *parser,
>>     attr = xml_find_attribute (attributes, "handle");
>>     if (attr != NULL)
>>       item.thread_handle = hex2bin ((const char *) attr->value.get ());
>> +
>> +  attr = xml_find_attribute (attributes, "tdesc");
>> +  if (attr != NULL)
>
> s/NULL/nullptr

Fixed.

>> diff --git a/gdb/xml-tdesc.h b/gdb/xml-tdesc.h
>> index 0fbfc7e043e9..c7cc97c5dfc0 100644
>> --- a/gdb/xml-tdesc.h
>> +++ b/gdb/xml-tdesc.h
>> @@ -38,6 +38,12 @@ const struct target_desc *file_read_description_xml (const char *filename);
>>     const struct target_desc *target_read_description_xml (struct target_ops *);
>>   +/* Read an XML target description with the given ID using OPS.  Parse it, and
>> +   return the parsed description.  */
>> +
>> +const struct target_desc *target_read_description_xml (struct target_ops *ops,
>> +						       ULONGEST id);
>> +
>>   /* Fetches an XML target description using OPS, processing includes,
>>      but not parsing it.  Used to dump whole tdesc as a single XML file.
>>      Returns the description on success, and a disengaged optional
>
> I noticed we're dealing with the target description id as ULONGEST on
> gdb's side, but as unsigned int on gdbserver's side.
>
> Should we make them the same, if possible?

My thinking was that since it's gdbserver that defines what the ID will
be, it can use a simpler type (2³² target descriptions should be enough
for anybody) since it knows what kind of IDs it will issue. But GDB
doesn't know what the remote stub will want to do, so it should use a
big type.

But with Andrew's idea of passing a value during target negotiation to
decide whether the ID will be a number or a hash, then we can use
unsigned int for both types.

-- 
Thiago