We are about to split softmmu and user mode helpers into different
files. To facilitate this we will need to share access to the GDBState
between those files.

To keep building we have to temporarily define CONFIG_USER_ONLY just
before we include internals.h for the user-mode side of things. This
will get removed once the state is fully moved.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 gdbstub/internals.h | 69 +++++++++++++++++++++++++++++++++++++++++++++
 gdbstub/gdbstub.c   | 60 ---------------------------------------
 gdbstub/softmmu.c   |  2 ++
 gdbstub/user.c      |  2 ++
 4 files changed, 73 insertions(+), 60 deletions(-)

diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index b444f24ef5..9784db2dc5 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -9,6 +9,75 @@
 #ifndef GDBSTUB_INTERNALS_H
 #define GDBSTUB_INTERNALS_H
 
+#define MAX_PACKET_LENGTH 4096
+
+/*
+ * Shared structures and definitions
+ */
+
+typedef struct GDBProcess {
+    uint32_t pid;
+    bool attached;
+
+    char target_xml[1024];
+} GDBProcess;
+
+enum RSState {
+    RS_INACTIVE,
+    RS_IDLE,
+    RS_GETLINE,
+    RS_GETLINE_ESC,
+    RS_GETLINE_RLE,
+    RS_CHKSUM1,
+    RS_CHKSUM2,
+};
+
+/* Temporary home */
+#ifdef CONFIG_USER_ONLY
+typedef struct {
+    int fd;
+    char *socket_path;
+    int running_state;
+} GDBUserState;
+#else
+typedef struct {
+    CharBackend chr;
+    Chardev *mon_chr;
+} GDBSystemState;
+#endif
+
+typedef struct GDBState {
+    bool init;       /* have we been initialised? */
+    CPUState *c_cpu; /* current CPU for step/continue ops */
+    CPUState *g_cpu; /* current CPU for other ops */
+    CPUState *query_cpu; /* for q{f|s}ThreadInfo */
+    enum RSState state; /* parsing state */
+    char line_buf[MAX_PACKET_LENGTH];
+    int line_buf_index;
+    int line_sum; /* running checksum */
+    int line_csum; /* checksum at the end of the packet */
+    GByteArray *last_packet;
+    int signal;
+#ifdef CONFIG_USER_ONLY
+    GDBUserState user;
+#else
+    GDBSystemState system;
+#endif
+    bool multiprocess;
+    GDBProcess *processes;
+    int process_num;
+    char syscall_buf[256];
+    gdb_syscall_complete_cb current_syscall_cb;
+    GString *str_buf;
+    GByteArray *mem_buf;
+    int sstep_flags;
+    int supported_sstep_flags;
+} GDBState;
+
+/*
+ * Break/Watch point support - there is an implementation for softmmu
+ * and user mode.
+ */
 bool gdb_supports_guest_debug(void);
 int gdb_breakpoint_insert(CPUState *cs, int type, hwaddr addr, hwaddr len);
 int gdb_breakpoint_remove(CPUState *cs, int type, hwaddr addr, hwaddr len);
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 42ae13b344..505beafad7 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -41,8 +41,6 @@
 #include "hw/boards.h"
 #endif
 
-#define MAX_PACKET_LENGTH 4096
-
 #include "qemu/sockets.h"
 #include "sysemu/hw_accel.h"
 #include "sysemu/runstate.h"
@@ -326,64 +324,6 @@ typedef struct GDBRegisterState {
     struct GDBRegisterState *next;
 } GDBRegisterState;
 
-typedef struct GDBProcess {
-    uint32_t pid;
-    bool attached;
-
-    char target_xml[1024];
-} GDBProcess;
-
-enum RSState {
-    RS_INACTIVE,
-    RS_IDLE,
-    RS_GETLINE,
-    RS_GETLINE_ESC,
-    RS_GETLINE_RLE,
-    RS_CHKSUM1,
-    RS_CHKSUM2,
-};
-
-#ifdef CONFIG_USER_ONLY
-typedef struct {
-    int fd;
-    char *socket_path;
-    int running_state;
-} GDBUserState;
-#else
-typedef struct {
-    CharBackend chr;
-    Chardev *mon_chr;
-} GDBSystemState;
-#endif
-
-typedef struct GDBState {
-    bool init;       /* have we been initialised? */
-    CPUState *c_cpu; /* current CPU for step/continue ops */
-    CPUState *g_cpu; /* current CPU for other ops */
-    CPUState *query_cpu; /* for q{f|s}ThreadInfo */
-    enum RSState state; /* parsing state */
-    char line_buf[MAX_PACKET_LENGTH];
-    int line_buf_index;
-    int line_sum; /* running checksum */
-    int line_csum; /* checksum at the end of the packet */
-    GByteArray *last_packet;
-    int signal;
-#ifdef CONFIG_USER_ONLY
-    GDBUserState user;
-#else
-    GDBSystemState system;
-#endif
-    bool multiprocess;
-    GDBProcess *processes;
-    int process_num;
-    char syscall_buf[256];
-    gdb_syscall_complete_cb current_syscall_cb;
-    GString *str_buf;
-    GByteArray *mem_buf;
-    int sstep_flags;
-    int supported_sstep_flags;
-} GDBState;
-
 static GDBState gdbserver_state;
 
 static void init_gdbserver_state(void)
