| Message ID | 20191115173000.21891-2-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gdbstub re-factor and SVE support | expand |
On 11/15/19 6:29 PM, Alex Bennée wrote: > We use g_new0() as it is the preferred form for such allocations. We > can also ensure that gdbserver_state is reset in one place. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > gdbstub.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 11/15/19 6:29 PM, Alex Bennée wrote: > > static GDBState *gdbserver_state; > > +static GDBState *gdb_allocate_state(void) > +{ > + g_assert(!gdbserver_state); > + gdbserver_state = g_new0(GDBState, 1); > + return gdbserver_state; > +} > + Actually, if we're only going to have one, why are we allocating it dynamically? We might as well allocate it statically and drop the pointer indirection. r~
On 11/18/19 8:41 AM, Richard Henderson wrote: > On 11/15/19 6:29 PM, Alex Bennée wrote: >> >> static GDBState *gdbserver_state; >> >> +static GDBState *gdb_allocate_state(void) >> +{ >> + g_assert(!gdbserver_state); >> + gdbserver_state = g_new0(GDBState, 1); >> + return gdbserver_state; >> +} >> + > > Actually, if we're only going to have one, why are we allocating it > dynamically? We might as well allocate it statically and drop the pointer > indirection. In use_gdb_syscalls(), we check if gdbserver_state is NULL: | /* -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 ? GDB_SYS_ENABLED | : GDB_SYS_DISABLED); | } So we cannot drop the pointer or we have to add some flag to do this test. Damien
On 11/15/19 6:29 PM, Alex Bennée wrote: > We use g_new0() as it is the preferred form for such allocations. We > can also ensure that gdbserver_state is reset in one place. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > gdbstub.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> -- Damien
On 11/18/19 10:19 AM, Damien Hedde wrote: > > > On 11/18/19 8:41 AM, Richard Henderson wrote: >> On 11/15/19 6:29 PM, Alex Bennée wrote: >>> >>> static GDBState *gdbserver_state; >>> >>> +static GDBState *gdb_allocate_state(void) >>> +{ >>> + g_assert(!gdbserver_state); >>> + gdbserver_state = g_new0(GDBState, 1); >>> + return gdbserver_state; >>> +} >>> + >> >> Actually, if we're only going to have one, why are we allocating it >> dynamically? We might as well allocate it statically and drop the pointer >> indirection. > > In use_gdb_syscalls(), we check if gdbserver_state is NULL: > | /* -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 ? GDB_SYS_ENABLED > | : GDB_SYS_DISABLED); > | } > > So we cannot drop the pointer or we have to add some flag to do this test. True, but perhaps a bool gdbserver_state.in_use is a clearer way to do this? r~
diff --git a/gdbstub.c b/gdbstub.c index 4cf8af365e2..c5b6701825f 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -374,6 +374,13 @@ static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER; static GDBState *gdbserver_state; +static GDBState *gdb_allocate_state(void) +{ + g_assert(!gdbserver_state); + gdbserver_state = g_new0(GDBState, 1); + return gdbserver_state; +} + bool gdb_has_xml; #ifdef CONFIG_USER_ONLY @@ -3083,15 +3090,13 @@ static bool gdb_accept(void) return false; } - s = g_malloc0(sizeof(GDBState)); + s = gdb_allocate_state(); create_default_process(s); s->processes[0].attached = true; s->c_cpu = gdb_first_attached_cpu(s); s->g_cpu = s->c_cpu; s->fd = fd; gdb_has_xml = false; - - gdbserver_state = s; return true; } @@ -3359,8 +3364,7 @@ int gdbserver_start(const char *device) s = gdbserver_state; if (!s) { - s = g_malloc0(sizeof(GDBState)); - gdbserver_state = s; + s = gdb_allocate_state(); qemu_add_vm_change_state_handler(gdb_vm_state_change, NULL);
We use g_new0() as it is the preferred form for such allocations. We can also ensure that gdbserver_state is reset in one place. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- gdbstub.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) -- 2.20.1