Re: [PATCH 2/2] gdbstub: Apply breakpoints only to the selected PID

["Alex Bennée" <alex.bennee@linaro.org>]

Roque Arcudia Hernandez <roqueh@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-iii@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 <iii@linux.ibm.com>

>
> Google-Bug-Id: 355027002
> Signed-off-by: Roque Arcudia Hernandez <roqueh@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