Re: [PATCH v2 02/14] gdbstub: stop passing GDBState * around and use global

[Damien Hedde]

On 11/30/19 9:45 AM, Alex Bennée wrote:
> We only have one GDBState which should be allocated at the time we
> process any commands. This will make further clean-up a bit easier.
> 
> Signed-off-by: Alex Bennée 
> ---
>  gdbstub.c | 539 +++---
>  1 file changed, 267 insertions(+), 272 deletions(-)
> 

> @@ -2919,33 +2914,33 @@ static void gdb_read_byte(GDBState *s, uint8_t ch)> 
> [...]
>  } else {
>  /* send ACK reply */
>  reply = '+';
> -put_buffer(s, , 1);
> -s->state = gdb_handle_packet(s, s->line_buf);
> +put_buffer(, 1);
> +gdbserver_state.state = gdb_handle_packet(s, 
> gdbserver_state.line_buf);

Is there a reason to keep the GDBState* first parameter in
gdb_handle_packet() ?

There is a few remaining functions still taking GDBState* parameter
+ gdb_handle_packet
+ run_cmd_parser
+ gdb_monitor_output
+ create_default_processes
+ create_processes
+ gdb_read_byte

We should probably clean them too, but otherwise it looks good.

--
Damien

Re: [PATCH v2 02/14] gdbstub: stop passing GDBState * around and use global

[Richard Henderson]

On 11/30/19 8:45 AM, Alex Bennée wrote:
> We only have one GDBState which should be allocated at the time we
> process any commands. This will make further clean-up a bit easier.
> 
> Signed-off-by: Alex Bennée 
> ---
>  gdbstub.c | 539 +++---
>  1 file changed, 267 insertions(+), 272 deletions(-)

Reviewed-by: Richard Henderson 

r~

[PATCH v2 02/14] gdbstub: stop passing GDBState * around and use global

[Alex Bennée]

We only have one GDBState which should be allocated at the time we
process any commands. This will make further clean-up a bit easier.

Signed-off-by: Alex Bennée 
---
 gdbstub.c | 539 +++---
 1 file changed, 267 insertions(+), 272 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 36b1d7a9408..23f513f709e 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -397,21 +397,21 @@ bool gdb_has_xml;
 /* XXX: This is not thread safe.  Do we care?  */
 static int gdbserver_fd = -1;
 
-static int get_char(GDBState *s)
+static int get_char(void)
 {
 uint8_t ch;
 int ret;
 
 for(;;) {
-ret = qemu_recv(s->fd, , 1, 0);
+ret = qemu_recv(gdbserver_state.fd, , 1, 0);
 if (ret < 0) {
 if (errno == ECONNRESET)
-s->fd = -1;
+gdbserver_state.fd = -1;
 if (errno != EINTR)
 return -1;
 } else if (ret == 0) {
-close(s->fd);
-s->fd = -1;
+close(gdbserver_state.fd);
+gdbserver_state.fd = -1;
 return -1;
 } else {
 break;
@@ -449,11 +449,11 @@ int use_gdb_syscalls(void)
 }
 
 /* Resume execution.  */
-static inline void gdb_continue(GDBState *s)
+static inline void gdb_continue(void)
 {
 
 #ifdef CONFIG_USER_ONLY
-s->running_state = 1;
+gdbserver_state.running_state = 1;
 trace_gdbstub_op_continue();
 #else
 if (!runstate_needs_reset()) {
@@ -467,7 +467,7 @@ static inline void gdb_continue(GDBState *s)
  * Resume execution, per CPU actions. For user-mode emulation it's
  * equivalent to gdb_continue.
  */
-static int gdb_continue_partial(GDBState *s, char *newstates)
+static int gdb_continue_partial(char *newstates)
 {
 CPUState *cpu;
 int res = 0;
@@ -482,7 +482,7 @@ static int gdb_continue_partial(GDBState *s, char 
*newstates)
 cpu_single_step(cpu, sstep_flags);
 }
 }
-s->running_state = 1;
+gdbserver_state.running_state = 1;
 #else
 int flag = 0;
 
@@ -520,13 +520,13 @@ static int gdb_continue_partial(GDBState *s, char 
*newstates)
 return res;
 }
 
-static void put_buffer(GDBState *s, const uint8_t *buf, int len)
+static void put_buffer(const uint8_t *buf, int len)
 {
 #ifdef CONFIG_USER_ONLY
 int ret;
 
 while (len > 0) {
-ret = send(s->fd, buf, len, 0);
+ret = send(gdbserver_state.fd, buf, len, 0);
 if (ret < 0) {
 if (errno != EINTR)
 return;
@@ -538,7 +538,7 @@ static void put_buffer(GDBState *s, 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(>chr, buf, len);
+qemu_chr_fe_write_all(_state.chr, buf, len);
 #endif
 }
 
@@ -620,17 +620,18 @@ static void hexdump(const char *buf, int len,
 }
 
 /* return -1 if error, 0 if OK */
-static int put_packet_binary(GDBState *s, const char *buf, int len, bool dump)
+static int put_packet_binary(const char *buf, int len, bool dump)
 {
 int csum, i;
 uint8_t *p;
+uint8_t *ps = _state.last_packet[0];
 
 if (dump && trace_event_get_state_backends(TRACE_GDBSTUB_IO_BINARYREPLY)) {
 hexdump(buf, len, trace_gdbstub_io_binaryreply);
 }
 
 for(;;) {
-p = s->last_packet;
+p = ps;
 *(p++) = '$';
 memcpy(p, buf, len);
 p += len;
@@ -642,11 +643,11 @@ static int put_packet_binary(GDBState *s, const char 
*buf, int len, bool dump)
 *(p++) = tohex((csum >> 4) & 0xf);
 *(p++) = tohex((csum) & 0xf);
 
-s->last_packet_len = p - s->last_packet;
-put_buffer(s, (uint8_t *)s->last_packet, s->last_packet_len);
+gdbserver_state.last_packet_len = p - ps;
+put_buffer(ps, gdbserver_state.last_packet_len);
 
 #ifdef CONFIG_USER_ONLY
-i = get_char(s);
+i = get_char();
 if (i < 0)
 return -1;
 if (i == '+')
@@ -659,11 +660,11 @@ static int put_packet_binary(GDBState *s, const char 
*buf, int len, bool dump)
 }
 
 /* return -1 if error, 0 if OK */
-static int put_packet(GDBState *s, const char *buf)
+static int put_packet(const char *buf)
 {
 trace_gdbstub_io_reply(buf);
 
-return put_packet_binary(s, buf, strlen(buf), false);
+return put_packet_binary(buf, strlen(buf), false);
 }
 
 /* Encode data using the encoding for 'x' packets.  */
@@ -687,37 +688,38 @@ static int memtox(char *buf, const char *mem, int len)
 return p - buf;
 }
 
-static uint32_t gdb_get_cpu_pid(const GDBState *s, CPUState *cpu)
+static uint32_t gdb_get_cpu_pid(CPUState *cpu)
 {
 /* TODO: In user mode, we should use the task state PID */
 if (cpu->cluster_index == UNASSIGNED_CLUSTER_INDEX) {
 /* Return the default process' PID */
-return s->processes[s->process_num - 1].pid;
+int index = gdbserver_state.process_num - 1;
+return