[RFC,01/11] gdbstub: move allocation of GDBState to one place
diff mbox series

Message ID 20191115173000.21891-2-alex.bennee@linaro.org
State New
Headers show
Series
  • gdbstub re-factor and SVE support
Related show

Commit Message

Alex Bennée Nov. 15, 2019, 5:29 p.m. UTC
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(-)

Comments

Richard Henderson Nov. 18, 2019, 7:37 a.m. UTC | #1
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~
Richard Henderson Nov. 18, 2019, 7:41 a.m. UTC | #2
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~
Damien Hedde Nov. 18, 2019, 9:19 a.m. UTC | #3
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
Damien Hedde Nov. 18, 2019, 9:50 a.m. UTC | #4
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
Richard Henderson Nov. 18, 2019, 11:24 a.m. UTC | #5
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~

Patch
diff mbox series

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);