diff --git a/gdbstub/softmmu.c b/gdbstub/softmmu.c
index 183dfb40e4..696894243b 100644
--- a/gdbstub/softmmu.c
+++ b/gdbstub/softmmu.c
@@ -14,6 +14,8 @@
 #include "exec/gdbstub.h"
 #include "exec/hwaddr.h"
 #include "sysemu/cpus.h"
+#include "chardev/char.h"
+#include "chardev/char-fe.h"
 #include "internals.h"
 
 bool gdb_supports_guest_debug(void)
diff --git a/gdbstub/user.c b/gdbstub/user.c
index a5f370bcf9..4c2b41eefa 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -13,6 +13,8 @@
 #include "exec/hwaddr.h"
 #include "exec/gdbstub.h"
 #include "hw/core/cpu.h"
+/* temp hack */
+#define CONFIG_USER_ONLY 1
 #include "internals.h"
 
 bool gdb_supports_guest_debug(void)
-- 
2.34.1

On 1/5/23 08:43, Alex Bennée wrote:
> We are about to split softmmu and user mode helpers into different
> files. To facilitate this we will need to share access to the GDBState
> between those files.
> 
> To keep building we have to temporarily define CONFIG_USER_ONLY just
> before we include internals.h for the user-mode side of things. This
> will get removed once the state is fully moved.

You don't have to have this hack if you don't ...

> +typedef struct GDBState {
> +    bool init;       /* have we been initialised? */
> +    CPUState *c_cpu; /* current CPU for step/continue ops */
> +    CPUState *g_cpu; /* current CPU for other ops */
> +    CPUState *query_cpu; /* for q{f|s}ThreadInfo */
> +    enum RSState state; /* parsing state */
> +    char line_buf[MAX_PACKET_LENGTH];
> +    int line_buf_index;
> +    int line_sum; /* running checksum */
> +    int line_csum; /* checksum at the end of the packet */
> +    GByteArray *last_packet;
> +    int signal;
> +#ifdef CONFIG_USER_ONLY
> +    GDBUserState user;
> +#else
> +    GDBSystemState system;
> +#endif

... nest these.  What's the point?


r~

Richard Henderson <richard.henderson@linaro.org> writes:

> On 1/5/23 08:43, Alex Bennée wrote:
>> We are about to split softmmu and user mode helpers into different
>> files. To facilitate this we will need to share access to the GDBState
>> between those files.
>> To keep building we have to temporarily define CONFIG_USER_ONLY just
>> before we include internals.h for the user-mode side of things. This
>> will get removed once the state is fully moved.
>
> You don't have to have this hack if you don't ...
>
>> +typedef struct GDBState {
>> +    bool init;       /* have we been initialised? */
>> +    CPUState *c_cpu; /* current CPU for step/continue ops */
>> +    CPUState *g_cpu; /* current CPU for other ops */
>> +    CPUState *query_cpu; /* for q{f|s}ThreadInfo */
>> +    enum RSState state; /* parsing state */
>> +    char line_buf[MAX_PACKET_LENGTH];
>> +    int line_buf_index;
>> +    int line_sum; /* running checksum */
>> +    int line_csum; /* checksum at the end of the packet */
>> +    GByteArray *last_packet;
>> +    int signal;
>> +#ifdef CONFIG_USER_ONLY
>> +    GDBUserState user;
>> +#else
>> +    GDBSystemState system;
>> +#endif
>
> ... nest these.  What's the point?

Well you end up having to ensure the chardev definitions are then
available in all files that include internals.h and I'm not sure that is
better:

--8<---------------cut here---------------start------------->8---
modified   gdbstub/internals.h
@@ -33,18 +33,16 @@ enum RSState {
 };
 
 /* Temporary home */
-#ifdef CONFIG_USER_ONLY
 typedef struct {
     int fd;
     char *socket_path;
     int running_state;
 } GDBUserState;
-#else
+
 typedef struct {
     CharBackend chr;
     Chardev *mon_chr;
 } GDBSystemState;
-#endif
 
 typedef struct GDBState {
     bool init;       /* have we been initialised? */
@@ -58,11 +56,8 @@ typedef struct GDBState {
     int line_csum; /* checksum at the end of the packet */
     GByteArray *last_packet;
     int signal;
-#ifdef CONFIG_USER_ONLY
     GDBUserState user;
-#else
     GDBSystemState system;
-#endif
     bool multiprocess;
     GDBProcess *processes;
     int process_num;
modified   gdbstub/gdbstub.c
@@ -48,6 +48,8 @@
 #include "exec/exec-all.h"
 #include "exec/hwaddr.h"
 #include "sysemu/replay.h"
+#include "chardev/char.h"
+#include "chardev/char-fe.h"
 
 #include "internals.h"
 
modified   gdbstub/user.c
@@ -13,8 +13,8 @@
 #include "exec/hwaddr.h"
 #include "exec/gdbstub.h"
 #include "hw/core/cpu.h"
-/* temp hack */
-#define CONFIG_USER_ONLY 1
+#include "chardev/char.h"
+#include "chardev/char-fe.h"
 #include "internals.h"
 
 bool gdb_supports_guest_debug(void)
--8<---------------cut here---------------end--------------->8---


>
>
> r~


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

On 2/21/23 00:24, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 1/5/23 08:43, Alex Bennée wrote:
>>> We are about to split softmmu and user mode helpers into different
>>> files. To facilitate this we will need to share access to the GDBState
>>> between those files.
>>> To keep building we have to temporarily define CONFIG_USER_ONLY just
>>> before we include internals.h for the user-mode side of things. This
>>> will get removed once the state is fully moved.
>>
>> You don't have to have this hack if you don't ...
>>
>>> +typedef struct GDBState {
>>> +    bool init;       /* have we been initialised? */
>>> +    CPUState *c_cpu; /* current CPU for step/continue ops */
>>> +    CPUState *g_cpu; /* current CPU for other ops */
>>> +    CPUState *query_cpu; /* for q{f|s}ThreadInfo */
>>> +    enum RSState state; /* parsing state */
>>> +    char line_buf[MAX_PACKET_LENGTH];
>>> +    int line_buf_index;
>>> +    int line_sum; /* running checksum */
>>> +    int line_csum; /* checksum at the end of the packet */
>>> +    GByteArray *last_packet;
>>> +    int signal;
>>> +#ifdef CONFIG_USER_ONLY
>>> +    GDBUserState user;
>>> +#else
>>> +    GDBSystemState system;
>>> +#endif
>>
>> ... nest these.  What's the point?
> 
> Well you end up having to ensure the chardev definitions are then
> available in all files that include internals.h and I'm not sure that is
> better:
> 
> --8<---------------cut here---------------start------------->8---
> modified   gdbstub/internals.h
> @@ -33,18 +33,16 @@ enum RSState {
>   };
>   
>   /* Temporary home */
> -#ifdef CONFIG_USER_ONLY
>   typedef struct {
>       int fd;
>       char *socket_path;
>       int running_state;
>   } GDBUserState;
> -#else
> +
>   typedef struct {
>       CharBackend chr;
>       Chardev *mon_chr;
>   } GDBSystemState;
> -#endif

No, these typedefs and their data can be completely private to the two implementations.


r~

Richard Henderson <richard.henderson@linaro.org> writes:

> On 2/21/23 00:24, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> On 1/5/23 08:43, Alex Bennée wrote:
>>>> We are about to split softmmu and user mode helpers into different
>>>> files. To facilitate this we will need to share access to the GDBState
>>>> between those files.
>>>> To keep building we have to temporarily define CONFIG_USER_ONLY just
>>>> before we include internals.h for the user-mode side of things. This
>>>> will get removed once the state is fully moved.
>>>
>>> You don't have to have this hack if you don't ...
>>>
>>>> +typedef struct GDBState {
>>>> +    bool init;       /* have we been initialised? */
>>>> +    CPUState *c_cpu; /* current CPU for step/continue ops */
>>>> +    CPUState *g_cpu; /* current CPU for other ops */
>>>> +    CPUState *query_cpu; /* for q{f|s}ThreadInfo */
>>>> +    enum RSState state; /* parsing state */
>>>> +    char line_buf[MAX_PACKET_LENGTH];
>>>> +    int line_buf_index;
>>>> +    int line_sum; /* running checksum */
>>>> +    int line_csum; /* checksum at the end of the packet */
>>>> +    GByteArray *last_packet;
>>>> +    int signal;
>>>> +#ifdef CONFIG_USER_ONLY
>>>> +    GDBUserState user;
>>>> +#else
>>>> +    GDBSystemState system;
>>>> +#endif
>>>
>>> ... nest these.  What's the point?
>> Well you end up having to ensure the chardev definitions are then
>> available in all files that include internals.h and I'm not sure that is
>> better:
>> --8<---------------cut here---------------start------------->8---
>> modified   gdbstub/internals.h
>> @@ -33,18 +33,16 @@ enum RSState {
>>   };
>>     /* Temporary home */
>> -#ifdef CONFIG_USER_ONLY
>>   typedef struct {
>>       int fd;
>>       char *socket_path;
>>       int running_state;
>>   } GDBUserState;
>> -#else
>> +
>>   typedef struct {
>>       CharBackend chr;
>>       Chardev *mon_chr;
>>   } GDBSystemState;
>> -#endif
>
> No, these typedefs and their data can be completely private to the two
> implementations.

Ahh right, so I've split that up now in:

  gdbstub: define separate user/system structures

and its made the subsequent patches much cleaner

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro