Re: [PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls

[Alex Bennée]

Keith Packard <keithp@keithp.com> writes:

> When checking whether there is a live gdb connection, code shouldn't
> use 'gdbserver_state.init' as that value is set when the
> gdbserver_state structure is initialized in init_gdbserver_state, not
> when the gdb socket has a valid connection.
>
> The 'handle_detach' function appears to use 'gdbserver_state.c_cpu' as
> an indication of whether there is a connection, so I've used the same
> in use_gdb_syscalls.

I guess it could be anything that is set by gdb_accept_init(). I'm a
little wary of c_cpu given it has a specific meaning of current cpu and
does move around depending on actions of the debugger.

It would be better to wrap the test in a function (static bool
is_connected()?) so the semantic meaning is clear in the code and we can
fix things in one place if needed.

> This avoids a segfault when qemu is run with the '-s' flag (create a
> gdb protocol socket), but without the '-S' flag (delay until 'c'
> command is received).

How exactly did you create the segfault? Just starting with -s and
attaching to a running tasks works fine for me although I Can see
semihosting stuff would never get to gdb after connection.

> I would like this patch to inform a discussion on whether the numerous
> other places using gdbserver_state.init are also incorrect (most of
> them appear to be using it in the same way use_gdb_syscalls does), and
> also whether use_gdb_syscalls should cache the result of this check or
> whether it should check each time it is called to see if a gdb
> connection is currently acive.

Hmm I don't see anything obviously wrong - although I note a bunch of
tests also check for ->fd which is probably a clearer indication of an
active connection. I'm sure this could be improved with a semantically
clearer code though.

> For the second question, I don't have a
> clear idea; mixing gdb and native calls seems problematic for stateful
> operations like file open/close.

Yes it's a bit of a hack. I can imagine starting with a remote GDB
connection and then loosing it after opening a file descriptor would
result in Bad Things (tm). I'm not sure what the cleanest approach is to
handling the resulting mess.

>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  gdbstub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d99bc0bf2e..4e709d16fd 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -460,7 +460,7 @@ int use_gdb_syscalls(void)
>      /* -semihosting-config target=auto */
>      /* On the first call check if gdb is connected and remember. */
>      if (gdb_syscall_mode == GDB_SYS_UNKNOWN) {
> -        gdb_syscall_mode = gdbserver_state.init ?
> +        gdb_syscall_mode = gdbserver_state.c_cpu != NULL ?
>              GDB_SYS_ENABLED : GDB_SYS_DISABLED;
>      }
>      return gdb_syscall_mode == GDB_SYS_ENABLED;


-- 
Alex Bennée