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

Luis Machado via Gdb-patches
2023-02-06T09:10:56Z

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.