Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* Re: [PATCH] Program Breakpoints
@ 2009-04-30 18:05 Ross Morley
  2009-04-30 18:30 ` Pedro Alves
  0 siblings, 1 reply; 15+ messages in thread
From: Ross Morley @ 2009-04-30 18:05 UTC (permalink / raw)
  To: GDB Patches

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

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: 29206 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] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-30 18:05 [PATCH] Program Breakpoints Ross Morley
@ 2009-04-30 18:30 ` Pedro Alves
  2009-04-30 19:14   ` Ross Morley
  0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2009-04-30 18:30 UTC (permalink / raw)
  To: gdb-patches; +Cc: Ross Morley

Before looking at the code, I'd like to see the interaction of
program breakpoints with decr_pc_after_break adjustment formalized.
I did a quick skim and couldn't find it handled.

E.g, if you were to implement support for this on x86 gdbserver,
assuming int3 traps, it appears to me that the only option is
for the target to always rewind the pc before reporting to GDB, and
for GDB to never adjust it itself, even for regular breakpoint hits,
otherwise, e.g., consecutive breakpoints will be mishandled.  This
also suggests that there has to be prior negotiation (qSupported) to
enable the support.  Has this been considered?

(A small request: please include the -p switch in your `cvs diff'
commands, or add it to .cvsrc.  I use -Nurp myself.)

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-30 18:30 ` Pedro Alves
@ 2009-04-30 19:14   ` Ross Morley
  2009-05-04 22:49     ` Ross Morley
  0 siblings, 1 reply; 15+ messages in thread
From: Ross Morley @ 2009-04-30 19:14 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches


Pedro Alves wrote:

>Before looking at the code, I'd like to see the interaction of
>program breakpoints with decr_pc_after_break adjustment formalized.
>I did a quick skim and couldn't find it handled.
>  
>
>E.g, if you were to implement support for this on x86 gdbserver,
>assuming int3 traps, it appears to me that the only option is
>for the target to always rewind the pc before reporting to GDB, and
>for GDB to never adjust it itself, even for regular breakpoint hits,
>otherwise, e.g., consecutive breakpoints will be mishandled.  This
>also suggests that there has to be prior negotiation (qSupported) to
>enable the support.  Has this been considered?
>  
>
I have thought about this. I understand decr_pc_after_break is for archs
that have already incremented the PC after hitting a trap, so GDB needs
to decrement it back to the trap in order to replace it with the original
code to step over it.

No special handling is needed for program breakpoints because:

- decr_pc_after_break only applies to breakpoints inserted by GDB
  and therefore known to GDB (adjust_pc_after_break in infrun.c
  appears to only adjust the PC if it hit a software break known
  to GDB). Program breakpoints are by definition not those, and
  their special handling is not applied to those. If a target were
  to adjust the PC for a program breakpoint, it would then have to
  report a non-zero size in STOPPED_BY_TRAP_INSTRUCTION (see next
  bullet), but that would not be the normal handling for such archs.

- decr_pc_after_break archs report size==0 in their implementation
  of STOPPED_BY_TRAP_INSTRUCTION(&size). That means the PC does not
  need adjustment to step over a program breakpoint. GDB will report
  it stopped for a program breakpoint (if it wasn't in the breakpoint
  table) but will not do anything special on resume. Perhaps I should
  make that clearer in the comment in target.h on that macro, by
  explicit reference to decr_pc_after_break.

>(A small request: please include the -p switch in your `cvs diff'
>commands, or add it to .cvsrc.  I use -Nurp myself.)
>
>  
>
OK.

