diff mbox series

[PULL,10/37] gdbstub: add multiprocess support to Xfer:features:read:

Message ID 20190107163117.16269-11-peter.maydell@linaro.org
State Accepted
Commit c145eeae1cc7bb826fd60056bbd072e3881dc128
Headers show
Series target-arm queue | expand

Commit Message

Peter Maydell Jan. 7, 2019, 4:30 p.m. UTC 
From: Luc Michel <luc.michel@greensocs.com>


Change the Xfer:features:read: packet handling to support the
multiprocess extension. This packet is used to request the XML
description of the CPU. In multiprocess mode, different descriptions can
be sent for different processes.

This function now takes the process to send the description for as a
parameter, and use a buffer in the process structure to store the
generated description.

It takes the first CPU of the process to generate the description.

Signed-off-by: Luc Michel <luc.michel@greensocs.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Message-id: 20181207090135.7651-9-luc.michel@greensocs.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 gdbstub.c | 52 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

-- 
2.19.2

Comments

Peter Maydell Jan. 17, 2019, 5:22 p.m. UTC | #1 
On Mon, 7 Jan 2019 at 16:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>

> From: Luc Michel <luc.michel@greensocs.com>

>

> Change the Xfer:features:read: packet handling to support the

> multiprocess extension. This packet is used to request the XML

> description of the CPU. In multiprocess mode, different descriptions can

> be sent for different processes.

>

> This function now takes the process to send the description for as a

> parameter, and use a buffer in the process structure to store the

> generated description.

>

> It takes the first CPU of the process to generate the description.


> @@ -1650,14 +1655,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)

>              const char *xml;

>              target_ulong total_len;

>

> -            cc = CPU_GET_CLASS(first_cpu);

> +            process = gdb_get_cpu_process(s, s->g_cpu);

> +            cc = CPU_GET_CLASS(s->g_cpu);

>              if (cc->gdb_core_xml_file == NULL) {

>                  goto unknown_command;

>              }

>

>              gdb_has_xml = true;

>              p += 19;

> -            xml = get_feature_xml(p, &p, cc);

> +            xml = get_feature_xml(s, p, &p, process);

>              if (!xml) {

>                  snprintf(buf, sizeof(buf), "E00");

>                  put_packet(s, buf);


I've just run in to a segv in the gdbstub code in this bit,
because s->g_cpu is NULL.

Looking at the protocol trace from the gdb end:
(gdb) set debug remote 1
(gdb) target remote :1234
Remote debugging using :1234
Sending packet:
$qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;xmlRegisters=i386#6a...Ack
Packet received: PacketSize=1000;qXfer:features:read+;multiprocess+
Packet qSupported (supported-packets) is supported
Sending packet: $vMustReplyEmpty#3a...Ack
Packet received:
Sending packet: $Hgp0.0#ad...Ack
Packet received: E22
Sending packet: $qXfer:features:read:target.xml:0,ffb#79...Ack
[here is where QEMU crashes]

What seems to happen is that GDB's attempt to set the
current thread with the "Hg" command fails because
gdb_get_cpu() fails[*], and so we return an E22 error status.
GDB (incorrectly) ignores this and issues a general command
anyway, and then QEMU crashes because we don't handle s->g_cpu
being NULL in this command's handling code.

[*] This happens with an out-of-tree board model I'm working
on that uses the cpu cluster stuff, so it's probably a bug in
my code that causes it.

I think it would be nice to do something more robust in these
cases, even if it does only happen when a QEMU bug and a GDB
bug collide :-)

thanks
-- PMM
diff mbox series

Patch

