qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix for multi-process gdbstub breakpoints
@ 2024-09-06 22:54 Roque Arcudia Hernandez
  0 siblings, 0 replies; 5+ messages in thread
From: Roque Arcudia Hernandez @ 2024-09-06 22:54 UTC (permalink / raw)
  To: richard.henderson, pbonzini, alex.bennee, philmd, slongfield,
	komlodi
  Cc: qemu-devel, Roque Arcudia Hernandez

This patch series modifies the gdbstub to address a bug running a
multi cluster machine in QEMU using TCG. The machine where the
problem was seen had several clusters of CPUs with similar
architectures and similar memory layout all working with physical
addresses. It was discovered under gdb debugging that a breakpoint
targeting one cluster misfired on the wrong cluster quite frequently
with no possible workaround since gdb was also unaware of any
breakpoint in that cluster and simply reported SIGTRAP.

The sequence that discovered the bug adds N inferiors and adds a
breakpoint on inferior N. Then after continuing an unrelated thread
stops the execution when its PC hits the same address of the break
targeting a different inferior.

target extended-remote :1234
add-inferior
inferior 2
attach 2
...
add-inferior
inferior N
attach N
add-symbol-file /path/to/foo.elf
break foo
> Breakpoint 1 at 0xf00add
info break
> Num     Type           Disp Enb Address    What
> 1       breakpoint     keep y   0x00f00add in foo
>                                            at foo.c:1234 inf N
continue
> Continuing.
>
> Thread 2.1 received signal SIGTRAP, Trace/breakpoint trap.
> [Switching to Thread 2.2]
> 0xf00add in ?? ()

The main problem is the current implementation of
'tcg_insert_breakpoint' and 'tcg_remove_breakpoint' insert and remove
breakpoints to all the CPUs in the system regardless of what the
remote gdb protocol implements.

If we look at the current source code of GDB we can see that the
function 'remote_target::insert_breakpoint' in file gdb/remote.c has
the intention to select the process ID of the inferior where the
break was inserted.

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 ();
...
}

https:sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/remote.c;h=2c3988cb5075655e8a799d1cc5d4760ad8ed426e;hb=HEAD#l11023

This would not happen when we input the 'break' in gdb but it is
deferred until the time we execute the 'continue' command. Because we
might be currently selecting an inferior that is not the one where we
previously set the breakpoint, the remote gdb has to make sure we
move the focus to the process ID of the inferior where we inserted
the break.

In the previous example this will translate to something like:

HgpN.M
Z0,00f00add,4

Another thing that is wrong with the current implementation (and it
affects both TCG and KVM mode) is that the remote gdb protocol uses
'Hg' and not 'Hc' to select the process. Functions
'gdb_breakpoint_insert' and 'gdb_breakpoint_remove' receive wrongly
'gdbserver_state.c_cpu' instead of 'gdbserver_state.g_cpu'.

This is supported by the documentation of 'H op thread-id' where op =
'c' is reserved to the step and continue:

https:sourceware.org/gdb/current/onlinedocs/gdb.html/Packets.html

And it can be verified that the function 'set_general_process' in the
previous code snippet will eventually call
'remote_target::set_general_thread' and not
'remote_target::set_continue_thread' if it needs to change focus.

A third scenario that has to be taken into account is the case of a
break on a specific thread, for instance the sequence:

inferior 1
break bar thread 1.3
break bar thread 1.4

The remote protocol expects the gdbstub to apply the break to the
process ID of inferior 1 and considers the specific thread-id as a
breakpoint condition (not too different from a 'break if').

In this case the packet exchange may look like:

Hgp1.1
Z0,00ba2add,4

There wouldn't be an independent set of packets for 'Hgp1.3' and
'Hgp1.4'.

In the gdb source code, the handling of the specific thread-id
happens during breakpoint evaluation in function
'bpstat_check_breakpoint_conditions' of file gdb/breakpoint.c

https:sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/breakpoint.c;h=17bd627f867cf3d4dc81322ed1919ba40cbb237d;hb=HEAD#l5550

The proposed fix inserts or removes a breakpoint to all the cpus
sharing the same process ID as the one selected with the latest
received 'Hg' packet.

