I'm reimplementing this in a new patchset with a new function
gdb_cpu_in_source_group instead of making public the PID and
multiptocess functions.
On Mon, Oct 7, 2024 at 3:15 AM Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Roque Arcudia Hernandez <roq...@google.com> writes:
>
> > In the context of using the remote gdb with multiple
> > processes/inferiors (multi cluster machine) a given breakpoint will
> > target an specific inferior. Current implementation of
> > tcg_insert_breakpoint and tcg_remove_breakpoint apply a given
> > breakpoint to all the cpus available in the system.
>
> OK I can see this needs fixing.
>
> > Only the CPUs associated with the selected process ID should insert
> > or remove the breakpoint. It is important to apply it to all the CPUs
> > in the process ID regardless of the particular thread selected by the
> > 'Hg' packet because even in the case of a thread specific breakpoint.
> > A breakpoint on a specific thread is treated as a conditional break
> > similar to a 'break if'. This can be read in the code and comments of
> > function bpstat_check_breakpoint_conditions in the file
> > gdb/breakpoint.c
> >
> > /* For breakpoints that are currently marked as telling gdb to stop,
> > check conditions (condition proper, frame, thread and ignore count)
> > of breakpoint referred to by BS. If we should not stop for this
> > breakpoint, set BS->stop to 0. */
> >
> > static void
> > bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
> >
> > The patch needs to expose the currently private function
> > gdb_get_cpu_pid to the TCG and also expose the value of
> > gdbserver_state.multiprocess. The PID filtering will only be
> > applicable to multiprocess gdb because the PIDs are only defined in
> > that context.
>
> I don't like exposing those private functions, especially as the PID
> terminology is confusing. Could we not keep all the logic in the gdbstub
> itself so the tests become something like:
>
> case GDB_BREAKPOINT_SW:
> case GDB_BREAKPOINT_HW:
> CPU_FOREACH(cpu) {
> if (gdb_cpu_in_source_group(cs, cpu)) {
> err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
> if (err) {
> break;
> }
> }
> }
> return err;
>
> ?
>
> Ilya has also posted a large series for non-stop mode on *-user guests
> which is on my review queue which I would want to check doesn't conflict
> with this:
>
> Message-ID: <20240923162208.90745-1-...@linux.ibm.com>
> Date: Mon, 23 Sep 2024 18:12:55 +0200
> Subject: [PATCH 00/18] Stop all qemu-cpu threads on a breakpoint
> From: Ilya Leoshkevich <i...@linux.ibm.com>
>
> >
> > Google-Bug-Id: 355027002
> > Signed-off-by: Roque Arcudia Hernandez <roq...@google.com>
> > ---
> > accel/tcg/tcg-accel-ops.c | 15 +++++++++++++++
> > gdbstub/gdbstub.c | 7 ++++++-
> > include/exec/gdbstub.h | 15 +++++++++++++++
> > 3 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> > index 3c19e68a79..557091c15e 100644
> > --- a/accel/tcg/tcg-accel-ops.c
> > +++ b/accel/tcg/tcg-accel-ops.c
> > @@ -33,6 +33,7 @@
> > #include "qemu/guest-random.h"
> > #include "qemu/timer.h"
> > #include "exec/exec-all.h"
> > +#include "exec/gdbstub.h"
> > #include "exec/hwaddr.h"
> > #include "exec/tb-flush.h"
> > #include "gdbstub/enums.h"
> > @@ -132,11 +133,15 @@ static int tcg_insert_breakpoint(CPUState *cs, int
> > type, vaddr addr, vaddr len)
> > {
> > CPUState *cpu;
> > int err = 0;
> > + bool match_pid = gdb_is_multiprocess();
> >
> > switch (type) {
> > case GDB_BREAKPOINT_SW:
> > case GDB_BREAKPOINT_HW:
> > CPU_FOREACH(cpu) {
> > + if (match_pid && gdb_get_cpu_pid(cpu) != gdb_get_cpu_pid(cs)) {
> > + continue;
> > + }
> > err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
> > if (err) {
> > break;
> > @@ -147,6 +152,9 @@ static int tcg_insert_breakpoint(CPUState *cs, int
> > type, vaddr addr, vaddr len)
> > case GDB_WATCHPOINT_READ:
> > case GDB_WATCHPOINT_ACCESS:
> > CPU_FOREACH(cpu) {
> > + if (match_pid && gdb_get_cpu_pid(cpu) != gdb_get_cpu_pid(cs)) {
> > + continue;
> > + }
> > err = cpu_watchpoint_insert(cpu, addr, len,
> > xlat_gdb_type(cpu, type), NULL);
> > if (err) {
> > @@ -163,11 +171,15 @@ static int tcg_remove_breakpoint(CPUState *cs, int
> > type, vaddr addr, vaddr len)
> > {
> > CPUState *cpu;
> > int err = 0;
> > + bool match_pid = gdb_is_multiprocess();
> >
> > switch (type) {
> > case GDB_BREAKPOINT_SW:
> > case GDB_BREAKPOINT_HW:
> > CPU_FOREACH(cpu) {
> > + if (match_pid && gdb_get_cpu_pid(cpu) != gdb_get_cpu_pid(cs)) {
> > + continue;
> > + }
> > err = cpu_breakpoint_remove(cpu, addr, BP_GDB);
> > if (err) {
> > break;
> > @@ -178,6 +190,9 @@ static int tcg_remove_breakpoint(CPUState *cs, int
> > type, vaddr addr, vaddr len)
> > case GDB_WATCHPOINT_READ:
> > case GDB_WATCHPOINT_ACCESS:
> > CPU_FOREACH(cpu) {
> > + if (match_pid && gdb_get_cpu_pid(cpu) != gdb_get_cpu_pid(cs)) {
> > + continue;
> > + }
> > err = cpu_watchpoint_remove(cpu, addr, len,
> > xlat_gdb_type(cpu, type));
> > if (err) {
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > index 98574eba68..628e599692 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -199,7 +199,12 @@ void gdb_memtox(GString *buf, const char *mem, int len)
> > }
> > }
> >
> > -static uint32_t gdb_get_cpu_pid(CPUState *cpu)
> > +bool gdb_is_multiprocess(void)
> > +{
> > + return gdbserver_state.multiprocess;
> > +}
> > +
> > +uint32_t gdb_get_cpu_pid(CPUState *cpu)
> > {
> > #ifdef CONFIG_USER_ONLY
> > return getpid();
> > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> > index d73f424f56..a7c95d215b 100644
> > --- a/include/exec/gdbstub.h
> > +++ b/include/exec/gdbstub.h
> > @@ -138,6 +138,21 @@ GArray *gdb_get_register_list(CPUState *cpu);
> >
> > void gdb_set_stop_cpu(CPUState *cpu);
> >
> > +/**
> > + * gdb_get_cpu_pid() - Get the PID assigned to a CPU
> > + * @cpu - the CPU to query
> > + *
> > + * Return: The PID associated with the cpu
> > + */
> > +uint32_t gdb_get_cpu_pid(CPUState *cpu);
> > +
> > +/**
> > + * gdb_is_multiprocess() - Tells if the gdb server supports multiple
> > processes
> > + *
> > + * Return: True if supported
> > + */
> > +bool gdb_is_multiprocess(void);
> > +
> > /* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
> > extern const GDBFeature gdb_static_features[];
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
