Thanks, applied.
On Thu, Mar 15, 2012 at 17:49, Peter Maydell <peter.mayd...@linaro.org> wrote:
> From: Meador Inge <mead...@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 <mead...@codesourcery.com>
> [PMM: trivial rebase, reinstated comma after last item in RSState enum]
> Signed-off-by: Peter Maydell <peter.mayd...@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
>
>