Roque Arcudia Hernandez (2):
  gdbstub: Fix wrong CPUState pointer in breakpoint functions
  gdbstub: Apply breakpoints only to the selected PID

 accel/tcg/tcg-accel-ops.c | 15 +++++++++++++++
 gdbstub/gdbstub.c         | 11 ++++++++---
 include/exec/gdbstub.h    | 15 +++++++++++++++
 3 files changed, 38 insertions(+), 3 deletions(-)

-- 
2.46.0.469.g59c65b2a67-goog



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 0/2] Fix for multi-process gdbstub breakpoints
@ 2025-05-08 22:45 Nabih Estefan
  2025-05-08 22:45 ` [PATCH 1/2] gdbstub: Fix wrong CPUState pointer in breakpoint functions Nabih Estefan
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Nabih Estefan @ 2025-05-08 22:45 UTC (permalink / raw)
  To: qemu-devel
  Cc: richard.henderson, pbonzini, alex.bennee, philmd, Nabih Estefan

This patch series modifies the gdbstub to address a bug running a
multi cluster machine in QEMU using TCG. The machine where the
problem was seen had several clusters of CPUs with similar
architectures and similar memory layout all working with physical
addresses. It was discovered under gdb debugging that a breakpoint
targeting one cluster misfired on the wrong cluster quite frequently
with no possible workaround since gdb was also unaware of any
breakpoint in that cluster and simply reported SIGTRAP.

The sequence that discovered the bug adds N inferiors and adds a
breakpoint on inferior N. Then after continuing an unrelated thread
stops the execution when its PC hits the same address of the break
targeting a different inferior.

target extended-remote :1234
add-inferior
inferior 2
attach 2
...
add-inferior
inferior N
attach N
add-symbol-file /path/to/foo.elf
break foo
> Breakpoint 1 at 0xf00add
info break
> Num     Type           Disp Enb Address    What
> 1       breakpoint     keep y   0x00f00add in foo
>                                            at foo.c:1234 inf N
continue
> Continuing.
>
> Thread 2.1 received signal SIGTRAP, Trace/breakpoint trap.
> [Switching to Thread 2.2]
> 0xf00add in ?? ()

The main problem is the current implementation of
'tcg_insert_breakpoint' and 'tcg_remove_breakpoint' insert and remove
breakpoints to all the CPUs in the system regardless of what the
remote gdb protocol implements.

If we look at the current source code of GDB we can see that the
function 'remote_target::insert_breakpoint' in file gdb/remote.c has
the intention to select the process ID of the inferior where the
break was inserted.

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 ();
...
}

https:sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/remote.c;h=2c3988cb5075655e8a799d1cc5d4760ad8ed426e;hb=HEAD#l11023

This would not happen when we input the 'break' in gdb but it is
deferred until the time we execute the 'continue' command. Because we
might be currently selecting an inferior that is not the one where we
previously set the breakpoint, the remote gdb has to make sure we
move the focus to the process ID of the inferior where we inserted
the break.

In the previous example this will translate to something like:

HgpN.M
Z0,00f00add,4

Another thing that is wrong with the current implementation (and it
affects both TCG and KVM mode) is that the remote gdb protocol uses
'Hg' and not 'Hc' to select the process. Functions
'gdb_breakpoint_insert' and 'gdb_breakpoint_remove' receive wrongly
'gdbserver_state.c_cpu' instead of 'gdbserver_state.g_cpu'.

This is supported by the documentation of 'H op thread-id' where op =
'c' is reserved to the step and continue:

https:sourceware.org/gdb/current/onlinedocs/gdb.html/Packets.html

And it can be verified that the function 'set_general_process' in the
previous code snippet will eventually call
'remote_target::set_general_thread' and not
'remote_target::set_continue_thread' if it needs to change focus.

A third scenario that has to be taken into account is the case of a
break on a specific thread, for instance the sequence:

inferior 1
break bar thread 1.3
break bar thread 1.4

The remote protocol expects the gdbstub to apply the break to the
process ID of inferior 1 and considers the specific thread-id as a
breakpoint condition (not too different from a 'break if').

In this case the packet exchange may look like:

Hgp1.1
Z0,00ba2add,4

There wouldn't be an independent set of packets for 'Hgp1.3' and
'Hgp1.4'.

In the gdb source code, the handling of the specific thread-id
happens during breakpoint evaluation in function
'bpstat_check_breakpoint_conditions' of file gdb/breakpoint.c

https:sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/breakpoint.c;h=17bd627f867cf3d4dc81322ed1919ba40cbb237d;hb=HEAD#l5550

The proposed fix inserts or removes a breakpoint to all the cpus
sharing the same process ID as the one selected with the latest
received 'Hg' packet.

Roque Arcudia Hernandez (2):
  gdbstub: Fix wrong CPUState pointer in breakpoint functions
  gdbstub: Apply breakpoints only to the selected PID

 accel/tcg/tcg-accel-ops.c | 37 +++++++++++++++++++++++--------------
 gdbstub/gdbstub.c         | 10 ++++++++--
 gdbstub/internals.h       | 13 +++++++++++--
 include/exec/gdbstub.h    | 12 ++++++++++++
 4 files changed, 54 insertions(+), 18 deletions(-)

-- 
2.49.0.1015.ga840276032-goog



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] gdbstub: Fix wrong CPUState pointer in breakpoint functions
  2025-05-08 22:45 [PATCH 0/2] Fix for multi-process gdbstub breakpoints Nabih Estefan
@ 2025-05-08 22:45 ` Nabih Estefan
  2025-05-08 22:45 ` [PATCH 2/2] gdbstub: Apply breakpoints only to the selected PID Nabih Estefan
  2025-05-20 14:25 ` [PATCH 0/2] Fix for multi-process gdbstub breakpoints Alex Bennée
  2 siblings, 0 replies; 5+ messages in thread
