Keith Packard <[email protected]> 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 <[email protected]> > --- > 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
