From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17347 invoked by alias); 3 Nov 2008 15:45:40 -0000 Received: (qmail 17000 invoked by uid 22791); 3 Nov 2008 15:45:36 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 03 Nov 2008 15:44:59 +0000 Received: (qmail 2506 invoked from network); 3 Nov 2008 15:44:57 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 3 Nov 2008 15:44:57 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: Target can step over breakpoints itself Date: Mon, 03 Nov 2008 15:45:00 -0000 User-Agent: KMail/1.9.9 Cc: Daniel Jacobowitz References: <200810271419.00304.pedro@codesourcery.com> <20081028141737.GB21659@caradoc.them.org> In-Reply-To: <20081028141737.GB21659@caradoc.them.org> MIME-Version: 1.0 Content-Disposition: inline Content-Type: Multipart/Mixed; boundary="Boundary-00=_6xxDJzvp0AfGpZX" Message-Id: <200811031544.58699.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-11/txt/msg00011.txt.bz2 --Boundary-00=_6xxDJzvp0AfGpZX Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 3011 On Tuesday 28 October 2008 14:17:37, Daniel Jacobowitz wrote: > On Mon, Oct 27, 2008 at 02:19:00PM +0000, Pedro Alves wrote: > > The attached patch adds a new "StepOverBreakpoints" feature to the > > remote protocol to the remote stub can tell GDB that the debug api has > > that feature, and exposes that knowledge to the rest of GDB by > > adding a new target_ops method (target_can_step_over_breakpoints), > > and teaching infrun.c to it doesn't need to do the traditional hold-and-step > > dance (remove breakpoints, single-step, insert breakpoints) from the > > inferior(s), if the target reports support for stepping over them > > itself. > > > > Any objections/thoughts on this? > > Does DICOS hit breakpoints at the current PC if you continue, rather > than step? I'd hope so - and if so, let's document that expectation. > It's details like that which confuse people trying to implement the > remote protocol. Yeah, thanks for the hint. It doesn't hit the breakpoint if proceeding at the PC where the thread stopped (an equivalent to stop_pc on the GDB side), but will hit it if telling it to resume at some other PC with a breakpoint there (e.g., jump *PC). Since GDB normally single-steps to move past a breakpoint at stop_pc, this hardly matters for DICOS (other cases where GDB expects to hit a breakpoint at PC happen around unix signals, but DICOS doesn't have a notion of unix signals). What I think we should do, is document that the feature only applies to single-stepping, as attached. > > Does this need to be conditional on the type of breakpoint (hardware > vs software, Z0 vs memory) or on the actual breakpoint? For instance, > platforms with per-thread hardware breakpoints can 'step over' > hardware breakpoints by temporarily removing them from just one > thread; and I believe setting IA64_PSR_DD lets you step or continue > over a breakpoint. > This feature is meant to apply to all breakpoints the stub side supports. Although it can be seen as an optimization in all-stop mode, this is an alternative to displaced stepping on the GDB side, for non-stop mode --- where we need to make sure running threads wouldn't miss a breakpoint. Memory breakpoints are off the chart. Having a remote stub support this feature with memory breakpoints on the GDB side would be too much of a hack. How about something like this? @item StepOverBreakpoints The remote stub knows how to step over breakpoints itself. If this feature is supported by the target, then @value{GDBN} does not need to lift breakpoints off of the inferior to step over them. This feature only applies to single-step requests. @value{GDBN} assumes that the target hits breakpoints at the current PC if told to continue, rather than single-step. This feature is only defined when the remote stub supports managing breakpoints itself (see @ref{insert breakpoint or watchpoint packet}). @value{GDBN} assumes that this feature is applicable to all breakpoints types supported by the stub. -- Pedro Alves --Boundary-00=_6xxDJzvp0AfGpZX Content-Type: text/x-diff; charset="iso 8859-15"; name="target_can_step_over_breakpoints.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="target_can_step_over_breakpoints.diff" Content-length: 12398 gdb/ 2008-11-03 Pedro Alves * infrun.c (resume, proceed, keep_going, handle_inferior_event): Check if target can step over breakpoints. * remote.c (struct remote_state): New member can_step_over_breakpoints. (remote_step_over_breakpoints_feature): New. (remote_protocol_features): Add StepOverBreakpoints feature. (remote_open_1): Clear can_step_over_breakpoints. (remote_can_step_over_breakpoints): New. (init_remote_ops): Set remote_ops.to_can_step_over_breakpoints to remote_can_step_over_breakpoints. * target.c (update_current_target): Inherit to_can_step_over_breakpoints, and default it to return 0. * target.h (struct target_ops): Add new to_can_step_over_breakpoints member. (target_can_step_over_breakpoints): New. gdb/doc/ 2008-11-03 Pedro Alves * gdb.texinfo (General Query Packets): Document StepOverBreakpoints under qSupported. * gdbint.texinfo (Single Stepping): Document target_can_step_over_breakpoints. --- gdb/doc/gdb.texinfo | 16 +++++++++++++ gdb/doc/gdbint.texinfo | 9 +++++++ gdb/infrun.c | 59 ++++++++++++++++++++++++++++++------------------- gdb/remote.c | 24 +++++++++++++++++++ gdb/target.c | 4 +++ gdb/target.h | 11 +++++++++ 6 files changed, 101 insertions(+), 22 deletions(-) Index: src/gdb/doc/gdb.texinfo =================================================================== --- src.orig/gdb/doc/gdb.texinfo 2008-11-03 13:24:04.000000000 +0000 +++ src/gdb/doc/gdb.texinfo 2008-11-03 15:09:52.000000000 +0000 @@ -26118,6 +26118,11 @@ These are the currently defined stub fea @tab @samp{-} @tab No +@item @samp{StepOverBreakpoints} +@tab No +@tab @samp{-} +@tab No + @end multitable These are the currently defined stub features, in more detail: @@ -26183,6 +26188,17 @@ debugging of more than one process at a multiprocess extensions in packet replies unless @value{GDBN} has also indicated it supports them in its @samp{qSupported} request. +@item StepOverBreakpoints +The remote stub knows how to step over breakpoints itself. If this +feature is supported by the target, then @value{GDBN} does not need to +lift breakpoints off of the inferior to step over them. This feature +only applies to single-step requests. @value{GDBN} assumes that the +target hits breakpoints at the current PC if told to continue, rather +than single-step. This feature is only defined when the remote stub +supports managing breakpoints itself (see @ref{insert breakpoint or +watchpoint packet}). @value{GDBN} assumes that this feature is +applicable to all breakpoints types supported by the stub. + @end table @item qSymbol:: Index: src/gdb/doc/gdbint.texinfo =================================================================== --- src.orig/gdb/doc/gdbint.texinfo 2008-11-03 13:24:04.000000000 +0000 +++ src/gdb/doc/gdbint.texinfo 2008-11-03 14:12:57.000000000 +0000 @@ -554,6 +554,15 @@ but @code{placed_size} may be. @section Single Stepping +@table @code +@cindex stepping over breakpoints +@findex target_can_step_over_breakpoints +@item target_can_step_over_breakpoints +Returns true if the target handles stepping over breakpoints +transparently, hence @value{GDBN} does not need to lift breakpoints +off the inferior when single-stepping over a breakpoint. +@end table + @section Signal Handling @section Thread Handling Index: src/gdb/target.h =================================================================== --- src.orig/gdb/target.h 2008-11-03 13:24:04.000000000 +0000 +++ src/gdb/target.h 2008-11-03 14:12:57.000000000 +0000 @@ -540,6 +540,11 @@ struct target_ops simultaneously? */ int (*to_supports_multi_process) (void); + /* Return true if GDB does not need to lift breakpoints from the + the inferior while stepping over a breakpoint; the target + handles that transparently. */ + int (*to_can_step_over_breakpoints) (void); + int to_magic; /* Need sub-structure for target machine related rather than comm related? */ @@ -657,6 +662,12 @@ extern void target_resume (ptid_t ptid, #define target_supports_multi_process() \ (*current_target.to_supports_multi_process) () +/* Return true if GDB does not need to lift breakpoints from the the + inferior while stepping over a breakpoint; the target handles that + transparently. */ +#define target_can_step_over_breakpoints() \ + (*current_target.to_can_step_over_breakpoints) () + extern DCACHE *target_dcache; extern int target_read_string (CORE_ADDR, char **, int, int *); Index: src/gdb/target.c =================================================================== --- src.orig/gdb/target.c 2008-11-03 14:12:48.000000000 +0000 +++ src/gdb/target.c 2008-11-03 14:12:57.000000000 +0000 @@ -472,6 +472,7 @@ update_current_target (void) INHERIT (to_get_ada_task_ptid, t); /* Do not inherit to_search_memory. */ INHERIT (to_supports_multi_process, t); + INHERIT (to_can_step_over_breakpoints, t); INHERIT (to_magic, t); /* Do not inherit to_memory_map. */ /* Do not inherit to_flash_erase. */ @@ -642,6 +643,9 @@ update_current_target (void) de_fault (to_supports_multi_process, (int (*) (void)) return_zero); + de_fault (to_can_step_over_breakpoints, + (int (*) (void)) + return_zero); #undef de_fault /* Finally, position the target-stack beneath the squashed Index: src/gdb/infrun.c =================================================================== --- src.orig/gdb/infrun.c 2008-11-03 13:27:20.000000000 +0000 +++ src/gdb/infrun.c 2008-11-03 14:12:57.000000000 +0000 @@ -1009,7 +1009,8 @@ a command like `return' or `jump' to con the comments for displaced_step_prepare explain why. The comments in the handle_inferior event for dealing with 'random signals' explain what we do instead. */ - if (use_displaced_stepping (gdbarch) + if (!target_can_step_over_breakpoints () + && use_displaced_stepping (gdbarch) && tp->trap_expected && sig == TARGET_SIGNAL_0) { @@ -1123,16 +1124,23 @@ a command like `return' or `jump' to con resume_ptid = inferior_ptid; } - if (gdbarch_cannot_step_breakpoint (gdbarch)) + /* Most targets can step a breakpoint instruction, thus + executing it normally (hitting the breakpoint). But if this + one cannot, just continue and we will hit it anyway. */ + /* A target that executes the instruction under a breakpoint + automatically when told to single-step, by definition also + must be told to continue, otherwise the breakpoint won't be + hit. */ + if (gdbarch_cannot_step_breakpoint (gdbarch) + || (target_can_step_over_breakpoints () + && !tp->trap_expected)) { - /* Most targets can step a breakpoint instruction, thus - executing it normally. But if this one cannot, just - continue and we will hit it anyway. */ if (step && breakpoint_inserted_here_p (pc)) step = 0; } - if (debug_displaced + if (!target_can_step_over_breakpoints () + && debug_displaced && use_displaced_stepping (gdbarch) && tp->trap_expected) { @@ -1329,19 +1337,22 @@ proceed (CORE_ADDR addr, enum target_sig if (oneproc) { tp->trap_expected = 1; - /* If displaced stepping is enabled, we can step over the - breakpoint without hitting it, so leave all breakpoints - inserted. Otherwise we need to disable all breakpoints, step - one instruction, and then re-add them when that step is - finished. */ - if (!use_displaced_stepping (gdbarch)) + /* If displaced stepping is enabled or the target can step over + breakpoints, we can step over the breakpoint without hitting + it, so leave all breakpoints inserted. Otherwise we need to + disable all breakpoints, step one instruction, and then + re-add them when that step is finished. */ + if (!target_can_step_over_breakpoints () + && !use_displaced_stepping (gdbarch)) remove_breakpoints (); } /* We can insert breakpoints if we're not trying to step over one, - or if we are stepping over one but we're using displaced stepping - to do so. */ - if (! tp->trap_expected || use_displaced_stepping (gdbarch)) + or if we are stepping over one but the target can step over them + transparently or we're using displaced stepping to do so. */ + if (! tp->trap_expected + || target_can_step_over_breakpoints () + || use_displaced_stepping (gdbarch)) insert_breakpoints (); if (!non_stop) @@ -2615,9 +2626,11 @@ targets should add new threads to the th singlestep_breakpoints_inserted_p = 0; } - /* If the arch can displace step, don't remove the - breakpoints. */ - if (!use_displaced_stepping (current_gdbarch)) + /* If the target can step over breakpoints transparently, or + we can use displace stepping with this arch, don't remove + the breakpoints. */ + if (!target_can_step_over_breakpoints () + && !use_displaced_stepping (current_gdbarch)) remove_status = remove_breakpoints (); /* Did we fail to remove breakpoints? If so, try @@ -3900,10 +3913,12 @@ keep_going (struct execution_control_sta if (ecs->event_thread->stepping_over_breakpoint) { - if (! use_displaced_stepping (current_gdbarch)) - /* Since we can't do a displaced step, we have to remove - the breakpoint while we step it. To keep things - simple, we remove them all. */ + if (!target_can_step_over_breakpoints () + && !use_displaced_stepping (current_gdbarch)) + /* Since neither the target can step over breakpoints + transparently, neither can we do a displaced step, we + have to remove the breakpoint while we step it. To + keep things simple, we remove them all. */ remove_breakpoints (); } else Index: src/gdb/remote.c =================================================================== --- src.orig/gdb/remote.c 2008-11-03 14:12:48.000000000 +0000 +++ src/gdb/remote.c 2008-11-03 14:12:57.000000000 +0000 @@ -301,6 +301,10 @@ struct remote_state /* True if the stub reports support for vCont;t. */ int support_vCont_t; + + /* True if the stub reports support for stepping over breakpoints + transparently. */ + int can_step_over_breakpoints; }; /* Returns true if the multi-process extensions are in effect. */ @@ -2932,6 +2936,15 @@ remote_non_stop_feature (const struct pr rs->non_stop_aware = (support == PACKET_ENABLE); } +static void +remote_step_over_breakpoints_feature (const struct protocol_feature *feature, + enum packet_support support, + const char *value) +{ + struct remote_state *rs = get_remote_state (); + rs->can_step_over_breakpoints = (support == PACKET_ENABLE); +} + static struct protocol_feature remote_protocol_features[] = { { "PacketSize", PACKET_DISABLE, remote_packet_size, -1 }, { "qXfer:auxv:read", PACKET_DISABLE, remote_supported_packet, @@ -2952,6 +2965,8 @@ static struct protocol_feature remote_pr PACKET_QStartNoAckMode }, { "multiprocess", PACKET_DISABLE, remote_multi_process_feature, -1 }, { "QNonStop", PACKET_DISABLE, remote_non_stop_feature, -1 }, + { "StepOverBreakpoints", PACKET_DISABLE, + remote_step_over_breakpoints_feature, -1 }, }; static void @@ -3185,6 +3200,7 @@ remote_open_1 (char *name, int from_tty, rs->extended = extended_p; rs->non_stop_aware = 0; rs->waiting_for_stop_reply = 0; + rs->can_step_over_breakpoints = 0; general_thread = not_sent_ptid; continue_thread = not_sent_ptid; @@ -8566,6 +8582,13 @@ remote_supports_multi_process (void) return remote_multi_process_p (rs); } +static int +remote_can_step_over_breakpoints (void) +{ + struct remote_state *rs = get_remote_state (); + return rs->can_step_over_breakpoints; +} + static void init_remote_ops (void) { @@ -8629,6 +8652,7 @@ Specify the serial device it is connecte remote_ops.to_terminal_ours = remote_terminal_ours; remote_ops.to_supports_non_stop = remote_supports_non_stop; remote_ops.to_supports_multi_process = remote_supports_multi_process; + remote_ops.to_can_step_over_breakpoints = remote_can_step_over_breakpoints; } /* Set up the extended remote vector by making a copy of the standard --Boundary-00=_6xxDJzvp0AfGpZX--