RE: [PATCH v2 07/47] gdb, gdbserver, gdbsupport: add 'device' tag to XML target description

Aktemur, Tankut Baris
2025-07-21T10:50:38Z

On Tuesday, July 8, 2025 6:04 AM, Thiago Jung Bauermann wrote:
> Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:
>
> > diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c
> > index 436c493d4f914eb71a80bcbc08dc2ddd38c72027..61cba42e9eee2ec4cc66e14a120c5bef909e48b0
> 100644
> > --- a/gdb/xml-tdesc.c
> > +++ b/gdb/xml-tdesc.c
> > @@ -137,6 +137,65 @@ tdesc_end_compatible (struct gdb_xml_parser *parser,
> > tdesc_add_compatible (data->tdesc, arch);
> > }
> >
> > +/* Handle the end of a <device> element and its value. */
>
> s/the end/the start/
>
> > +static void
> > +tdesc_start_device (struct gdb_xml_parser *parser,
> > + const gdb_xml_element *element,
> > + void *user_data, std::vector<gdb_xml_value> &attributes)
> > +{
>
> <snip>
>
> > +void
> > +print_xml_feature::visit (const tdesc_device *device)
> > +{
> > + if (device == nullptr)
> > + return;
> > +
> > + std::string tmp = "<device";
> > + if (device->vendor_id.has_value ())
> > + string_appendf (tmp, " vendor-id=\"0x%04" PRIx32 "\"",
> > + *device->vendor_id);
> > +
> > + if (device->target_id.has_value ())
> > + string_appendf (tmp, " target-id=\"0x%04" PRIx32 "\"",
> > + *device->target_id);
> > +
> > + string_appendf (tmp, " family=\"%s\"", device->family.c_str ());
> > + string_appendf (tmp, " model=\"%s\"", device->model.c_str ());
>
> For all the string attributes in device: isn't it better to check
> whether the string is empty and only emit the attribute if it isn't?

Added the checks.

> > + if (device->stepping.has_value ())
> > + string_appendf (tmp, " stepping=\"%d\"", *device->stepping);
> > +
> > + string_appendf (tmp, " name=\"%s\"", device->name.c_str ());
> > + string_appendf (tmp, " pci-slot=\"%s\"", device->pci_slot.c_str ());
> > + string_appendf (tmp, " uuid=\"%s\"", device->uuid.c_str ());
> > +
> > + if (device->total_cores.has_value ())
> > + string_appendf (tmp, " total-cores=\"%ld\"", *device->total_cores);
> > +
> > + if (device->total_threads.has_value ())
> > + string_appendf (tmp, " total-threads=\"%ld\"", *device->total_threads);
> > +
> > + if (device->subdevice_id.has_value ())
> > + string_appendf (tmp, " subdevice-id=\"%d\"", *device->subdevice_id);
> > +
> > + string_appendf (tmp, "/>");
> > + add_line (tmp);
> > +}
> > +
> > /* See gdbsupport/tdesc.h. */
> >
> > void
> > diff --git a/gdbsupport/tdesc.h b/gdbsupport/tdesc.h
> > index c9e7603369cbf986187908c2d1f8fbe35a0dc02d..29de4d231f16667c8297bcbe403f683c30a28762
> 100644
> > --- a/gdbsupport/tdesc.h
> > +++ b/gdbsupport/tdesc.h
> > @@ -26,6 +26,7 @@ struct tdesc_type_builtin;
> > struct tdesc_type_vector;
> > struct tdesc_type_with_fields;
> > struct tdesc_reg;
> > +struct tdesc_device;
> > struct target_desc;
> >
> > /* The interface to visit different elements of target description. */
> > @@ -56,6 +57,9 @@ class tdesc_element_visitor
> >
> > virtual void visit (const tdesc_reg *e)
> > {}
> > +
> > + virtual void visit (const tdesc_device *e)
> > + {}
> > };
> >
> > class tdesc_element
> > @@ -315,6 +319,84 @@ struct tdesc_feature : tdesc_element
> >
> > typedef std::unique_ptr<tdesc_feature> tdesc_feature_up;
> >
> > +/* The device information in a target description. */
> > +
> > +struct tdesc_device : tdesc_element
> > +{
> > + tdesc_device ()
> > + {
> > + family = "-";
> > + model = "-";
> > + name = "-";
> > + pci_slot = "-";
> > + uuid = "-";
>
> Why aren't the strings left empty?

Not included in this series but we have a patch that defines an "info devices"
command to display the device properties. It was for convenience to print "-"
if the feature was not included. To be more generic, it makes sense to initialize
the strings empty and then do the checks at the usage side.

Thanks,
-Baris