Thanks,
Ross


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-30 19:14   ` Ross Morley
@ 2009-05-04 22:49     ` Ross Morley
  2009-05-04 23:14       ` Pedro Alves
  0 siblings, 1 reply; 15+ messages in thread
From: Ross Morley @ 2009-05-04 22:49 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

Pedro,

Did that make sense to you?
Are you waiting for me to redo the patch with -Nurp before
you look at the code? I had assumed you could look at the
code and I'll redo the patch once I incorporate any feedback.

Or if someone else has time to look at it... ?
Refer to http://sourceware.org/ml/gdb-patches/2009-04/msg00579.html
for discussion and patch.

Thanks,
Ross


Ross Morley wrote:

>
> Pedro Alves wrote:
>
>> Before looking at the code, I'd like to see the interaction of
>> program breakpoints with decr_pc_after_break adjustment formalized.
>> I did a quick skim and couldn't find it handled.
>>  
>>
>> E.g, if you were to implement support for this on x86 gdbserver,
>> assuming int3 traps, it appears to me that the only option is
>> for the target to always rewind the pc before reporting to GDB, and
>> for GDB to never adjust it itself, even for regular breakpoint hits,
>> otherwise, e.g., consecutive breakpoints will be mishandled.  This
>> also suggests that there has to be prior negotiation (qSupported) to
>> enable the support.  Has this been considered?
>>  
>>
> I have thought about this. I understand decr_pc_after_break is for archs
> that have already incremented the PC after hitting a trap, so GDB needs
> to decrement it back to the trap in order to replace it with the original
> code to step over it.
>
> No special handling is needed for program breakpoints because:
>
> - decr_pc_after_break only applies to breakpoints inserted by GDB
>  and therefore known to GDB (adjust_pc_after_break in infrun.c
>  appears to only adjust the PC if it hit a software break known
>  to GDB). Program breakpoints are by definition not those, and
>  their special handling is not applied to those. If a target were
>  to adjust the PC for a program breakpoint, it would then have to
>  report a non-zero size in STOPPED_BY_TRAP_INSTRUCTION (see next
>  bullet), but that would not be the normal handling for such archs.
>
> - decr_pc_after_break archs report size==0 in their implementation
>  of STOPPED_BY_TRAP_INSTRUCTION(&size). That means the PC does not
>  need adjustment to step over a program breakpoint. GDB will report
>  it stopped for a program breakpoint (if it wasn't in the breakpoint
>  table) but will not do anything special on resume. Perhaps I should
>  make that clearer in the comment in target.h on that macro, by
>  explicit reference to decr_pc_after_break.
>
>> (A small request: please include the -p switch in your `cvs diff'
>> commands, or add it to .cvsrc.  I use -Nurp myself.)
>>
>>  
>>
> OK.
>
> Thanks,
> Ross
>
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-05-04 22:49     ` Ross Morley
@ 2009-05-04 23:14       ` Pedro Alves
  2009-05-04 23:17         ` Ross Morley
  0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2009-05-04 23:14 UTC (permalink / raw)
  To: Ross Morley; +Cc: gdb-patches

Hi Ross,

sorry for the quiet delay,

On Monday 04 May 2009 23:49:11, Ross Morley wrote:
> Pedro,
> 
> Did that make sense to you?

It did, thanks.

> Are you waiting for me to redo the patch with -Nurp before
> you look at the code?

> Or if someone else has time to look at it... ?
> Refer to http://sourceware.org/ml/gdb-patches/2009-04/msg00579.html
> for discussion and patch.

Nope.  Just the usual defer-to-the-weekend-due-to-busy-week-but-then-get-caught-up-with-busy-weekend.
If nobody beats me to it, I'll likelly get to it in the following
days, but not sooner than Wednesday, I think.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-05-04 23:14       ` Pedro Alves
@ 2009-05-04 23:17         ` Ross Morley
  2009-05-13  4:24           ` Ross Morley
  0 siblings, 1 reply; 15+ messages in thread
From: Ross Morley @ 2009-05-04 23:17 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

Pedro Alves wrote:

 >Nope.  Just the usual 
defer-to-the-weekend-due-to-busy-week-but-then-get-caught-up-with-busy-weekend.
 >If nobody beats me to it, I'll likelly get to it in the following
 >days, but not sooner than Wednesday, I think.

Thanks. At least I know it's not in limbo.

