[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_sys
From: |
Alex Bennée |
Subject: |
Re: [PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls |
Date: |
Fri, 08 Jan 2021 12:36:06 +0000 |
User-agent: |
mu4e 1.5.7; emacs 28.0.50 |
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
- Re: [PATCH] gdbstub.c uses incorrect check for active gdb in use_gdb_syscalls,
Alex Bennée <=