gdbstub improvements for gdb kill/detach/quit
PATCH 1/1 gdbstub improvements for gdb kill/detach/quit
Attached is a patch to improve the gdb stub 'D' 'k' and '?' packets. See the patch header for details.
Jason.
From: Jason Wessel <[email protected]>
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.
--- cpu-all.h | 1 + exec.c | 14 ++++++++++++++ gdbstub.c | 16 ++++++++++++++++ 3 files changed, 31 insertions(+)
cpu-all.h +1 -0
@@ -759,6 +759,7 @@ void cpu_interrupt(CPUState *s, int maskvoid 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);
gdbstub.c +16 -0
@@ -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 ); +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 );
exec.c +14 -0
@@ -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 +} +/* remove a breakpoint */ int cpu_breakpoint_remove (CPUState *env,target_ulong pc) {
Quoted 17 lines · Jason Wessel
Attached is a patch to improve the gdb stub 'D' 'k' and '?' packets. See the patch header for details.
Jason.
From: Jason Wessel <[email protected]>
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_allfunction to cleanup all the breakpoints prior to leaving the gdb stub else simulation can stop with no debugger attached.
Quoted 6 lines
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 <[email protected]>
Thanks, a few comments inlined.
Quoted 31 lines
--- cpu-all.h | 1 + exec.c | 14 ++++++++++++++ gdbstub.c | 16 ++++++++++++++++ 3 files changed, 31 insertions(+)cpu-all.h +1
-0@@ -759,6 +759,7 @@ void cpu_interrupt(CPUState *s, int maskvoid 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);gdbstub.c +5
-0@@ -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' ) {
gdbstub.c static int gdb_handle_packet(GDBState *s · line 935
Quoted 7 lines
gdbstub.c +4
-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?
Quoted 19 lines
gdbstub.c +7
-0+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 );exec.c +3
-0@@ -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 )
exec.c int cpu_breakpoint_insert(CPUState *env, · line 1165
Quoted 10 lines
exec.c +10
-0+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
Signature · 2 lines
Edgar E. Iglesias Axis Communications AB
More comments below.
Quoted 23 lines · Edgar E. Iglesias
Quoted 22 lines · Jason Wessel
Attached is a patch to improve the gdb stub 'D' 'k' and '?' packets. See the patch header for details.
gdbstub.c +5
-0@@ -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') {gdbstub.c +0
-0@@ -924,6 +929,17 @@ static int gdb_handle_packet(GDBState *s }gdb_continue (s);return RS_IDLE ;
Quoted 8 lines
gdbstub.c static int gdb_handle_packet(GDBState *s · line 935
Quoted 4 lines
gdbstub.c +4
-0+case 'k' : +/* Kill the target */ +fprintf (stderr,"\nQEMU: Terminated via GDBstub\n" ); +exit (0 );Should we consider
qemu_system_shutdown_request()here?
We desire the same action as if you run "quit" from the monitor. Today the monitor invokes do_quit() which just does exit(). If there is a more graceful way to exit the qemu instance which saves/flushes state, the monitor should probably be changed this way as well.
I figure you can use the monitor commands from the gdbstub or directly in the monitor if you want to do any kind of saving of the state of the machine at this point.
Quoted 18 lines
Quoted 18 lines
gdbstub.c +7
-0+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);exec.c +0
-0@@ -1150,6 +1150,20 @@ int cpu_breakpoint_insert(CPUState *env,#endif }
Quoted 15 lines
exec.c int cpu_breakpoint_insert(CPUState *env, · line 1165
Quoted 13 lines
exec.c +13
-0+/* 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?
This seems reasonable. Previously I had simply used the same call/return semantics as removing a single break point. Also I noticed the watch points have the exact same problem as the breakpoints with respect to detaching the debugger, so I fixed that in the next version of the patch which will be posted shortly.
Jason.