Re: [PATCH] Support targets that know how to step over breakpoints

Luis Machado
2012-11-27T15:20:22Z

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On 10/30/2012 08:57 AM, Luis Machado wrote:
> On 10/17/2012 08:42 AM, Luis Machado wrote:
>> Ping?
>>
>> On 10/04/2012 09:48 AM, Luis Machado wrote:
>>> Hi,
>>>
>>> Most of the targets we deal with need to be told to lift off breakpoints
>>> from the inferior prior to single-stepping. A few targets are able to
>>> just step over those breakpoints without being told so.
>>>
>>> In the latter case, GDB assumes the target knows how to manage
>>> breakpoints on its own.
>>>
>>> gdbserver does not know how to do this unless we force this mechanism
>>> into it, but, honestly, i don't see the point. Usually targets that know
>>> how to step over breakpoints do so via a more low level interface like
>>> the kernel. Let me know if you think otherwise.
>>
>>
>
> Ping? http://sourceware.org/ml/gdb-patches/2012-10/msg00067.html
>

Meanwhile i've updated this patch for the latest cvs head.

I'm wondering if the patch is too ugly for someone to take a look at it
or if it is too odd a feature to add. I suppose not.

Hopefully i can get some traction with this new refreshed and shiny
version! :-)

Luis

