Re: [Qemu-devel] [PATCH] gdbstub improvements for gdb kill/detach/quit

[Edgar E. Iglesias]

On Fri, May 09, 2008 at 08:51:38AM -0500, Jason Wessel wrote:
> Attached is a patch to improve the gdb stub 'D' 'k' and '?' packets. 
> See the patch header for details.
> 
> Jason.

> From: Jason Wessel <address@hidden>
> Subject: [PATCH] support for gdb "detach/kill/quit"
> 
> Implement the 'k' gdbserial packet which kills the qemu instance via
> the debugger stub.
> 
> Implement the 'D' detach packet for the gdb stub such that you can
> disconnect gdb with the "detach" command.  This required implementing
> a cpu_breakpoint_remove_all function to cleanup all the breakpoints
> prior to leaving the gdb stub else simulation can stop with no
> debugger attached.
> 
> On a '?' packet remove all the breakpoints.  This is considered more
> of a safety net in case you force killed gdb or it crashed and you are
> reconnecting.  The identical behavior exists for kgdb in the linux
> kernel.
> 
> Signed-off-by: Jason Wessel <address@hidden>

Thanks, a few comments inlined.

> ---
>  cpu-all.h |    1 +
>  exec.c    |   14 ++++++++++++++
>  gdbstub.c |   16 ++++++++++++++++
>  3 files changed, 31 insertions(+)
> 
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -759,6 +759,7 @@ void cpu_interrupt(CPUState *s, int mask
>  void cpu_reset_interrupt(CPUState *env, int mask);
>  
>  int cpu_watchpoint_insert(CPUState *env, target_ulong addr);
> +int cpu_breakpoint_remove_all(CPUState *env);
>  int cpu_watchpoint_remove(CPUState *env, target_ulong addr);
>  int cpu_breakpoint_insert(CPUState *env, target_ulong pc);
>  int cpu_breakpoint_remove(CPUState *env, target_ulong pc);
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -901,6 +901,11 @@ static int gdb_handle_packet(GDBState *s
>          /* TODO: Make this return the correct value for user-mode.  */
>          snprintf(buf, sizeof(buf), "S%02x", SIGTRAP);
>          put_packet(s, buf);
> +        /* Remove all the breakpoints when this query is issued,
> +         * because gdb is doing and initial connect and the state
> +         * should be cleaned up. 
> +         */
> +        cpu_breakpoint_remove_all(env);
>          break;
>      case 'c':
>          if (*p != '\0') {
> @@ -924,6 +929,17 @@ static int gdb_handle_packet(GDBState *s
>          }
>          gdb_continue(s);
>       return RS_IDLE;
> +    case 'k':
> +        /* Kill the target */
> +        fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");
> +        exit(0);



Should we consider qemu_system_shutdown_request() here?


> +    case 'D':
> +        /* Detach packet */
> +        if (!cpu_breakpoint_remove_all(env)) {
> +            gdb_continue(s);
> +            put_packet(s, "OK");
> +            break;
> +        }
>      case 's':
>          if (*p != '\0') {
>              addr = strtoull(p, (char **)&p, 16);
> --- a/exec.c
> +++ b/exec.c
> @@ -1150,6 +1150,20 @@ int cpu_breakpoint_insert(CPUState *env,
>  #endif
>  }
>  
> +/* remove all breakpoints */
> +int cpu_breakpoint_remove_all(CPUState *env) {
> +#if defined(TARGET_HAS_ICE)
> +    int i;
> +    for(i = 0; i < env->nb_breakpoints; i++) {
> +        breakpoint_invalidate(env, env->breakpoints[i]);
> +    }
> +    env->nb_breakpoints = 0;
> +    return 0;
> +#else
> +    return -1;
> +#endif
> +}

Why not just drop the return value and make the 'D' command always succeed?

Best regards
-- 
Edgar E. Iglesias
Axis Communications AB