Re: [RFC PATCH 12/24] gdbstub: Simplify XML lookup

[Akihiko Odaki]

On 2023/08/14 22:27, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
> > Now we know all instances of GDBFeature that is used in CPU so we can
> > traverse them to find XML. This removes the need for a CPU-specific
> > lookup function for dynamic XMLs.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   gdbstub/gdbstub.c | 28 +++++++++-------------------
> >   1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > index 182efe7e0f..e5bb2c89ba 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -354,8 +354,7 @@ static const char *get_feature_xml(const char *p, const 
> > char **newp,
> >                                      GDBProcess *process)
> >   {
> >       size_t len;
> > -    int i;
> > -    const char *name;
> > +    GDBRegisterState *r;
> >       CPUState *cpu = gdb_get_first_cpu_in_process(process);
> >       CPUClass *cc = CPU_GET_CLASS(cpu);
> >   
> > @@ -364,15 +363,12 @@ static const char *get_feature_xml(const char *p, const char **newp,
> >           len++;
> >       *newp = p + len;
> >   
> > -    name = NULL;
> >       if (strncmp(p, "target.xml", len) == 0) {
> >           char *buf = process->target_xml;
> >           const size_t buf_sz = sizeof(process->target_xml);
> >   
> >           /* Generate the XML description for this CPU.  */
> >           if (!buf[0]) {
> > -            GDBRegisterState *r;
> > -
> >               pstrcat(buf, buf_sz,
> >                       "<?xml version=\"1.0\"?>"
> >                       "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"
> > @@ -389,28 +385,22 @@ static const char *get_feature_xml(const char *p, const 
> > char **newp,
> >               pstrcat(buf, buf_sz, "\"/>");
> >               for (r = cpu->gdb_regs; r; r = r->next) {
> >                   pstrcat(buf, buf_sz, "<xi:include href=\"");
> > -                pstrcat(buf, buf_sz, r->feature->xml);
> > +                pstrcat(buf, buf_sz, r->feature->xmlname);
> >                   pstrcat(buf, buf_sz, "\"/>");
> >               }
> >               pstrcat(buf, buf_sz, "</target>");
> >           }
> >           return buf;
> >       }
>
> It would be nice to modernise this code before adding to it. The static
> target_xml buffer and use of pstrcat could be replaced by GString code
> that is less sketchy.

I saw you did that yourself. Nevertheless I included my own 
implementation for the suggestion in v3. It uses 
g_markup_printf_escaped() for extra caution and better readability (i.e. 
the xi:include tags are written in a format: <xi:include href=\"%s\"/>).

> > -    if (cc->gdb_get_dynamic_xml) {
> > -        char *xmlname = g_strndup(p, len);
> > -        const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
> > -
> > -        g_free(xmlname);
> > -        if (xml) {
> > -            return xml;
> > -        }
> > +    if (strncmp(p, cc->gdb_core_feature->xmlname, len) == 0) {
> > +        return cc->gdb_core_feature->xml;
> >       }
> > -    for (i = 0; ; i++) {
> > -        name = gdb_features[i].xmlname;
> > -        if (!name || (strncmp(name, p, len) == 0 && strlen(name) == len))
> > -            break;
> > +    for (r = cpu->gdb_regs; r; r = r->next) {
> > +        if (strncmp(p, r->feature->xmlname, len) == 0) {
> > +            return r->feature->xml;
> > +        }
> >       }
> > -    return name ? gdb_features[i].xml : NULL;
> > +    return NULL;
> >   }
> >   
> >   const GDBFeature *gdb_find_static_feature(const char *xmlname)