From: Nabih Estefan @ 2025-05-08 22:45 UTC (permalink / raw)
  To: qemu-devel
  Cc: richard.henderson, pbonzini, alex.bennee, philmd,
	Roque Arcudia Hernandez, Nabih Estefan

From: Roque Arcudia Hernandez <roqueh@google.com>

In the context of using the remote gdb with multiple
processes/inferiors (multiple cluster machine) a given breakpoint
will target an specific inferior. If needed the remote protocol will
use the packet 'H op thread-id' with op = 'g' to change focus to the
inferior we want to insert/remove the breakpoint to, for instance
'Hgp3.3' and not 'Hcp3.3'.

This is supported by the documentation of the H packets:

 > 'H op thread-id'
 > Set thread for subsequent operations ('m', 'M', 'g', 'G',
 > et.al.). Depending on the operation to be performed, op should be
 > 'c' for step and continue operations (note that this is
 > deprecated, supporting the 'vCont' command is a better option),
 > and 'g' for other operations.

This can also be verified in the GDB source code file gdb/remote.c.
Functions remote_target::insert_breakpoint and
remote_target::remove_breakpoint will eventually call
remote_target::set_general_thread if it needs to change the process
focus and not remote_target::set_continue_thread.

This can be seen around a comment that says:

      /* Make sure the remote is pointing at the right process, if
         necessary.  */

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 gdbstub/gdbstub.c   |  4 ++--
 gdbstub/internals.h | 13 +++++++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 565f6b33a9..0e2e10fbaa 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1160,7 +1160,7 @@ static void handle_insert_bp(GArray *params, void *user_ctx)
         return;
     }
 
-    res = gdb_breakpoint_insert(gdbserver_state.c_cpu,
+    res = gdb_breakpoint_insert(gdbserver_state.g_cpu,
                                 gdb_get_cmd_param(params, 0)->val_ul,
                                 gdb_get_cmd_param(params, 1)->val_ull,
                                 gdb_get_cmd_param(params, 2)->val_ull);
@@ -1184,7 +1184,7 @@ static void handle_remove_bp(GArray *params, void *user_ctx)
         return;
     }
 
