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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

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


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