Ross


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-05-04 23:17         ` Ross Morley
@ 2009-05-13  4:24           ` Ross Morley
  0 siblings, 0 replies; 15+ messages in thread
From: Ross Morley @ 2009-05-13  4:24 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

Ping. Which Wednesday? :-)

Thanks,
Ross


Ross Morley wrote:

>Pedro Alves wrote:
>
> >Nope.  Just the usual 
>defer-to-the-weekend-due-to-busy-week-but-then-get-caught-up-with-busy-weekend.
> >If nobody beats me to it, I'll likelly get to it in the following
> >days, but not sooner than Wednesday, I think.
>
>Thanks. At least I know it's not in limbo.
>
>Ross
>
>
>  
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-05-19 18:58 ` Pedro Alves
@ 2009-05-27  1:49   ` Ross Morley
  0 siblings, 0 replies; 15+ 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] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-22  3:12   ` Eli Zaretskii
@ 2009-04-27  7:07     ` Ross Morley
  0 siblings, 0 replies; 15+ messages in thread
From: Ross Morley @ 2009-04-27  7:07 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches, maxim

Thanks for approving the doc. I will add the cross reference as you suggest
after the code is reviewed in case other changes are needed.

Still waiting for code review.

Ross


Eli Zaretskii wrote:

>>Date: Tue, 21 Apr 2009 18:21:52 -0700
>>From: Ross Morley <ross@tensilica.com>
>>CC: Maxim Grigoriev <maxim@tensilica.com>
>>
>>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.
>>    
>>
>
>Thanks.  I have only one comment:
>
>  
>
>>+@item program-breakpoint
>>+A program breakpoint (trap instruction unknown to @value{GDBN}) was reached.
>>    
>>
>
>This place is far from where you explained what a program breakpoint
>is.  So I suggest a cross-reference here to the node where program
>breakpoints are described.
>
>Sorry I didn't notice this in the original review.
>
>Other than that, the patch for the manual is approved.
>
>  
>
>>Index: gdb/NEWS
>>    
>>
>
>This is fine.
>
>  
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-22  1:22 ` [PATCH] Program Breakpoints Ross Morley
@ 2009-04-22  3:12   ` Eli Zaretskii
  2009-04-27  7:07     ` Ross Morley
  0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2009-04-22  3:12 UTC (permalink / raw)
  To: Ross Morley; +Cc: gdb-patches, maxim

> Date: Tue, 21 Apr 2009 18:21:52 -0700
> From: Ross Morley <ross@tensilica.com>
> CC: Maxim Grigoriev <maxim@tensilica.com>
> 
> 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.

Thanks.  I have only one comment:

> +@item program-breakpoint
> +A program breakpoint (trap instruction unknown to @value{GDBN}) was reached.

This place is far from where you explained what a program breakpoint
is.  So I suggest a cross-reference here to the node where program
breakpoints are described.

Sorry I didn't notice this in the original review.

Other than that, the patch for the manual is approved.

> Index: gdb/NEWS

This is fine.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-18  1:51 [Fwd: [PATCH] Program Breakpoints] Ross Morley
@ 2009-04-22  1:22 ` Ross Morley
  2009-04-22  3:12   ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Ross Morley @ 2009-04-22  1:22 UTC (permalink / raw)
  To: GDB Patches; +Cc: Maxim Grigoriev

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

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: 29204 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] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-18  6:21 ` Eli Zaretskii
  2009-04-18  6:30   ` Eli Zaretskii