[-- Attachment #2: 0001-remote_step_over_breaks.diff --]
[-- Type: text/x-patch, Size: 12727 bytes --]

2012-11-27 Luis Machado <lgustavo@codesourcery.com>
Pedro Alves <pedro@codesourcery.com>

gdb/
* 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/
* gdb.texinfo (General Query Packets): Document
StepOverBreakpoints under qSupported.

* gdbint.texinfo (Single Stepping): Document
target_can_step_over_breakpoints.

Index: gdb_head/gdb/doc/gdb.texinfo
===================================================================
--- gdb_head.orig/gdb/doc/gdb.texinfo 2012-11-27 09:24:30.955089370 -0200
+++ gdb_head/gdb/doc/gdb.texinfo 2012-11-27 09:33:05.411095450 -0200
@@ -37083,6 +37083,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:
@@ -37237,6 +37242,17 @@ See @ref{Bytecode Descriptions} for deta
The remote stub supports running a breakpoint's command list itself,
rather than reporting the hit to @value{GDBN}.

+@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: gdb_head/gdb/doc/gdbint.texinfo
===================================================================
--- gdb_head.orig/gdb/doc/gdbint.texinfo 2012-10-10 15:35:25.384415678 -0300
+++ gdb_head/gdb/doc/gdbint.texinfo 2012-11-27 09:33:05.415095451 -0200
@@ -593,6 +593,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: gdb_head/gdb/infrun.c
===================================================================
--- gdb_head.orig/gdb/infrun.c 2012-11-27 09:24:30.987089370 -0200
+++ gdb_head/gdb/infrun.c 2012-11-27 09:33:05.423095450 -0200
@@ -1786,7 +1786,8 @@ a command like `return' or `jump' to con
We can't use displaced stepping when we are waiting for vfork_done
event, displaced stepping breaks the vfork child similarly as single
step software breakpoint. */
- if (use_displaced_stepping (gdbarch)
+ if (!target_can_step_over_breakpoints ()
+ && use_displaced_stepping (gdbarch)
&& (tp->control.trap_expected
|| (step && gdbarch_software_single_step_p (gdbarch)))
&& sig == GDB_SIGNAL_0
@@ -1912,16 +1913,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->control.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 (aspace, pc))
step = 0;
}

- if (debug_displaced
+ if (!target_can_step_over_breakpoints ()
+ && debug_displaced
&& use_displaced_stepping (gdbarch)
&& tp->control.trap_expected)
{
@@ -2219,19 +2227,22 @@ proceed (CORE_ADDR addr, enum gdb_signal
if (oneproc)
{
tp->control.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->control.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->control.trap_expected
+ || target_can_step_over_breakpoints ()
+ || use_displaced_stepping (gdbarch))
insert_breakpoints ();

if (!non_stop)
@@ -3941,10 +3952,12 @@ handle_inferior_event (struct execution_
singlestep_breakpoints_inserted_p = 0;
}

- /* If the arch can displace step, don't remove the
- breakpoints. */
+ /* If the target can step over breakpoints transparently, or
+ we can use displace stepping with this arch, don't remove
+ the breakpoints. */
thread_regcache = get_thread_regcache (ecs->ptid);
- if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
+ if (!target_can_step_over_breakpoints ()
+ && !use_displaced_stepping (get_regcache_arch (thread_regcache)))
remove_status = remove_breakpoints ();

/* Did we fail to remove breakpoints? If so, try
@@ -5715,10 +5728,12 @@ keep_going (struct execution_control_sta
{
struct regcache *thread_regcache = get_thread_regcache (ecs->ptid);

- if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
- /* 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 (get_regcache_arch (thread_regcache)))
+ /* 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: gdb_head/gdb/remote.c
===================================================================
--- gdb_head.orig/gdb/remote.c 2012-11-27 09:24:31.007089371 -0200
+++ gdb_head/gdb/remote.c 2012-11-27 09:33:05.423095450 -0200
@@ -319,6 +319,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;
+
/* True if the stub reports support for conditional tracepoints. */
int cond_tracepoints;

@@ -3879,6 +3883,15 @@ remote_string_tracing_feature (const str
rs->string_tracing = (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,
@@ -3909,6 +3922,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 },
{ "qXfer:siginfo:read", PACKET_DISABLE, remote_supported_packet,
PACKET_qXfer_siginfo_read },
{ "qXfer:siginfo:write", PACKET_DISABLE, remote_supported_packet,
@@ -4236,6 +4251,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;
rs->ctrlc_pending_p = 0;

general_thread = not_sent_ptid;
@@ -11001,6 +11017,13 @@ remote_can_use_agent (void)
return (remote_protocol_packets[PACKET_QAgent].support != PACKET_DISABLE);
}

+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)
{
@@ -11070,6 +11093,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;
remote_ops.to_supports_disable_randomization
= remote_supports_disable_randomization;
remote_ops.to_fileio_open = remote_hostio_open;
Index: gdb_head/gdb/target.c
===================================================================
--- gdb_head.orig/gdb/target.c 2012-11-27 09:24:31.023089369 -0200
+++ gdb_head/gdb/target.c 2012-11-27 09:33:05.427095452 -0200
@@ -673,6 +673,7 @@ update_current_target (void)
INHERIT (to_supports_multi_process, t);
INHERIT (to_supports_enable_disable_tracepoint, t);
INHERIT (to_supports_string_tracing, t);
+ INHERIT (to_can_step_over_breakpoints, t);
INHERIT (to_trace_init, t);
INHERIT (to_download_tracepoint, t);
INHERIT (to_can_download_tracepoint, t);
@@ -930,6 +931,9 @@ update_current_target (void)
de_fault (to_traceframe_info,
(struct traceframe_info * (*) (void))
tcomplain);
+ de_fault (to_can_step_over_breakpoints,
+ (int (*) (void))
+ return_zero);
de_fault (to_supports_evaluation_of_breakpoint_conditions,
(int (*) (void))
return_zero);
Index: gdb_head/gdb/target.h
===================================================================
--- gdb_head.orig/gdb/target.h 2012-11-27 09:24:31.023089369 -0200
+++ gdb_head/gdb/target.h 2012-11-27 09:33:05.431095451 -0200
@@ -857,6 +857,11 @@ struct target_ops
/* Is the target able to use agent in current state? */
int (*to_can_use_agent) (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?
*/
@@ -1011,6 +1016,12 @@ int target_supports_disable_randomizatio
#define target_can_run_breakpoint_commands() \
(*current_target.to_can_run_breakpoint_commands) ()

+/* Return true if GDB does not need to lift breakpoints from 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) ()
+
/* Invalidate all target dcaches. */
extern void target_dcache_invalidate (void);