Re: [PATCH 2/2] gdb: New maintenance command to print XML target description

[Pedro Alves <palves@redhat.com>]

On 6/11/20 11:41 AM, Andrew Burgess wrote:

> However, that class produced XML which seems designed for a machine to
> read, with only minimal indentation.  With a bit of tweaking I was
> able to restructure things so I could inherit from the existing XML
> producer and add indentation.
> 

Cool.

An interesting testcase this could add would be, run a program
to main, and then:

(gdb) pipe maint print xml-tdesc | cat > target.xml    (*)
(gdb) set tdesc filename target.xml

and then step to fetch registers with the new tesc.


Also another test that this could do is make make sure 
a roundtrip through GDB of a GDB-generated XML produces the
exact same XML.  Like:

(gdb) pipe maint print xml-tdesc input.xml | cat > out-1.xml
(gdb) pipe maint print xml-tdesc out-1.xml | cat > out-2.xml

After this, input.xml and out-*.xml may be different, but
out-1.xml and out-2.xml should be exactly the same, right?


(*) - or something like
  maint print xml-tdesc -o target.xml

> +/* Class to format the target description XML with nesting.  */
> +
> +class gdb_print_xml_feature : public print_xml_feature
> +{
> +public:
> +  /* Constructor.  */
> +
> +  gdb_print_xml_feature (std::string *arg)

explicit.

> +    :print_xml_feature (arg)

Space after ":".

> +  { /* Nothing.  */ }
> +
> +  /* Override this from the parent so we can add the compatibility
> +     information.   */

Curious.  Do you know why this isn't done in the parent?

> +
> +  void visit_pre (const target_desc *e) override
> +  {
> +    print_xml_feature::visit_pre (e);
> +    for (const bfd_arch_info_type *compatible : e->compatible)
> +      add_line ("<compatible>%s</compatible>", compatible->printable_name);
> +  };
> +
> +protected:
> +
> +  /* Pull in all copies of add_line.  */
> +
> +  using print_xml_feature::add_line;
> +
> +  /* Override this version of add_line to add white space for indentation
> +     at the start of the line.  */
> +
> +  void add_line (const std::string &str) override
> +  {
> +    std::string tmp = string_printf ("%*s", m_depth, "");
> +    tmp += str;
> +    print_xml_feature::add_line (tmp);
> +  }
> +
> +  /* Increase our nesting by ADJUST.  We use two spaces for every level of
> +     indentation.  */
> +
> +  void indent (int adjust) override
> +  {
> +    m_depth += (adjust * 2);
> +  }
> +
> +private:
> +
> +  /* The current level of indentation.  */
> +  int m_depth = 0;
> +};
> +
> +/* Implement the maintenance print xml-tdesc command.  */
> +
> +static void
> +maint_print_xml_tdesc_cmd (const char *args, int from_tty)
> +{
> +  const struct target_desc *tdesc;
> +
> +  if (args == NULL)
> +    {
> +      /* Use the global target-supplied description, not the current
> +	 architecture's.  This lets a GDB for one architecture generate C
> +	 for another architecture's description, even though the gdbarch

The "generate C" bit (copied from "maint print c-tdesc") should be
tweaked to say XML instead.

> +	 initialization code will reject the new description.  */
> +      tdesc = current_target_desc;
> +    }
> +  else
> +    {
> +      /* Use the target description from the XML file.  */
> +      tdesc = file_read_description_xml (args);
> +    }
> +
> +  if (tdesc == NULL)
> +    error (_("There is no target description to print."));
> +
> +  std::string buf;
> +  gdb_print_xml_feature v (&buf);
> +  tdesc->accept (v);
> +  puts_unfiltered (buf.c_str ());
> +}
> +

> +#
> +# 4. Indentation of lines will be preserved so your input file needs
> +#    to follow the expected indentation.
> +if {[gdb_skip_xml_test]} {
> +    unsupported "maint-xml-dump.exp"

This just results in:

UNSUPPORTED: gdb.xml/maint-xml-dump.exp: maint-xml-dump.exp

See:
 https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#A.22untested.22_calls

> +    return -1
> +}
> +
> +gdb_start


> +    close $ifd
> +
> +    # Due handling the <?xml ...?> tags we can end up with a stray

This reads a bit strange: is there a word missing in "Due handling"?

> +    # '\r\n' at the start of the output pattern.  Remove it here.
> +    if {[string range $pattern 0 1] == "\r\n"} {
> +	set pattern [string range $pattern 2 end]
> +    }
> +
> +    return $pattern
> +}

> --- a/gdbsupport/tdesc.h
> +++ b/gdbsupport/tdesc.h
> @@ -401,6 +401,21 @@ class print_xml_feature : public tdesc_element_visitor
>    void visit (const tdesc_type_with_fields *type) override;
>    void visit (const tdesc_reg *reg) override;
>  
> +protected:
> +
> +  /* Called with a positive value of ADJUST  we move inside an element, for

Spurious double space.  Maybe missing "when" as in "ADJUST when we" ?

> +     example inside <target>, and with a negative value when we leave the
> +     element.  In this class this function does nothing, but a sub-class
> +     can override this to track the current level of nesting.  */
> +  virtual void indent (int adjust)
> +  { /* Nothing.  */ }
> +
> +  /* Functions to add lines to the output buffer M_BUFFER.  Each of these
> +     functions appends a newline, so don't include one the strings being
> +     sent.  */

Did you mean "don't include one IN the" ?

> +  virtual void add_line (const std::string &str);
> +  virtual void add_line (const char *fmt, ...);
> +
>  private:
>    std::string *m_buffer;
>  };
> 

Thanks,
Pedro Alves