[Qemu-devel] [PATCH] gdbstub: fix gdb_get_cpu(s, pid, tid) when pid and/or tid are 0

[Qemu-devel] [PATCH] gdbstub: fix gdb_get_cpu(s, pid, tid) when pid and/or tid are 0

[Luc Michel]

a TID or PID value means "any thread" (resp. "any process"). This commit
fixes the different combinations when at least one value is 0.

When both are 0, the function now returns the first attached CPU,
instead of the CPU with TID 1, which is not necessarily attached or even
existent.

When PID is specified but TID is 0, the function returns the first CPU
in the process, or NULL if the process does not exist or is not
attached.

In other cases, it returns the corresponding CPU, while ignoring the PID
check when PID is 0.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---
 gdbstub.c | 72 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 29 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index bfc7afb509..d4cc6ecf99 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -754,39 +754,10 @@ static CPUState *gdb_next_cpu_in_process(const GDBState *s, CPUState *cpu)
     }
 
     return cpu;
 }
 
-static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid)
-{
-    GDBProcess *process;
-    CPUState *cpu;
-
-    if (!tid) {
-        /* 0 means any thread, we take the first one */
-        tid = 1;
-    }
-
-    cpu = find_cpu(tid);
-
-    if (cpu == NULL) {
-        return NULL;
-    }
-
-    process = gdb_get_cpu_process(s, cpu);
-
-    if (process->pid != pid) {
-        return NULL;
-    }
-
-    if (!process->attached) {
-        return NULL;
-    }
-
-    return cpu;
-}
-
 /* Return the cpu following @cpu, while ignoring unattached processes. */
 static CPUState *gdb_next_attached_cpu(const GDBState *s, CPUState *cpu)
 {
     cpu = CPU_NEXT(cpu);
 
@@ -812,10 +783,53 @@ static CPUState *gdb_first_attached_cpu(const GDBState *s)
     }
 
     return cpu;
 }
 
+static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid)
+{
+    GDBProcess *process;
+    CPUState *cpu;
+
+    if (!pid && !tid) {
+        /* 0 means any process/thread, we take the first attached one */
+        return gdb_first_attached_cpu(s);
+    } else if (pid && !tid) {
+        /* any thread in a specific process */
+        process = gdb_get_process(s, pid);
+
+        if (process == NULL) {
+            return NULL;
+        }
+
+        if (!process->attached) {
+            return NULL;
+        }
+
+        return get_first_cpu_in_process(s, process);
+    } else {
+        /* a specific thread */
+        cpu = find_cpu(tid);
+
+        if (cpu == NULL) {
+            return NULL;
+        }
+
+        process = gdb_get_cpu_process(s, cpu);
+
+        if (pid && process->pid != pid) {
+            return NULL;
+        }
+
+        if (!process->attached) {
+            return NULL;
+        }
+
+        return cpu;
+    }
+}
+
 static const char *get_feature_xml(const GDBState *s, const char *p,
                                    const char **newp, GDBProcess *process)
 {
     size_t len;
     int i;
-- 
2.20.1

Re: [Qemu-devel] [PATCH] gdbstub: fix gdb_get_cpu(s, pid, tid) when pid and/or tid are 0

[Peter Maydell]

On Sat, 19 Jan 2019 at 14:00, Luc Michel <luc.michel@greensocs.com> wrote:
>
> a TID or PID value means "any thread" (resp. "any process"). This commit
> fixes the different combinations when at least one value is 0.
>
> When both are 0, the function now returns the first attached CPU,
> instead of the CPU with TID 1, which is not necessarily attached or even
> existent.
>
> When PID is specified but TID is 0, the function returns the first CPU
> in the process, or NULL if the process does not exist or is not
> attached.
>
> In other cases, it returns the corresponding CPU, while ignoring the PID
> check when PID is 0.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

I've tracked down why my code was running into this, incidentally:
it turned out to be because I wasn't getting the parenting of the CPU
objects into the container correct, so the gdbstub ended up with
three processes, of which the first two were empty (and the CPUs
in the third default process). So the first thread (cpu 0) wasn't
in the first process, but in the last one.


thanks
-- PMM

Re: [Qemu-devel] [PATCH] gdbstub: fix gdb_get_cpu(s, pid, tid) when pid and/or tid are 0

[Peter Maydell]

On Mon, 21 Jan 2019 at 13:53, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sat, 19 Jan 2019 at 14:00, Luc Michel <luc.michel@greensocs.com> wrote:
> >
> > a TID or PID value means "any thread" (resp. "any process"). This commit
> > fixes the different combinations when at least one value is 0.
> >
> > When both are 0, the function now returns the first attached CPU,
> > instead of the CPU with TID 1, which is not necessarily attached or even
> > existent.
> >
> > When PID is specified but TID is 0, the function returns the first CPU
> > in the process, or NULL if the process does not exist or is not
> > attached.
> >
> > In other cases, it returns the corresponding CPU, while ignoring the PID
> > check when PID is 0.
> >
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> > ---
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I've tracked down why my code was running into this, incidentally:
> it turned out to be because I wasn't getting the parenting of the CPU
> objects into the container correct, so the gdbstub ended up with
> three processes, of which the first two were empty (and the CPUs
> in the third default process). So the first thread (cpu 0) wasn't
> in the first process, but in the last one.



Applied to target-arm.next, thanks.

-- PMM