diff --git a/gdbstub.c b/gdbstub.c
index f70b5a326fe..1f2b155490d 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -300,6 +300,8 @@  typedef struct GDBRegisterState {
 typedef struct GDBProcess {
     uint32_t pid;
     bool attached;
+
+    char target_xml[1024];
 } GDBProcess;
 
 enum RSState {
@@ -812,13 +814,14 @@  static CPUState *gdb_first_attached_cpu(const GDBState *s)
     return cpu;
 }
 
-static const char *get_feature_xml(const char *p, const char **newp,
-                                   CPUClass *cc)
+static const char *get_feature_xml(const GDBState *s, const char *p,
+                                   const char **newp, GDBProcess *process)
 {
     size_t len;
     int i;
     const char *name;
-    static char target_xml[1024];
+    CPUState *cpu = get_first_cpu_in_process(s, process);
+    CPUClass *cc = CPU_GET_CLASS(cpu);
 
     len = 0;
     while (p[len] && p[len] != ':')
@@ -827,36 +830,37 @@  static const char *get_feature_xml(const char *p, const char **newp,
 
     name = NULL;
     if (strncmp(p, "target.xml", len) == 0) {
-        /* Generate the XML description for this CPU.  */
-        if (!target_xml[0]) {
-            GDBRegisterState *r;
-            CPUState *cpu = first_cpu;
+        char *buf = process->target_xml;
+        const size_t buf_sz = sizeof(process->target_xml);
 
-            pstrcat(target_xml, sizeof(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\">"
                     "<target>");
             if (cc->gdb_arch_name) {
                 gchar *arch = cc->gdb_arch_name(cpu);
-                pstrcat(target_xml, sizeof(target_xml), "<architecture>");
-                pstrcat(target_xml, sizeof(target_xml), arch);
-                pstrcat(target_xml, sizeof(target_xml), "</architecture>");
+                pstrcat(buf, buf_sz, "<architecture>");
+                pstrcat(buf, buf_sz, arch);
+                pstrcat(buf, buf_sz, "</architecture>");
                 g_free(arch);
             }
-            pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
-            pstrcat(target_xml, sizeof(target_xml), cc->gdb_core_xml_file);
-            pstrcat(target_xml, sizeof(target_xml), "\"/>");
+            pstrcat(buf, buf_sz, "<xi:include href=\"");
+            pstrcat(buf, buf_sz, cc->gdb_core_xml_file);
+            pstrcat(buf, buf_sz, "\"/>");
             for (r = cpu->gdb_regs; r; r = r->next) {
-                pstrcat(target_xml, sizeof(target_xml), "<xi:include href=\"");
-                pstrcat(target_xml, sizeof(target_xml), r->xml);
-                pstrcat(target_xml, sizeof(target_xml), "\"/>");
+                pstrcat(buf, buf_sz, "<xi:include href=\"");
+                pstrcat(buf, buf_sz, r->xml);
+                pstrcat(buf, buf_sz, "\"/>");
             }
-            pstrcat(target_xml, sizeof(target_xml), "</target>");
+            pstrcat(buf, buf_sz, "</target>");
         }
-        return target_xml;
+        return buf;
     }
     if (cc->gdb_get_dynamic_xml) {
-        CPUState *cpu = first_cpu;
         char *xmlname = g_strndup(p, len);
         const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
 
@@ -1266,6 +1270,7 @@  out:
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
+    GDBProcess *process;
     CPUClass *cc;
     const char *p;
     uint32_t pid, tid;
@@ -1650,14 +1655,15 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
             const char *xml;
             target_ulong total_len;
 
-            cc = CPU_GET_CLASS(first_cpu);
+            process = gdb_get_cpu_process(s, s->g_cpu);
+            cc = CPU_GET_CLASS(s->g_cpu);
             if (cc->gdb_core_xml_file == NULL) {
                 goto unknown_command;
             }
 
             gdb_has_xml = true;
             p += 19;
-            xml = get_feature_xml(p, &p, cc);
+            xml = get_feature_xml(s, p, &p, process);
             if (!xml) {
                 snprintf(buf, sizeof(buf), "E00");
                 put_packet(s, buf);
@@ -2070,6 +2076,7 @@  static void create_default_process(GDBState *s)
 
     process->pid = max_pid + 1;
     process->attached = false;
+    process->target_xml[0] = '\0';
 }
 
 #ifdef CONFIG_USER_ONLY
@@ -2345,6 +2352,7 @@  static int find_cpu_clusters(Object *child, void *opaque)
         assert(cluster->cluster_id != UINT32_MAX);
         process->pid = cluster->cluster_id + 1;
         process->attached = false;
+        process->target_xml[0] = '\0';
 
         return 0;
     }