@ 2009-04-18  7:58   ` Ross Morley
  1 sibling, 0 replies; 15+ messages in thread
From: Ross Morley @ 2009-04-18  7:58 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: gdb-patches, maxim

Thanks Eli for your valuable comments. I did indeed miss a bunch of "GDB"s,
and double periods after sentences. More in-line...


Eli Zaretskii wrote:

>>Date: Fri, 17 Apr 2009 18:32:59 -0700
>>From: Ross Morley <ross@tensilica.com>
>>CC: Maxim Grigoriev <maxim@tensilica.com>
>>
>>The user manual has been updated. I will update the internals
>>manual after this patch has been (possibly changed and) accepted.
>>    
>>
>
>Thank you.  I have a few comments for the documentation patch:
>
>  
>
>>+Some programs may contain embedded break or trap instructions that are
>>+unknown to GDB. These are called @dfn{program breakpoints} because they 
>>    
>>
>              ^^^
>"@value{GDBN}".
>
>  
>
>>+belong to the program itself. If encountered, a target may stop execution 
>>+and report this to GDB along with how much to increment the PC to step 
>>+over it (a target is not required to do this).
>>    
>>
>
>Please don't talk about a "target" in the user's manual: this
>terminology is unknown to them.  Please talk about the "inferior" or
>the "program being debugged".
>  
>

You're quite right that I should discuss the target less in the user manual
and more in in the internals manual (which I intend to update once this 
patch
has been reviewed and any changes are made). However I'm quite used to 
seeing
the term "target" in the user manual, so am a bit confused on that. I 
did wish
to convey that not all targets (inferiors) will support this feature 
(perhaps
I should just omit the parenthetical part above). Note that in the remote
stop-reply section the term "target" is used this way in several places.

I will incorparate all of your feedback.

Thanks,
Ross

>  
>
>>+@item program-breakpoint
>>+A program breakpoint (trap instruction unknown to GDB) was reached.
>>    
>>
>                                                     ^^^
>"@value{GDBN}"
>
>  
>
>>@@ -26723,6 +26731,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 target-specific 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 planted by @value{GDBN}.
>>+
>> The packet indicates that the target cannot continue replaying 
>>    
>>
>
>Something strange happened with this hunk.  In the current CVS, the
>manual in this part looks like this:
>
>    @cindex shared library events, remote reply
>    @item library
>    The packet indicates that the loaded libraries have changed.
>    @value{GDBN} should use @samp{qXfer:libraries:read} to fetch a new
>    list of loaded libraries.  @var{r} is ignored.
>
>    @cindex replay log events, remote reply
>    @item replaylog
>    The packet indicates that the target cannot continue replaying
>    logged execution events, because it has reached the end (or the
>    beginning when executing backward) of the log.  The value of @var{r}
>    will be either @samp{begin} or @samp{end}.  @xref{Reverse Execution},
>    for more information.
>
>So it seems to me that you are plugging your part in the middle of
>another @item, but then where's the "@item replaylog" line and the one
>preceding it, with @cindex"?  What am I missing?
>
>Finally, please make sure every period that ends a sentence has 2
>spaces, not 1, after it.
>
>  
>
>>--- gdb/NEWS	31 Mar 2009 20:21:06 -0000	1.305
>>+++ gdb/NEWS	17 Apr 2009 18:55:25 -0000
>>@@ -3,6 +3,10 @@
>> 
>> *** Changes since GDB 6.8
>> 
>>+* GDB now supports handling (embedded) program break or trap instructions
>>+that are unknown to GDB. These are called program breakpoints because they
>>+belong to the program itself.
>>    
>>
>
>This is good, but I suggest to add a sentence telling how would GDB
>manifest these program breakpoints.  Also, "program breakpoints"
>should be in quotes, as you are introducing a new term.
>
>Thanks.
>
>  
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-18  6:21 ` Eli Zaretskii
@ 2009-04-18  6:30   ` Eli Zaretskii
  2009-04-18  7:58   ` Ross Morley
  1 sibling, 0 replies; 15+ messages in thread
From: Eli Zaretskii @ 2009-04-18  6:30 UTC (permalink / raw)
  To: ross, gdb-patches, maxim

> Date: Sat, 18 Apr 2009 09:21:19 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gdb-patches@sourceware.org, maxim@tensilica.com
> 
> > +Some programs may contain embedded break or trap instructions that are
> > +unknown to GDB. These are called @dfn{program breakpoints} because they 
>               ^^^
> "@value{GDBN}".
> 
> > +belong to the program itself. If encountered, a target may stop execution 
> > +and report this to GDB along with how much to increment the PC to step 
                        ^^^
Missed another one, sorry.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] Program Breakpoints
  2009-04-18  1:33 Ross Morley
@ 2009-04-18  6:21 ` Eli Zaretskii
  2009-04-18  6:30   ` Eli Zaretskii
  2009-04-18  7:58   ` Ross Morley
  0 siblings, 2 replies; 15+ messages in thread
From: Eli Zaretskii @ 2009-04-18  6:21 UTC (permalink / raw)
  To: Ross Morley; +Cc: gdb-patches, maxim

> Date: Fri, 17 Apr 2009 18:32:59 -0700
> From: Ross Morley <ross@tensilica.com>
> CC: Maxim Grigoriev <maxim@tensilica.com>
> 
> The user manual has been updated. I will update the internals
> manual after this patch has been (possibly changed and) accepted.

Thank you.  I have a few comments for the documentation patch:

> +Some programs may contain embedded break or trap instructions that are
> +unknown to GDB. These are called @dfn{program breakpoints} because they 
              ^^^
"@value{GDBN}".

> +belong to the program itself. If encountered, a target may stop execution 
> +and report this to GDB along with how much to increment the PC to step 
> +over it (a target is not required to do this).

Please don't talk about a "target" in the user's manual: this
terminology is unknown to them.  Please talk about the "inferior" or
the "program being debugged".

> +@item program-breakpoint
> +A program breakpoint (trap instruction unknown to GDB) was reached.
                                                     ^^^
"@value{GDBN}"

> @@ -26723,6 +26731,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 target-specific 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 planted by @value{GDBN}.
> +
>  The packet indicates that the target cannot continue replaying 

Something strange happened with this hunk.  In the current CVS, the
manual in this part looks like this:

    @cindex shared library events, remote reply
    @item library
    The packet indicates that the loaded libraries have changed.
    @value{GDBN} should use @samp{qXfer:libraries:read} to fetch a new
    list of loaded libraries.  @var{r} is ignored.

    @cindex replay log events, remote reply
    @item replaylog
    The packet indicates that the target cannot continue replaying
    logged execution events, because it has reached the end (or the
    beginning when executing backward) of the log.  The value of @var{r}
    will be either @samp{begin} or @samp{end}.  @xref{Reverse Execution},
    for more information.

So it seems to me that you are plugging your part in the middle of
another @item, but then where's the "@item replaylog" line and the one
preceding it, with @cindex"?  What am I missing?

Finally, please make sure every period that ends a sentence has 2
spaces, not 1, after it.

> --- gdb/NEWS	31 Mar 2009 20:21:06 -0000	1.305
> +++ gdb/NEWS	17 Apr 2009 18:55:25 -0000
> @@ -3,6 +3,10 @@
>  
>  *** Changes since GDB 6.8
>  
> +* GDB now supports handling (embedded) program break or trap instructions
> +that are unknown to GDB. These are called program breakpoints because they
> +belong to the program itself.

This is good, but I suggest to add a sentence telling how would GDB
manifest these program breakpoints.  Also, "program breakpoints"
should be in quotes, as you are introducing a new term.

Thanks.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] Program Breakpoints
@ 2009-04-18  1:33 Ross Morley
  2009-04-18  6:21 ` Eli Zaretskii
  0 siblings, 1 reply; 15+ messages in thread
From: Ross Morley @ 2009-04-18  1:33 UTC (permalink / raw)
  To: GDB Patches; +Cc: Maxim Grigoriev

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

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-20090417.diff --]
[-- Type: text/x-patch, Size: 28711 bytes --]


2009-04-17  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; consolidate tp.
	* 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.580
