* [Fwd: Re: [PATCH] Program Breakpoints]
@ 2009-05-19 17:30 Ross Morley
2009-05-19 18:58 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Ross Morley @ 2009-05-19 17:30 UTC (permalink / raw)
To: GDB Patches
[-- Attachment #1: Type: text/plain, Size: 6861 bytes --]
Time for the weekly ping. It's been almost a month now.
Is there anybody who can review this?
Thanks,
Ross
--
Ross Morley
ross@tensilica.com
ross@computer.org
-------- Original Message --------
Subject: Re: [PATCH] Program Breakpoints
Date: Thu, 30 Apr 2009 11:05:01 -0700
From: Ross Morley <ross@tensilica.com>
To: GDB Patches <gdb-patches@sourceware.org>
Ping code reviewers... (it's been over a week and doc is approved).
I know you're all busy. I hope to get this wrapped up while its fresh
in my mind and before the code base diverges too much. :-)
Thanks,
Ross
-------- Original Message --------
Subject: Re: [PATCH] Program Breakpoints
Date: Tue, 21 Apr 2009 18:21:52 -0700
From: Ross Morley <ross@tensilica.com>
To: GDB Patches <gdb-patches@sourceware.org>
CC: Maxim Grigoriev <maxim@tensilica.com>
References: <49E93221.7010502@tensilica.com>
Hello,
I was able to run the regressions on x86 linux native, and fixed a problem.
I've also incorporated Eli's feedback on the doc and NEWS (maybe I went
a bit verbose on the NEWS, but you can tell me what to cut). I kept using
the term "target" in the remote protocol section because it's common there.
New patch attached, replaces previous.
Awaiting technical review. I am least confident about my merge with non-stop
thread support, though it causes no new test failures on x86 linux native.
Thanks,
Ross
--
Ross Morley
ross@tensilica.com
ross@computer.org
Ross Morley wrote:
> Sorry, there was a minor technical glitch with the patch. Fixed patch
> attached.
>
> -------- Original Message --------
> Subject: [PATCH] Program Breakpoints
> Date: Fri, 17 Apr 2009 18:32:59 -0700
> From: Ross Morley <ross@tensilica.com>
> To: GDB Patches <gdb-patches@sourceware.org>
> CC: Maxim Grigoriev <maxim@tensilica.com>
>
>
>
> Hello GDB reviewers,
>
> This is the result of the RFC discussion on the GDB list late March.
> I have simplified and updated the feature as discussed. In addition
> I have supported it in gdbserver (only the Xtensa target so far).
> In gdb and gdbserver, target support is optional, and targets that
> do not support it behave as before. User guide updates are included.
>
> *** A patch is submitted for your review (attached). ***
>
> A detailed discussion follows...
>
> A "program breakpoint" is a trap instruction inherent to the target
> program and unknown to GDB. It is like a permanent breakpoint except:
> - it is unknown to GDB (not in the breakpoint table).
> - it need not be the same instruction used by GDB for breakpoints.
>
> Here is the current behavior this patch fixes:
>
> GDB cannot distinguish a SIGTRAP due to a trap instruction in the
> program from one due to single stepping (or other reasons). Breakpoints
> are recognized only if they are known to (and usually planted by) GDB.
> A trap instruction inherent in the target program is not recognized.
> If the target hits one during normal run, GDB receives a SIGTRAP and
> sees it as a random signal, reports SIGTRAP and (fortuituously) stops.
> If this happens during stepping, GDB assumes the SIGTRAP is from a
> single-step and keeps going, repeatedly hitting the trap. A user using
> software watchpoints expects slow exection, so may wait a long time
> before suspecting a problem, only to find out GDB is hung.
>
> Here's a description of the feature and the reasons for it:
>
> A new target.h macro is provided to allow GDB to determine that it
> was stopped by a trap instruction: STOPPED_BY_TRAP_INSTRUCTION(size).
> The optional size parameter is a pointer into which the size of the
> instruction is written if the PC is pointing to it, allowing GDB to
> skip it on resume or step. Targets that implement this recognize
> program breakpoints by knowing they hit a trap that is not in the
> breakpoint table. A GDB planted breakpoint always takes precedence
> over a program breakpoint (in fact GDB might plant a breakpoint
> over a program breakpoint). I have only implemented it for the
> (remote) targets we support.
>
> Any target may implement this macro, receiving information from the
> inferior or probing the instruction in memory, as appropriate. For
> remote targets it is usually much more efficient to have the target
> communicate that it hit a trap instruction (especially since some
> targets may modify the PC to back out of an exception handler the
> trap is used in, leaving GDB no way to know directly).
>
> Remote targets may notify GDB of hitting a trap instruction by an
> extension to the stop-reply packet for SIGTRAP: "TAAtrap:r", where
> r is a single digit representing the size of the trap instruction
> if and only if the PC is pointing to it (it needs to be skipped).
> That result is passed to infrun and infcmd via the macro above.
> Note that the remote target is not expected to distinguish a
> program breakpoint from a GDB-planted software breakpoint (that
> distinction is made in infrun by looking up the breakpoint table).
>
> When GDB detects a program breakpoint (even while stepping) it
> stops and reports it to both CLI and MI. When stopped by one (and
> if the PC has not been altered while stopped), on resume or step
> GDB will step over it by incrementing the PC by the size (if != 0).
> In the case of step-instruction where PC points to the trap, the
> first step does not actually run the inferior but merely increments
> the PC, simulating the expected advance of 1 instruction.
>
> gdbserver has been modified to recognize a trap as distinct from
> a single step, and pass the information from the low target up
> to the remote interface. The target must support this for the
> information to be passed to GDB via the stop-reply extension.
>
> This has been implemented and well tested in Xtensa toolchains
> with the Xtensa instruction set simulator as the target agent.
> It has now also been tested with gdbserver on Xtensa. No other
> targets yet implement this.
>
> The user manual has been updated. I will update the internals
> manual after this patch has been (possibly changed and) accepted.
>
> We have a target specific regression test internally. However I
> don't know how to make a generic test case because it needs to
> embed an arch-specific trap instruction with an __asm__ construct.
> Advice on this would be appreciated. We could submit the test as
> a target or arch specific test.
>
> In merging from our GDB 6.8 based internal source base (on which
> our extensive testing was done) to the CVS head, I had to merge
> with non-stop thread support and reverse debugging. I believe
> I have handled those correctly, but have no target to test them
> on. I hope someone can integrate this patch and run all the
> regressions to make sure I didn't break anything.
>
> Thanks and best regards,
> Ross
>
> --
> Ross Morley
> ross@tensilica.com
> ross@computer.org
>
>
[-- Attachment #2: program-breakpoints-20090421.diff --]
[-- Type: text/x-patch, Size: 29207 bytes --]
2009-04-21 Ross Morley <ross@computer.org>
doc
* doc/gdb.texinfo: Mention program breakpoints and associated remote
protocol 'TAAtrap:r' stop-reply packet extension, and MI async record.
gdb
* NEWS: GDB recognizes program breakpoints (trap instructions in
the target program and unknown to GDB) and can step over them.
Includes remote protocol extension and requires target support.
* gdbthread.h (struct thread_info): Add program_breakpoint_hit and
program_breakpoint_size.
* remote.c (remote_stopped_by_trap_instruction_p): New.
(remote_trap_instruction_size): New.
(struct stop_reply): New stopped_by_trap_instruction_p and
trap_instruction_size.
(remote_parse_stop_reply): Handle 'trap:size' remote protocol extension.
(process_stop_reply): Set remote_stopped_by_trap_instruction_p and
remote_trap_instruction_size from stop_reply.
(remote_wait_as): Initialize remote_stopped_by_trap_instruction_p.
(remote_stopped_by_trap_instruction): New.
(init_remote_ops): Initialize new callback in remote_ops.
* target.h (struct target_ops): Add to_stopped_by_trap_instruction.
(STOPPED_BY_TRAP_INSTRUCTION): New.
* target.c (update_current_target): Inherit and de_fault
to_stopped_by_trap_instruction.
(debug_to_stopped_by_trap_instruction): New.
(setup_target_debug): Initialize to_stopped_by_trap_instruction.
* infcmd.c: Include mi/mi-common.h.
(step_1): Step-i over program breakpoint and report.
* infrun.c (resume): Skip program breakpoint.
(enum inferior_stop_reason): Add PROGRAM_BREAKPOINT stop reason.
(handle_inferior_event): Recognize program breakpoint hit and avoid
treating it as a random signal. Add debug diagnostic.
(process_event_stop_test): Handle program breakpoint hit.
(print_stop_reason): Handle PROGRAM_BREAKPOINT stop reason.
(normal_stop): Print stop reason and location for program breakpoint.
* mi/mi_common.h (enum async_reply_reason): New async reply reason
EXEC_ASYNC_PROGRAM_BREAKPOINT.
* mi/mi_common.c (async_reason_string_lookup): New program-breakpoint
reason string for EXEC_ASYNC_PROGRAM_BREAKPOINT.
gdbserver
* NEWS: gdbserver recognizes trap instructions whether or not they
are used by GDB for breakpoints, and reports them to the host GDB.
* linux-low.c (linux_stopped_by_trap_instruction): New.
(struct target_ops): Add linux_stopped_by_trap_instruction.
* linux-low.h (struct linux_target_ops): New trap_size_at function.
Comment breakpoint_at to clarify distinction from trap_size_at.
* linux-xtensa-low.c (XTENSA_BREAKPOINT): Comment only 'density'.
(xtensa_trap_size-at): New.
(struct linux_target_ops): Add xtensa_trap_size_at + padding.
Change '0' entries to 'NULL' for pointer fields.
* remote-utils.c (prepare_resume_reply): Append 'trap:r' extension
to 'TAA' stop-reply for SIGTRAP when stopped by trap instruction.
* target.h (struct target_ops): Add stopped_by_trap_instruction.
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.582
diff -u -r1.582 gdb.texinfo
--- gdb/doc/gdb.texinfo 21 Apr 2009 16:31:06 -0000 1.582
+++ gdb/doc/gdb.texinfo 21 Apr 2009 22:23:28 -0000
@@ -2961,6 +2961,14 @@
(for example, routines that are arguments in a @code{pthread_create}
call).
+Some programs may contain embedded break or trap instructions that are
+unknown to @value{GDBN}. These are called @dfn{program breakpoints}
+because they belong to the program itself. If encountered, the inferior
+may stop execution and report this to @value{GDBN} along with how much to
+increment the PC to step over it. Because they are part of the program
+code, program breakpoints cannot be deleted or disabled and do not show
+up in @value{GDBN}'s list of breakpoints.
+
@cindex watchpoints
@cindex data breakpoints
@cindex memory tracing
@@ -20185,6 +20193,8 @@
The inferior exited normally.
@item signal-received
A signal was received by the inferior.
+@item program-breakpoint
+A program breakpoint (trap instruction unknown to @value{GDBN}) was reached.
@end table
The @var{id} field identifies the thread that directly caused the stop
@@ -26736,6 +26746,12 @@
@value{GDBN} should use @samp{qXfer:libraries:read} to fetch a new
list of loaded libraries. @var{r} is ignored.
+@item trap
+The packet indicates that a break or trap instruction was hit.
+@var{r} is the size of the instruction if the PC is pointing to it,
+else 0 (for example if the hardware already incremented the PC).
+@var{r} is ignored if the instruction was inserted by @value{GDBN}.
+
@cindex replay log events, remote reply
@item replaylog
The packet indicates that the target cannot continue replaying
Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.308
diff -u -r1.308 NEWS
--- gdb/NEWS 20 Apr 2009 21:11:05 -0000 1.308
+++ gdb/NEWS 21 Apr 2009 22:23:30 -0000
@@ -7,6 +7,19 @@
feature is available with a native GDB running on kernel version
2.6.28 or later.
+* GDB now supports handling embedded trap instructions that are unknown
+to GDB. These are called "program breakpoints" because they belong to
+the program itself. An inferior can report hitting a trap instruction
+along with its size (it need not know whether the trap was inserted by
+GDB as a breakpoint). If a trap is reported that GDB did not insert,
+GDB will stop and report to the user or MI that a program breakpoint
+was hit, and will be able to step over it on resume. Remote targets
+use a new extension to the 'T' stop reply packet to report hitting a
+trap ("TAAtrap:size"). Previously GDB was unable to distinguish a trap
+instruction from a single step stop reason, so if stepping would try to
+keep going and repeatedly hit the trap. Initially only Xtensa remote
+targets report hitting a trap instruction.
+
* GDB now has support for multi-byte and wide character sets on the
target. Strings whose character type is wchar_t, char16_t, or
char32_t are now correctly printed. GDB supports wide- and unicode-
Index: gdb/gdbthread.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbthread.h,v
retrieving revision 1.47
diff -u -r1.47 gdbthread.h
--- gdb/gdbthread.h 31 Mar 2009 15:23:57 -0000 1.47
+++ gdb/gdbthread.h 21 Apr 2009 22:23:31 -0000
@@ -131,6 +131,13 @@
back to user code before stopping and reporting the event. */
int stepping_through_solib_after_catch;
+ /* program_breakpoint_hit is set when a break or trap instruction was hit at
+ an address not in the breakpoint table (it's part of the program). If so,
+ program_breakpoint_size is the size of the instruction if it needs to be
+ skipped, else 0 (for example, the target already incremented the PC). */
+ int program_breakpoint_hit;
+ int program_breakpoint_size;
+
/* When stepping_through_solib_after_catch is TRUE, this is a
list of the catchpoints that should be reported as triggering
when we finally do stop stepping. */
Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.354
diff -u -r1.354 remote.c
--- gdb/remote.c 16 Apr 2009 19:31:03 -0000 1.354
+++ gdb/remote.c 21 Apr 2009 22:23:34 -0000
@@ -528,6 +528,17 @@
/* This is non-zero if target stopped for a watchpoint. */
static int remote_stopped_by_watchpoint_p;
+/* This is non-zero if target stopped for a trap instruction, whether or
+ not it was a software breakpoint planted by GDB. */
+static int remote_stopped_by_trap_instruction_p = 0;
+
+/* When remote_stopped_by_trap_instruction_p is non-zero, this is the size
+ of the instruction (or 0 if it should not be skipped) as reported by the
+ target. If non-zero GDB must increment the PC by size before resuming.
+ Generally this is non-zero if and only if the target stopped with PC at
+ the breakpoint. */
+static int remote_trap_instruction_size = 0;
+
static struct target_ops remote_ops;
static struct target_ops extended_remote_ops;
@@ -4157,6 +4168,9 @@
int stopped_by_watchpoint_p;
CORE_ADDR watch_data_address;
+ int stopped_by_trap_instruction_p;
+ int trap_instruction_size;
+
int solibs_changed;
int replay_event;
};
@@ -4321,6 +4335,7 @@
event->solibs_changed = 0;
event->replay_event = 0;
event->stopped_by_watchpoint_p = 0;
+ event->stopped_by_trap_instruction_p = 0;
event->regcache = NULL;
switch (buf[0])
@@ -4343,6 +4358,7 @@
char *p_temp;
int fieldsize;
LONGEST pnum = 0;
+ ULONGEST size;
/* If the packet contains a register number, save it in
pnum and set p1 to point to the character following it.
@@ -4387,6 +4403,12 @@
event->solibs_changed = 1;
p = p_temp;
}
+ else if (strncmp (p, "trap", p1 - p) == 0)
+ {
+ event->stopped_by_trap_instruction_p = 1;
+ p = unpack_varlen_hex (++p1, &size);
+ event->trap_instruction_size = (int) size;
+ }
else if (strncmp (p, "replaylog", p1 - p) == 0)
{
/* NO_HISTORY event.
@@ -4637,6 +4659,10 @@
remote_stopped_by_watchpoint_p = stop_reply->stopped_by_watchpoint_p;
remote_watch_data_address = stop_reply->watch_data_address;
+ remote_stopped_by_trap_instruction_p
+ = stop_reply->stopped_by_trap_instruction_p;
+ remote_trap_instruction_size = stop_reply->trap_instruction_size;
+
remote_notice_new_inferior (ptid, 0);
}
@@ -4756,6 +4782,7 @@
buf = rs->buf;
remote_stopped_by_watchpoint_p = 0;
+ remote_stopped_by_trap_instruction_p = 0;
/* We got something. */
rs->waiting_for_stop_reply = 0;
@@ -7041,6 +7068,13 @@
return rc;
}
+static int
+remote_stopped_by_trap_instruction (int *size)
+{
+ if (remote_stopped_by_trap_instruction_p && size)
+ *size = remote_trap_instruction_size;
+ return remote_stopped_by_trap_instruction_p;
+}
static int
remote_insert_hw_breakpoint (struct bp_target_info *bp_tgt)
@@ -8800,6 +8834,8 @@
remote_ops.to_remove_breakpoint = remote_remove_breakpoint;
remote_ops.to_stopped_by_watchpoint = remote_stopped_by_watchpoint;
remote_ops.to_stopped_data_address = remote_stopped_data_address;
+ remote_ops.to_stopped_by_trap_instruction =
+ remote_stopped_by_trap_instruction;
remote_ops.to_can_use_hw_breakpoint = remote_check_watch_resources;
remote_ops.to_insert_hw_breakpoint = remote_insert_hw_breakpoint;
remote_ops.to_remove_hw_breakpoint = remote_remove_hw_breakpoint;
Index: gdb/target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.149
diff -u -r1.149 target.h
--- gdb/target.h 17 Mar 2009 19:28:09 -0000 1.149
+++ gdb/target.h 21 Apr 2009 22:23:35 -0000
@@ -372,6 +372,7 @@
int (*to_watchpoint_addr_within_range) (struct target_ops *,
CORE_ADDR, CORE_ADDR, int);
int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, int);
+ int (*to_stopped_by_trap_instruction) (int *);
void (*to_terminal_init) (void);
void (*to_terminal_inferior) (void);
void (*to_terminal_ours_for_output) (void);
@@ -1127,6 +1128,14 @@
extern const struct target_desc *target_read_description (struct target_ops *);
+/* Returns non-zero if we were stopped by a trap or break instruction,
+ whether or not it is a software break planted by GDB. If size != 0,
+ sets *size to that of the instruction or 0 if it need not be skipped. */
+
+#ifndef STOPPED_BY_TRAP_INSTRUCTION
+#define STOPPED_BY_TRAP_INSTRUCTION(size) \
+ (*current_target.to_stopped_by_trap_instruction) (size)
+#endif
#define target_get_ada_task_ptid(lwp, tid) \
(*current_target.to_get_ada_task_ptid) (lwp,tid)
Index: gdb/target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.206
diff -u -r1.206 target.c
--- gdb/target.c 14 Apr 2009 16:48:07 -0000 1.206
+++ gdb/target.c 21 Apr 2009 22:23:36 -0000
@@ -115,6 +115,8 @@
static int debug_to_remove_watchpoint (CORE_ADDR, int, int);
+static int debug_to_stopped_by_trap_instruction (int *);
+
static int debug_to_stopped_by_watchpoint (void);
static int debug_to_stopped_data_address (struct target_ops *, CORE_ADDR *);
@@ -435,6 +437,7 @@
INHERIT (to_insert_watchpoint, t);
INHERIT (to_remove_watchpoint, t);
INHERIT (to_stopped_data_address, t);
+ INHERIT (to_stopped_by_trap_instruction, t);
INHERIT (to_have_steppable_watchpoint, t);
INHERIT (to_have_continuable_watchpoint, t);
INHERIT (to_stopped_by_watchpoint, t);
@@ -551,6 +554,9 @@
de_fault (to_stopped_data_address,
(int (*) (struct target_ops *, CORE_ADDR *))
return_zero);
+ de_fault (to_stopped_by_trap_instruction,
+ (int (*) (int*))
+ return_zero);
de_fault (to_watchpoint_addr_within_range,
default_watchpoint_addr_within_range);
de_fault (to_region_ok_for_hw_watchpoint,
@@ -2994,6 +3000,19 @@
return retval;
}
+static int
+debug_to_stopped_by_trap_instruction (int *size)
+{
+ int retval;
+
+ retval = debug_target.to_stopped_by_trap_instruction (size);
+
+ fprintf_unfiltered (gdb_stdlog,
+ "target_stopped_by_trap_instruction (%d) = %d\n",
+ *size, retval);
+ return retval;
+}
+
static void
debug_to_terminal_init (void)
{
@@ -3230,6 +3249,7 @@
current_target.to_remove_watchpoint = debug_to_remove_watchpoint;
current_target.to_stopped_by_watchpoint = debug_to_stopped_by_watchpoint;
current_target.to_stopped_data_address = debug_to_stopped_data_address;
+ current_target.to_stopped_by_trap_instruction = debug_to_stopped_by_trap_instruction;
current_target.to_watchpoint_addr_within_range = debug_to_watchpoint_addr_within_range;
current_target.to_region_ok_for_hw_watchpoint = debug_to_region_ok_for_hw_watchpoint;
current_target.to_terminal_init = debug_to_terminal_init;
Index: gdb/infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.238
diff -u -r1.238 infcmd.c
--- gdb/infcmd.c 25 Mar 2009 21:42:34 -0000 1.238
+++ gdb/infcmd.c 21 Apr 2009 22:23:37 -0000
@@ -52,6 +52,7 @@
#include "cli/cli-decode.h"
#include "gdbthread.h"
#include "valprint.h"
+#include "mi/mi-common.h"
/* Functions exported for general use, in inferior.h: */
@@ -822,6 +823,39 @@
make_cleanup (delete_longjmp_breakpoint_cleanup, &thread);
}
+ /* "stepi" off program breakpoint: the first step is to just increment
+ the PC past the break, then there are count-1 steps to go.
+ Note proceed() won't be called the first time, and on subsequent
+ steps the PC will already be off the break, so the entire handling
+ of "stepi" off a program breakpoint is done here. If stopping after
+ the break, display location information as for normal_stop. */
+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
+ {
+ struct thread_info *tp = inferior_thread ();
+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
+ && count > 0)
+ {
+ count--;
+ write_pc (read_pc () + tp->program_breakpoint_size);
+ if (count == 0)
+ {
+ reinit_frame_cache ();
+ if (ui_out_is_mi_like_p (uiout))
+ {
+ ui_out_field_string
+ (uiout, "reason",
+ async_reason_lookup (EXEC_ASYNC_END_STEPPING_RANGE));
+ ui_out_field_int (uiout, "thread-id",
+ pid_to_thread_id (inferior_ptid));
+ print_stack_frame (get_selected_frame (NULL), -1, LOC_AND_ADDRESS);
+ }
+ else
+ print_stack_frame (get_selected_frame (NULL), -1, SRC_LINE);
+ }
+ }
+ }
+
/* In synchronous case, all is well; each step_once call will step once. */
if (!target_can_async_p ())
{
Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.369
diff -u -r1.369 infrun.c
--- gdb/infrun.c 20 Apr 2009 21:11:06 -0000 1.369
+++ gdb/infrun.c 21 Apr 2009 22:23:39 -0000
@@ -1029,6 +1029,11 @@
a command like `return' or `jump' to continue execution."));
}
+ /* Skip a program breakpoint (unless PC changed without running inferior). */
+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc)
+ write_pc (read_pc () + tp->program_breakpoint_size);
+
/* If enabled, step over breakpoints by executing a copy of the
instruction at a different address.
@@ -1564,7 +1569,9 @@
/* Inferior received signal, and user asked to be notified. */
SIGNAL_RECEIVED,
/* Reverse execution -- target ran out of history info. */
- NO_HISTORY
+ NO_HISTORY,
+ /* Inferior stopped because of program breakpoint. */
+ PROGRAM_BREAKPOINT
};
/* The PTID we'll do a target_wait on.*/
@@ -2937,6 +2944,13 @@
|| stop_soon == STOP_QUIETLY || stop_soon == STOP_QUIETLY_NO_SIGSTOP
|| stop_soon == STOP_QUIETLY_REMOTE)
{
+ /* If we hit a trap not inserted by GDB, it must be in the program. */
+ ecs->event_thread->program_breakpoint_hit
+ = (STOPPED_BY_TRAP_INSTRUCTION (&ecs->event_thread->program_breakpoint_size)
+ && !software_breakpoint_inserted_here_p (stop_pc));
+ if (debug_infrun && ecs->event_thread->program_breakpoint_hit)
+ fprintf_unfiltered (gdb_stdlog, "infrun: program breakpoint hit\n");
+
if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP && stop_after_trap)
{
if (debug_infrun)
@@ -3016,6 +3030,7 @@
if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP)
ecs->random_signal
= !(bpstat_explains_signal (ecs->event_thread->stop_bpstat)
+ || ecs->event_thread->program_breakpoint_hit
|| ecs->event_thread->trap_expected
|| (ecs->event_thread->step_range_end
&& ecs->event_thread->step_resume_breakpoint == NULL));
@@ -3136,6 +3151,21 @@
return;
}
+ /* Handle case of hitting a program breakpoint (break instruction
+ in target program, not planted by or known to GDB). */
+
+ if (ecs->event_thread->program_breakpoint_hit)
+ {
+ stop_print_frame = 1;
+
+ /* We are about to nuke the step_resume_breakpoint and
+ through_sigtramp_breakpoint via the cleanup chain, so
+ no need to worry about it here. */
+
+ stop_stepping (ecs);
+ return;
+ }
+
/* Handle cases caused by hitting a breakpoint. */
{
CORE_ADDR jmp_buf_pc;
@@ -4232,6 +4262,17 @@
/* Reverse execution: target ran out of history info. */
ui_out_text (uiout, "\nNo more reverse-execution history.\n");
break;
+ case PROGRAM_BREAKPOINT:
+ /* The inferior was stopped by a break/trap inherent in the program. */
+ {
+ if (ui_out_is_mi_like_p (uiout))
+ ui_out_field_string
+ (uiout, "reason",
+ async_reason_lookup (EXEC_ASYNC_PROGRAM_BREAKPOINT));
+ ui_out_text (uiout, "\nProgram breakpoint, ");
+ /* to be followed by source location, as for planted breakpoint */
+ }
+ break;
default:
internal_error (__FILE__, __LINE__,
_("print_stop_reason: unrecognized enum value"));
@@ -4370,45 +4411,54 @@
int do_frame_printing = 1;
struct thread_info *tp = inferior_thread ();
- bpstat_ret = bpstat_print (tp->stop_bpstat);
- switch (bpstat_ret)
+ if (tp->program_breakpoint_hit)
+ {
+ /* Print program break stop reason and description. */
+ print_stop_reason (PROGRAM_BREAKPOINT, 0);
+ source_flag = SRC_AND_LOC; /* print location and source line */
+ }
+ else
{
- case PRINT_UNKNOWN:
- /* If we had hit a shared library event breakpoint,
- bpstat_print would print out this message. If we hit
- an OS-level shared library event, do the same
- thing. */
- if (last.kind == TARGET_WAITKIND_LOADED)
+ bpstat_ret = bpstat_print (tp->stop_bpstat);
+ switch (bpstat_ret)
{
- printf_filtered (_("Stopped due to shared library event\n"));
+ case PRINT_UNKNOWN:
+ /* If we had hit a shared library event breakpoint,
+ bpstat_print would print out this message. If we hit
+ an OS-level shared library event, do the same
+ thing. */
+ if (last.kind == TARGET_WAITKIND_LOADED)
+ {
+ printf_filtered (_("Stopped due to shared library event\n"));
+ source_flag = SRC_LINE; /* something bogus */
+ do_frame_printing = 0;
+ break;
+ }
+
+ /* FIXME: cagney/2002-12-01: Given that a frame ID does
+ (or should) carry around the function and does (or
+ should) use that when doing a frame comparison. */
+ if (tp->stop_step
+ && frame_id_eq (tp->step_frame_id,
+ get_frame_id (get_current_frame ()))
+ && step_start_function == find_pc_function (stop_pc))
+ source_flag = SRC_LINE; /* finished step, print source line */
+ else
+ source_flag = SRC_AND_LOC; /* print location and source line */
+ break;
+ case PRINT_SRC_AND_LOC:
+ source_flag = SRC_AND_LOC; /* print location and source line */
+ break;
+ case PRINT_SRC_ONLY:
+ source_flag = SRC_LINE;
+ break;
+ case PRINT_NOTHING:
source_flag = SRC_LINE; /* something bogus */
do_frame_printing = 0;
break;
+ default:
+ internal_error (__FILE__, __LINE__, _("Unknown value."));
}
-
- /* FIXME: cagney/2002-12-01: Given that a frame ID does
- (or should) carry around the function and does (or
- should) use that when doing a frame comparison. */
- if (tp->stop_step
- && frame_id_eq (tp->step_frame_id,
- get_frame_id (get_current_frame ()))
- && step_start_function == find_pc_function (stop_pc))
- source_flag = SRC_LINE; /* finished step, just print source line */
- else
- source_flag = SRC_AND_LOC; /* print location and source line */
- break;
- case PRINT_SRC_AND_LOC:
- source_flag = SRC_AND_LOC; /* print location and source line */
- break;
- case PRINT_SRC_ONLY:
- source_flag = SRC_LINE;
- break;
- case PRINT_NOTHING:
- source_flag = SRC_LINE; /* something bogus */
- do_frame_printing = 0;
- break;
- default:
- internal_error (__FILE__, __LINE__, _("Unknown value."));
}
/* The behavior of this routine with respect to the source
Index: gdb/mi/mi-common.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-common.h,v
retrieving revision 1.7
diff -u -r1.7 mi-common.h
--- gdb/mi/mi-common.h 3 Jan 2009 05:57:57 -0000 1.7
+++ gdb/mi/mi-common.h 21 Apr 2009 22:23:41 -0000
@@ -35,6 +35,7 @@
EXEC_ASYNC_EXITED,
EXEC_ASYNC_EXITED_NORMALLY,
EXEC_ASYNC_SIGNAL_RECEIVED,
+ EXEC_ASYNC_PROGRAM_BREAKPOINT,
/* This is here only to represent the number of enums. */
EXEC_ASYNC_LAST
};
Index: gdb/mi/mi-common.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-common.c,v
retrieving revision 1.7
diff -u -r1.7 mi-common.c
--- gdb/mi/mi-common.c 21 Feb 2009 16:14:50 -0000 1.7
+++ gdb/mi/mi-common.c 21 Apr 2009 22:23:44 -0000
@@ -33,6 +33,7 @@
"exited",
"exited-normally",
"signal-received",
+ "program-breakpoint",
NULL
};
Index: gdb/gdbserver/linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.97
diff -u -r1.97 linux-low.c
--- gdb/gdbserver/linux-low.c 3 Apr 2009 11:40:02 -0000 1.97
+++ gdb/gdbserver/linux-low.c 21 Apr 2009 22:23:45 -0000
@@ -2886,6 +2886,29 @@
return 0;
}
+static int
+linux_stopped_by_trap_instruction (int *size)
+{
+ CORE_ADDR stop_pc = get_stop_pc();
+ int trap_size = 0;
+
+ if (the_low_target.trap_size_at != NULL)
+ trap_size = (*the_low_target.trap_size_at) (stop_pc);
+ else if (the_low_target.breakpoint_at != NULL
+ && (*the_low_target.breakpoint_at) (stop_pc))
+ trap_size = the_low_target.breakpoint_len;
+
+ if (trap_size != 0)
+ {
+ *size = (the_low_target.get_pc != NULL
+ && (*the_low_target.get_pc) () == stop_pc)
+ ? trap_size : 0;
+ return 1;
+ }
+ else
+ return 0;
+}
+
static struct target_ops linux_target_ops = {
linux_create_inferior,
linux_attach,
@@ -2923,6 +2946,7 @@
linux_supports_non_stop,
linux_async,
linux_start_non_stop,
+ linux_stopped_by_trap_instruction,
};
static void
Index: gdb/gdbserver/linux-low.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
retrieving revision 1.28
diff -u -r1.28 linux-low.h
--- gdb/gdbserver/linux-low.h 1 Apr 2009 22:50:24 -0000 1.28
+++ gdb/gdbserver/linux-low.h 21 Apr 2009 22:23:46 -0000
@@ -77,6 +77,9 @@
int decr_pc_after_break;
+
+ /* Returns non-zero if there is a breakpoint at the PC (specifically, if
+ the target memory at PC matches the breakpoint field of this struct). */
int (*breakpoint_at) (CORE_ADDR pc);
/* Watchpoint related functions. See target.h for comments. */
@@ -89,6 +92,11 @@
for registers smaller than an xfer unit). */
void (*collect_ptrace_register) (int regno, char *buf);
void (*supply_ptrace_register) (int regno, const char *buf);
+
+ /* Returns non-zero if there is any kind of trap instruction at the PC,
+ including but not limited to the instruction used for GDB breakpoints.
+ If so, the result is the size of the trap (PC increment to skip). */
+ int (*trap_size_at) (CORE_ADDR pc);
};
extern struct linux_target_ops the_low_target;
Index: gdb/gdbserver/linux-xtensa-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-xtensa-low.c,v
retrieving revision 1.4
diff -u -r1.4 linux-xtensa-low.c
--- gdb/gdbserver/linux-xtensa-low.c 22 Mar 2009 23:57:10 -0000 1.4
+++ gdb/gdbserver/linux-xtensa-low.c 21 Apr 2009 22:23:47 -0000
@@ -140,6 +140,7 @@
{ 0, 0, -1, -1, NULL, NULL }
};
+/* Currently assumes the Xtensa 'density' option is configured: 'break.n 0'. */
#if XCHAL_HAVE_BE
#define XTENSA_BREAKPOINT {0xd2,0x0f}
#else
@@ -175,12 +176,37 @@
return memcmp((char *)&insn, xtensa_breakpoint, xtensa_breakpoint_len) == 0;
}
+int
+xtensa_trap_size_at (CORE_ADDR pc)
+{
+ unsigned char insn[3];
+
+ (*the_target->read_memory) (pc, insn, 3);
+
+#if XCHAL_HAVE_BE
+ if (insn[1] == 0xd2 && (insn[2] & 0x0f) == 0x0f)
+#else
+ if (insn[0] == 0x2d && (insn[1] & 0xf0) == 0xf0)
+#endif
+ /* break.n s (density) */
+ return 2;
+#if XCHAL_HAVE_BE
+ else if ((insn[2] & 0xf0) == 0x00 && (insn[1] & 0x0f) == 0x04 && insn[0] == 0x00)
+#else
+ else if ((insn[0] & 0x0f) == 0x00 && (insn[1] & 0xf0) == 0x40 && insn[2] == 0x00)
+#endif
+ /* break s, t */
+ return 3;
+ else
+ return 0;
+}
+
struct linux_target_ops the_low_target = {
init_registers_xtensa,
0,
- 0,
- 0,
- 0,
+ NULL,
+ NULL,
+ NULL,
xtensa_get_pc,
xtensa_set_pc,
xtensa_breakpoint,
@@ -188,4 +214,11 @@
NULL,
0,
xtensa_breakpoint_at,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ NULL,
+ xtensa_trap_size_at,
};
Index: gdb/gdbserver/remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.66
diff -u -r1.66 remote-utils.c
--- gdb/gdbserver/remote-utils.c 3 Apr 2009 14:38:38 -0000 1.66
+++ gdb/gdbserver/remote-utils.c 21 Apr 2009 22:23:49 -0000
@@ -1083,6 +1083,7 @@
{
struct thread_info *saved_inferior;
const char **regp;
+ int size;
sprintf (buf, "T%02x", status->value.sig);
buf += strlen (buf);
@@ -1113,6 +1114,14 @@
*buf++ = ';';
}
+ else if (the_target->stopped_by_trap_instruction != NULL
+ && (*the_target->stopped_by_trap_instruction) (&size))
+ {
+ sprintf (buf, "trap:%1x", size);
+ buf += 6;
+ *buf++ = ';';
+ }
+
while (*regp)
{
buf = outreg (find_regno (*regp), buf);
Index: gdb/gdbserver/target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.36
diff -u -r1.36 target.h
--- gdb/gdbserver/target.h 1 Apr 2009 22:50:24 -0000 1.36
+++ gdb/gdbserver/target.h 21 Apr 2009 22:23:50 -0000
@@ -275,6 +275,14 @@
/* Switch to non-stop (1) or all-stop (0) mode. Return 0 on
success, -1 otherwise. */
int (*start_non_stop) (int);
+
+ /* Returns non-zero if target was stopped by any trap instruction, else 0.
+ It may or may not have been a breakpoint planted by gdbserver or GDB.
+ If stopped by a trap and size != NULL, then *size is set to the size of
+ the trap instruction if the PC still points to it (for skipping), else 0.
+ This information is passed via the remote protocol to GDB. */
+
+ int (*stopped_by_trap_instruction) (int *size);
};
extern struct target_ops *the_target;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Fwd: Re: [PATCH] Program Breakpoints]
2009-05-19 17:30 [Fwd: Re: [PATCH] Program Breakpoints] Ross Morley
@ 2009-05-19 18:58 ` Pedro Alves
2009-05-19 19:53 ` Ross Morley
2009-05-27 1:49 ` [PATCH] Program Breakpoints Ross Morley
0 siblings, 2 replies; 6+ messages in thread
From: Pedro Alves @ 2009-05-19 18:58 UTC (permalink / raw)
To: gdb-patches; +Cc: Ross Morley
Okay, first try. Sorry for the delay...
On Tuesday 19 May 2009 18:28:35, Ross Morley wrote:
> +/* Returns non-zero if we were stopped by a trap or break instruction,
> + whether or not it is a software break planted by GDB. If size != 0,
> + sets *size to that of the instruction or 0 if it need not be skipped. */
> +
> +#ifndef STOPPED_BY_TRAP_INSTRUCTION
> +#define STOPPED_BY_TRAP_INSTRUCTION(size) \
> + (*current_target.to_stopped_by_trap_instruction) (size)
> +#endif
> #define target_get_ada_task_ptid(lwp, tid) \
> (*current_target.to_get_ada_task_ptid) (lwp,tid)
>
Please rename this to target_stopped_by_trap_instruction, and
make it unconditionally defined. We don't support overriding
of these target macros anymore.
> + if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
> + {
> + struct thread_info *tp = inferior_thread ();
> + if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
> + && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
> + && count > 0)
Sorry, `read_pc' is now gone from the sources. Replace that by
regcache_read_pc (get_current_regcache ()), or get_frame_pc if there's a
frame handy.
+ /* "stepi" off program breakpoint: the first step is to just increment
+ the PC past the break, then there are count-1 steps to go.
+ Note proceed() won't be called the first time, and on subsequent
+ steps the PC will already be off the break, so the entire handling
+ of "stepi" off a program breakpoint is done here. If stopping after
+ the break, display location information as for normal_stop. */
+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
+ {
+ struct thread_info *tp = inferior_thread ();
+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
+ && count > 0)
+ {
+ count--;
+ write_pc (read_pc () + tp->program_breakpoint_size);
+ if (count == 0)
+ {
+ reinit_frame_cache ();
+ if (ui_out_is_mi_like_p (uiout))
+ {
+ ui_out_field_string
+ (uiout, "reason",
+ async_reason_lookup (EXEC_ASYNC_END_STEPPING_RANGE));
+ ui_out_field_int (uiout, "thread-id",
+ pid_to_thread_id (inferior_ptid));
+ print_stack_frame (get_selected_frame (NULL), -1, LOC_AND_ADDRESS);
+ }
+ else
+ print_stack_frame (get_selected_frame (NULL), -1, SRC_LINE);
+ }
+ }
+ }
+
This is broken for the !single_inst cases (step/next). step N is mishandled.
It also doesn't take into account that there may be a
breakpoint at ORIG_PC + program_breakpoint_size.
What if a "program breakpoint" is reported for a GDB breakpoint? You shouldn't
just move the PC in that case. I don't see where that is handled. See comment
on next hunk. I suspect this short circuit should be done elsewhere, somewhere
where it will be possible to pass through handle_inferior_event after
moving the PC forward.
Shouldn't you do something in prepare_to_proceed as well?
Use case is:
<thread 1 hit program breakpoint>
(gdb) thread 2
(gdb) continue
GDB is expected to make progress here, instead of reporting
the same program breakpoint again.
>
> + /* Handle case of hitting a program breakpoint (break instruction
> + in target program, not planted by or known to GDB). */
> +
> + if (ecs->event_thread->program_breakpoint_hit)
> + {
> + stop_print_frame = 1;
> +
> + /* We are about to nuke the step_resume_breakpoint and
> + through_sigtramp_breakpoint via the cleanup chain, so
> + no need to worry about it here. */
> +
> + stop_stepping (ecs);
> + return;
> + }
> +
> /* Handle cases caused by hitting a breakpoint. */
> {
Hmmm, confused: can't the target report a program breakpoint hit
for a PC where there's a gdb breakpoint inserted as well? Wouldn't
it better to have program breakpoints checked from inside
stop_bpstat/bpstop_status/bpstat_explains_signal/bpstat_print?
Then you wouldn't need to add
those if (program_breakpoint) do_this; else handle bpstat;
+static int
+linux_stopped_by_trap_instruction (int *size)
+{
+ CORE_ADDR stop_pc = get_stop_pc();
+ int trap_size = 0;
+
+ if (the_low_target.trap_size_at != NULL)
+ trap_size = (*the_low_target.trap_size_at) (stop_pc);
+ else if (the_low_target.breakpoint_at != NULL
+ && (*the_low_target.breakpoint_at) (stop_pc))
+ trap_size = the_low_target.breakpoint_len;
+
+ if (trap_size != 0)
+ {
+ *size = (the_low_target.get_pc != NULL
+ && (*the_low_target.get_pc) () == stop_pc)
+ ? trap_size : 0;
I think I got this bit, but it would be nice to have a
comment here.
+ return 1;
+ }
+ else
+ return 0;
+}
Before we stick with this forever, it would be very
important to have tests covering corner cases like:
single-stepping through consecutive
"program breakpoint"->gdb breakpoint
single-stepping through consecutive
"program breakpoint"->"program breakpoint"
single- stepping through consecutive
gdb breakpoint->"program breakpoint"
(step, stepi, stepi n)
insn1
"program breakpoint"
foo: gdb breakpoint
insn2
insn3
jmp foo
"step" over this:
function ()
line 1:
insn1
"program breakpoint"
insn2
insn3
line 2:
...
... and have "program breakpoint" be reported, instead
of end-stepping-range at line 2.
From the looks of it, you don't have to do much, if anything
else at all, to have these cases covered on x86; I'd really
like to make sure this is sane on decr_pc_after_break archs, as
this is the way to fix the "step over range with trap
doesn't stop at trap" "bug".
If I read the code correctly, on x86, if you continue
this:
0001 insn1
0002 "program breakpoint"
... you'll report a "program breakpoint" hit at 0003, with
program breakpoint size 0. If you instead stepi through that same
sequence, you'll get a "program breakpoint" hit at 0002, with
program breakpoint size 1. That sounds good, but, if there's a
GDB breakpoint installed at "0002", gdb shouldn't be moving the PC
forward magically, without executing the instruction that
was under the breakpoint originally.
So, if I get this correctly, adjust_pc_after_break should
never try to roll back if program size > 0, even on
decr_pc_after_break archs, but we don't need to check this,
because this will only happen if single-stepping, and it
already does nothing in that case. Right?
--
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Fwd: Re: [PATCH] Program Breakpoints]
2009-05-19 18:58 ` Pedro Alves
@ 2009-05-19 19:53 ` Ross Morley
2009-05-19 20:21 ` Pedro Alves
2009-05-27 1:49 ` [PATCH] Program Breakpoints Ross Morley
1 sibling, 1 reply; 6+ messages in thread
From: Ross Morley @ 2009-05-19 19:53 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Thanks Pedro. This is the kind of feedback I was looking for
in regard to effects on threads and other archs. I will have
to go through your comments and think about those scenarios
you mentioned with stepping. Before I do that I do want to
clarify a couple of things for you.
First, the intention is that a program breakpoint is not
reported in the case of hitting a GDB breakpoint. That is
because you hit the inserted GDB breakpoint. You will execute
the program breakpoint at the same location when you remove
the GDB breakpoint to step over it. You cannot hit both at
the same time. I think this corner case (GDB breakpoint over
program breakpoint) will be handled as for other stop reasons
that occur while stepping over a GDB breakpoint, but I will
look more closely at this.
Test cases seem impossible to write completely generically
because the test needs a target-specific break instruction.
We have one (not in this patch) I could submit for Xtensa.
It places 2 sizes of program breakpoints for Xtensa as follows:
__asm__("break 7,15");
__asm__("break.n 7");
Is there a way to write a generic test with a program break?
I will go through your feedback thouroughly in the next couple
of days.
Thanks,
Ross
Pedro Alves wrote:
>Okay, first try. Sorry for the delay...
>
>On Tuesday 19 May 2009 18:28:35, Ross Morley wrote:
>
>
>>+/* Returns non-zero if we were stopped by a trap or break instruction,
>>+ whether or not it is a software break planted by GDB. If size != 0,
>>+ sets *size to that of the instruction or 0 if it need not be skipped. */
>>+
>>+#ifndef STOPPED_BY_TRAP_INSTRUCTION
>>+#define STOPPED_BY_TRAP_INSTRUCTION(size) \
>>+ (*current_target.to_stopped_by_trap_instruction) (size)
>>+#endif
>> #define target_get_ada_task_ptid(lwp, tid) \
>> (*current_target.to_get_ada_task_ptid) (lwp,tid)
>>
>>
>>
>
>Please rename this to target_stopped_by_trap_instruction, and
>make it unconditionally defined. We don't support overriding
>of these target macros anymore.
>
>
>
>>+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
>>+ {
>>+ struct thread_info *tp = inferior_thread ();
>>+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
>>+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
>>+ && count > 0)
>>
>>
>
>Sorry, `read_pc' is now gone from the sources. Replace that by
>
>regcache_read_pc (get_current_regcache ()), or get_frame_pc if there's a
>frame handy.
>
>+ /* "stepi" off program breakpoint: the first step is to just increment
>+ the PC past the break, then there are count-1 steps to go.
>+ Note proceed() won't be called the first time, and on subsequent
>+ steps the PC will already be off the break, so the entire handling
>+ of "stepi" off a program breakpoint is done here. If stopping after
>+ the break, display location information as for normal_stop. */
>+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
>+ {
>+ struct thread_info *tp = inferior_thread ();
>+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
>+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
>+ && count > 0)
>+ {
>+ count--;
>+ write_pc (read_pc () + tp->program_breakpoint_size);
>+ if (count == 0)
>+ {
>+ reinit_frame_cache ();
>+ if (ui_out_is_mi_like_p (uiout))
>+ {
>+ ui_out_field_string
>+ (uiout, "reason",
>+ async_reason_lookup (EXEC_ASYNC_END_STEPPING_RANGE));
>+ ui_out_field_int (uiout, "thread-id",
>+ pid_to_thread_id (inferior_ptid));
>+ print_stack_frame (get_selected_frame (NULL), -1, LOC_AND_ADDRESS);
>+ }
>+ else
>+ print_stack_frame (get_selected_frame (NULL), -1, SRC_LINE);
>+ }
>+ }
>+ }
>+
>
>This is broken for the !single_inst cases (step/next). step N is mishandled.
>It also doesn't take into account that there may be a
>breakpoint at ORIG_PC + program_breakpoint_size.
>
>What if a "program breakpoint" is reported for a GDB breakpoint? You shouldn't
>just move the PC in that case. I don't see where that is handled. See comment
>on next hunk. I suspect this short circuit should be done elsewhere, somewhere
>where it will be possible to pass through handle_inferior_event after
>moving the PC forward.
>
>Shouldn't you do something in prepare_to_proceed as well?
>
> Use case is:
>
> <thread 1 hit program breakpoint>
> (gdb) thread 2
> (gdb) continue
>
> GDB is expected to make progress here, instead of reporting
> the same program breakpoint again.
>
>
>
>>+ /* Handle case of hitting a program breakpoint (break instruction
>>+ in target program, not planted by or known to GDB). */
>>+
>>+ if (ecs->event_thread->program_breakpoint_hit)
>>+ {
>>+ stop_print_frame = 1;
>>+
>>+ /* We are about to nuke the step_resume_breakpoint and
>>+ through_sigtramp_breakpoint via the cleanup chain, so
>>+ no need to worry about it here. */
>>+
>>+ stop_stepping (ecs);
>>+ return;
>>+ }
>>+
>> /* Handle cases caused by hitting a breakpoint. */
>> {
>>
>>
>
>Hmmm, confused: can't the target report a program breakpoint hit
>for a PC where there's a gdb breakpoint inserted as well? Wouldn't
>it better to have program breakpoints checked from inside
>stop_bpstat/bpstop_status/bpstat_explains_signal/bpstat_print?
>Then you wouldn't need to add
>those if (program_breakpoint) do_this; else handle bpstat;
>
>+static int
>+linux_stopped_by_trap_instruction (int *size)
>+{
>+ CORE_ADDR stop_pc = get_stop_pc();
>+ int trap_size = 0;
>+
>+ if (the_low_target.trap_size_at != NULL)
>+ trap_size = (*the_low_target.trap_size_at) (stop_pc);
>+ else if (the_low_target.breakpoint_at != NULL
>+ && (*the_low_target.breakpoint_at) (stop_pc))
>+ trap_size = the_low_target.breakpoint_len;
>+
>+ if (trap_size != 0)
>+ {
>+ *size = (the_low_target.get_pc != NULL
>+ && (*the_low_target.get_pc) () == stop_pc)
>+ ? trap_size : 0;
>
>I think I got this bit, but it would be nice to have a
>comment here.
>
>+ return 1;
>+ }
>+ else
>+ return 0;
>+}
>
>
>Before we stick with this forever, it would be very
>important to have tests covering corner cases like:
>
> single-stepping through consecutive
> "program breakpoint"->gdb breakpoint
>
> single-stepping through consecutive
> "program breakpoint"->"program breakpoint"
>
>single- stepping through consecutive
> gdb breakpoint->"program breakpoint"
>
>(step, stepi, stepi n)
>
> insn1
> "program breakpoint"
>foo: gdb breakpoint
> insn2
> insn3
> jmp foo
>
>"step" over this:
>
>function ()
>
>line 1:
> insn1
> "program breakpoint"
> insn2
> insn3
>line 2:
> ...
>
>... and have "program breakpoint" be reported, instead
>of end-stepping-range at line 2.
>
>>From the looks of it, you don't have to do much, if anything
>else at all, to have these cases covered on x86; I'd really
>like to make sure this is sane on decr_pc_after_break archs, as
>this is the way to fix the "step over range with trap
>doesn't stop at trap" "bug".
>
>If I read the code correctly, on x86, if you continue
>this:
>
>0001 insn1
>0002 "program breakpoint"
>
>... you'll report a "program breakpoint" hit at 0003, with
>program breakpoint size 0. If you instead stepi through that same
>sequence, you'll get a "program breakpoint" hit at 0002, with
>program breakpoint size 1. That sounds good, but, if there's a
>GDB breakpoint installed at "0002", gdb shouldn't be moving the PC
>forward magically, without executing the instruction that
>was under the breakpoint originally.
>
>So, if I get this correctly, adjust_pc_after_break should
>never try to roll back if program size > 0, even on
>decr_pc_after_break archs, but we don't need to check this,
>because this will only happen if single-stepping, and it
>already does nothing in that case. Right?
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Fwd: Re: [PATCH] Program Breakpoints]
2009-05-19 19:53 ` Ross Morley
@ 2009-05-19 20:21 ` Pedro Alves
2009-05-19 21:33 ` Ross Morley
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2009-05-19 20:21 UTC (permalink / raw)
To: Ross Morley; +Cc: gdb-patches
On Tuesday 19 May 2009 20:52:01, Ross Morley wrote:
> Thanks Pedro. This is the kind of feedback I was looking for
> in regard to effects on threads and other archs. I will have
> to go through your comments and think about those scenarios
> you mentioned with stepping. Before I do that I do want to
> clarify a couple of things for you.
>
> First, the intention is that a program breakpoint is not
> reported in the case of hitting a GDB breakpoint.
You mean, reported to the user, right? I agree. However,
the target will report to gdb a program breakpoint for
gdb breakpoints as well:
+ if (the_low_target.trap_size_at != NULL)
+ trap_size = (*the_low_target.trap_size_at) (stop_pc);
+ else if (the_low_target.breakpoint_at != NULL
+ && (*the_low_target.breakpoint_at) (stop_pc))
+ trap_size = the_low_target.breakpoint_len;
Only way to tell the difference is if there's a breakpoint
at PC in gdb's tables or not.
Hmm, isn't there an off-by-one error in the XCHAL_HAVE_BE
case here? :
+#if XCHAL_HAVE_BE
+ if (insn[1] == 0xd2 && (insn[2] & 0x0f) == 0x0f)
+#else
+ if (insn[0] == 0x2d && (insn[1] & 0xf0) == 0xf0)
+#endif
+ /* break.n s (density) */
> Test cases seem impossible to write completely generically
> because the test needs a target-specific break instruction.
> We have one (not in this patch) I could submit for Xtensa.
> It places 2 sizes of program breakpoints for Xtensa as follows:
> __asm__("break 7,15");
> __asm__("break.n 7");
> Is there a way to write a generic test with a program break?
I think you can do something reasonably generic enough by
passing "additional_flags=-DNOP=mysmallnopinsn -DBREAK_INSN1=foo -DBREAK_INSN2=bar"
to gdb_compile, and write the test in c with embedded asm, as:
int foo ()
{
NOP
NOP
BREAK_INSN1
jmphere:
NOP /* breakpoint here*/
NOP
BREAK_INSN1
NOP
goto jmphere;
}
etc, etc.
Then, the .exp file passes the appropriate defines/flags
based on target triplet.
if { ([istarget "xtensa*-*-*"] } {
additional_flags={-DNOP=mysmallnopinsn -DBREAK_INSN1="break 7,15" -DBREAK_INSN2="break.n 7"}
} else if { ([istarget "fooarch*-*-*"] } {
...
}
--
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Fwd: Re: [PATCH] Program Breakpoints]
2009-05-19 20:21 ` Pedro Alves
@ 2009-05-19 21:33 ` Ross Morley
0 siblings, 0 replies; 6+ messages in thread
From: Ross Morley @ 2009-05-19 21:33 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Pedro Alves wrote:
>On Tuesday 19 May 2009 20:52:01, Ross Morley wrote:
>
>
>>First, the intention is that a program breakpoint is not
>>reported in the case of hitting a GDB breakpoint.
>>
>>
>
>You mean, reported to the user, right?
>
No, I mean reported to the workings of infrun (high level
of GDB). The target interface reports a trap which could
be a GDB breakpoint. But handle_inferior_event distinguishes
a program breakpoint from a GDB one then sets a flag in the
thread structure if it's a program breakpoint. This chunk in
infrun.c:
/* The PTID we'll do a target_wait on.*/
@@ -2937,6 +2944,13 @@
|| stop_soon == STOP_QUIETLY || stop_soon == STOP_QUIETLY_NO_SIGSTOP
|| stop_soon == STOP_QUIETLY_REMOTE)
{
+ /* If we hit a trap not inserted by GDB, it must be in the
program. */
+ ecs->event_thread->program_breakpoint_hit
+ = (STOPPED_BY_TRAP_INSTRUCTION
(&ecs->event_thread->program_breakpoint_size)
+ && !software_breakpoint_inserted_here_p (stop_pc));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+ if (debug_infrun && ecs->event_thread->program_breakpoint_hit)
+ fprintf_unfiltered (gdb_stdlog, "infrun: program breakpoint hit\n");
+
So a program breakpoint is deemed hit if a trap was hit and
there was not a software breakpoint inserted there. The idea
is to avoid interfering with normal breakpoint handling and
only handle cases where it's a program breakpoint.
> I agree. However,
>the target will report to gdb a program breakpoint for
>gdb breakpoints as well:
>
>+ if (the_low_target.trap_size_at != NULL)
>+ trap_size = (*the_low_target.trap_size_at) (stop_pc);
>+ else if (the_low_target.breakpoint_at != NULL
>+ && (*the_low_target.breakpoint_at) (stop_pc))
>+ trap_size = the_low_target.breakpoint_len;
>
>Only way to tell the difference is if there's a breakpoint
>at PC in gdb's tables or not.
>
>
Exactly.
>Hmm, isn't there an off-by-one error in the XCHAL_HAVE_BE
>case here? :
>
>+#if XCHAL_HAVE_BE
>+ if (insn[1] == 0xd2 && (insn[2] & 0x0f) == 0x0f)
>+#else
>+ if (insn[0] == 0x2d && (insn[1] & 0xf0) == 0xf0)
>+#endif
>+ /* break.n s (density) */
>
>
>
You are right. There's also a byte reversal in the big endian
case of the 3 byte break. I did gdbserver in a hurry just to
test with the FSF source tree since our toolchain doesn't have
the top of trunk changes yet so I couldn't use our simulator.
I used a little endian configuration and didn't test big endian.
I will test this on big endian before submitting a final patch.
>
>
>>Test cases seem impossible to write completely generically
>>because the test needs a target-specific break instruction.
>>We have one (not in this patch) I could submit for Xtensa.
>>It places 2 sizes of program breakpoints for Xtensa as follows:
>> __asm__("break 7,15");
>> __asm__("break.n 7");
>>Is there a way to write a generic test with a program break?
>>
>>
>
>I think you can do something reasonably generic enough by
>passing "additional_flags=-DNOP=mysmallnopinsn -DBREAK_INSN1=foo -DBREAK_INSN2=bar"
>to gdb_compile, and write the test in c with embedded asm, as:
>
>int foo ()
>{
> NOP
> NOP
> BREAK_INSN1
>jmphere:
> NOP /* breakpoint here*/
> NOP
> BREAK_INSN1
> NOP
>goto jmphere;
>}
>
>etc, etc.
>
>Then, the .exp file passes the appropriate defines/flags
>based on target triplet.
>
>if { ([istarget "xtensa*-*-*"] } {
> additional_flags={-DNOP=mysmallnopinsn -DBREAK_INSN1="break 7,15" -DBREAK_INSN2="break.n 7"}
>} else if { ([istarget "fooarch*-*-*"] } {
> ...
>}
>
>
>
OK, I'll try that.
It will be a day or two before I can look at the other issues you raised.
Thanks,
Ross
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Program Breakpoints
2009-05-19 18:58 ` Pedro Alves
2009-05-19 19:53 ` Ross Morley
@ 2009-05-27 1:49 ` Ross Morley
1 sibling, 0 replies; 6+ messages in thread
From: Ross Morley @ 2009-05-27 1:49 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
target_stopped_by_trap_instruction
Thanks for your feedback. Detailed reply in-line below.
Pedro Alves wrote:
>Okay, first try. Sorry for the delay...
>
>On Tuesday 19 May 2009 18:28:35, Ross Morley wrote:
>
>
>>+/* Returns non-zero if we were stopped by a trap or break instruction,
>>+ whether or not it is a software break planted by GDB. If size != 0,
>>+ sets *size to that of the instruction or 0 if it need not be skipped. */
>>+
>>+#ifndef STOPPED_BY_TRAP_INSTRUCTION
>>+#define STOPPED_BY_TRAP_INSTRUCTION(size) \
>>+ (*current_target.to_stopped_by_trap_instruction) (size)
>>+#endif
>> #define target_get_ada_task_ptid(lwp, tid) \
>> (*current_target.to_get_ada_task_ptid) (lwp,tid)
>>
>>
>>
>
>Please rename this to target_stopped_by_trap_instruction, and
>make it unconditionally defined. We don't support overriding
>of these target macros anymore.
>
>
OK.
>
>
>>+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
>>+ {
>>+ struct thread_info *tp = inferior_thread ();
>>+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
>>+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
>>+ && count > 0)
>>
>>
>
>Sorry, `read_pc' is now gone from the sources. Replace that by
>
>regcache_read_pc (get_current_regcache ()), or get_frame_pc if there's a
>frame handy.
>
>
OK.
>+ /* "stepi" off program breakpoint: the first step is to just increment
>+ the PC past the break, then there are count-1 steps to go.
>+ Note proceed() won't be called the first time, and on subsequent
>+ steps the PC will already be off the break, so the entire handling
>+ of "stepi" off a program breakpoint is done here. If stopping after
>+ the break, display location information as for normal_stop. */
>+ if (target_has_execution && !ptid_equal (inferior_ptid, null_ptid))
>+ {
>+ struct thread_info *tp = inferior_thread ();
>+ if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
>+ && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
>+ && count > 0)
>+ {
>+ count--;
>+ write_pc (read_pc () + tp->program_breakpoint_size);
>+ if (count == 0)
>+ {
>+ reinit_frame_cache ();
>+ if (ui_out_is_mi_like_p (uiout))
>+ {
>+ ui_out_field_string
>+ (uiout, "reason",
>+ async_reason_lookup (EXEC_ASYNC_END_STEPPING_RANGE));
>+ ui_out_field_int (uiout, "thread-id",
>+ pid_to_thread_id (inferior_ptid));
>+ print_stack_frame (get_selected_frame (NULL), -1, LOC_AND_ADDRESS);
>+ }
>+ else
>+ print_stack_frame (get_selected_frame (NULL), -1, SRC_LINE);
>+ }
>+ }
>+ }
>+
>
>This is broken for the !single_inst cases (step/next). step N is mishandled.
>
>
You are right, it can mess up with step/next where count is
lines and not insns. In case of step/next, I need to decrement
count only if the PC increment would change the line number.
Stepping over a program breakpoint is primarily handled in resume.
This fixup just handles a special case in which a stepi/nexti would
step one instruction farther than expected. It re-"executes" the
trap as a no-op when in fact the inferior has not run at all.
Without this code a stepi would execute the instruction following:
1: program breakpoint
2: insn2
3: insn3
Starting at 1, stepi/nexti (with no arg or 1) should result
in PC at 2, but without this fixup it ends up at 3. Of course,
this does not apply for decr_pc_after_break targets. If you
think ending up at 3 is OK, we can just omit this whole chunk.
>It also doesn't take into account that there may be a
>breakpoint at ORIG_PC + program_breakpoint_size.
>
>
I hadn't noticed this before, that GDB reports a breakpoint
hit when it stops with PC at a breakpoint location, even it
hasn't yet tried to execute at that location. So a breakpoint
immediately after the program breakpoint would need to be
reported when the program breakpoint is stepped over, to be
consistent (also when a program breakpoint is first hit on a
decr_pc_after_break target). There may be other cases.
When I first did program breakpoints I studied bpstat, and
it seemed much harder and less appropriate. bpstat is very
tied to the breakpoint table, but program breakpoints are
encountered (not known in advance) and are not in the table.
A bpstat has a bp_location which has an "owner" breakpoint in
the table. bpstat_stop_status scans the breakpoint table to
find possible reasons for the stop (it won't find a program
breakpoint). So I'm not sure it would be appropriate to use
bpstat, and it would likely amount to a much bigger change.
I also noticed that x86 native is not entirely consistent in
this case. It does not report a breakpoint if it hit while
stepping over another. Take the following example transcript:
(gdb) b main
Breakpoint 1 at 0x8048384: file breakop.c, line 14.
(gdb) r
Starting program: /opt/local/ross/test/breakop/breakop
Breakpoint 1, main (argc=0x1, argv=0xbff6a674) at breakop.c:14
14 printf("About to execute 'break'...\n");
(gdb) disass
Dump of assembler code for function main:
0x08048368 <main+0>: push %ebp
0x08048369 <main+1>: mov %esp,%ebp
0x0804836b <main+3>: sub $0x8,%esp
0x0804836e <main+6>: and $0xfffffff0,%esp
0x08048371 <main+9>: mov $0x0,%eax
0x08048376 <main+14>: add $0xf,%eax
0x08048379 <main+17>: add $0xf,%eax
0x0804837c <main+20>: shr $0x4,%eax
0x0804837f <main+23>: shl $0x4,%eax
0x08048382 <main+26>: sub %eax,%esp
0x08048384 <main+28>: sub $0xc,%esp
0x08048387 <main+31>: push $0x80484a0
0x0804838c <main+36>: call 0x80482b0 <_init+56>
0x08048391 <main+41>: add $0x10,%esp
0x08048394 <main+44>: mov 0x80495f0,%eax
0x08048399 <main+49>: incl 0x80495f0
0x0804839f <main+55>: sub $0x4,%esp
0x080483a2 <main+58>: push %eax
0x080483a3 <main+59>: pushl 0x80495f0
0x080483a9 <main+65>: push $0x80484c0
0x080483ae <main+70>: call 0x80482b0 <_init+56>
0x080483b3 <main+75>: add $0x10,%esp
0x080483b6 <main+78>: mov $0x0,%eax
0x080483bb <main+83>: leave
0x080483bc <main+84>: ret
End of assembler dump.
(gdb) p/x $pc
$1 = 0x8048384
(gdb) b * 0x08048387
Breakpoint 2 at 0x8048387: file breakop.c, line 14.
(gdb) b * 0x0804838c
Breakpoint 3 at 0x804838c: file breakop.c, line 14.
(gdb) si
0x08048387 14 printf("About to execute 'break'...\n");
-------> Note the breakpoint was not reported.
(gdb) si
0x0804838c 14 printf("About to execute 'break'...\n");
-------> Nor was this one.
(gdb) si
0x080482b0 in ?? ()
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /opt/local/ross/test/breakop/breakop
Breakpoint 1, main (argc=0x1, argv=0xbffd1164) at breakop.c:14
14 printf("About to execute 'break'...\n");
(gdb) dis 2
(gdb) si
0x08048387 14 printf("About to execute 'break'...\n");
(gdb) si
-------> With the preceding BP diabled, this one is reported.
Breakpoint 3, 0x0804838c in main (argc=0x1, argv=0xbffd1164) at breakop.c:14
14 printf("About to execute 'break'...\n");
(gdb)
So I don't have an ideal way to handle these corner cases.
I lean toward a philosophy that a breakpoint is not hit until
there is an attempt to execute at that location, so I'm not
sure it really makes sense for GDB to report a breakpoint when
it stops (for some other reason) just before it. Perhaps in
this case it doesn't matter because on resume or continued
stepping you will certainly hit the subsequent breakpoint and
GDB should report it then. Keep in mind the goal here is to
make sure GDB stops for a program breakpoint and (secondarily)
can resume afterward. If a GDB breakpoint immediately following
is not reported until resuming, that's not so bad.
>What if a "program breakpoint" is reported for a GDB breakpoint? You shouldn't
>just move the PC in that case. I don't see where that is handled.
>
I gather you are talking about the coincident case (GDB breakpoint
at the same location as a program breakpoint). They cannot both hit
at the same time. One could think of a GDB breakpoint conceptually
lying just before an instruction, to be hit before it is executed.
A program breakpoint is reported only when a trap is executed and
there is no GDB s/w breakpoint inserted at that location. A h/w
breakpoint would be hit before the trap is actually executed.
Here is my understanding of what happens for a s/w breakpoint
coincident with a program breakpoint. GDB first hits the s/w
breakpoint by executing the inserted trap. It stops and reports
a GDB breakpoint (and if decr_pc_after_break, backs up the PC).
On resume it replaces the program breakpoint trap and hits it
while stepping over the GDB breakpoint, and reports a program
breakpoint. On resume from that it increments the PC over the
program breakpoint (if size != 0), reinserts the GDB breakpoint
and continues as usual.
Here is my understanding of what happens for a h/w breakpoint
coincident with a program breakpoint. GDB first hits the h/w
breakpoint by triggering on the fetch PC. It stops and reports
a GDB breakpoint. On resume it disables the h/w breakpoint to
step over it, executes the program breakpoint, and reports it.
On resume from there it increments the PC over the program
breakpoint (if size != 0), enables the h/w breakpoint and
continues as normal.
Is that a reasonable assessment of the coincident case?
> See comment
>on next hunk. I suspect this short circuit should be done elsewhere, somewhere
>where it will be possible to pass through handle_inferior_event after
>moving the PC forward.
>
>
It seems it might be possible to arrange for step_1 or
step_once to call normal_stop directly. I'm not deeply
familiar with the infrun control flow however, and I don't
have the time to study it that much. I wonder if this is
something that could be improved later by an expert, but
in the meantime my less than perfect implementation would
improve current behavior and not lock us into implementation
details. The only things we'd be stuck with would be the
interface function to_stopped_by_trap_instruction(size)
and the remote protocol stop-reply extension "TNNtrap:size".
Improving the way infrun handles things would be possible
without changing how hitting a trap is reported to infrun.
The critical thing is to make sure no regression tests are
broken by this, espeically for targets that don't yet support
reporting the trap (and I did run the testsuite with no
regressions on x86/linux native).
Please read on for comments on your other suggestions.
>Shouldn't you do something in prepare_to_proceed as well?
> Use case is:
>
> <thread 1 hit program breakpoint>
> (gdb) thread 2
> (gdb) continue
>
> GDB is expected to make progress here, instead of reporting
> the same program breakpoint again.
Thread support was added since I originally did program breakpoints
and I am unfamiliar with it (it's not supported on our target). In
merging with cvs head, I tried to guess what would be needed, and I
moved the program_breakpoint_{hit,size} variables into the thread
structure (previously they were global like some other things that
moved to the thread structure).
Now I wonder if I should have left it as it was. The stop state
seems more associated with the inferior than with the GDB thread
(which is only which thread GDB is looking at). Would it be correct
then to just put it back how it was, with those flags global? Both?
Otherwise, I will have to add a test in prepare_to_proceed to
switch back to the original thread like for a GDB breakpoint,
ie.
if (breakpoint_here_p (regcache_read_pc (regcache))
|| (tp->program_breakpoint_hit
&& tp->program_breakpoint_size != 0
&& regcache_read_pc (regcache) == stop_pc))
{
:
}
I'd have to compute tp and stop_pc because they're not passed
into prepare_to_proceed.
Once we get to resume, stepping over a program breakpoint is done
at the same point skipping a permanent breakpoint is done. It's the
same problem except skip_permanent_breakpoint has implicit knowledge
of the trap size. I've previously suggested making a single function
that could be used for both cases, with a size parameter. But for
now I did not want to interfere with permanent breakpoints, so kept
it simple thinking could be a future enhancement after program
breakpoints are accepted, if anyone cares.
>>+ /* Handle case of hitting a program breakpoint (break instruction
>>+ in target program, not planted by or known to GDB). */
>>+
>>+ if (ecs->event_thread->program_breakpoint_hit)
>>+ {
>>+ stop_print_frame = 1;
>>+
>>+ /* We are about to nuke the step_resume_breakpoint and
>>+ through_sigtramp_breakpoint via the cleanup chain, so
>>+ no need to worry about it here. */
>>+
>>+ stop_stepping (ecs);
>>+ return;
>>+ }
>>+
>>
>
Above was copied from other cases in infrun where we need to stop.
I don't mind if that comment gets nuked.
>
>Hmmm, confused: can't the target report a program breakpoint hit
>for a PC where there's a gdb breakpoint inserted as well?
Not if it's inserted - the program breakpoint trap won't be
seen. GDB will see the trap instruction it inserted (which
may or may not be the same instruction). If it's a h/w
breakpoint, GDB will stop before it executes the trap.
> Wouldn't
>it better to have program breakpoints checked from inside
>stop_bpstat/bpstop_status/bpstat_explains_signal/bpstat_print?
>Then you wouldn't need to add
>those if (program_breakpoint) do_this; else handle bpstat;
I thought about this, but it seemed bpstat is not well suited
to this. See earlier discussion.
>+static int
>+linux_stopped_by_trap_instruction (int *size)
>+{
>+ CORE_ADDR stop_pc = get_stop_pc();
>+ int trap_size = 0;
>+
>+ if (the_low_target.trap_size_at != NULL)
>+ trap_size = (*the_low_target.trap_size_at) (stop_pc);
>+ else if (the_low_target.breakpoint_at != NULL
>+ && (*the_low_target.breakpoint_at) (stop_pc))
>+ trap_size = the_low_target.breakpoint_len;
>+
>+ if (trap_size != 0)
>+ {
>+ *size = (the_low_target.get_pc != NULL
>+ && (*the_low_target.get_pc) () == stop_pc)
>+ ? trap_size : 0;
>
>I think I got this bit, but it would be nice to have a
>comment here.
OK. Something like:
/* if we hit a trap insn and the PC still points to it,
report the size of the trap so GDB can step over it. */
?
>+ return 1;
>+ }
>+ else
>+ return 0;
>+}
>
>
>Before we stick with this forever, it would be very
>important to have tests covering corner cases like:
>
> single-stepping through consecutive
> "program breakpoint"->gdb breakpoint
>
> single-stepping through consecutive
> "program breakpoint"->"program breakpoint"
>
>single- stepping through consecutive
> gdb breakpoint->"program breakpoint"
>
>(step, stepi, stepi n)
I agree we need test cases. I'll develop some before
submitting a revised patch (which I hope to do only
once after we hash out the basic issues).
> insn1
> "program breakpoint"
>foo: gdb breakpoint
> insn2
> insn3
> jmp foo
>
>"step" over this:
>
>function ()
>
>line 1:
> insn1
> "program breakpoint"
> insn2
> insn3
>line 2:
> ...
>
>... and have "program breakpoint" be reported, instead
>of end-stepping-range at line 2.
>>From the looks of it, you don't have to do much, if anything
>else at all, to have these cases covered on x86; I'd really
>like to make sure this is sane on decr_pc_after_break archs, as
>this is the way to fix the "step over range with trap
>doesn't stop at trap" "bug".
I think that's the same bug I'm trying to fix, but it
manifests differently for a non decr_pc_after_break
arch. It keeps executing the trap, never advancing.
I guess on x86 it just keeps going.
I think decr_pc_after_break archs will just fall out
except perhaps for a few corner cases. But not being
familiar with the x86 I'd have to learn now to implement
target_ops.to_stopped_by_trap_instruction for x86 native.
It would be better if someone else could implement that,
and I could test it.
Certainly I will run the test suite on native linux/x86
to make sure nothing breaks without that function.
Each target/arch can implement it if/when someone cares.
GDB behavior should not change until that is implemented
for any given target. Meanwhile the program breakpoint
test cases would be expected failures.
>If I read the code correctly, on x86, if you continue
>this:
>
>0001 insn1
>0002 "program breakpoint"
>
>... you'll report a "program breakpoint" hit at 0003,
I'll report it hit, and the PC will be 0003. It was actually
hit at 0002, but on this arch the PC is incremented.
>with
>program breakpoint size 0.
Yes.
> If you instead stepi through that same
>sequence, you'll get a "program breakpoint" hit at 0002, with
>program breakpoint size 1.
That's up to target_ops.to_stopped_by_trap_instruction.
I gather on x86, when stepping the PC is not incremented
after a trap, and the trap size is 1?
> That sounds good, but, if there's a
>GDB breakpoint installed at "0002", gdb shouldn't be moving the PC
>forward magically, without executing the instruction that
>was under the breakpoint originally.
The instruction under the breakpoint is the program
breakpoint trap insn. The program breakpoint won't be
executed while the GDB breakpoint is inserted.
So GDB will first hit the GDB breakpoint. If it's a s/w
breakpoint it will be removed and the program breakpoint
replaced (the latter has not yet been executed/hit).
If it's a h/w breakpoint, I'm guessing the target stopped
before the program breakpoint was executed (else the target
would have undo the effects of any insn at a h/w breakpoint).
On continuing from there, GDB will remove the GDB breakpoint
and single-step. Target will execute the program breakpoint,
stop at 0002 (since it single-stepped), and report being
stopped by a trap with size 1. Since the GDB breakpoint was
not inserted, GDB will interpret it as a program breakpoint.
I'm assuming now that GDB forgets it was trying to step over
a GDB breakpoint, and reinserts it as usual on continue. Now
it knows it hit a program breakpoint and the PC has not changed.
so it will increment the PC by the size (1) to 0003 and continue.
Suppose a decr_pc_after_break arch also increments the pc
after single-stepping a trap insn. In that case the program
breakpoint will be reported with size 0 when pc is at 0003,
and no step-over will be done.
>So, if I get this correctly, adjust_pc_after_break should
>never try to roll back if program size > 0, even on
>decr_pc_after_break archs, but we don't need to check this,
>because this will only happen if single-stepping, and it
>already does nothing in that case. Right?
>
adjust_pc_after_break does exactly the right thing already.
It should only roll back the PC if decr_pc_after_break and
software_breakpoint_inserted_here_p (breakpoint_pc) . The
only reason to roll back the PC is to execute the instruction
under a GDB s/w breakpoint.
In summary, if you agree, I need to make the following changes:
- Rename STOPPED_BY_TRAP_INSTRUCTION to
target_stopped_by_trap_instruction (unconditionally defined).
- Replace read_pc () with regcache_read_pc (get_current_regcache ()).
- Move program_breakpoint_{hit,size} out of the thread structure
back to global variables?
- In step_1, !single_inst cases, decrement count only if
the line number would change.
- Write generic test cases, passing in the trap insn at compile.
- Test on Xtensa.
- Test on x86/native (program breakpoint tests would XFAIL but
all others that passed before would still pass).
- Add the cross-ref to the doc that Eli requested.
- Re-base and resubmit patch.
Thanks,
Ross
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-05-27 1:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-19 17:30 [Fwd: Re: [PATCH] Program Breakpoints] Ross Morley
2009-05-19 18:58 ` Pedro Alves
2009-05-19 19:53 ` Ross Morley
2009-05-19 20:21 ` Pedro Alves
2009-05-19 21:33 ` Ross Morley
2009-05-27 1:49 ` [PATCH] Program Breakpoints Ross Morley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox