diff mbox series

[RFC,12/24] gdbstub: Simplify XML lookup

Message ID 20230731084354.115015-13-akihiko.odaki@daynix.com
State New
Headers show
Series plugins: Allow to read registers | expand

Commit Message

Akihiko Odaki July 31, 2023, 8:43 a.m. UTC 
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(-)

Comments

Alex Bennée Aug. 14, 2023, 1:27 p.m. UTC | #1 
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.


> -    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)
Akihiko Odaki Aug. 16, 2023, 1:51 p.m. UTC | #2 
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)
> 
>
diff mbox series

Patch

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;
     }
-    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)