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

[Luis Machado <luis.machado@arm.com>]

On 2/5/23 00:06, Thiago Jung Bauermann wrote:
> 
> 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?
> 

Looks good!


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

Got it.