Re: [PATCH 1/2] gdbstub: Fix wrong CPUState pointer in breakpoint functions

[Alex Bennée]

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

> 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.

Can we better comment:

    CPUState *c_cpu; /* current CPU for step/continue ops */
    CPUState *g_cpu; /* current CPU for other ops */

in GDBState?

>
> 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.  */
>
> Google-Bug-Id: 355027002
> Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> ---
>  gdbstub/gdbstub.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index d08568cea0..98574eba68 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1148,7 +1148,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);
> @@ -1172,7 +1172,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);

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro