| Message ID | 1331833785-9377-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
Am 15.03.2012 18:49, schrieb Peter Maydell: > From: Meador Inge <meadori@codesourcery.com> > > Fix an issue where the GDB server implementation was sending GDB syscall > requests while the system CPU was still running. Syscall requests must > be sent while the CPU is stopped otherwise replies from the GDB client > might get dropped and the GDB server might be incorrectly transitioned > into a 'RUN_STATE_PAUSED' state. > > Signed-off-by: Meador Inge <meadori@codesourcery.com> > [PMM: trivial rebase, reinstated comma after last item in RSState enum] > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Andreas Färber <afaerber@suse.de> > --- > This patch got (trivially) busted by Andreas' commits changing > CPUState to CPUArchState so I've rebased and resent it. I've also > made the trivial style nit fix of not deleting the final comma in > the RSState enum, based on conversation with Andreas in IRC. Thanks, Peter! Andreas > This patch has sitting on the list for about a month reviewed but > unapplied (http://patchwork.ozlabs.org/patch/141867/) -- can > somebody with commit rights apply it please? > > gdbstub.c | 42 +++++++++++++++++++++++++++--------------- > 1 files changed, 27 insertions(+), 15 deletions(-)
Ping^3... -- PMM On 15 March 2012 17:49, Peter Maydell <peter.maydell@linaro.org> wrote: > From: Meador Inge <meadori@codesourcery.com> > > Fix an issue where the GDB server implementation was sending GDB syscall > requests while the system CPU was still running. Syscall requests must > be sent while the CPU is stopped otherwise replies from the GDB client > might get dropped and the GDB server might be incorrectly transitioned > into a 'RUN_STATE_PAUSED' state. > > Signed-off-by: Meador Inge <meadori@codesourcery.com> > [PMM: trivial rebase, reinstated comma after last item in RSState enum] > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This patch got (trivially) busted by Andreas' commits changing > CPUState to CPUArchState so I've rebased and resent it. I've also > made the trivial style nit fix of not deleting the final comma in > the RSState enum, based on conversation with Andreas in IRC. > > This patch has sitting on the list for about a month reviewed but > unapplied (http://patchwork.ozlabs.org/patch/141867/) -- can > somebody with commit rights apply it please? > > gdbstub.c | 42 +++++++++++++++++++++++++++--------------- > 1 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index f4e97f7..6a7e2c4 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -284,7 +284,6 @@ enum RSState { > RS_GETLINE, > RS_CHKSUM1, > RS_CHKSUM2, > - RS_SYSCALL, > }; > typedef struct GDBState { > CPUArchState *c_cpu; /* current CPU for step/continue ops */ > @@ -304,6 +303,8 @@ typedef struct GDBState { > CharDriverState *chr; > CharDriverState *mon_chr; > #endif > + char syscall_buf[256]; > + gdb_syscall_complete_cb current_syscall_cb; > } GDBState; > > /* By default use no IRQs and no timers while single stepping so as to > @@ -346,8 +347,6 @@ static int get_char(GDBState *s) > } > #endif > > -static gdb_syscall_complete_cb gdb_current_syscall_cb; > - > static enum { > GDB_SYS_UNKNOWN, > GDB_SYS_ENABLED, > @@ -2097,8 +2096,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > if (*p == ',') > p++; > type = *p; > - if (gdb_current_syscall_cb) > - gdb_current_syscall_cb(s->c_cpu, ret, err); > + if (s->current_syscall_cb) { > + s->current_syscall_cb(s->c_cpu, ret, err); > + s->current_syscall_cb = NULL; > + } > if (type == 'C') { > put_packet(s, "T02"); > } else { > @@ -2398,7 +2399,12 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state) > const char *type; > int ret; > > - if (running || s->state == RS_INACTIVE || s->state == RS_SYSCALL) { > + if (running || s->state == RS_INACTIVE) { > + return; > + } > + /* Is there a GDB syscall waiting to be sent? */ > + if (s->current_syscall_cb) { > + put_packet(s, s->syscall_buf); > return; > } > switch (state) { > @@ -2468,8 +2474,8 @@ send_packet: > void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) > { > va_list va; > - char buf[256]; > char *p; > + char *p_end; > target_ulong addr; > uint64_t i64; > GDBState *s; > @@ -2477,14 +2483,13 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) > s = gdbserver_state; > if (!s) > return; > - gdb_current_syscall_cb = cb; > - s->state = RS_SYSCALL; > + s->current_syscall_cb = cb; > #ifndef CONFIG_USER_ONLY > vm_stop(RUN_STATE_DEBUG); > #endif > - s->state = RS_IDLE; > va_start(va, fmt); > - p = buf; > + p = s->syscall_buf; > + p_end = &s->syscall_buf[sizeof(s->syscall_buf)]; > *(p++) = 'F'; > while (*fmt) { > if (*fmt == '%') { > @@ -2492,17 +2497,17 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) > switch (*fmt++) { > case 'x': > addr = va_arg(va, target_ulong); > - p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx, addr); > + p += snprintf(p, p_end - p, TARGET_FMT_lx, addr); > break; > case 'l': > if (*(fmt++) != 'x') > goto bad_format; > i64 = va_arg(va, uint64_t); > - p += snprintf(p, &buf[sizeof(buf)] - p, "%" PRIx64, i64); > + p += snprintf(p, p_end - p, "%" PRIx64, i64); > break; > case 's': > addr = va_arg(va, target_ulong); > - p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx "/%x", > + p += snprintf(p, p_end - p, TARGET_FMT_lx "/%x", > addr, va_arg(va, int)); > break; > default: > @@ -2517,10 +2522,16 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) > } > *p = 0; > va_end(va); > - put_packet(s, buf); > #ifdef CONFIG_USER_ONLY > + put_packet(s, s->syscall_buf); > gdb_handlesig(s->c_cpu, 0); > #else > + /* In this case wait to send the syscall packet until notification that > + the CPU has stopped. This must be done because if the packet is sent > + now the reply from the syscall request could be received while the CPU > + is still in the running state, which can cause packets to be dropped > + and state transition 'T' packets to be sent while the syscall is still > + being processed. */ > cpu_exit(s->c_cpu); > #endif > } > @@ -2919,6 +2930,7 @@ int gdbserver_start(const char *device) > s->chr = chr; > s->state = chr ? RS_IDLE : RS_INACTIVE; > s->mon_chr = mon_chr; > + s->current_syscall_cb = NULL; > > return 0; > } > -- > 1.7.1
Thanks, applied. On Thu, Mar 15, 2012 at 17:49, Peter Maydell <peter.maydell@linaro.org> wrote: > From: Meador Inge <meadori@codesourcery.com> > > Fix an issue where the GDB server implementation was sending GDB syscall > requests while the system CPU was still running. Syscall requests must > be sent while the CPU is stopped otherwise replies from the GDB client > might get dropped and the GDB server might be incorrectly transitioned > into a 'RUN_STATE_PAUSED' state. > > Signed-off-by: Meador Inge <meadori@codesourcery.com> > [PMM: trivial rebase, reinstated comma after last item in RSState enum] > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This patch got (trivially) busted by Andreas' commits changing > CPUState to CPUArchState so I've rebased and resent it. I've also > made the trivial style nit fix of not deleting the final comma in > the RSState enum, based on conversation with Andreas in IRC. > > This patch has sitting on the list for about a month reviewed but > unapplied (http://patchwork.ozlabs.org/patch/141867/) -- can > somebody with commit rights apply it please? > > gdbstub.c | 42 +++++++++++++++++++++++++++--------------- > 1 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index f4e97f7..6a7e2c4 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -284,7 +284,6 @@ enum RSState { > RS_GETLINE, > RS_CHKSUM1, > RS_CHKSUM2, > - RS_SYSCALL, > }; > typedef struct GDBState { > CPUArchState *c_cpu; /* current CPU for step/continue ops */ > @@ -304,6 +303,8 @@ typedef struct GDBState { > CharDriverState *chr; > CharDriverState *mon_chr; > #endif > + char syscall_buf[256]; > + gdb_syscall_complete_cb current_syscall_cb; > } GDBState; > > /* By default use no IRQs and no timers while single stepping so as to > @@ -346,8 +347,6 @@ static int get_char(GDBState *s) > } > #endif > > -static gdb_syscall_complete_cb gdb_current_syscall_cb; > - > static enum { > GDB_SYS_UNKNOWN, > GDB_SYS_ENABLED, > @@ -2097,8 +2096,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > if (*p == ',') > p++; > type = *p; > - if (gdb_current_syscall_cb) > - gdb_current_syscall_cb(s->c_cpu, ret, err); > + if (s->current_syscall_cb) { > + s->current_syscall_cb(s->c_cpu, ret, err); > + s->current_syscall_cb = NULL; > + } > if (type == 'C') { > put_packet(s, "T02"); > } else { > @@ -2398,7 +2399,12 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state) > const char *type; > int ret; > > - if (running || s->state == RS_INACTIVE || s->state == RS_SYSCALL) { > + if (running || s->state == RS_INACTIVE) { > + return; > + } > + /* Is there a GDB syscall waiting to be sent? */ > + if (s->current_syscall_cb) { > + put_packet(s, s->syscall_buf); > return; > } > switch (state) { > @@ -2468,8 +2474,8 @@ send_packet: > void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) > { > va_list va; > - char buf[256]; > char *p; > + char *p_end; > target_ulong addr; > uint64_t i64; > GDBState *s; > @@ -2477,14 +2483,13 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) > s = gdbserver_state; > if (!s) > return; > - gdb_current_syscall_cb = cb; > - s->state = RS_SYSCALL; > + s->current_syscall_cb = cb; > #ifndef CONFIG_USER_ONLY > vm_stop(RUN_STATE_DEBUG); > #endif > - s->state = RS_IDLE; > va_start(va, fmt); > - p = buf; > + p = s->syscall_buf; > + p_end = &s->syscall_buf[sizeof(s->syscall_buf)]; > *(p++) = 'F'; > while (*fmt) { > if (*fmt == '%') { > @@ -2492,17 +2497,17 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) > switch (*fmt++) { > case 'x': > addr = va_arg(va, target_ulong); > - p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx, addr); > + p += snprintf(p, p_end - p, TARGET_FMT_lx, addr); > break; > case 'l': > if (*(fmt++) != 'x') > goto bad_format; > i64 = va_arg(va, uint64_t); > - p += snprintf(p, &buf[sizeof(buf)] - p, "%" PRIx64, i64); > + p += snprintf(p, p_end - p, "%" PRIx64, i64); > break; > case 's': > addr = va_arg(va, target_ulong); > - p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx "/%x", > + p += snprintf(p, p_end - p, TARGET_FMT_lx "/%x", > addr, va_arg(va, int)); > break; > default: > @@ -2517,10 +2522,16 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) > } > *p = 0; > va_end(va); > - put_packet(s, buf); > #ifdef CONFIG_USER_ONLY > + put_packet(s, s->syscall_buf); > gdb_handlesig(s->c_cpu, 0); > #else > + /* In this case wait to send the syscall packet until notification that > + the CPU has stopped. This must be done because if the packet is sent > + now the reply from the syscall request could be received while the CPU > + is still in the running state, which can cause packets to be dropped > + and state transition 'T' packets to be sent while the syscall is still > + being processed. */ > cpu_exit(s->c_cpu); > #endif > } > @@ -2919,6 +2930,7 @@ int gdbserver_start(const char *device) > s->chr = chr; > s->state = chr ? RS_IDLE : RS_INACTIVE; > s->mon_chr = mon_chr; > + s->current_syscall_cb = NULL; > > return 0; > } > -- > 1.7.1 > >
diff --git a/gdbstub.c b/gdbstub.c index f4e97f7..6a7e2c4 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -284,7 +284,6 @@ enum RSState { RS_GETLINE, RS_CHKSUM1, RS_CHKSUM2, - RS_SYSCALL, }; typedef struct GDBState { CPUArchState *c_cpu; /* current CPU for step/continue ops */ @@ -304,6 +303,8 @@ typedef struct GDBState { CharDriverState *chr; CharDriverState *mon_chr; #endif + char syscall_buf[256]; + gdb_syscall_complete_cb current_syscall_cb; } GDBState; /* By default use no IRQs and no timers while single stepping so as to @@ -346,8 +347,6 @@ static int get_char(GDBState *s) } #endif -static gdb_syscall_complete_cb gdb_current_syscall_cb; - static enum { GDB_SYS_UNKNOWN, GDB_SYS_ENABLED, @@ -2097,8 +2096,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) if (*p == ',') p++; type = *p; - if (gdb_current_syscall_cb) - gdb_current_syscall_cb(s->c_cpu, ret, err); + if (s->current_syscall_cb) { + s->current_syscall_cb(s->c_cpu, ret, err); + s->current_syscall_cb = NULL; + } if (type == 'C') { put_packet(s, "T02"); } else { @@ -2398,7 +2399,12 @@ static void gdb_vm_state_change(void *opaque, int running, RunState state) const char *type; int ret; - if (running || s->state == RS_INACTIVE || s->state == RS_SYSCALL) { + if (running || s->state == RS_INACTIVE) { + return; + } + /* Is there a GDB syscall waiting to be sent? */ + if (s->current_syscall_cb) { + put_packet(s, s->syscall_buf); return; } switch (state) { @@ -2468,8 +2474,8 @@ send_packet: void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) { va_list va; - char buf[256]; char *p; + char *p_end; target_ulong addr; uint64_t i64; GDBState *s; @@ -2477,14 +2483,13 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) s = gdbserver_state; if (!s) return; - gdb_current_syscall_cb = cb; - s->state = RS_SYSCALL; + s->current_syscall_cb = cb; #ifndef CONFIG_USER_ONLY vm_stop(RUN_STATE_DEBUG); #endif - s->state = RS_IDLE; va_start(va, fmt); - p = buf; + p = s->syscall_buf; + p_end = &s->syscall_buf[sizeof(s->syscall_buf)]; *(p++) = 'F'; while (*fmt) { if (*fmt == '%') { @@ -2492,17 +2497,17 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) switch (*fmt++) { case 'x': addr = va_arg(va, target_ulong); - p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx, addr); + p += snprintf(p, p_end - p, TARGET_FMT_lx, addr); break; case 'l': if (*(fmt++) != 'x') goto bad_format; i64 = va_arg(va, uint64_t); - p += snprintf(p, &buf[sizeof(buf)] - p, "%" PRIx64, i64); + p += snprintf(p, p_end - p, "%" PRIx64, i64); break; case 's': addr = va_arg(va, target_ulong); - p += snprintf(p, &buf[sizeof(buf)] - p, TARGET_FMT_lx "/%x", + p += snprintf(p, p_end - p, TARGET_FMT_lx "/%x", addr, va_arg(va, int)); break; default: @@ -2517,10 +2522,16 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) } *p = 0; va_end(va); - put_packet(s, buf); #ifdef CONFIG_USER_ONLY + put_packet(s, s->syscall_buf); gdb_handlesig(s->c_cpu, 0); #else + /* In this case wait to send the syscall packet until notification that + the CPU has stopped. This must be done because if the packet is sent + now the reply from the syscall request could be received while the CPU + is still in the running state, which can cause packets to be dropped + and state transition 'T' packets to be sent while the syscall is still + being processed. */ cpu_exit(s->c_cpu); #endif } @@ -2919,6 +2930,7 @@ int gdbserver_start(const char *device) s->chr = chr; s->state = chr ? RS_IDLE : RS_INACTIVE; s->mon_chr = mon_chr; + s->current_syscall_cb = NULL; return 0; }