diff mbox series

[v1,04/10] gdbstub: split GDBConnection from main structure

Message ID 20221216112206.3171578-5-alex.bennee@linaro.org
State New
Headers show
Series split user and system code in gdbstub | expand

Commit Message

Alex Bennée Dec. 16, 2022, 11:22 a.m. UTC 
In preparation for moving user/softmmu specific bits from the main
gdbstub file we need to separate the connection details to what will
eventually become an anonymous pointer.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub/gdbstub.c | 99 +++++++++++++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 41 deletions(-)

Comments

Fabiano Rosas Dec. 16, 2022, 3:29 p.m. UTC | #1 
Alex Bennée <alex.bennee@linaro.org> writes:

> In preparation for moving user/softmmu specific bits from the main
> gdbstub file we need to separate the connection details to what will
> eventually become an anonymous pointer.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  gdbstub/gdbstub.c | 99 +++++++++++++++++++++++++++--------------------
>  1 file changed, 58 insertions(+), 41 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index be88ca0d71..14ce911bf2 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -342,6 +342,20 @@ enum RSState {
>      RS_CHKSUM1,
>      RS_CHKSUM2,
>  };
> +
> +#ifdef CONFIG_USER_ONLY
> +typedef struct {
> +    int fd;
> +    char *socket_path;
> +    int running_state;
> +} GDBConnection;
> +#else
> +typedef struct {
> +    CharBackend chr;
> +    Chardev *mon_chr;
> +} GDBConnection;
> +#endif
> +
>  typedef struct GDBState {
>      bool init;       /* have we been initialised? */
>      CPUState *c_cpu; /* current CPU for step/continue ops */
> @@ -354,14 +368,7 @@ typedef struct GDBState {
>      int line_csum; /* checksum at the end of the packet */
>      GByteArray *last_packet;
>      int signal;
> -#ifdef CONFIG_USER_ONLY
> -    int fd;
> -    char *socket_path;
> -    int running_state;
> -#else
> -    CharBackend chr;
> -    Chardev *mon_chr;
> -#endif
> +    GDBConnection *connection;
>      bool multiprocess;
>      GDBProcess *processes;
>      int process_num;
> @@ -392,6 +399,12 @@ static void init_gdbserver_state(void)
>      gdbserver_state.supported_sstep_flags = accel_supported_gdbstub_sstep_flags();
>      gdbserver_state.sstep_flags = SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
>      gdbserver_state.sstep_flags &= gdbserver_state.supported_sstep_flags;
> +
> +    /*
> +     * The following is differs depending on USER/SOFTMMU, we just
> +     * hid it in the typedef.
> +     */
> +    gdbserver_state.connection = g_new(GDBConnection, 1);

Does it need freeing at reset_gdbserver_state()?
Richard Henderson Dec. 16, 2022, 8:11 p.m. UTC | #2 
On 12/16/22 03:22, Alex Bennée wrote:
> In preparation for moving user/softmmu specific bits from the main
> gdbstub file we need to separate the connection details to what will
> eventually become an anonymous pointer.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   gdbstub/gdbstub.c | 99 +++++++++++++++++++++++++++--------------------
>   1 file changed, 58 insertions(+), 41 deletions(-)
> 
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index be88ca0d71..14ce911bf2 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -342,6 +342,20 @@ enum RSState {
>       RS_CHKSUM1,
>       RS_CHKSUM2,
>   };
> +
> +#ifdef CONFIG_USER_ONLY
> +typedef struct {
> +    int fd;
> +    char *socket_path;
> +    int running_state;
> +} GDBConnection;
> +#else
> +typedef struct {
> +    CharBackend chr;
> +    Chardev *mon_chr;
> +} GDBConnection;
> +#endif

You added the struct tag in the next patch.  Move it back if you need it, otherwise drop it.

> +    GDBConnection *connection;
>       bool multiprocess;
>       GDBProcess *processes;
>       int process_num;
> @@ -392,6 +399,12 @@ static void init_gdbserver_state(void)
>       gdbserver_state.supported_sstep_flags = accel_supported_gdbstub_sstep_flags();
>       gdbserver_state.sstep_flags = SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
>       gdbserver_state.sstep_flags &= gdbserver_state.supported_sstep_flags;
> +
> +    /*
> +     * The following is differs depending on USER/SOFTMMU, we just
> +     * hid it in the typedef.
> +     */
> +    gdbserver_state.connection = g_new(GDBConnection, 1);

Does this structure really need this pointer?

Given that GDBState gdbserver_state is a global singleton, I would imagine that 
GDBConnection gdbserver_{sys,user}_state could also be global singletons, static within 
the two respective implementations.


r~
Alex Bennée Dec. 21, 2022, 4:56 p.m. UTC | #3 
Fabiano Rosas <farosas@suse.de> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> In preparation for moving user/softmmu specific bits from the main
>> gdbstub file we need to separate the connection details to what will
>> eventually become an anonymous pointer.
<snip>
>> +
>> +    /*
>> +     * The following is differs depending on USER/SOFTMMU, we just
>> +     * hid it in the typedef.
>> +     */
>> +    gdbserver_state.connection = g_new(GDBConnection, 1);
>
> Does it need freeing at reset_gdbserver_state()?

Following Richard's suggestion I'll move to having separate static
structures instead to sidestep that problem.
diff mbox series

Patch

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index be88ca0d71..14ce911bf2 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -342,6 +342,20 @@  enum RSState {
     RS_CHKSUM1,
     RS_CHKSUM2,
 };
+
+#ifdef CONFIG_USER_ONLY
+typedef struct {
+    int fd;
+    char *socket_path;
+    int running_state;
+} GDBConnection;
+#else
+typedef struct {
+    CharBackend chr;
+    Chardev *mon_chr;
+} GDBConnection;
+#endif
+
 typedef struct GDBState {
     bool init;       /* have we been initialised? */
     CPUState *c_cpu; /* current CPU for step/continue ops */
@@ -354,14 +368,7 @@  typedef struct GDBState {
     int line_csum; /* checksum at the end of the packet */
     GByteArray *last_packet;
     int signal;
-#ifdef CONFIG_USER_ONLY
-    int fd;
-    char *socket_path;
-    int running_state;
-#else
-    CharBackend chr;
-    Chardev *mon_chr;
-#endif
+    GDBConnection *connection;
     bool multiprocess;
     GDBProcess *processes;
     int process_num;
@@ -392,6 +399,12 @@  static void init_gdbserver_state(void)
     gdbserver_state.supported_sstep_flags = accel_supported_gdbstub_sstep_flags();
     gdbserver_state.sstep_flags = SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
     gdbserver_state.sstep_flags &= gdbserver_state.supported_sstep_flags;
+
+    /*
+     * The following is differs depending on USER/SOFTMMU, we just
+     * hid it in the typedef.
+     */
+    gdbserver_state.connection = g_new(GDBConnection, 1);
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -413,15 +426,17 @@  static int get_char(void)
     int ret;
 
     for(;;) {
-        ret = recv(gdbserver_state.fd, &ch, 1, 0);
+        ret = recv(gdbserver_state.connection->fd, &ch, 1, 0);
         if (ret < 0) {
-            if (errno == ECONNRESET)
-                gdbserver_state.fd = -1;
-            if (errno != EINTR)
+            if (errno == ECONNRESET) {
+                gdbserver_state.connection->fd = -1;
+            }
+            if (errno != EINTR) {
                 return -1;
+            }
         } else if (ret == 0) {
-            close(gdbserver_state.fd);
-            gdbserver_state.fd = -1;
+            close(gdbserver_state.connection->fd);
+            gdbserver_state.connection->fd = -1;
             return -1;
         } else {
             break;
@@ -480,7 +495,7 @@  static inline void gdb_continue(void)
 {
 
 #ifdef CONFIG_USER_ONLY
-    gdbserver_state.running_state = 1;
+    gdbserver_state.connection->running_state = 1;
     trace_gdbstub_op_continue();
 #else
     if (!runstate_needs_reset()) {
@@ -509,7 +524,7 @@  static int gdb_continue_partial(char *newstates)
             cpu_single_step(cpu, gdbserver_state.sstep_flags);
         }
     }
-    gdbserver_state.running_state = 1;
+    gdbserver_state.connection->running_state = 1;
 #else
     int flag = 0;
 
@@ -561,7 +576,7 @@  static void put_buffer(const uint8_t *buf, int len)
     int ret;
 
     while (len > 0) {
-        ret = send(gdbserver_state.fd, buf, len, 0);
+        ret = send(gdbserver_state.connection->fd, buf, len, 0);
         if (ret < 0) {
             if (errno != EINTR)
                 return;
@@ -573,7 +588,7 @@  static void put_buffer(const uint8_t *buf, int len)
 #else
     /* XXX this blocks entire thread. Rewrite to use
      * qemu_chr_fe_write and background I/O callbacks */
-    qemu_chr_fe_write_all(&gdbserver_state.chr, buf, len);
+    qemu_chr_fe_write_all(&gdbserver_state.connection->chr, buf, len);
 #endif
 }
 
@@ -2095,7 +2110,8 @@  static void handle_query_rcmd(GArray *params, void *user_ctx)
     len = len / 2;
     hextomem(gdbserver_state.mem_buf, get_param(params, 0)->data, len);
     g_byte_array_append(gdbserver_state.mem_buf, &zero, 1);
-    qemu_chr_be_write(gdbserver_state.mon_chr, gdbserver_state.mem_buf->data,
+    qemu_chr_be_write(gdbserver_state.connection->mon_chr,
+                      gdbserver_state.mem_buf->data,
                       gdbserver_state.mem_buf->len);
     put_packet("OK");
 }
@@ -3028,10 +3044,10 @@  void gdb_exit(int code)
       return;
   }
 #ifdef CONFIG_USER_ONLY
-  if (gdbserver_state.socket_path) {
-      unlink(gdbserver_state.socket_path);
+  if (gdbserver_state.connection->socket_path) {
+      unlink(gdbserver_state.connection->socket_path);
   }
-  if (gdbserver_state.fd < 0) {
+  if (gdbserver_state.connection->fd < 0) {
       return;
   }
 #endif
@@ -3042,7 +3058,7 @@  void gdb_exit(int code)
   put_packet(buf);
 
 #ifndef CONFIG_USER_ONLY
-  qemu_chr_fe_deinit(&gdbserver_state.chr, true);
+  qemu_chr_fe_deinit(&gdbserver_state.connection->chr, true);
 #endif
 }
 
@@ -3078,7 +3094,7 @@  gdb_handlesig(CPUState *cpu, int sig)
     char buf[256];
     int n;
 
-    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
+    if (!gdbserver_state.init || gdbserver_state.connection->fd < 0) {
         return sig;
     }
 
@@ -3096,15 +3112,15 @@  gdb_handlesig(CPUState *cpu, int sig)
     }
     /* put_packet() might have detected that the peer terminated the
        connection.  */
-    if (gdbserver_state.fd < 0) {
+    if (gdbserver_state.connection->fd < 0) {
         return sig;
     }
 
     sig = 0;
     gdbserver_state.state = RS_IDLE;
-    gdbserver_state.running_state = 0;
-    while (gdbserver_state.running_state == 0) {
-        n = read(gdbserver_state.fd, buf, 256);
+    gdbserver_state.connection->running_state = 0;
+    while (gdbserver_state.connection->running_state == 0) {
+        n = read(gdbserver_state.connection->fd, buf, 256);
         if (n > 0) {
             int i;
 
@@ -3115,9 +3131,9 @@  gdb_handlesig(CPUState *cpu, int sig)
             /* XXX: Connection closed.  Should probably wait for another
                connection before continuing.  */
             if (n == 0) {
-                close(gdbserver_state.fd);
+                close(gdbserver_state.connection->fd);
             }
-            gdbserver_state.fd = -1;
+            gdbserver_state.connection->fd = -1;
             return sig;
         }
     }
@@ -3131,7 +3147,7 @@  void gdb_signalled(CPUArchState *env, int sig)
 {
     char buf[4];
 
-    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
+    if (!gdbserver_state.init || gdbserver_state.connection->fd < 0) {
         return;
     }
 
@@ -3146,7 +3162,7 @@  static void gdb_accept_init(int fd)
     gdbserver_state.processes[0].attached = true;
     gdbserver_state.c_cpu = gdb_first_attached_cpu();
     gdbserver_state.g_cpu = gdbserver_state.c_cpu;
-    gdbserver_state.fd = fd;
+    gdbserver_state.connection->fd = fd;
     gdb_has_xml = false;
 }
 
@@ -3278,7 +3294,7 @@  int gdbserver_start(const char *port_or_path)
     if (port > 0 && gdb_accept_tcp(gdb_fd)) {
         return 0;
     } else if (gdb_accept_socket(gdb_fd)) {
-        gdbserver_state.socket_path = g_strdup(port_or_path);
+        gdbserver_state.connection->socket_path = g_strdup(port_or_path);
         return 0;
     }
 
@@ -3290,11 +3306,11 @@  int gdbserver_start(const char *port_or_path)
 /* Disable gdb stub for child processes.  */
 void gdbserver_fork(CPUState *cpu)
 {
-    if (!gdbserver_state.init || gdbserver_state.fd < 0) {
+    if (!gdbserver_state.init || gdbserver_state.connection->fd < 0) {
         return;
     }
-    close(gdbserver_state.fd);
-    gdbserver_state.fd = -1;
+    close(gdbserver_state.connection->fd);
+    gdbserver_state.connection->fd = -1;
     cpu_breakpoint_remove_all(cpu, BP_GDB);
     cpu_watchpoint_remove_all(cpu, BP_GDB);
 }
@@ -3488,21 +3504,22 @@  int gdbserver_start(const char *device)
                                    NULL, NULL, &error_abort);
         monitor_init_hmp(mon_chr, false, &error_abort);
     } else {
-        qemu_chr_fe_deinit(&gdbserver_state.chr, true);
-        mon_chr = gdbserver_state.mon_chr;
+        qemu_chr_fe_deinit(&gdbserver_state.connection->chr, true);
+        mon_chr = gdbserver_state.connection->mon_chr;
         reset_gdbserver_state();
     }
 
     create_processes(&gdbserver_state);
 
     if (chr) {
-        qemu_chr_fe_init(&gdbserver_state.chr, chr, &error_abort);
-        qemu_chr_fe_set_handlers(&gdbserver_state.chr, gdb_chr_can_receive,
+        qemu_chr_fe_init(&gdbserver_state.connection->chr, chr, &error_abort);
+        qemu_chr_fe_set_handlers(&gdbserver_state.connection->chr,
+                                 gdb_chr_can_receive,
                                  gdb_chr_receive, gdb_chr_event,
                                  NULL, &gdbserver_state, NULL, true);
     }
     gdbserver_state.state = chr ? RS_IDLE : RS_INACTIVE;
-    gdbserver_state.mon_chr = mon_chr;
+    gdbserver_state.connection->mon_chr = mon_chr;
     gdbserver_state.current_syscall_cb = NULL;
 
     return 0;