| Message ID | 20240906225451.1039718-3-roqueh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix for multi-process gdbstub breakpoints | expand |
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[];
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.bennee@linaro.org> wrote: > > 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
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[];
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. This is not how gdb expects the remote protocol to behave. If we refer to the current source of gdb, in the function remote_target::insert_breakpoint in the file gdb/remote.c we can see the intention to have the right process selected before breakpoint insertion: int remote_target::insert_breakpoint (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { ... /* Make sure the remote is pointing at the right process, if necessary. */ if (!gdbarch_has_global_breakpoints (current_inferior ()->arch ())) set_general_process (); ... } Since most platforms do not have global breakpoints, this typically will result in an 'Hg' Packet sent before 'z/Z', if gdb is not pointing to the desired process. For instance this is a concrete example obtained with a trace dump: gdbstub_io_command Received: Hgp2.2 gdbstub_io_command Received: Z0,4000ded0,4 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. 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(-)