-    res = gdb_breakpoint_remove(gdbserver_state.c_cpu,
+    res = gdb_breakpoint_remove(gdbserver_state.g_cpu,
                                 gdb_get_cmd_param(params, 0)->val_ul,
                                 gdb_get_cmd_param(params, 1)->val_ull,
                                 gdb_get_cmd_param(params, 2)->val_ull);
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index bf5a5c6302..e67fb6fe37 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -48,8 +48,17 @@ enum RSState {
 
 typedef struct GDBState {
     bool init;       /* have we been initialised? */
-    CPUState *c_cpu; /* current CPU for step/continue ops */
-    CPUState *g_cpu; /* current CPU for other ops */
+    /*
+     * Current CPU for step/continue ops. Updated by the remote packet
+     * 'Hc thread-id'
+     */
+    CPUState *c_cpu;
+    /*
+     * Current CPU for other ops such as memory accesses ('m'/'M'), general
+     * register accesses ('g'/'G'), breakpoint management ('z'/'Z'), etc.
+     * Updated by the remote packet 'Hg thread-id'
+     */
+    CPUState *g_cpu;
     CPUState *query_cpu; /* for q{f|s}ThreadInfo */
     enum RSState state; /* parsing state */
     char line_buf[MAX_PACKET_LENGTH];
-- 
2.49.0.1015.ga840276032-goog



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] gdbstub: Apply breakpoints only to the selected PID
  2025-05-08 22:45 [PATCH 0/2] Fix for multi-process gdbstub breakpoints Nabih Estefan
  2025-05-08 22:45 ` [PATCH 1/2] gdbstub: Fix wrong CPUState pointer in breakpoint functions Nabih Estefan
@ 2025-05-08 22:45 ` Nabih Estefan
  2025-05-20 14:25 ` [PATCH 0/2] Fix for multi-process gdbstub breakpoints Alex Bennée
  2 siblings, 0 replies; 5+ messages in thread
From: Nabih Estefan @ 2025-05-08 22:45 UTC (permalink / raw)
  To: qemu-devel
  Cc: richard.henderson, pbonzini, alex.bennee, philmd,
	Roque Arcudia Hernandez, Nabih Estefan

From: Roque Arcudia Hernandez <roqueh@google.com>

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.

Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 accel/tcg/tcg-accel-ops.c | 37 +++++++++++++++++++++++--------------
 gdbstub/gdbstub.c         |  6 ++++++
 include/exec/gdbstub.h    | 12 ++++++++++++
 3 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index b24d6a7562..aca476cdf5 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -34,6 +34,7 @@
 #include "qemu/guest-random.h"
 #include "qemu/timer.h"
 #include "exec/cputlb.h"
+#include "exec/gdbstub.h"
 #include "exec/hwaddr.h"
 #include "exec/tb-flush.h"
 #include "exec/translation-block.h"
@@ -139,9 +140,11 @@ static int tcg_insert_breakpoint(CPUState *cs, int type, vaddr addr, vaddr len)
     case GDB_BREAKPOINT_SW:
     case GDB_BREAKPOINT_HW:
         CPU_FOREACH(cpu) {
-            err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
-            if (err) {
-                break;
+            if (gdb_cpu_in_source_group(cs, cpu)) {
+                err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
+                if (err) {
+                    break;
+                }
             }
         }
         return err;
@@ -149,10 +152,12 @@ static int tcg_insert_breakpoint(CPUState *cs, int type, vaddr addr, vaddr len)
     case GDB_WATCHPOINT_READ:
     case GDB_WATCHPOINT_ACCESS:
         CPU_FOREACH(cpu) {
-            err = cpu_watchpoint_insert(cpu, addr, len,
-                                        xlat_gdb_type(cpu, type), NULL);
-            if (err) {
-                break;
+            if (gdb_cpu_in_source_group(cs, cpu)) {
+                err = cpu_watchpoint_insert(cpu, addr, len,
+                                            xlat_gdb_type(cpu, type), NULL);
+                if (err) {
+                    break;
+                }
             }
         }
         return err;
@@ -170,9 +175,11 @@ static int tcg_remove_breakpoint(CPUState *cs, int type, vaddr addr, vaddr len)
     case GDB_BREAKPOINT_SW:
     case GDB_BREAKPOINT_HW:
         CPU_FOREACH(cpu) {
-            err = cpu_breakpoint_remove(cpu, addr, BP_GDB);
-            if (err) {
-                break;
+            if (gdb_cpu_in_source_group(cs, cpu)) {
+                err = cpu_breakpoint_remove(cpu, addr, BP_GDB);
+                if (err) {
+                    break;
+                }
             }
         }
         return err;
@@ -180,10 +187,12 @@ static int tcg_remove_breakpoint(CPUState *cs, int type, vaddr addr, vaddr len)
     case GDB_WATCHPOINT_READ:
     case GDB_WATCHPOINT_ACCESS:
         CPU_FOREACH(cpu) {
-            err = cpu_watchpoint_remove(cpu, addr, len,
-                                        xlat_gdb_type(cpu, type));
-            if (err) {
-                break;
+            if (gdb_cpu_in_source_group(cs, cpu)) {
+                err = cpu_watchpoint_remove(cpu, addr, len,
+                                            xlat_gdb_type(cpu, type));
+                if (err) {
+                    break;
+                }
             }
         }
         return err;
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 0e2e10fbaa..652e0e768f 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -492,6 +492,12 @@ const GDBFeature *gdb_find_static_feature(const char *xmlname)
     g_assert_not_reached();
 }
 
+bool gdb_cpu_in_source_group(CPUState *cs, CPUState *cpu)
+{
+    return !gdbserver_state.multiprocess ||
+           (gdb_get_cpu_pid(cs) == gdb_get_cpu_pid(cpu));
+}
+
 GArray *gdb_get_register_list(CPUState *cpu)
 {
     GArray *results = g_array_new(true, true, sizeof(GDBRegDesc));
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 0675b0b646..f92641793c 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -114,6 +114,18 @@ void gdb_feature_builder_end(const GDBFeatureBuilder *builder);
  */
 const GDBFeature *gdb_find_static_feature(const char *xmlname);
 
+/**
+ * Tests if the CPUs belong to the same group for the purposes of breakpoint
+ * insertion and deletion when running multiprocesses gdb. The test is only
+ * valid for multiprocess gdb and should not affect the insertion or deletion of
+ * breakpoints when we are not running in that mode.
+ * @cs: The CPU used as reference
+ * @cpu: The CPU to test
+ *
+ * Return: True if they belong to the same group or it is not a multiprocess gdb
+ */
+bool gdb_cpu_in_source_group(CPUState *cs, CPUState *cpu);
+
 /**
  * gdb_read_register() - Read a register associated with a CPU.
  * @cpu: The CPU associated with the register.
-- 
2.49.0.1015.ga840276032-goog



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 0/2] Fix for multi-process gdbstub breakpoints
  2025-05-08 22:45 [PATCH 0/2] Fix for multi-process gdbstub breakpoints Nabih Estefan
  2025-05-08 22:45 ` [PATCH 1/2] gdbstub: Fix wrong CPUState pointer in breakpoint functions Nabih Estefan
  2025-05-08 22:45 ` [PATCH 2/2] gdbstub: Apply breakpoints only to the selected PID Nabih Estefan
@ 2025-05-20 14:25 ` Alex Bennée
  2 siblings, 0 replies; 5+ messages in thread
From: Alex Bennée @ 2025-05-20 14:25 UTC (permalink / raw)
  To: Nabih Estefan; +Cc: qemu-devel, richard.henderson, pbonzini, philmd

Nabih Estefan <nabihestefan@google.com> writes:

> This patch series modifies the gdbstub to address a bug running a
> multi cluster machine in QEMU using TCG.

Was this a downstream multi-cluster machine? Do we have any examples for
upstream? It would be nice to add a gdbstub test case to cover the
multi-inferior behaviour.

> The machine where the
> problem was seen had several clusters of CPUs with similar
> architectures and similar memory layout all working with physical
> addresses. It was discovered under gdb debugging that a breakpoint
> targeting one cluster misfired on the wrong cluster quite frequently
> with no possible workaround since gdb was also unaware of any
> breakpoint in that cluster and simply reported SIGTRAP.
>
> The sequence that discovered the bug adds N inferiors and adds a
> breakpoint on inferior N. Then after continuing an unrelated thread
> stops the execution when its PC hits the same address of the break
> targeting a different inferior.
>
> target extended-remote :1234
> add-inferior
> inferior 2
> attach 2
> ...
> add-inferior
> inferior N
> attach N
> add-symbol-file /path/to/foo.elf
> break foo
>> Breakpoint 1 at 0xf00add
> info break
>> Num     Type           Disp Enb Address    What
>> 1       breakpoint     keep y   0x00f00add in foo
>>                                            at foo.c:1234 inf N
> continue
>> Continuing.
>>
>> Thread 2.1 received signal SIGTRAP, Trace/breakpoint trap.
>> [Switching to Thread 2.2]
>> 0xf00add in ?? ()
>
> The main problem is the current implementation of
> 'tcg_insert_breakpoint' and 'tcg_remove_breakpoint' insert and remove
> breakpoints to all the CPUs in the system regardless of what the
> remote gdb protocol implements.
>
> If we look at the current source code of GDB we can see that the
> function 'remote_target::insert_breakpoint' in file gdb/remote.c has
> the intention to select the process ID of the inferior where the
> break was inserted.
>
> 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 ();
> ...
> }
>
> https:sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/remote.c;h=2c3988cb5075655e8a799d1cc5d4760ad8ed426e;hb=HEAD#l11023
>
> This would not happen when we input the 'break' in gdb but it is
> deferred until the time we execute the 'continue' command. Because we
> might be currently selecting an inferior that is not the one where we
> previously set the breakpoint, the remote gdb has to make sure we
> move the focus to the process ID of the inferior where we inserted
> the break.
>
> In the previous example this will translate to something like:
>
> HgpN.M
> Z0,00f00add,4
>
> Another thing that is wrong with the current implementation (and it
> affects both TCG and KVM mode) is that the remote gdb protocol uses
> 'Hg' and not 'Hc' to select the process. Functions
> 'gdb_breakpoint_insert' and 'gdb_breakpoint_remove' receive wrongly
> 'gdbserver_state.c_cpu' instead of 'gdbserver_state.g_cpu'.
>
> This is supported by the documentation of 'H op thread-id' where op =
> 'c' is reserved to the step and continue:
>
> https:sourceware.org/gdb/current/onlinedocs/gdb.html/Packets.html
>
> And it can be verified that the function 'set_general_process' in the
> previous code snippet will eventually call
> 'remote_target::set_general_thread' and not
> 'remote_target::set_continue_thread' if it needs to change focus.
>
> A third scenario that has to be taken into account is the case of a
> break on a specific thread, for instance the sequence:
>
> inferior 1
> break bar thread 1.3
> break bar thread 1.4
>
> The remote protocol expects the gdbstub to apply the break to the
> process ID of inferior 1 and considers the specific thread-id as a
> breakpoint condition (not too different from a 'break if').
>
> In this case the packet exchange may look like:
>
> Hgp1.1
> Z0,00ba2add,4
>
> There wouldn't be an independent set of packets for 'Hgp1.3' and
> 'Hgp1.4'.
>
> In the gdb source code, the handling of the specific thread-id
> happens during breakpoint evaluation in function
> 'bpstat_check_breakpoint_conditions' of file gdb/breakpoint.c
>
> https:sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/breakpoint.c;h=17bd627f867cf3d4dc81322ed1919ba40cbb237d;hb=HEAD#l5550
>
> The proposed fix inserts or removes a breakpoint to all the cpus
> sharing the same process ID as the one selected with the latest
> received 'Hg' packet.
>
> Roque Arcudia Hernandez (2):
>   gdbstub: Fix wrong CPUState pointer in breakpoint functions
>   gdbstub: Apply breakpoints only to the selected PID
>
>  accel/tcg/tcg-accel-ops.c | 37 +++++++++++++++++++++++--------------
>  gdbstub/gdbstub.c         | 10 ++++++++--
>  gdbstub/internals.h       | 13 +++++++++++--
>  include/exec/gdbstub.h    | 12 ++++++++++++
>  4 files changed, 54 insertions(+), 18 deletions(-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-05-20 14:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-08 22:45 [PATCH 0/2] Fix for multi-process gdbstub breakpoints Nabih Estefan
2025-05-08 22:45 ` [PATCH 1/2] gdbstub: Fix wrong CPUState pointer in breakpoint functions Nabih Estefan
2025-05-08 22:45 ` [PATCH 2/2] gdbstub: Apply breakpoints only to the selected PID Nabih Estefan
2025-05-20 14:25 ` [PATCH 0/2] Fix for multi-process gdbstub breakpoints Alex Bennée
  -- strict thread matches above, loose matches on Subject: below --
2024-09-06 22:54 Roque Arcudia Hernandez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).