diff -u -r1.580 gdb.texinfo
--- gdb/doc/gdb.texinfo	15 Apr 2009 22:20:32 -0000	1.580
+++ gdb/doc/gdb.texinfo	17 Apr 2009 18:55:23 -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 GDB. These are called @dfn{program breakpoints} because they 
+belong to the program itself. If encountered, a target may stop execution 
+and report this to GDB along with how much to increment the PC to step 
+over it (a target is not required to do this). 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
@@ -20171,6 +20179,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 GDB) was reached.
 @end table

 The @var{id} field identifies the thread that directly caused the stop
@@ -26723,6 +26731,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 target-specific 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 planted by @value{GDBN}.
+
 The packet indicates that the target cannot continue replaying 
Index: gdb/NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.305
diff -u -r1.305 NEWS
--- gdb/NEWS	31 Mar 2009 20:21:06 -0000	1.305
+++ gdb/NEWS	17 Apr 2009 18:55:25 -0000
@@ -3,6 +3,10 @@
 
 *** Changes since GDB 6.8
 
+* GDB now supports handling (embedded) program break or trap instructions
+that are unknown to GDB. These are called program breakpoints because they
+belong to the program itself.
+
 * 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	17 Apr 2009 18:55:26 -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	17 Apr 2009 18:55:29 -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	17 Apr 2009 18:55:31 -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	17 Apr 2009 18:55:32 -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	17 Apr 2009 18:55:34 -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: */
 
@@ -786,6 +787,7 @@
 {
   int count = 1;
   struct frame_info *frame;
+  struct thread_info *tp = inferior_thread ();
   struct cleanup *cleanups = make_cleanup (null_cleanup, NULL);
   int async_exec = 0;
   int thread = -1;
@@ -821,13 +823,40 @@
 
       make_cleanup (delete_longjmp_breakpoint_cleanup, &thread);
     }
+  if (tp->program_breakpoint_hit && tp->program_breakpoint_size != 0
+      && execution_direction != EXEC_REVERSE && read_pc () == stop_pc
+      && count > 0)
+    {
+      /* "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.  */
+      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 ())
     {
       for (; count > 0; count--)
 	{
-	  struct thread_info *tp;
 	  step_once (skip_subroutines, single_inst, count, thread);
 
 	  if (target_has_execution
Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.368
diff -u -r1.368 infrun.c
--- gdb/infrun.c	14 Apr 2009 00:59:47 -0000	1.368
+++ gdb/infrun.c	17 Apr 2009 18:55:36 -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	17 Apr 2009 18:55:37 -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	17 Apr 2009 18:55:38 -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	17 Apr 2009 18:55:39 -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	17 Apr 2009 18:55:40 -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	17 Apr 2009 18:55:41 -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	17 Apr 2009 18:55:42 -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	17 Apr 2009 18:55:43 -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] 15+ messages in thread

end of thread, other threads:[~2009-05-27  1:49 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-30 18:05 [PATCH] Program Breakpoints Ross Morley
2009-04-30 18:30 ` Pedro Alves
2009-04-30 19:14   ` Ross Morley
2009-05-04 22:49     ` Ross Morley
2009-05-04 23:14       ` Pedro Alves
2009-05-04 23:17         ` Ross Morley
2009-05-13  4:24           ` Ross Morley
  -- strict thread matches above, loose matches on Subject: below --
2009-05-19 17:30 [Fwd: Re: [PATCH] Program Breakpoints] Ross Morley
2009-05-19 18:58 ` Pedro Alves
2009-05-27  1:49   ` [PATCH] Program Breakpoints Ross Morley
2009-04-18  1:51 [Fwd: [PATCH] Program Breakpoints] Ross Morley
2009-04-22  1:22 ` [PATCH] Program Breakpoints Ross Morley
2009-04-22  3:12   ` Eli Zaretskii
2009-04-27  7:07     ` Ross Morley
2009-04-18  1:33 Ross Morley
2009-04-18  6:21 ` Eli Zaretskii
2009-04-18  6:30   ` Eli Zaretskii
2009-04-18  7:58   ` Ross Morley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox