Re: [PATCH v2 06/21] gdbstub: move GDBState to shared internals header

[Alex Bennée]

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