A patch for ia32 hardware watchpoint.

A patch for ia32 hardware watchpoint.

[H . J . Lu]

This is a patch for

http://sourceware.cygnus.com/ml/gdb/2000-q1/msg00564.html

I only did it for Linux since it is not a perfect solution.

Thanks.


---
2000-03-07  H.J. Lu  <hjl@gnu.org>

	* breakpoint.c (insert_breakpoints): Also pass b->number to
	target_insert_watchpoint () if NEED_WATCHPOINT_NUMBER is
	defined. Don't return error if errno is EBUSY.
	(remove_breakpoint): Also pass b->number to
	target_remove_watchpoint () if NEED_WATCHPOINT_NUMBER is
	defined.
	(delete_breakpoint): Call target_delete_watchpoint () for
	watchpoint if NEED_WATCHPOINT_NUMBER is defined.

	* breakpoint.c (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT):
	Modified to only take "struct value *".
	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Likewise.
	(can_use_hardware_watchpoint): Likewise.
	* config/i386/nm-go32.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT):
	Likwise.
	* config/pa/nm-hppah.h (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT):
	Likwise.

	* i386-linux-nat.c (i386_register_u_addr): Copied from
	i386v-nat.c.
	(kernel_u_size): Likewise.
	Copy hardware watchpoint runtines from i386v-nat.c and modify.

	* config/i386/linux.mh (NATDEPFILES): Remove i386v-nat.o.

	* config/i386/nm-linux.h (NEED_WATCHPOINT_NUMBER): Defined.
	(target_insert_watchpoint): Add breakpoint number.
	(target_remove_watchpoint): Likewise.
	(target_delete_watchpoint): New.

	* config/i386/tm-linux.h (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT):
	Also return 1 for long long, double and long double.

Index: breakpoint.c
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/breakpoint.c,v
retrieving revision 1.1.1.8
diff -u -p -r1.1.1.8 breakpoint.c
--- breakpoint.c	2000/03/07 18:42:09	1.1.1.8
+++ breakpoint.c	2000/03/07 18:44:05
@@ -984,7 +984,12 @@ insert_breakpoints ()
 		    else if (b->type == bp_access_watchpoint)
 		      type = hw_access;
 
+#ifdef NEED_WATCHPOINT_NUMBER
+		    val = target_insert_watchpoint (b->number, addr,
+		    				    len, type);
+#else
 		    val = target_insert_watchpoint (addr, len, type);
+#endif
 		    if (val == -1)
 		      {
 			b->inserted = 0;
@@ -1000,8 +1005,11 @@ insert_breakpoints ()
 		remove_breakpoint (b, mark_uninserted);
 		warning ("Could not insert hardware watchpoint %d.",
 			 b->number);
-		val = -1;
-	      }               
+		/* Don't return an error if we fail to insert
+		   a hardware watchpoint due to the limited number
+		   of hardware watchpoints availabel.  */
+		val = (errno == EBUSY) ? 0 : -1;
+	      }
 	  }
 	else
 	  {
@@ -1337,7 +1345,12 @@ remove_breakpoint (b, is)
 	      else if (b->type == bp_access_watchpoint)
 		type = hw_access;
 
+#ifdef NEED_WATCHPOINT_NUMBER
+	      val = target_remove_watchpoint (b->number, addr, len,
+					      type);
+#else
 	      val = target_remove_watchpoint (addr, len, type);
+#endif
 	      if (val == -1)
 		b->inserted = 1;
 	      val = 0;
@@ -5500,13 +5513,13 @@ watch_command_1 (arg, accessflag, from_t
    in hardware return zero.  */
 
 #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
-#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
-    ((BYTE_SIZE) <= (REGISTER_SIZE))
+#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL) \
+    (TYPE_LENGTH (VALUE_TYPE (VAL)) <= (REGISTER_SIZE))
 #endif
 
 #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
-#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
-     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN)
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(VAL) \
+     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL)
 #endif
 
 static int
@@ -5551,10 +5564,7 @@ can_use_hardware_watchpoint (v)
 	    {
 	      /* Ahh, memory we actually used!  Check if we can cover
                  it with hardware watchpoints.  */
-	      CORE_ADDR vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
-	      int       len   = TYPE_LENGTH (VALUE_TYPE (v));
-
-	      if (!TARGET_REGION_OK_FOR_HW_WATCHPOINT (vaddr, len))
+	      if (!TARGET_REGION_OK_FOR_HW_WATCHPOINT (v))
 		return 0;
 	      else
 		found_memory_cnt++;
@@ -6852,6 +6862,11 @@ delete_breakpoint (bpt)
 
   if (bpt->inserted)
     remove_breakpoint (bpt, mark_uninserted);
+
+#ifdef NEED_WATCHPOINT_NUMBER
+  if (bpt->type == bp_hardware_watchpoint)
+    target_delete_watchpoint (bpt->number);
+#endif
 
   if (breakpoint_chain == bpt)
     breakpoint_chain = bpt->next;
Index: i386-linux-nat.c
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/i386-linux-nat.c,v
retrieving revision 1.1.1.5
diff -u -p -r1.1.1.5 i386-linux-nat.c
--- i386-linux-nat.c	2000/03/07 18:42:13	1.1.1.5
+++ i386-linux-nat.c	2000/03/07 19:01:36
@@ -35,6 +35,21 @@
 #include <sys/reg.h>
 #endif
 
+/* FIXME: The following used to be just "#include <sys/debugreg.h>", but
+ * the the Linux 2.1.x kernel and glibc 2.0.x are not in sync; including
+ * <sys/debugreg.h> will result in an error.  With luck, these losers
+ * will get their act together and we can trash this hack in the near future.
+ * --jsm 1998-10-21
+ */
+
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+#ifdef HAVE_ASM_DEBUGREG_H
+#include <asm/debugreg.h>
+#else
+#include <sys/debugreg.h>
+#endif
+#endif
+
 /* On Linux, threads are implemented as pseudo-processes, in which
    case we may be tracing more than one process at a time.  In that
    case, inferior_pid will contain the main process ID and the
@@ -1058,3 +1073,300 @@ _initialize_i386_linux_nat ()
 {
   add_core_fns (&linux_elf_core_fns);
 }
+
+/* blockend is the value of u.u_ar0, and points to the
+ * place where GS is stored
+ */
+
+int
+i386_register_u_addr (blockend, regnum)
+     int blockend;
+     int regnum;
+{
+  struct user u;
+  int fpstate;
+  int ubase;
+
+  ubase = blockend;
+  /* FIXME:  Should have better way to test floating point range */
+  if (regnum >= FP0_REGNUM && regnum <= (FP0_REGNUM + 7))
+    {
+#ifdef KSTKSZ			/* SCO, and others? */
+      ubase += 4 * (SS + 1) - KSTKSZ;
+      fpstate = ubase + ((char *) &u.u_fps.u_fpstate - (char *) &u);
+      return (fpstate + 0x1c + 10 * (regnum - FP0_REGNUM));
+#else
+      fpstate = ubase + ((char *) &u.i387.st_space - (char *) &u);
+      return (fpstate + 10 * (regnum - FP0_REGNUM));
+#endif
+    }
+  else
+    {
+      return (ubase + 4 * regmap[regnum]);
+    }
+
+}
+\f
+int
+kernel_u_size ()
+{
+  return (sizeof (struct user));
+}
+\f
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+
+/* Record the value of the debug control register.  */
+static int debug_control_mirror;
+
+static struct
+{
+  /* Record which address associates with which register.  */
+  CORE_ADDR address;
+  /* Number assigned to distinguish watchpoints.  */
+  int number;
+} address_lookup [DR_LASTADDR - DR_FIRSTADDR + 1];
+
+static int
+i386_insert_aligned_watchpoint PARAMS ((int, int, CORE_ADDR, CORE_ADDR,
+					int, int));
+
+static int
+i386_insert_nonaligned_watchpoint PARAMS ((int, int, CORE_ADDR,
+					   CORE_ADDR, int, int));
+
+/* Insert a watchpoint.  */
+
+int
+i386_insert_watchpoint (pid, num, addr, len, rw)
+     int pid;
+     int num;
+     CORE_ADDR addr;
+     int len;
+     int rw;
+{
+  return i386_insert_aligned_watchpoint (pid, num, addr, addr, len, rw);
+}
+
+static int
+i386_insert_aligned_watchpoint (pid, num, waddr, addr, len, rw)
+     int pid;
+     int num;
+     CORE_ADDR waddr;
+     CORE_ADDR addr;
+     int len;
+     int rw;
+{
+  int i;
+  int read_write_bits, len_bits;
+  int free_debug_register;
+  int register_number;
+
+  switch (len)
+    {
+    case 1: case 2: case 4: case 8: case 10: case 12:
+      break;
+
+    default:
+      /* This is a kludge. x86 only has limited number of hardware
+	 waitpoint registers. If LEN is too big, we will check
+
+	1. If we have seen the same watch point number in
+	   address_lookup, we just return 0 to save the hardware
+	   waitpoint registers for others. We fake it to make gdb
+	   happy.
+	2. Otherwise, return -1 and set errno EBUSY.
+      */
+      for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR + 1; i++)
+	if (address_lookup[i].number == num
+	    && address_lookup[i].address != 0)
+	  return 0;
+
+      errno = EBUSY;
+      return -1;
+      break;
+    }
+
+  /* Look for a free debug register.  */
+  for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
+    {
+      if (address_lookup[i - DR_FIRSTADDR].address == 0
+	  || address_lookup[i - DR_FIRSTADDR].address == addr)
+	break;
+    }
+
+  /* No more debug registers!  */
+  if (i > DR_LASTADDR)
+    {
+      errno = EBUSY;
+      return -1;
+    }
+
+  read_write_bits = (rw & 1) ? DR_RW_READ : DR_RW_WRITE;
+
+  if (len == 1)
+    len_bits = DR_LEN_1;
+  else if (len == 2)
+    {
+      if (addr % 2)
+	return i386_insert_nonaligned_watchpoint (pid, num, waddr,
+						  addr, len, rw);
+      len_bits = DR_LEN_2;
+    }
+
+  else if (len == 4)
+    {
+      if (addr % 4)
+	return i386_insert_nonaligned_watchpoint (pid, num, waddr,
+						  addr, len, rw);
+      len_bits = DR_LEN_4;
+    }
+  else if (len == 8 || len == 10 || len == 12)
+    {
+      /* It should only happen with long long, double or long double.
+	 All should be updated at the same time. We just watch the
+	 first 4 bytes. */
+      len = 4;
+      if (addr % 4)
+	return i386_insert_nonaligned_watchpoint (pid, num, waddr,
+						  addr, len, rw);
+      len_bits = DR_LEN_4;
+    }
+  else
+    return i386_insert_nonaligned_watchpoint (pid, num, waddr, addr,
+					      len, rw);
+  
+  free_debug_register = i;
+  register_number = free_debug_register - DR_FIRSTADDR;
+  debug_control_mirror |=
+    ((read_write_bits | len_bits)
+     << (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * register_number));
+  debug_control_mirror |=
+    (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * register_number));
+  debug_control_mirror |= DR_LOCAL_SLOWDOWN;
+  debug_control_mirror &= ~DR_CONTROL_RESERVED;
+
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[DR_CONTROL]),
+	  debug_control_mirror);
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[free_debug_register]),
+	  addr);
+
+  /* Record where we came from.  */
+  address_lookup[register_number].address = addr;
+  address_lookup[register_number].number = num;
+  return 0;
+}
+
+static int
+i386_insert_nonaligned_watchpoint (pid, num, waddr, addr, len, rw)
+     int pid;
+     int num;
+     CORE_ADDR waddr;
+     CORE_ADDR addr;
+     int len;
+     int rw;
+{
+  int align;
+  int size;
+  int rv;
+
+  static int size_try_array[16] =
+  {
+    1, 1, 1, 1,			/* trying size one */
+    2, 1, 2, 1,			/* trying size two */
+    2, 1, 2, 1,			/* trying size three */
+    4, 1, 2, 1			/* trying size four */
+  };
+
+  rv = 0;
+  while (len > 0)
+    {
+      align = addr % 4;
+      /* Four is the maximum length for 386.  */
+      size = (len > 4) ? 3 : len - 1;
+      size = size_try_array[size * 4 + align];
+
+      rv = i386_insert_aligned_watchpoint (pid, num, waddr, addr,
+					   size, rw);
+      if (rv)
+	{
+	  i386_remove_watchpoint (pid, num, waddr, size);
+	  return rv;
+	}
+      addr += size;
+      len -= size;
+    }
+  return rv;
+}
+
+/* Remove a watchpoint.  */
+
+int
+i386_remove_watchpoint (pid, num, addr, len)
+     int pid;
+     int num;
+     CORE_ADDR addr;
+     int len;
+{
+  int i;
+  int register_number;
+
+  for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
+    {
+      register_number = i - DR_FIRSTADDR;
+      if (address_lookup[register_number].address == addr)
+	{
+	  debug_control_mirror &=
+	    ~(1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * register_number));
+	  address_lookup[register_number].address = 0;
+	  address_lookup[register_number].number = 0;
+	}
+    }
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[DR_CONTROL]),
+	  debug_control_mirror);
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0);
+
+  return 0;
+}
+
+/* Delete a watchpoint.  */
+
+int
+i386_delete_watchpoint (pid, num)
+     int pid;
+     int num;
+{
+  int i;
+  int retval = -1;
+
+  for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR + 1; i++)
+    if (address_lookup[i].number == num
+	&& address_lookup[i].address != 0)
+      {
+	i386_remove_watchpoint (pid, num, address_lookup[i].address, 0);
+	retval = 0;
+      }
+
+  return retval;
+}
+
+/* Check if stopped by a watchpoint.  */
+CORE_ADDR
+i386_stopped_by_watchpoint (pid)
+     int pid;
+{
+  int i;
+  int status;
+
+  status = ptrace (PT_READ_U, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0);
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0);
+
+  for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
+    {
+      if (status & (1 << (i - DR_FIRSTADDR)))
+	return address_lookup[i - DR_FIRSTADDR].address;
+    }
+
+  return 0;
+}
+
+#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */
Index: config/i386/nm-go32.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/i386/nm-go32.h,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 nm-go32.h
--- config/i386/nm-go32.h	2000/03/07 18:42:21	1.1.1.2
+++ config/i386/nm-go32.h	2000/03/07 18:53:48
@@ -44,10 +44,10 @@
 #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
 
 /* Returns non-zero if we can use hardware watchpoints to watch a region
-   whose address is ADDR and whose length is LEN.  */
+   which represents VAL.  */
 
-#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
-	go32_region_ok_for_watchpoint(addr,len)
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(val) \
+	go32_region_ok_for_watchpoint((VALUE_ADDRESS (val) + VALUE_OFFSET (val)),TYPE_LENGTH (VALUE_TYPE (val)))
 extern int go32_region_ok_for_watchpoint (CORE_ADDR, int);
 
 /* After a watchpoint trap, the PC points to the instruction after the
Index: config/i386/linux.mh
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/i386/linux.mh,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 linux.mh
--- config/i386/linux.mh	2000/01/18 17:07:18	1.1.1.2
+++ config/i386/linux.mh	2000/01/18 17:57:36
@@ -1,10 +1,14 @@
 # Host: Intel 386 running GNU/Linux
 
+# We should use the one in glibc 2.
+REGEX=
+REGEX1=
+
 XM_FILE= xm-linux.h
 XDEPFILES= ser-tcp.o
 
 NAT_FILE= nm-linux.h
 NATDEPFILES= infptrace.o solib.o inftarg.o fork-child.o corelow.o \
-	core-aout.o i386v-nat.o i386-linux-nat.o linux-thread.o lin-thread.o
+	core-aout.o i386-linux-nat.o linux-thread.o lin-thread.o
 
 LOADLIBES = -ldl -rdynamic
Index: config/i386/nm-linux.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/i386/nm-linux.h,v
retrieving revision 1.1.1.4
diff -u -p -r1.1.1.4 nm-linux.h
--- config/i386/nm-linux.h	2000/03/07 18:42:21	1.1.1.4
+++ config/i386/nm-linux.h	2000/03/07 18:44:07
@@ -46,14 +46,19 @@ extern int kernel_u_size PARAMS ((void))
 #define STOPPED_BY_WATCHPOINT(W)  \
   i386_stopped_by_watchpoint (inferior_pid)
 
-/* Use these macros for watchpoint insertion/removal.  */
+#define NEED_WATCHPOINT_NUMBER
 
-#define target_insert_watchpoint(addr, len, type)  \
-  i386_insert_watchpoint (inferior_pid, addr, len, type)
+/* Use these macros for watchpoint insertion/removal/deletion.  */
 
-#define target_remove_watchpoint(addr, len, type)  \
-  i386_remove_watchpoint (inferior_pid, addr, len)
+#define target_insert_watchpoint(num, addr, len, type)  \
+  i386_insert_watchpoint (inferior_pid, num, addr, len, type)
 
+#define target_remove_watchpoint(num, addr, len, type)  \
+  i386_remove_watchpoint (inferior_pid, num, addr, len)
+
+#define target_delete_watchpoint(num) \
+  i386_delete_watchpoint (inferior_pid, num)
+
 /* We define this if link.h is available, because with ELF we use SVR4 style
    shared libraries. */
 
@@ -74,9 +79,11 @@ extern int kernel_u_size PARAMS ((void))
 
 extern CORE_ADDR
   i386_stopped_by_watchpoint PARAMS ((int));
+extern int
+  i386_insert_watchpoint PARAMS ((int pid, int num, CORE_ADDR addr, int len, int rw));
 extern int
-i386_insert_watchpoint PARAMS ((int pid, CORE_ADDR addr, int len, int rw));
+  i386_remove_watchpoint PARAMS ((int pid, int num, CORE_ADDR addr, int len));
 extern int
-i386_remove_watchpoint PARAMS ((int pid, CORE_ADDR addr, int len));
+  i386_delete_watchpoint PARAMS ((int pid, int num));
 
 #endif /* #ifndef NM_LINUX_H */
Index: config/i386/tm-linux.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/i386/tm-linux.h,v
retrieving revision 1.1.1.5
diff -u -p -r1.1.1.5 tm-linux.h
--- config/i386/tm-linux.h	2000/03/07 18:42:21	1.1.1.5
+++ config/i386/tm-linux.h	2000/03/07 18:54:47
@@ -166,4 +166,14 @@ extern CORE_ADDR i386_linux_skip_solib_r
    to be relocated. */
 #define SOFUN_ADDRESS_MAYBE_MISSING
 
+/* Does a value fit in a hardware debug register? Although a long long,
+   double or long double doesn't fit in a register, since x86 has to
+   update all bytes at once, it should be ok to just watch the first
+   few bytes. */
+#undef TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT
+#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(v) \
+  (TYPE_LENGTH (VALUE_TYPE (v)) <= REGISTER_SIZE \
+   || TYPE_CODE (VALUE_TYPE (v)) == TYPE_CODE_INT \
+   || TYPE_CODE (VALUE_TYPE (v)) == TYPE_CODE_FLT)
+
 #endif /* #ifndef TM_LINUX_H */
Index: config/pa/nm-hppah.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/pa/nm-hppah.h,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 nm-hppah.h
--- config/pa/nm-hppah.h	2000/01/18 17:07:28	1.1.1.3
+++ config/pa/nm-hppah.h	2000/01/18 17:08:57
@@ -142,7 +142,7 @@ extern int hppa_require_detach PARAMS ((
 /* The PA can also watch memory regions of arbitrary size, since we're using
    a page-protection scheme.  (On some targets, apparently watch registers
    are used, which can only accomodate regions of REGISTER_SIZE.) */
-#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \
+#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(val) \
         (1)
 
 /* However, some addresses may not be profitable to use hardware to watch,

Re: A patch for ia32 hardware watchpoint.

[J.T. Conklin]

>>>>> "hjl" == H J Lu <hjl@valinux.com> writes:
hjl> 2000-03-07  H.J. Lu  <hjl@gnu.org>
hjl> 	* breakpoint.c (insert_breakpoints): Also pass b->number to
hjl> 	target_insert_watchpoint () if NEED_WATCHPOINT_NUMBER is
hjl> 	defined. Don't return error if errno is EBUSY.
hjl> 	(remove_breakpoint): Also pass b->number to
hjl> 	target_remove_watchpoint () if NEED_WATCHPOINT_NUMBER is
hjl> 	defined.
hjl> 	(delete_breakpoint): Call target_delete_watchpoint () for
hjl> 	watchpoint if NEED_WATCHPOINT_NUMBER is defined.

Having the API for target_{insert,remove}_watchpoint() change
depending on whether NEED_BREAKPOINT_NUMBER is defined seems dangerous
to me.  An extra parameter that is ignored by those targets that don't
care about it seems like a better option to me.

I was planning to propose that the breakpoint pointer itself be passed
to the target_{insert,remove}_{break,watch}point() functions, so this
is as good of time as any.  

The reason I want this is so that GDB's target code can record target-
specific info about the breakpoint.  For example, if the target gives
the break/watchpoint an ID, the insert function can record it in the
breakpoint so it is available for use in the delete function.  Right
now, my WDB target code inserts a record into a data structure which
maps WDB's breakpoint IDs with the thread+address when a breakpoint is 
inserted.  The remove code searches for any breakpoint that matches the
thread+address and removes that ID.  

This adds unnecessary complexity, and in the case where two break-
points are inserted at the same address you might end up removing a
different breakpoint than was inserted.  This isn't a problem now, but
I'd also like to be able to support smarter breakpoints.  For example,
breakpoints where the ignore count, or perhaps even some sort of
simple conditions are handled on the target.  In those cases, we'd want
to make sure to delete the same breakpoint that was inserted.

Ok, enough on that tangent.


Can you explain the reasoning about not returning error if errno is
EBUSY.  If there aren't enough watchpoint registers, shouldn't the
watchpoint create itself fail?  I thought TARGET_CAN_USE_HARDWARE_-
WATCHPOINT() was intended to prevent this.  It appears that you're
trying to handle oversubscription of watchpoint resources rather than
preventing such oversubscription in the first place.

Also, while many target vectors report error reasons through errno, 
it leaves a bad taste in my mouth.  Perhaps this is because we have
to map target errors into UNIX/POSIX errnos, and there isn't always
a clean mapping.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: A patch for ia32 hardware watchpoint.

[Todd Whitesel]

> I was planning to propose that the breakpoint pointer itself be passed
> to the target_{insert,remove}_{break,watch}point() functions, so this
> is as good of time as any.  

I say Just Do It. I am sitting on some local code here that tracks
breakpoints added to the target by a third party, and I ended up needing
the breakpoint shadow field to be available to those functions.

In my code I have functions called "target_better_{insert,remove}_breakpoint"
which take a struct breakpoint * and a CORE_ADDR although the CORE_ADDR is
only needed in a couple cases that show up in the overlay support (IIRC).
Previous users of the regular target_[a-z]*_breakpoint functions have been
changed to use the new ones, and I hack the target stack interface (don't
ask) in order to get the information down to where I need it. If the real
target interface passed the breakpoint pointer down, presto, no more hack.

As far as I'm concerned, passing the breakpoint pointer is the right way
to go; we should get away from the assumption that a target side breakpoint
is an address with some #define'd size, and push that stuff into a default
implementation that can be invoked easily by people writing new target
support.

> The reason I want this is so that GDB's target code can record target-
> specific info about the breakpoint.  For example, if the target gives
> the break/watchpoint an ID, the insert function can record it in the
> breakpoint so it is available for use in the delete function.  Right
> now, my WDB target code inserts a record into a data structure which
> maps WDB's breakpoint IDs with the thread+address when a breakpoint is 
> inserted.  The remove code searches for any breakpoint that matches the
> thread+address and removes that ID.  

I'm doing much the same (except it's more complicated for historical
reasons) to handle WTX breakpoints.

Actually I ended up having a bunch of functions to search the breakpoint
list for a given ID and {delete,insert,remove} it. Since other host programs
can delete my breakpoints, I had to be able to react appropriately when the
removal message showed up.

> Also, while many target vectors report error reasons through errno, 
> it leaves a bad taste in my mouth.  Perhaps this is because we have
> to map target errors into UNIX/POSIX errnos, and there isn't always
> a clean mapping.

Yeah. This is about as bad as crunching target events through unix
signals, something that only ever made sense on ptrace() targets
which couldn't do any better. "But the code lives on"

-- 
Todd Whitesel
toddpw @ windriver.com

Re: A patch for ia32 hardware watchpoint.

[Eli Zaretskii]

> This is a patch for
> 
> http://sourceware.cygnus.com/ml/gdb/2000-q1/msg00564.html

I have problems with this patch.  See below.

> I only did it for Linux since it is not a perfect solution.

No, you also changed nm-go32.h, which is not related to Linux (AFAIK),
and changed the global definition of two macros on breakpoint.c.

> +#ifdef NEED_WATCHPOINT_NUMBER
> +		    val = target_insert_watchpoint (b->number, addr,
> +		    				    len, type);
> +#else

Please explain why is this needed.  The DJGPP version works well
without knowing the breakpoint number, but if there's a good reason
for passing b->number, it should be done on all x86 platforms.  So
let's discuss this.

> +		/* Don't return an error if we fail to insert
> +		   a hardware watchpoint due to the limited number
> +		   of hardware watchpoints availabel.  */
> +		val = (errno == EBUSY) ? 0 : -1;
> +	      }

Why is this a good idea?  The result of this is that GDB will not know
that it cannot insert a watchpoint until it actually resumes the
debuggee, which is too late in many cases; and the user gets confusing
error messages.  x86 doesn't have a good way of checking whether the
debug registers are enough to cover the requests, but whenever it
does, why not use it?

> @@ -5500,13 +5513,13 @@ watch_command_1 (arg, accessflag, from_t
>     in hardware return zero.  */
>  
>  #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> -    ((BYTE_SIZE) <= (REGISTER_SIZE))
> +#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL) \
> +    (TYPE_LENGTH (VALUE_TYPE (VAL)) <= (REGISTER_SIZE))
>  #endif
>  
>  #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
> -     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN)
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(VAL) \
> +     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL)
>  #endif

These are IMHO wrong: the number of debug registers required for a
particular region is a function of the address, not only size (e.g., a
single x86 debug register cannot watch a 32-bit region that isn't
aligned on 4-byte boundary).  If Linux, for some reason, doesn't need
the address (although I cannot see how could this be right, at least
for native debugging), please define a platform-specific macro instead
of overwriting system-wide defaults.

The DJGPP version actually *uses* the ADDR part of the above
definition, since it knows how to cover a region with several
watchpoints.

> --- config/i386/nm-go32.h	2000/03/07 18:42:21	1.1.1.2
> +++ config/i386/nm-go32.h	2000/03/07 18:53:48
> @@ -44,10 +44,10 @@
>  #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
>  
>  /* Returns non-zero if we can use hardware watchpoints to watch a region
> -   whose address is ADDR and whose length is LEN.  */
> +   which represents VAL.  */
>  
> -#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
> -	go32_region_ok_for_watchpoint(addr,len)
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(val) \
> +	go32_region_ok_for_watchpoint((VALUE_ADDRESS (val) + VALUE_OFFSET (val)),TYPE_LENGTH (VALUE_TYPE (val)))

Please do not commit this one, it disables a valuable feature in the
DJGPP version.

Re: A patch for ia32 hardware watchpoint.

[Eli Zaretskii]

> > @@ -5500,13 +5513,13 @@ watch_command_1 (arg, accessflag, from_t
> >     in hardware return zero.  */
> >  
> >  #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> > -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> > -    ((BYTE_SIZE) <= (REGISTER_SIZE))
> > +#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL) \
> > +    (TYPE_LENGTH (VALUE_TYPE (VAL)) <= (REGISTER_SIZE))
> >  #endif
> >  
> >  #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> > -#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
> > -     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN)
> > +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(VAL) \
> > +     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL)
> >  #endif
> 
> These are IMHO wrong: the number of debug registers required for a
> particular region is a function of the address, not only size (e.g., a
> single x86 debug register cannot watch a 32-bit region that isn't
> aligned on 4-byte boundary).  If Linux, for some reason, doesn't need
> the address (although I cannot see how could this be right, at least
> for native debugging), please define a platform-specific macro instead
> of overwriting system-wide defaults.

Sorry, I talked too soon.  These changes supply all the required info,
since they pass the entire struct value to the macro.  So they are
okay with me.

I'm terribly sorry for jumping the gun for no reason.

The rest of my comments about these patches are still valid, though.

Re: A patch for ia32 hardware watchpoint.

[Jim Kingdon]

> Yeah. This is about as bad as crunching target events through unix
> signals, something that only ever made sense on ptrace() targets
> which couldn't do any better. "But the code lives on"

Always amusing to hear people talking about my designs :-) (also see
comment in target.h at enum target_signal).

The deep question is whether you want GDB to canonicalize things.  I
do see some value in getting (for example) a SEGV when you access
memory which is not mapped by the MMU (if you have one) across all
targets.  Both for users and for scripts.

If you answer pro-canonical, then the right solution is to add
additional fields (most native platforms have arch-dependent signal
codes, for example, and GDB could be taught to pass them along or
generate them for stubs and such).  I think that ptrace() targets
could get at the signal codes but not necessarily in a clean way
(wait4 and friends don't seem to return them).

If you answer anti-canonical, then you want functions in the target
vector to do things like report stop status to the user.

The two approaches aren't mutually exclusive, actually, we probably
want both a canonicalized status and a way to get more specific
information in a free-form way.

The situation might be analogous for errno, although errno is such a
poor fit for things which happen to targets (e.g. "address out of
range" maps to EIO.  Bletch), that I kind of doubt it is worth the
bother to do the canonicalization thing.  Unless perhaps if we wanted
to come up with a GDB-specific canonical set of errors.

Re: A patch for ia32 hardware watchpoint.

[Todd Whitesel]

> The two approaches aren't mutually exclusive, actually, we probably
> want both a canonicalized status and a way to get more specific
> information in a free-form way.

The numbering scheme I prefer has multiple layers. For each event type
that GDB handles in a generic way, there is a value of TARGET_WAITKIND_*
that is always preferred. For miscellaneous events that GDB doesn't know
how to handle specifically, you have TARGET_WAITKIND_SIGNALLED or
TARGET_WAITKIND_CPU_EXCEPTION, both of which activate a target-defined
auxiliary field that carries the signal/exception type. When this happens:

For unixy targets, you get TARGET_WAITKIND_SIGNALLED and a signal number.
For raw targets, you get TARGET_WAITKIND_CPU_EXCEPTION and a vector number
(or something like it -- on many RISC chips this is not a simple integer).

The reactionary position is:

It is not the job of the generic inferior event stuff to disambiguate
unixy signals like SIGTRAP. That is what unix tdep files are for.

The more reasonable position is:

As early as possible, I want a SIGTRAP to become a TARGET_WAITKIND_BREAKPOINT
so that above some layer I never have to think about SIGTRAP on any target,
except in the case of a spurious SIGTRAP that matches no breakpoint, which
should be treated like any other uncaught signal.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: A patch for ia32 hardware watchpoint.

[Eli Zaretskii]

> As far as I'm concerned, passing the breakpoint pointer is the right way
> to go; we should get away from the assumption that a target side breakpoint
> is an address with some #define'd size, and push that stuff into a default
> implementation that can be invoked easily by people writing new target
> support.

I agree.

But if we do that, I'd also suggest to leave it to the target to
decide whether a particular watchpoint fired or not.

Right now, the API presented by GDB is based solely on the address:
bpstat_stop_status calls target_stopped_data_address and does all the
decision-making based solely on that address (and some info it keeps
internally about each watchpoint).

This API is extremely limited.  Typically, the target knows much more
about the watchpoint which triggered than the generic GDB code does,
so it can make smarter decisions.  But in order to do that, the target
needs more information about the watchpoint, and it needs to pass back
to GDB the result (whether the watchpoint triggered or not), not its
address.

Btw, if we let the target decide whether a given watchpoint triggered,
we can also resolve, once and for all, all the various conflicts
between target-specific limitations of hardware-assisted watchpoints,
which now need to be dealt with on the generic level.

Re: A patch for ia32 hardware watchpoint.

[Andrew Cagney]

Jim Kingdon wrote:
> 
> > Yeah. This is about as bad as crunching target events through unix
> > signals, something that only ever made sense on ptrace() targets
> > which couldn't do any better. "But the code lives on"
> 
> Always amusing to hear people talking about my designs :-) (also see
> comment in target.h at enum target_signal).
> 
> The deep question is whether you want GDB to canonicalize things.  I
> do see some value in getting (for example) a SEGV when you access
> memory which is not mapped by the MMU (if you have one) across all
> targets.  Both for users and for scripts.

Have a poke around the sim directories (mips) where the coders pulled
various nasties because GDB assumes a signal instead of an event.

I think the target should return an arbitrary event that can be queried
for its ``kind'' or poked at to create additional detail.

More importantly, you should be able to edit that event and then throw
it back at the target.

	Andrew

breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
jtc> I was planning to propose that the breakpoint pointer itself be passed
jtc> to the target_{insert,remove}_{break,watch}point() functions, so this
jtc> is as good of time as any.  

Todd> I say Just Do It. I am sitting on some local code here that tracks
Todd> breakpoints added to the target by a third party, and I ended up needing
Todd> the breakpoint shadow field to be available to those functions.

I spent some time yesterday investigating what would be necessary to
change the target_{insert,remove}_breakpoint() API to pass a pointer
to struct breakpoint.

What I have so far is change the API from:
        int foo_insert_breakpoint (CORE_ADDR addr, char *shadow_contents);
to:
        int foo_insert_breakpoint (struct breakpoint *bp, CORE_ADDR addr);

I would have prefered to pass only the breakpoint pointer, but the
overlay support code in breakpoint.c calls target_insert_breakpoint()
twice with different addresses; once for the VMA and once for the LMA.
I don't want to address that, at least not yet.

For the most part, this change was just tedious grunt work.  All those
targets to change...  However, while I was editing my way through GDBs
files, I noticed ARM RDI target also has breakpoints with "handles"
and maintains a local database mapping handles to breakpoint.  I'm
pleased to see this, because this change could be taken advantage by a
target already in the tree.

One issue that I'm not sure how to address is that there are several
places breakpoints are inserted where a breakpoint has not been
constructed.  Most of these occur in tdep code which implements single
step with breakpoints on processors without a trace mode.

For example, from arc-tdep.c:
 
      /* Always set a breakpoint for the insn after the branch.  */
      next_pc = pc + ((type == NORMAL8 || type == BRANCH8) ? 8 : 4);
      target_insert_breakpoint (next_pc, break_mem[0]);

My current thinking about how to handle these cases is to create a new
breakpoint type bp_singlestep and to use set_momentary_breakpoint() to
create "real" breakpoints.  

I'd appreciate if anyone can share their thoughts on this matter.

        --jtc
   
-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Jim Kingdon]

> I noticed ARM RDI target also has breakpoints with "handles" and
> maintains a local database mapping handles to breakpoint.  I'm
> pleased to see this, because this change could be taken advantage by
> a target already in the tree.

So you'd add a void* to the struct breakpoint for the target to do
with what it wants (together with a new deallocation function in the
target vector which delete_breakpoint would call)?

> My current thinking about how to handle these cases is to create a new
> breakpoint type bp_singlestep and to use set_momentary_breakpoint() to
> create "real" breakpoints.  

Well, I can't quite resist mentioning the contrary idea which has been
floated which is to put all the single stepping stuff down into the
wait/resume level (I guess a new level between the existing
handle_inferior_event and friends, and the target vector).  Having
mentioned it I'm not sure an extra level is really warranted here.

So bp_singlestep might work nicely too - of course it would be added
to the list of types which insert_breakpoints doesn't mess with.  And
this has a further consequence - that you can't just detect this as a
normal breakpoint, you need to keep the
singlestep_breakpoints_inserted_p machinery in infrun.c

I realize I haven't really put much of a recommendation in there, but
but it sounds like going ahead with bp_singlestep might be the best
plan.

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
> jtc> I was planning to propose that the breakpoint pointer itself be passed
> jtc> to the target_{insert,remove}_{break,watch}point() functions, so this
> jtc> is as good of time as any.
> 
> Todd> I say Just Do It. I am sitting on some local code here that tracks
> Todd> breakpoints added to the target by a third party, and I ended up needing
> Todd> the breakpoint shadow field to be available to those functions.
> 
> I spent some time yesterday investigating what would be necessary to
> change the target_{insert,remove}_breakpoint() API to pass a pointer
> to struct breakpoint.
> 
> What I have so far is change the API from:
>         int foo_insert_breakpoint (CORE_ADDR addr, char *shadow_contents);
> to:
>         int foo_insert_breakpoint (struct breakpoint *bp, CORE_ADDR addr);

J.T.,

One aspect of this gives me cold feet and sweety palms.  You're giving
the target code access to the entire bp struct.  While I don't have any
problems with handing the code a breakpoint handle, I have strong
reservations towards any moves that give the target unfettered access to
the entire ``struct breakpoint''.  We'll be spending the next 10 years
trying to get control back again :-)

I'd prefer to see something that tightens rather than loosens access to
``struct breakpoint''.  Perhaphs something along the lines of multi-arch
where the target is notified of breakpoint create, insert, remove,
delete operations.

	Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> One aspect of this gives me cold feet and sweety palms.  You're giving
> the target code access to the entire bp struct.  While I don't have any
> problems with handing the code a breakpoint handle, I have strong
> reservations towards any moves that give the target unfettered access to
> the entire ``struct breakpoint''.  We'll be spending the next 10 years
> trying to get control back again :-)
...
> I'd prefer to see something that tightens rather than loosens access to
> ``struct breakpoint''.  Perhaphs something along the lines of multi-arch
> where the target is notified of breakpoint create, insert, remove,
> delete operations.

Basically what I need is the moral equivalent of a "user" field tacked
onto every breakpoint struct.

Based on my existing code, the contents of this field would be:

	UINT32	breakpointID;	/* WTX-supplied handle for this breakpoint */
	bool	is_foreign;	/* true if breakpoint was set by someone else */
	bool	is_global;	/* true if breakpoint active for all tasks */

The is_foreign field is an interesting concept which I don't think anyone
else has implemented; it signifies a breakpoint that GDB does not "own".
One of the events I can receive from the target is "foreign breakpoint
created" and I find a safe time to call "break *<address>" in order to
let the breakpoint show up properly in GDB.

Foreign breakpoints look like normal breakpoints to GDB users except that
if they aren't recreated (new breakpoint struct allocated) by GDB commands,
then they are still marked "foreign" and when GDB exits they will not be
automatically removed. This is because some other command session usually
still exists, which created that breakpoint, and expects it to remain set
unless explicitly deleted by a user.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> One issue that I'm not sure how to address is that there are several
> places breakpoints are inserted where a breakpoint has not been
> constructed.  Most of these occur in tdep code which implements single
> step with breakpoints on processors without a trace mode.

Aiee! Such code is evil and must be destroyed.

One important value of the full breakpoint machinery is that it can help
avoid the same location being patched twice. Any time we patch the same
instruction twice, we must un-patch it in exactly reverse order or else
we leave a breakpoint instruction sitting in the code -- Not Good.

So I would have to argue that the singlestep logic must either happen at a
really low level (this guarantees it will patch last and un-patch first) or
must go through the real breakpoint logic which can do duplicate detection.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
Andrew> One aspect of this gives me cold feet and sweety palms.
Andrew> You're giving the target code access to the entire bp struct.
Andrew> While I don't have any problems with handing the code a
Andrew> breakpoint handle, I have strong reservations towards any
Andrew> moves that give the target unfettered access to the entire
Andrew> ``struct breakpoint''.  We'll be spending the next 10 years
Andrew> trying to get control back again :-)

I appreciate this argument, and agree that the target interface should
be as robust as possible.  After reading your message, I thought about
hiding the fact that the argument is a struct breakpoint pointer and
providing macros to access "public" fields, but any subversion we do
can be undone by a clever programmer.  I'm still working on something 
that is simple, elegant, and efficent.

However, I think it's a bit of a stretch to say such changes in and of
themselves will lead to GDB being out of control.  IMHO, this can only
happen if the GDB maintainers fail to do their jobs and integrate code
that disrupts GDB's architectural integrity.  I'll stipulate that
we've done a rather poor job of this in the past, but I'm hopeful that
attitudes have changed.

Andrew> I'd prefer to see something that tightens rather than loosens
Andrew> access to ``struct breakpoint''.  Perhaphs something along the
Andrew> lines of multi-arch where the target is notified of breakpoint
Andrew> create, insert, remove, delete operations.

I don't quite follow.  Care to elaborate?

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
jtc> One issue that I'm not sure how to address is that there are several
jtc> places breakpoints are inserted where a breakpoint has not been
jtc> constructed.  Most of these occur in tdep code which implements single
jtc> step with breakpoints on processors without a trace mode.

Todd> Aiee! Such code is evil and must be destroyed.

I don't think it's that bad.  

Todd> One important value of the full breakpoint machinery is that it
Todd> can help avoid the same location being patched twice. Any time
Todd> we patch the same instruction twice, we must un-patch it in
Todd> exactly reverse order or else we leave a breakpoint instruction
Todd> sitting in the code -- Not Good.

Todd> So I would have to argue that the singlestep logic must either
Todd> happen at a really low level (this guarantees it will patch last
Todd> and un-patch first) or must go through the real breakpoint logic
Todd> which can do duplicate detection.

The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
it inserts and remove trap instructions without effecting GDB's high-
level breakpoints.  So I think we're OK.  As a result, I wouldn't be
suprised if steping into a breakpoint didn't behave the same as on a
machine with hardware single-step.  

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
Andrew> I'd prefer to see something that tightens rather than loosens
Andrew> access to ``struct breakpoint''.  Perhaphs something along the
Andrew> lines of multi-arch where the target is notified of breakpoint
Andrew> create, insert, remove, delete operations.

Todd> Basically what I need is the moral equivalent of a "user" field
Todd> tacked onto every breakpoint struct.
Todd>
Todd> Based on my existing code, the contents of this field would be:
Todd> 	UINT32	breakpointID; /* WTX-supplied handle for this breakpoint */
Todd> 	bool	is_foreign;   /* true if breakpoint was set by someone else */
Todd> 	bool	is_global;    /* true if breakpoint active for all tasks */

The reason I was passing the breakpoint pointer (with the intention of
adding a "void *private_data" field), is that I don't want to preclude
any target implementation.  A 32 bit integer ID is probably the most
common breakpoint handle, but you never can tell...

I don't think your is_foreign flag is suitable for the target specific
data, because it changes the behavior of generic breakpoint code.  You
didn't describe is_global, so I'm not sure about it.  How is it
different from (bp->thread == -1)?

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> The reason I was passing the breakpoint pointer (with the intention of
> adding a "void *private_data" field), is that I don't want to preclude
> any target implementation.  A 32 bit integer ID is probably the most
> common breakpoint handle, but you never can tell...

I didn't pretend that this was general enough for everyone, I only said
this was what I used. In my case the UINT32 is dictated by the WTX layer
that assigns the ID's.

> I don't think your is_foreign flag is suitable for the target specific
> data, because it changes the behavior of generic breakpoint code.

Hmm, actually yeah that's right. It probably should be generic.

> You didn't describe is_global, so I'm not sure about it.  How is it
> different from (bp->thread == -1)?

It probably isn't, and is more an artifact of my implementation.

The foreign/global stuff was added together, and happened late enough
in a development cycle that I needed to keep it isolated and easy to
compile back out if we had to cancel it. I had two distinct #ifdef's
so I could pinpoint all the changes; I've never taken those out.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
> Andrew> One aspect of this gives me cold feet and sweety palms.
> Andrew> You're giving the target code access to the entire bp struct.
> Andrew> While I don't have any problems with handing the code a
> Andrew> breakpoint handle, I have strong reservations towards any
> Andrew> moves that give the target unfettered access to the entire
> Andrew> ``struct breakpoint''.  We'll be spending the next 10 years
> Andrew> trying to get control back again :-)
> 
> I appreciate this argument, and agree that the target interface should
> be as robust as possible.  After reading your message, I thought about
> hiding the fact that the argument is a struct breakpoint pointer and
> providing macros to access "public" fields, but any subversion we do
> can be undone by a clever programmer.  I'm still working on something
> that is simple, elegant, and efficent.

In the case of gdbarch, everything is manipulated via methods.  Each
legacy macro is backed by a C function.  The programmer does not have
direct access to the gdbarch object.

GDBARCH set a very very high standard (possibly too high).  However,
whats interesting is the problems so far identified have related to
things like defaults and not to the actual interface.

> However, I think it's a bit of a stretch to say such changes in and of
> themselves will lead to GDB being out of control.  IMHO, this can only
> happen if the GDB maintainers fail to do their jobs and integrate code
> that disrupts GDB's architectural integrity.  I'll stipulate that
> we've done a rather poor job of this in the past, but I'm hopeful that
> attitudes have changed.

I'm looking at it from the view of what is involved in making a change
to ``struct breakpoint''.  If targets directly depend on random bits
than that change becomes that much harder.

If target code can poke around with ``struct breakpoint'' internals then
there is that extra bit of pressure to let through that patch which
solves the problem that everyone is concerned about but no one has a
clean fix for.

> Andrew> I'd prefer to see something that tightens rather than loosens
> Andrew> access to ``struct breakpoint''.  Perhaphs something along the
> Andrew> lines of multi-arch where the target is notified of breakpoint
> Andrew> create, insert, remove, delete operations.

Possibly a side issue.  When a new architecture is created.  gdbarch
notifies all interested parties so that they can update their local
architecture specific information.  See gdbtypes.c for a very good
example.

If the target wants to know about breakpoint changes then notify it
(mumble something about the observer pattern).

	Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:

> Todd> So I would have to argue that the singlestep logic must either
> Todd> happen at a really low level (this guarantees it will patch last
> Todd> and un-patch first) or must go through the real breakpoint logic
> Todd> which can do duplicate detection.
> 
> The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
> it inserts and remove trap instructions without effecting GDB's high-
> level breakpoints.  So I think we're OK.  As a result, I wouldn't be
> suprised if steping into a breakpoint didn't behave the same as on a
> machine with hardware single-step.

Its got long-term problems.

The code assumes that it is going to retain control, blocked on the
target, while the actual step is performed.
This doesn't work well within a pure event model where control is ment
to return to the event-loop when ever the target is resumed.

	enjoy,
		Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Richard Earnshaw]

> "J.T. Conklin" wrote:
> 
> > Todd> So I would have to argue that the singlestep logic must either
> > Todd> happen at a really low level (this guarantees it will patch last
> > Todd> and un-patch first) or must go through the real breakpoint logic
> > Todd> which can do duplicate detection.
> > 
> > The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
> > it inserts and remove trap instructions without effecting GDB's high-
> > level breakpoints.  So I think we're OK.  As a result, I wouldn't be
> > suprised if steping into a breakpoint didn't behave the same as on a
> > machine with hardware single-step.
> 
> Its got long-term problems.
> 
> The code assumes that it is going to retain control, blocked on the
> target, while the actual step is performed.
> This doesn't work well within a pure event model where control is ment
> to return to the event-loop when ever the target is resumed.

It doesn't work properly with INSTRUCTION_NULLIFIED either -- at least it 
didn't last time I tried it...

R.

Re: A patch for ia32 hardware watchpoint.

[Eli Zaretskii]

> This is a patch for
> 
> http://sourceware.cygnus.com/ml/gdb/2000-q1/msg00564.html

I have problems with this patch.  See below.

> I only did it for Linux since it is not a perfect solution.

No, you also changed nm-go32.h, which is not related to Linux (AFAIK),
and changed the global definition of two macros on breakpoint.c.

> +#ifdef NEED_WATCHPOINT_NUMBER
> +		    val = target_insert_watchpoint (b->number, addr,
> +		    				    len, type);
> +#else

Please explain why is this needed.  The DJGPP version works well
without knowing the breakpoint number, but if there's a good reason
for passing b->number, it should be done on all x86 platforms.  So
let's discuss this.

> +		/* Don't return an error if we fail to insert
> +		   a hardware watchpoint due to the limited number
> +		   of hardware watchpoints availabel.  */
> +		val = (errno == EBUSY) ? 0 : -1;
> +	      }

Why is this a good idea?  The result of this is that GDB will not know
that it cannot insert a watchpoint until it actually resumes the
debuggee, which is too late in many cases; and the user gets confusing
error messages.  x86 doesn't have a good way of checking whether the
debug registers are enough to cover the requests, but whenever it
does, why not use it?

> @@ -5500,13 +5513,13 @@ watch_command_1 (arg, accessflag, from_t
>     in hardware return zero.  */
>  
>  #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> -    ((BYTE_SIZE) <= (REGISTER_SIZE))
> +#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL) \
> +    (TYPE_LENGTH (VALUE_TYPE (VAL)) <= (REGISTER_SIZE))
>  #endif
>  
>  #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
> -     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN)
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(VAL) \
> +     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL)
>  #endif

These are IMHO wrong: the number of debug registers required for a
particular region is a function of the address, not only size (e.g., a
single x86 debug register cannot watch a 32-bit region that isn't
aligned on 4-byte boundary).  If Linux, for some reason, doesn't need
the address (although I cannot see how could this be right, at least
for native debugging), please define a platform-specific macro instead
of overwriting system-wide defaults.

The DJGPP version actually *uses* the ADDR part of the above
definition, since it knows how to cover a region with several
watchpoints.

> --- config/i386/nm-go32.h	2000/03/07 18:42:21	1.1.1.2
> +++ config/i386/nm-go32.h	2000/03/07 18:53:48
> @@ -44,10 +44,10 @@
>  #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
>  
>  /* Returns non-zero if we can use hardware watchpoints to watch a region
> -   whose address is ADDR and whose length is LEN.  */
> +   which represents VAL.  */
>  
> -#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
> -	go32_region_ok_for_watchpoint(addr,len)
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(val) \
> +	go32_region_ok_for_watchpoint((VALUE_ADDRESS (val) + VALUE_OFFSET (val)),TYPE_LENGTH (VALUE_TYPE (val)))

Please do not commit this one, it disables a valuable feature in the
DJGPP version.

Re: A patch for ia32 hardware watchpoint.

[Todd Whitesel]

> The two approaches aren't mutually exclusive, actually, we probably
> want both a canonicalized status and a way to get more specific
> information in a free-form way.

The numbering scheme I prefer has multiple layers. For each event type
that GDB handles in a generic way, there is a value of TARGET_WAITKIND_*
that is always preferred. For miscellaneous events that GDB doesn't know
how to handle specifically, you have TARGET_WAITKIND_SIGNALLED or
TARGET_WAITKIND_CPU_EXCEPTION, both of which activate a target-defined
auxiliary field that carries the signal/exception type. When this happens:

For unixy targets, you get TARGET_WAITKIND_SIGNALLED and a signal number.
For raw targets, you get TARGET_WAITKIND_CPU_EXCEPTION and a vector number
(or something like it -- on many RISC chips this is not a simple integer).

The reactionary position is:

It is not the job of the generic inferior event stuff to disambiguate
unixy signals like SIGTRAP. That is what unix tdep files are for.

The more reasonable position is:

As early as possible, I want a SIGTRAP to become a TARGET_WAITKIND_BREAKPOINT
so that above some layer I never have to think about SIGTRAP on any target,
except in the case of a spurious SIGTRAP that matches no breakpoint, which
should be treated like any other uncaught signal.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Jim Kingdon]

> I noticed ARM RDI target also has breakpoints with "handles" and
> maintains a local database mapping handles to breakpoint.  I'm
> pleased to see this, because this change could be taken advantage by
> a target already in the tree.

So you'd add a void* to the struct breakpoint for the target to do
with what it wants (together with a new deallocation function in the
target vector which delete_breakpoint would call)?

> My current thinking about how to handle these cases is to create a new
> breakpoint type bp_singlestep and to use set_momentary_breakpoint() to
> create "real" breakpoints.  

Well, I can't quite resist mentioning the contrary idea which has been
floated which is to put all the single stepping stuff down into the
wait/resume level (I guess a new level between the existing
handle_inferior_event and friends, and the target vector).  Having
mentioned it I'm not sure an extra level is really warranted here.

So bp_singlestep might work nicely too - of course it would be added
to the list of types which insert_breakpoints doesn't mess with.  And
this has a further consequence - that you can't just detect this as a
normal breakpoint, you need to keep the
singlestep_breakpoints_inserted_p machinery in infrun.c

I realize I haven't really put much of a recommendation in there, but
but it sounds like going ahead with bp_singlestep might be the best
plan.

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
> jtc> I was planning to propose that the breakpoint pointer itself be passed
> jtc> to the target_{insert,remove}_{break,watch}point() functions, so this
> jtc> is as good of time as any.
> 
> Todd> I say Just Do It. I am sitting on some local code here that tracks
> Todd> breakpoints added to the target by a third party, and I ended up needing
> Todd> the breakpoint shadow field to be available to those functions.
> 
> I spent some time yesterday investigating what would be necessary to
> change the target_{insert,remove}_breakpoint() API to pass a pointer
> to struct breakpoint.
> 
> What I have so far is change the API from:
>         int foo_insert_breakpoint (CORE_ADDR addr, char *shadow_contents);
> to:
>         int foo_insert_breakpoint (struct breakpoint *bp, CORE_ADDR addr);

J.T.,

One aspect of this gives me cold feet and sweety palms.  You're giving
the target code access to the entire bp struct.  While I don't have any
problems with handing the code a breakpoint handle, I have strong
reservations towards any moves that give the target unfettered access to
the entire ``struct breakpoint''.  We'll be spending the next 10 years
trying to get control back again :-)

I'd prefer to see something that tightens rather than loosens access to
``struct breakpoint''.  Perhaphs something along the lines of multi-arch
where the target is notified of breakpoint create, insert, remove,
delete operations.

	Andrew

breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
jtc> I was planning to propose that the breakpoint pointer itself be passed
jtc> to the target_{insert,remove}_{break,watch}point() functions, so this
jtc> is as good of time as any.  

Todd> I say Just Do It. I am sitting on some local code here that tracks
Todd> breakpoints added to the target by a third party, and I ended up needing
Todd> the breakpoint shadow field to be available to those functions.

I spent some time yesterday investigating what would be necessary to
change the target_{insert,remove}_breakpoint() API to pass a pointer
to struct breakpoint.

What I have so far is change the API from:
        int foo_insert_breakpoint (CORE_ADDR addr, char *shadow_contents);
to:
        int foo_insert_breakpoint (struct breakpoint *bp, CORE_ADDR addr);

I would have prefered to pass only the breakpoint pointer, but the
overlay support code in breakpoint.c calls target_insert_breakpoint()
twice with different addresses; once for the VMA and once for the LMA.
I don't want to address that, at least not yet.

For the most part, this change was just tedious grunt work.  All those
targets to change...  However, while I was editing my way through GDBs
files, I noticed ARM RDI target also has breakpoints with "handles"
and maintains a local database mapping handles to breakpoint.  I'm
pleased to see this, because this change could be taken advantage by a
target already in the tree.

One issue that I'm not sure how to address is that there are several
places breakpoints are inserted where a breakpoint has not been
constructed.  Most of these occur in tdep code which implements single
step with breakpoints on processors without a trace mode.

For example, from arc-tdep.c:
 
      /* Always set a breakpoint for the insn after the branch.  */
      next_pc = pc + ((type == NORMAL8 || type == BRANCH8) ? 8 : 4);
      target_insert_breakpoint (next_pc, break_mem[0]);

My current thinking about how to handle these cases is to create a new
breakpoint type bp_singlestep and to use set_momentary_breakpoint() to
create "real" breakpoints.  

I'd appreciate if anyone can share their thoughts on this matter.

        --jtc
   
-- 
J.T. Conklin
RedBack Networks

Re: A patch for ia32 hardware watchpoint.

[J.T. Conklin]

>>>>> "hjl" == H J Lu <hjl@valinux.com> writes:
hjl> 2000-03-07  H.J. Lu  <hjl@gnu.org>
hjl> 	* breakpoint.c (insert_breakpoints): Also pass b->number to
hjl> 	target_insert_watchpoint () if NEED_WATCHPOINT_NUMBER is
hjl> 	defined. Don't return error if errno is EBUSY.
hjl> 	(remove_breakpoint): Also pass b->number to
hjl> 	target_remove_watchpoint () if NEED_WATCHPOINT_NUMBER is
hjl> 	defined.
hjl> 	(delete_breakpoint): Call target_delete_watchpoint () for
hjl> 	watchpoint if NEED_WATCHPOINT_NUMBER is defined.

Having the API for target_{insert,remove}_watchpoint() change
depending on whether NEED_BREAKPOINT_NUMBER is defined seems dangerous
to me.  An extra parameter that is ignored by those targets that don't
care about it seems like a better option to me.

I was planning to propose that the breakpoint pointer itself be passed
to the target_{insert,remove}_{break,watch}point() functions, so this
is as good of time as any.  

The reason I want this is so that GDB's target code can record target-
specific info about the breakpoint.  For example, if the target gives
the break/watchpoint an ID, the insert function can record it in the
breakpoint so it is available for use in the delete function.  Right
now, my WDB target code inserts a record into a data structure which
maps WDB's breakpoint IDs with the thread+address when a breakpoint is 
inserted.  The remove code searches for any breakpoint that matches the
thread+address and removes that ID.  

This adds unnecessary complexity, and in the case where two break-
points are inserted at the same address you might end up removing a
different breakpoint than was inserted.  This isn't a problem now, but
I'd also like to be able to support smarter breakpoints.  For example,
breakpoints where the ignore count, or perhaps even some sort of
simple conditions are handled on the target.  In those cases, we'd want
to make sure to delete the same breakpoint that was inserted.

Ok, enough on that tangent.


Can you explain the reasoning about not returning error if errno is
EBUSY.  If there aren't enough watchpoint registers, shouldn't the
watchpoint create itself fail?  I thought TARGET_CAN_USE_HARDWARE_-
WATCHPOINT() was intended to prevent this.  It appears that you're
trying to handle oversubscription of watchpoint resources rather than
preventing such oversubscription in the first place.

Also, while many target vectors report error reasons through errno, 
it leaves a bad taste in my mouth.  Perhaps this is because we have
to map target errors into UNIX/POSIX errnos, and there isn't always
a clean mapping.

        --jtc

-- 
J.T. Conklin
RedBack Networks

A patch for ia32 hardware watchpoint.

[H . J . Lu]

This is a patch for

http://sourceware.cygnus.com/ml/gdb/2000-q1/msg00564.html

I only did it for Linux since it is not a perfect solution.

Thanks.


---
2000-03-07  H.J. Lu  <hjl@gnu.org>

	* breakpoint.c (insert_breakpoints): Also pass b->number to
	target_insert_watchpoint () if NEED_WATCHPOINT_NUMBER is
	defined. Don't return error if errno is EBUSY.
	(remove_breakpoint): Also pass b->number to
	target_remove_watchpoint () if NEED_WATCHPOINT_NUMBER is
	defined.
	(delete_breakpoint): Call target_delete_watchpoint () for
	watchpoint if NEED_WATCHPOINT_NUMBER is defined.

	* breakpoint.c (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT):
	Modified to only take "struct value *".
	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Likewise.
	(can_use_hardware_watchpoint): Likewise.
	* config/i386/nm-go32.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT):
	Likwise.
	* config/pa/nm-hppah.h (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT):
	Likwise.

	* i386-linux-nat.c (i386_register_u_addr): Copied from
	i386v-nat.c.
	(kernel_u_size): Likewise.
	Copy hardware watchpoint runtines from i386v-nat.c and modify.

	* config/i386/linux.mh (NATDEPFILES): Remove i386v-nat.o.

	* config/i386/nm-linux.h (NEED_WATCHPOINT_NUMBER): Defined.
	(target_insert_watchpoint): Add breakpoint number.
	(target_remove_watchpoint): Likewise.
	(target_delete_watchpoint): New.

	* config/i386/tm-linux.h (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT):
	Also return 1 for long long, double and long double.

Index: breakpoint.c
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/breakpoint.c,v
retrieving revision 1.1.1.8
diff -u -p -r1.1.1.8 breakpoint.c
--- breakpoint.c	2000/03/07 18:42:09	1.1.1.8
+++ breakpoint.c	2000/03/07 18:44:05
@@ -984,7 +984,12 @@ insert_breakpoints ()
 		    else if (b->type == bp_access_watchpoint)
 		      type = hw_access;
 
+#ifdef NEED_WATCHPOINT_NUMBER
+		    val = target_insert_watchpoint (b->number, addr,
+		    				    len, type);
+#else
 		    val = target_insert_watchpoint (addr, len, type);
+#endif
 		    if (val == -1)
 		      {
 			b->inserted = 0;
@@ -1000,8 +1005,11 @@ insert_breakpoints ()
 		remove_breakpoint (b, mark_uninserted);
 		warning ("Could not insert hardware watchpoint %d.",
 			 b->number);
-		val = -1;
-	      }               
+		/* Don't return an error if we fail to insert
+		   a hardware watchpoint due to the limited number
+		   of hardware watchpoints availabel.  */
+		val = (errno == EBUSY) ? 0 : -1;
+	      }
 	  }
 	else
 	  {
@@ -1337,7 +1345,12 @@ remove_breakpoint (b, is)
 	      else if (b->type == bp_access_watchpoint)
 		type = hw_access;
 
+#ifdef NEED_WATCHPOINT_NUMBER
+	      val = target_remove_watchpoint (b->number, addr, len,
+					      type);
+#else
 	      val = target_remove_watchpoint (addr, len, type);
+#endif
 	      if (val == -1)
 		b->inserted = 1;
 	      val = 0;
@@ -5500,13 +5513,13 @@ watch_command_1 (arg, accessflag, from_t
    in hardware return zero.  */
 
 #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
-#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
-    ((BYTE_SIZE) <= (REGISTER_SIZE))
+#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL) \
+    (TYPE_LENGTH (VALUE_TYPE (VAL)) <= (REGISTER_SIZE))
 #endif
 
 #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
-#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
-     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN)
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(VAL) \
+     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL)
 #endif
 
 static int
@@ -5551,10 +5564,7 @@ can_use_hardware_watchpoint (v)
 	    {
 	      /* Ahh, memory we actually used!  Check if we can cover
                  it with hardware watchpoints.  */
-	      CORE_ADDR vaddr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
-	      int       len   = TYPE_LENGTH (VALUE_TYPE (v));
-
-	      if (!TARGET_REGION_OK_FOR_HW_WATCHPOINT (vaddr, len))
+	      if (!TARGET_REGION_OK_FOR_HW_WATCHPOINT (v))
 		return 0;
 	      else
 		found_memory_cnt++;
@@ -6852,6 +6862,11 @@ delete_breakpoint (bpt)
 
   if (bpt->inserted)
     remove_breakpoint (bpt, mark_uninserted);
+
+#ifdef NEED_WATCHPOINT_NUMBER
+  if (bpt->type == bp_hardware_watchpoint)
+    target_delete_watchpoint (bpt->number);
+#endif
 
   if (breakpoint_chain == bpt)
     breakpoint_chain = bpt->next;
Index: i386-linux-nat.c
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/i386-linux-nat.c,v
retrieving revision 1.1.1.5
diff -u -p -r1.1.1.5 i386-linux-nat.c
--- i386-linux-nat.c	2000/03/07 18:42:13	1.1.1.5
+++ i386-linux-nat.c	2000/03/07 19:01:36
@@ -35,6 +35,21 @@
 #include <sys/reg.h>
 #endif
 
+/* FIXME: The following used to be just "#include <sys/debugreg.h>", but
+ * the the Linux 2.1.x kernel and glibc 2.0.x are not in sync; including
+ * <sys/debugreg.h> will result in an error.  With luck, these losers
+ * will get their act together and we can trash this hack in the near future.
+ * --jsm 1998-10-21
+ */
+
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+#ifdef HAVE_ASM_DEBUGREG_H
+#include <asm/debugreg.h>
+#else
+#include <sys/debugreg.h>
+#endif
+#endif
+
 /* On Linux, threads are implemented as pseudo-processes, in which
    case we may be tracing more than one process at a time.  In that
    case, inferior_pid will contain the main process ID and the
@@ -1058,3 +1073,300 @@ _initialize_i386_linux_nat ()
 {
   add_core_fns (&linux_elf_core_fns);
 }
+
+/* blockend is the value of u.u_ar0, and points to the
+ * place where GS is stored
+ */
+
+int
+i386_register_u_addr (blockend, regnum)
+     int blockend;
+     int regnum;
+{
+  struct user u;
+  int fpstate;
+  int ubase;
+
+  ubase = blockend;
+  /* FIXME:  Should have better way to test floating point range */
+  if (regnum >= FP0_REGNUM && regnum <= (FP0_REGNUM + 7))
+    {
+#ifdef KSTKSZ			/* SCO, and others? */
+      ubase += 4 * (SS + 1) - KSTKSZ;
+      fpstate = ubase + ((char *) &u.u_fps.u_fpstate - (char *) &u);
+      return (fpstate + 0x1c + 10 * (regnum - FP0_REGNUM));
+#else
+      fpstate = ubase + ((char *) &u.i387.st_space - (char *) &u);
+      return (fpstate + 10 * (regnum - FP0_REGNUM));
+#endif
+    }
+  else
+    {
+      return (ubase + 4 * regmap[regnum]);
+    }
+
+}
+\f
+int
+kernel_u_size ()
+{
+  return (sizeof (struct user));
+}
+\f
+#ifdef TARGET_HAS_HARDWARE_WATCHPOINTS
+
+/* Record the value of the debug control register.  */
+static int debug_control_mirror;
+
+static struct
+{
+  /* Record which address associates with which register.  */
+  CORE_ADDR address;
+  /* Number assigned to distinguish watchpoints.  */
+  int number;
+} address_lookup [DR_LASTADDR - DR_FIRSTADDR + 1];
+
+static int
+i386_insert_aligned_watchpoint PARAMS ((int, int, CORE_ADDR, CORE_ADDR,
+					int, int));
+
+static int
+i386_insert_nonaligned_watchpoint PARAMS ((int, int, CORE_ADDR,
+					   CORE_ADDR, int, int));
+
+/* Insert a watchpoint.  */
+
+int
+i386_insert_watchpoint (pid, num, addr, len, rw)
+     int pid;
+     int num;
+     CORE_ADDR addr;
+     int len;
+     int rw;
+{
+  return i386_insert_aligned_watchpoint (pid, num, addr, addr, len, rw);
+}
+
+static int
+i386_insert_aligned_watchpoint (pid, num, waddr, addr, len, rw)
+     int pid;
+     int num;
+     CORE_ADDR waddr;
+     CORE_ADDR addr;
+     int len;
+     int rw;
+{
+  int i;
+  int read_write_bits, len_bits;
+  int free_debug_register;
+  int register_number;
+
+  switch (len)
+    {
+    case 1: case 2: case 4: case 8: case 10: case 12:
+      break;
+
+    default:
+      /* This is a kludge. x86 only has limited number of hardware
+	 waitpoint registers. If LEN is too big, we will check
+
+	1. If we have seen the same watch point number in
+	   address_lookup, we just return 0 to save the hardware
+	   waitpoint registers for others. We fake it to make gdb
+	   happy.
+	2. Otherwise, return -1 and set errno EBUSY.
+      */
+      for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR + 1; i++)
+	if (address_lookup[i].number == num
+	    && address_lookup[i].address != 0)
+	  return 0;
+
+      errno = EBUSY;
+      return -1;
+      break;
+    }
+
+  /* Look for a free debug register.  */
+  for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
+    {
+      if (address_lookup[i - DR_FIRSTADDR].address == 0
+	  || address_lookup[i - DR_FIRSTADDR].address == addr)
+	break;
+    }
+
+  /* No more debug registers!  */
+  if (i > DR_LASTADDR)
+    {
+      errno = EBUSY;
+      return -1;
+    }
+
+  read_write_bits = (rw & 1) ? DR_RW_READ : DR_RW_WRITE;
+
+  if (len == 1)
+    len_bits = DR_LEN_1;
+  else if (len == 2)
+    {
+      if (addr % 2)
+	return i386_insert_nonaligned_watchpoint (pid, num, waddr,
+						  addr, len, rw);
+      len_bits = DR_LEN_2;
+    }
+
+  else if (len == 4)
+    {
+      if (addr % 4)
+	return i386_insert_nonaligned_watchpoint (pid, num, waddr,
+						  addr, len, rw);
+      len_bits = DR_LEN_4;
+    }
+  else if (len == 8 || len == 10 || len == 12)
+    {
+      /* It should only happen with long long, double or long double.
+	 All should be updated at the same time. We just watch the
+	 first 4 bytes. */
+      len = 4;
+      if (addr % 4)
+	return i386_insert_nonaligned_watchpoint (pid, num, waddr,
+						  addr, len, rw);
+      len_bits = DR_LEN_4;
+    }
+  else
+    return i386_insert_nonaligned_watchpoint (pid, num, waddr, addr,
+					      len, rw);
+  
+  free_debug_register = i;
+  register_number = free_debug_register - DR_FIRSTADDR;
+  debug_control_mirror |=
+    ((read_write_bits | len_bits)
+     << (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * register_number));
+  debug_control_mirror |=
+    (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * register_number));
+  debug_control_mirror |= DR_LOCAL_SLOWDOWN;
+  debug_control_mirror &= ~DR_CONTROL_RESERVED;
+
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[DR_CONTROL]),
+	  debug_control_mirror);
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[free_debug_register]),
+	  addr);
+
+  /* Record where we came from.  */
+  address_lookup[register_number].address = addr;
+  address_lookup[register_number].number = num;
+  return 0;
+}
+
+static int
+i386_insert_nonaligned_watchpoint (pid, num, waddr, addr, len, rw)
+     int pid;
+     int num;
+     CORE_ADDR waddr;
+     CORE_ADDR addr;
+     int len;
+     int rw;
+{
+  int align;
+  int size;
+  int rv;
+
+  static int size_try_array[16] =
+  {
+    1, 1, 1, 1,			/* trying size one */
+    2, 1, 2, 1,			/* trying size two */
+    2, 1, 2, 1,			/* trying size three */
+    4, 1, 2, 1			/* trying size four */
+  };
+
+  rv = 0;
+  while (len > 0)
+    {
+      align = addr % 4;
+      /* Four is the maximum length for 386.  */
+      size = (len > 4) ? 3 : len - 1;
+      size = size_try_array[size * 4 + align];
+
+      rv = i386_insert_aligned_watchpoint (pid, num, waddr, addr,
+					   size, rw);
+      if (rv)
+	{
+	  i386_remove_watchpoint (pid, num, waddr, size);
+	  return rv;
+	}
+      addr += size;
+      len -= size;
+    }
+  return rv;
+}
+
+/* Remove a watchpoint.  */
+
+int
+i386_remove_watchpoint (pid, num, addr, len)
+     int pid;
+     int num;
+     CORE_ADDR addr;
+     int len;
+{
+  int i;
+  int register_number;
+
+  for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
+    {
+      register_number = i - DR_FIRSTADDR;
+      if (address_lookup[register_number].address == addr)
+	{
+	  debug_control_mirror &=
+	    ~(1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * register_number));
+	  address_lookup[register_number].address = 0;
+	  address_lookup[register_number].number = 0;
+	}
+    }
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[DR_CONTROL]),
+	  debug_control_mirror);
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0);
+
+  return 0;
+}
+
+/* Delete a watchpoint.  */
+
+int
+i386_delete_watchpoint (pid, num)
+     int pid;
+     int num;
+{
+  int i;
+  int retval = -1;
+
+  for (i = 0; i < DR_LASTADDR - DR_FIRSTADDR + 1; i++)
+    if (address_lookup[i].number == num
+	&& address_lookup[i].address != 0)
+      {
+	i386_remove_watchpoint (pid, num, address_lookup[i].address, 0);
+	retval = 0;
+      }
+
+  return retval;
+}
+
+/* Check if stopped by a watchpoint.  */
+CORE_ADDR
+i386_stopped_by_watchpoint (pid)
+     int pid;
+{
+  int i;
+  int status;
+
+  status = ptrace (PT_READ_U, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0);
+  ptrace (PT_WRITE_U, pid, offsetof (struct user, u_debugreg[DR_STATUS]), 0);
+
+  for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
+    {
+      if (status & (1 << (i - DR_FIRSTADDR)))
+	return address_lookup[i - DR_FIRSTADDR].address;
+    }
+
+  return 0;
+}
+
+#endif /* TARGET_HAS_HARDWARE_WATCHPOINTS */
Index: config/i386/nm-go32.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/i386/nm-go32.h,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 nm-go32.h
--- config/i386/nm-go32.h	2000/03/07 18:42:21	1.1.1.2
+++ config/i386/nm-go32.h	2000/03/07 18:53:48
@@ -44,10 +44,10 @@
 #define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
 
 /* Returns non-zero if we can use hardware watchpoints to watch a region
-   whose address is ADDR and whose length is LEN.  */
+   which represents VAL.  */
 
-#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr,len) \
-	go32_region_ok_for_watchpoint(addr,len)
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(val) \
+	go32_region_ok_for_watchpoint((VALUE_ADDRESS (val) + VALUE_OFFSET (val)),TYPE_LENGTH (VALUE_TYPE (val)))
 extern int go32_region_ok_for_watchpoint (CORE_ADDR, int);
 
 /* After a watchpoint trap, the PC points to the instruction after the
Index: config/i386/linux.mh
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/i386/linux.mh,v
retrieving revision 1.1.1.2
diff -u -p -r1.1.1.2 linux.mh
--- config/i386/linux.mh	2000/01/18 17:07:18	1.1.1.2
+++ config/i386/linux.mh	2000/01/18 17:57:36
@@ -1,10 +1,14 @@
 # Host: Intel 386 running GNU/Linux
 
+# We should use the one in glibc 2.
+REGEX=
+REGEX1=
+
 XM_FILE= xm-linux.h
 XDEPFILES= ser-tcp.o
 
 NAT_FILE= nm-linux.h
 NATDEPFILES= infptrace.o solib.o inftarg.o fork-child.o corelow.o \
-	core-aout.o i386v-nat.o i386-linux-nat.o linux-thread.o lin-thread.o
+	core-aout.o i386-linux-nat.o linux-thread.o lin-thread.o
 
 LOADLIBES = -ldl -rdynamic
Index: config/i386/nm-linux.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/i386/nm-linux.h,v
retrieving revision 1.1.1.4
diff -u -p -r1.1.1.4 nm-linux.h
--- config/i386/nm-linux.h	2000/03/07 18:42:21	1.1.1.4
+++ config/i386/nm-linux.h	2000/03/07 18:44:07
@@ -46,14 +46,19 @@ extern int kernel_u_size PARAMS ((void))
 #define STOPPED_BY_WATCHPOINT(W)  \
   i386_stopped_by_watchpoint (inferior_pid)
 
-/* Use these macros for watchpoint insertion/removal.  */
+#define NEED_WATCHPOINT_NUMBER
 
-#define target_insert_watchpoint(addr, len, type)  \
-  i386_insert_watchpoint (inferior_pid, addr, len, type)
+/* Use these macros for watchpoint insertion/removal/deletion.  */
 
-#define target_remove_watchpoint(addr, len, type)  \
-  i386_remove_watchpoint (inferior_pid, addr, len)
+#define target_insert_watchpoint(num, addr, len, type)  \
+  i386_insert_watchpoint (inferior_pid, num, addr, len, type)
 
+#define target_remove_watchpoint(num, addr, len, type)  \
+  i386_remove_watchpoint (inferior_pid, num, addr, len)
+
+#define target_delete_watchpoint(num) \
+  i386_delete_watchpoint (inferior_pid, num)
+
 /* We define this if link.h is available, because with ELF we use SVR4 style
    shared libraries. */
 
@@ -74,9 +79,11 @@ extern int kernel_u_size PARAMS ((void))
 
 extern CORE_ADDR
   i386_stopped_by_watchpoint PARAMS ((int));
+extern int
+  i386_insert_watchpoint PARAMS ((int pid, int num, CORE_ADDR addr, int len, int rw));
 extern int
-i386_insert_watchpoint PARAMS ((int pid, CORE_ADDR addr, int len, int rw));
+  i386_remove_watchpoint PARAMS ((int pid, int num, CORE_ADDR addr, int len));
 extern int
-i386_remove_watchpoint PARAMS ((int pid, CORE_ADDR addr, int len));
+  i386_delete_watchpoint PARAMS ((int pid, int num));
 
 #endif /* #ifndef NM_LINUX_H */
Index: config/i386/tm-linux.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/i386/tm-linux.h,v
retrieving revision 1.1.1.5
diff -u -p -r1.1.1.5 tm-linux.h
--- config/i386/tm-linux.h	2000/03/07 18:42:21	1.1.1.5
+++ config/i386/tm-linux.h	2000/03/07 18:54:47
@@ -166,4 +166,14 @@ extern CORE_ADDR i386_linux_skip_solib_r
    to be relocated. */
 #define SOFUN_ADDRESS_MAYBE_MISSING
 
+/* Does a value fit in a hardware debug register? Although a long long,
+   double or long double doesn't fit in a register, since x86 has to
+   update all bytes at once, it should be ok to just watch the first
+   few bytes. */
+#undef TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT
+#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(v) \
+  (TYPE_LENGTH (VALUE_TYPE (v)) <= REGISTER_SIZE \
+   || TYPE_CODE (VALUE_TYPE (v)) == TYPE_CODE_INT \
+   || TYPE_CODE (VALUE_TYPE (v)) == TYPE_CODE_FLT)
+
 #endif /* #ifndef TM_LINUX_H */
Index: config/pa/nm-hppah.h
===================================================================
RCS file: /work/cvs/gnu/gdb/gdb/config/pa/nm-hppah.h,v
retrieving revision 1.1.1.3
diff -u -p -r1.1.1.3 nm-hppah.h
--- config/pa/nm-hppah.h	2000/01/18 17:07:28	1.1.1.3
+++ config/pa/nm-hppah.h	2000/01/18 17:08:57
@@ -142,7 +142,7 @@ extern int hppa_require_detach PARAMS ((
 /* The PA can also watch memory regions of arbitrary size, since we're using
    a page-protection scheme.  (On some targets, apparently watch registers
    are used, which can only accomodate regions of REGISTER_SIZE.) */
-#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \
+#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(val) \
         (1)
 
 /* However, some addresses may not be profitable to use hardware to watch,

Re: A patch for ia32 hardware watchpoint.

[Jim Kingdon]

> Yeah. This is about as bad as crunching target events through unix
> signals, something that only ever made sense on ptrace() targets
> which couldn't do any better. "But the code lives on"

Always amusing to hear people talking about my designs :-) (also see
comment in target.h at enum target_signal).

The deep question is whether you want GDB to canonicalize things.  I
do see some value in getting (for example) a SEGV when you access
memory which is not mapped by the MMU (if you have one) across all
targets.  Both for users and for scripts.

If you answer pro-canonical, then the right solution is to add
additional fields (most native platforms have arch-dependent signal
codes, for example, and GDB could be taught to pass them along or
generate them for stubs and such).  I think that ptrace() targets
could get at the signal codes but not necessarily in a clean way
(wait4 and friends don't seem to return them).

If you answer anti-canonical, then you want functions in the target
vector to do things like report stop status to the user.

The two approaches aren't mutually exclusive, actually, we probably
want both a canonicalized status and a way to get more specific
information in a free-form way.

The situation might be analogous for errno, although errno is such a
poor fit for things which happen to targets (e.g. "address out of
range" maps to EIO.  Bletch), that I kind of doubt it is worth the
bother to do the canonicalization thing.  Unless perhaps if we wanted
to come up with a GDB-specific canonical set of errors.

Re: A patch for ia32 hardware watchpoint.

[Eli Zaretskii]

> As far as I'm concerned, passing the breakpoint pointer is the right way
> to go; we should get away from the assumption that a target side breakpoint
> is an address with some #define'd size, and push that stuff into a default
> implementation that can be invoked easily by people writing new target
> support.

I agree.

But if we do that, I'd also suggest to leave it to the target to
decide whether a particular watchpoint fired or not.

Right now, the API presented by GDB is based solely on the address:
bpstat_stop_status calls target_stopped_data_address and does all the
decision-making based solely on that address (and some info it keeps
internally about each watchpoint).

This API is extremely limited.  Typically, the target knows much more
about the watchpoint which triggered than the generic GDB code does,
so it can make smarter decisions.  But in order to do that, the target
needs more information about the watchpoint, and it needs to pass back
to GDB the result (whether the watchpoint triggered or not), not its
address.

Btw, if we let the target decide whether a given watchpoint triggered,
we can also resolve, once and for all, all the various conflicts
between target-specific limitations of hardware-assisted watchpoints,
which now need to be dealt with on the generic level.

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
Andrew> One aspect of this gives me cold feet and sweety palms.
Andrew> You're giving the target code access to the entire bp struct.
Andrew> While I don't have any problems with handing the code a
Andrew> breakpoint handle, I have strong reservations towards any
Andrew> moves that give the target unfettered access to the entire
Andrew> ``struct breakpoint''.  We'll be spending the next 10 years
Andrew> trying to get control back again :-)

I appreciate this argument, and agree that the target interface should
be as robust as possible.  After reading your message, I thought about
hiding the fact that the argument is a struct breakpoint pointer and
providing macros to access "public" fields, but any subversion we do
can be undone by a clever programmer.  I'm still working on something 
that is simple, elegant, and efficent.

However, I think it's a bit of a stretch to say such changes in and of
themselves will lead to GDB being out of control.  IMHO, this can only
happen if the GDB maintainers fail to do their jobs and integrate code
that disrupts GDB's architectural integrity.  I'll stipulate that
we've done a rather poor job of this in the past, but I'm hopeful that
attitudes have changed.

Andrew> I'd prefer to see something that tightens rather than loosens
Andrew> access to ``struct breakpoint''.  Perhaphs something along the
Andrew> lines of multi-arch where the target is notified of breakpoint
Andrew> create, insert, remove, delete operations.

I don't quite follow.  Care to elaborate?

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: A patch for ia32 hardware watchpoint.

[Todd Whitesel]

> I was planning to propose that the breakpoint pointer itself be passed
> to the target_{insert,remove}_{break,watch}point() functions, so this
> is as good of time as any.  

I say Just Do It. I am sitting on some local code here that tracks
breakpoints added to the target by a third party, and I ended up needing
the breakpoint shadow field to be available to those functions.

In my code I have functions called "target_better_{insert,remove}_breakpoint"
which take a struct breakpoint * and a CORE_ADDR although the CORE_ADDR is
only needed in a couple cases that show up in the overlay support (IIRC).
Previous users of the regular target_[a-z]*_breakpoint functions have been
changed to use the new ones, and I hack the target stack interface (don't
ask) in order to get the information down to where I need it. If the real
target interface passed the breakpoint pointer down, presto, no more hack.

As far as I'm concerned, passing the breakpoint pointer is the right way
to go; we should get away from the assumption that a target side breakpoint
is an address with some #define'd size, and push that stuff into a default
implementation that can be invoked easily by people writing new target
support.

> The reason I want this is so that GDB's target code can record target-
> specific info about the breakpoint.  For example, if the target gives
> the break/watchpoint an ID, the insert function can record it in the
> breakpoint so it is available for use in the delete function.  Right
> now, my WDB target code inserts a record into a data structure which
> maps WDB's breakpoint IDs with the thread+address when a breakpoint is 
> inserted.  The remove code searches for any breakpoint that matches the
> thread+address and removes that ID.  

I'm doing much the same (except it's more complicated for historical
reasons) to handle WTX breakpoints.

Actually I ended up having a bunch of functions to search the breakpoint
list for a given ID and {delete,insert,remove} it. Since other host programs
can delete my breakpoints, I had to be able to react appropriately when the
removal message showed up.

> Also, while many target vectors report error reasons through errno, 
> it leaves a bad taste in my mouth.  Perhaps this is because we have
> to map target errors into UNIX/POSIX errnos, and there isn't always
> a clean mapping.

Yeah. This is about as bad as crunching target events through unix
signals, something that only ever made sense on ptrace() targets
which couldn't do any better. "But the code lives on"

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
jtc> One issue that I'm not sure how to address is that there are several
jtc> places breakpoints are inserted where a breakpoint has not been
jtc> constructed.  Most of these occur in tdep code which implements single
jtc> step with breakpoints on processors without a trace mode.

Todd> Aiee! Such code is evil and must be destroyed.

I don't think it's that bad.  

Todd> One important value of the full breakpoint machinery is that it
Todd> can help avoid the same location being patched twice. Any time
Todd> we patch the same instruction twice, we must un-patch it in
Todd> exactly reverse order or else we leave a breakpoint instruction
Todd> sitting in the code -- Not Good.

Todd> So I would have to argue that the singlestep logic must either
Todd> happen at a really low level (this guarantees it will patch last
Todd> and un-patch first) or must go through the real breakpoint logic
Todd> which can do duplicate detection.

The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
it inserts and remove trap instructions without effecting GDB's high-
level breakpoints.  So I think we're OK.  As a result, I wouldn't be
suprised if steping into a breakpoint didn't behave the same as on a
machine with hardware single-step.  

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> One issue that I'm not sure how to address is that there are several
> places breakpoints are inserted where a breakpoint has not been
> constructed.  Most of these occur in tdep code which implements single
> step with breakpoints on processors without a trace mode.

Aiee! Such code is evil and must be destroyed.

One important value of the full breakpoint machinery is that it can help
avoid the same location being patched twice. Any time we patch the same
instruction twice, we must un-patch it in exactly reverse order or else
we leave a breakpoint instruction sitting in the code -- Not Good.

So I would have to argue that the singlestep logic must either happen at a
really low level (this guarantees it will patch last and un-patch first) or
must go through the real breakpoint logic which can do duplicate detection.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> The reason I was passing the breakpoint pointer (with the intention of
> adding a "void *private_data" field), is that I don't want to preclude
> any target implementation.  A 32 bit integer ID is probably the most
> common breakpoint handle, but you never can tell...

I didn't pretend that this was general enough for everyone, I only said
this was what I used. In my case the UINT32 is dictated by the WTX layer
that assigns the ID's.

> I don't think your is_foreign flag is suitable for the target specific
> data, because it changes the behavior of generic breakpoint code.

Hmm, actually yeah that's right. It probably should be generic.

> You didn't describe is_global, so I'm not sure about it.  How is it
> different from (bp->thread == -1)?

It probably isn't, and is more an artifact of my implementation.

The foreign/global stuff was added together, and happened late enough
in a development cycle that I needed to keep it isolated and easy to
compile back out if we had to cancel it. I had two distinct #ifdef's
so I could pinpoint all the changes; I've never taken those out.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:
> 
> >>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
> Andrew> One aspect of this gives me cold feet and sweety palms.
> Andrew> You're giving the target code access to the entire bp struct.
> Andrew> While I don't have any problems with handing the code a
> Andrew> breakpoint handle, I have strong reservations towards any
> Andrew> moves that give the target unfettered access to the entire
> Andrew> ``struct breakpoint''.  We'll be spending the next 10 years
> Andrew> trying to get control back again :-)
> 
> I appreciate this argument, and agree that the target interface should
> be as robust as possible.  After reading your message, I thought about
> hiding the fact that the argument is a struct breakpoint pointer and
> providing macros to access "public" fields, but any subversion we do
> can be undone by a clever programmer.  I'm still working on something
> that is simple, elegant, and efficent.

In the case of gdbarch, everything is manipulated via methods.  Each
legacy macro is backed by a C function.  The programmer does not have
direct access to the gdbarch object.

GDBARCH set a very very high standard (possibly too high).  However,
whats interesting is the problems so far identified have related to
things like defaults and not to the actual interface.

> However, I think it's a bit of a stretch to say such changes in and of
> themselves will lead to GDB being out of control.  IMHO, this can only
> happen if the GDB maintainers fail to do their jobs and integrate code
> that disrupts GDB's architectural integrity.  I'll stipulate that
> we've done a rather poor job of this in the past, but I'm hopeful that
> attitudes have changed.

I'm looking at it from the view of what is involved in making a change
to ``struct breakpoint''.  If targets directly depend on random bits
than that change becomes that much harder.

If target code can poke around with ``struct breakpoint'' internals then
there is that extra bit of pressure to let through that patch which
solves the problem that everyone is concerned about but no one has a
clean fix for.

> Andrew> I'd prefer to see something that tightens rather than loosens
> Andrew> access to ``struct breakpoint''.  Perhaphs something along the
> Andrew> lines of multi-arch where the target is notified of breakpoint
> Andrew> create, insert, remove, delete operations.

Possibly a side issue.  When a new architecture is created.  gdbarch
notifies all interested parties so that they can update their local
architecture specific information.  See gdbtypes.c for a very good
example.

If the target wants to know about breakpoint changes then notify it
(mumble something about the observer pattern).

	Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Richard Earnshaw]

> "J.T. Conklin" wrote:
> 
> > Todd> So I would have to argue that the singlestep logic must either
> > Todd> happen at a really low level (this guarantees it will patch last
> > Todd> and un-patch first) or must go through the real breakpoint logic
> > Todd> which can do duplicate detection.
> > 
> > The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
> > it inserts and remove trap instructions without effecting GDB's high-
> > level breakpoints.  So I think we're OK.  As a result, I wouldn't be
> > suprised if steping into a breakpoint didn't behave the same as on a
> > machine with hardware single-step.
> 
> Its got long-term problems.
> 
> The code assumes that it is going to retain control, blocked on the
> target, while the actual step is performed.
> This doesn't work well within a pure event model where control is ment
> to return to the event-loop when ever the target is resumed.

It doesn't work properly with INSTRUCTION_NULLIFIED either -- at least it 
didn't last time I tried it...

R.

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Andrew Cagney]

"J.T. Conklin" wrote:

> Todd> So I would have to argue that the singlestep logic must either
> Todd> happen at a really low level (this guarantees it will patch last
> Todd> and un-patch first) or must go through the real breakpoint logic
> Todd> which can do duplicate detection.
> 
> The SOFTWARE_SINGLE_STEP() macro is called at a low enough level that
> it inserts and remove trap instructions without effecting GDB's high-
> level breakpoints.  So I think we're OK.  As a result, I wouldn't be
> suprised if steping into a breakpoint didn't behave the same as on a
> machine with hardware single-step.

Its got long-term problems.

The code assumes that it is going to retain control, blocked on the
target, while the actual step is performed.
This doesn't work well within a pure event model where control is ment
to return to the event-loop when ever the target is resumed.

	enjoy,
		Andrew

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Todd Whitesel]

> One aspect of this gives me cold feet and sweety palms.  You're giving
> the target code access to the entire bp struct.  While I don't have any
> problems with handing the code a breakpoint handle, I have strong
> reservations towards any moves that give the target unfettered access to
> the entire ``struct breakpoint''.  We'll be spending the next 10 years
> trying to get control back again :-)
...
> I'd prefer to see something that tightens rather than loosens access to
> ``struct breakpoint''.  Perhaphs something along the lines of multi-arch
> where the target is notified of breakpoint create, insert, remove,
> delete operations.

Basically what I need is the moral equivalent of a "user" field tacked
onto every breakpoint struct.

Based on my existing code, the contents of this field would be:

	UINT32	breakpointID;	/* WTX-supplied handle for this breakpoint */
	bool	is_foreign;	/* true if breakpoint was set by someone else */
	bool	is_global;	/* true if breakpoint active for all tasks */

The is_foreign field is an interesting concept which I don't think anyone
else has implemented; it signifies a breakpoint that GDB does not "own".
One of the events I can receive from the target is "foreign breakpoint
created" and I find a safe time to call "break *<address>" in order to
let the breakpoint show up properly in GDB.

Foreign breakpoints look like normal breakpoints to GDB users except that
if they aren't recreated (new breakpoint struct allocated) by GDB commands,
then they are still marked "foreign" and when GDB exits they will not be
automatically removed. This is because some other command session usually
still exists, which created that breakpoint, and expects it to remain set
unless explicitly deleted by a user.

-- 
Todd Whitesel
toddpw @ windriver.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Todd" == Todd Whitesel <toddpw@windriver.com> writes:
Andrew> I'd prefer to see something that tightens rather than loosens
Andrew> access to ``struct breakpoint''.  Perhaphs something along the
Andrew> lines of multi-arch where the target is notified of breakpoint
Andrew> create, insert, remove, delete operations.

Todd> Basically what I need is the moral equivalent of a "user" field
Todd> tacked onto every breakpoint struct.
Todd>
Todd> Based on my existing code, the contents of this field would be:
Todd> 	UINT32	breakpointID; /* WTX-supplied handle for this breakpoint */
Todd> 	bool	is_foreign;   /* true if breakpoint was set by someone else */
Todd> 	bool	is_global;    /* true if breakpoint active for all tasks */

The reason I was passing the breakpoint pointer (with the intention of
adding a "void *private_data" field), is that I don't want to preclude
any target implementation.  A 32 bit integer ID is probably the most
common breakpoint handle, but you never can tell...

I don't think your is_foreign flag is suitable for the target specific
data, because it changes the behavior of generic breakpoint code.  You
didn't describe is_global, so I'm not sure about it.  How is it
different from (bp->thread == -1)?

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: A patch for ia32 hardware watchpoint.

[Andrew Cagney]

Jim Kingdon wrote:
> 
> > Yeah. This is about as bad as crunching target events through unix
> > signals, something that only ever made sense on ptrace() targets
> > which couldn't do any better. "But the code lives on"
> 
> Always amusing to hear people talking about my designs :-) (also see
> comment in target.h at enum target_signal).
> 
> The deep question is whether you want GDB to canonicalize things.  I
> do see some value in getting (for example) a SEGV when you access
> memory which is not mapped by the MMU (if you have one) across all
> targets.  Both for users and for scripts.

Have a poke around the sim directories (mips) where the coders pulled
various nasties because GDB assumes a signal instead of an event.

I think the target should return an arbitrary event that can be queried
for its ``kind'' or poked at to create additional detail.

More importantly, you should be able to edit that event and then throw
it back at the target.

	Andrew

Re: A patch for ia32 hardware watchpoint.

[Eli Zaretskii]

> > @@ -5500,13 +5513,13 @@ watch_command_1 (arg, accessflag, from_t
> >     in hardware return zero.  */
> >  
> >  #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> > -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> > -    ((BYTE_SIZE) <= (REGISTER_SIZE))
> > +#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL) \
> > +    (TYPE_LENGTH (VALUE_TYPE (VAL)) <= (REGISTER_SIZE))
> >  #endif
> >  
> >  #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> > -#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
> > -     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN)
> > +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(VAL) \
> > +     TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(VAL)
> >  #endif
> 
> These are IMHO wrong: the number of debug registers required for a
> particular region is a function of the address, not only size (e.g., a
> single x86 debug register cannot watch a 32-bit region that isn't
> aligned on 4-byte boundary).  If Linux, for some reason, doesn't need
> the address (although I cannot see how could this be right, at least
> for native debugging), please define a platform-specific macro instead
> of overwriting system-wide defaults.

Sorry, I talked too soon.  These changes supply all the required info,
since they pass the entire struct value to the macro.  So they are
okay with me.

I'm terribly sorry for jumping the gun for no reason.

The rest of my comments about these patches are still valid, though.

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:

jtc> One issue that I'm not sure how to address is that there are several
jtc> places breakpoints are inserted where a breakpoint has not been
jtc> constructed.  Most of these occur in tdep code which implements single
jtc> step with breakpoints on processors without a trace mode.

Todd> Aiee! Such code is evil and must be destroyed.

Stephane> It seems to me that this single_step function is actually
Stephane> not in use.  I don't know about WindRiver, but here we have
Stephane> two targets: an ISS and a remote target that use the generic
Stephane> breakpoint functions before stepping.  I don't mind to
Stephane> remove this code.

I assume you are refering to the ARC port?

I see three GDB ports that do single step in software: arc, rs6000,
and sparc.  The arc and sparc ports, insert breakpoints with target-
_insert_breakpoint().  The rs6000 port by reading and writing memory
directly.  Of the two methods, I prefer target_insert_breakpoint().

However, if the breakpoint insert/remove API was changed to require a
struct breakpoint pointer, we couldn't call target_insert_breakpoint()
in the foo_single_step() functions without creating a real breakpoint
object to pass through that first pointer.   

Another reason we might want a real breakpoint for this is that when
my memory region attribute code is complete we'll be able to say that
breakpoints in a region should be done with hardware breakpoints.  A
user can debug his/her own code in read-only memory by using hbreak,
but there is no way to tell GDB to use hardware breakpoints for step,
next, continue, finish, until, etc.  One thing that surprised me was
that the single step code used multiple breakpoints.  That would put a
lot of pressure on hardware breakpoint registers.  I suspect that GDB
has all the information in order to insert a single breakpoint (by
evaluating which way a conditional branch will go, etc.).


Since you say you can remove the single step code, I assume that the
ISS target and remote protocol agents can perform the single step by
themselves?  If so, it would be advantageous to disable GDB's single
step support.  GDB would then issue a single "step" command instead of
"insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
latency of a step should be greatly improved.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

> >>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:
>
> jtc> One issue that I'm not sure how to address is that there are several
> jtc> places breakpoints are inserted where a breakpoint has not been
> jtc> constructed.  Most of these occur in tdep code which implements single
> jtc> step with breakpoints on processors without a trace mode.
>
> Todd> Aiee! Such code is evil and must be destroyed.
>
> Stephane> It seems to me that this single_step function is actually
> Stephane> not in use.  I don't know about WindRiver, but here we have
> Stephane> two targets: an ISS and a remote target that use the generic
> Stephane> breakpoint functions before stepping.  I don't mind to
> Stephane> remove this code.
>
> I assume you are refering to the ARC port?

yes. But I was actually wrong in my last email, the single step function in
arc-tdep is
used, I was thinking to another function, sorry.
What do you mean by breakpoint without a trace mode?

>
> I see three GDB ports that do single step in software: arc, rs6000,
> and sparc.  The arc and sparc ports, insert breakpoints with target-
> _insert_breakpoint().  The rs6000 port by reading and writing memory
> directly.  Of the two methods, I prefer target_insert_breakpoint().
>
> However, if the breakpoint insert/remove API was changed to require a
> struct breakpoint pointer, we couldn't call target_insert_breakpoint()
> in the foo_single_step() functions without creating a real breakpoint
> object to pass through that first pointer.

I don't know what are the foo_single_step functions.

I also use the breakpoint structure as an extern variable. I needed
it to implement the set_hw_breakpoint routine for the remote protocol.
I think it's not the nicer way to do but ....

> Another reason we might want a real breakpoint for this is that when
> my memory region attribute code is complete we'll be able to say that
> breakpoints in a region should be done with hardware breakpoints.
> A user can debug his/her own code in read-only memory by using hbreak,
> but there is no way to tell GDB to use hardware breakpoints for step,
> next, continue, finish, until, etc.  One thing that surprised me was
> that the single step code used multiple breakpoints.  That would put a
> lot of pressure on hardware breakpoint registers.  I suspect that GDB
> has all the information in order to insert a single breakpoint (by
> evaluating which way a conditional branch will go, etc.).
>

What would be a "real breakpoint"? a pointer to the breakpoint structure?

>
> Since you say you can remove the single step code, I assume that the
> ISS target and remote protocol agents can perform the single step by
> themselves?  If so, it would be advantageous to disable GDB's single
> step support.  GDB would then issue a single "step" command instead of
> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
> latency of a step should be greatly improved.

I will implement this for the remote target only since the hardware supports
single-stepping.
I'm not sure if this feasible in the ISS - Yuri?

If not feasible I won't disable the GDB's single step support (thus enabling a
call to single_step())
but I will implement a single_step call to gdbstub in remote_resume().

Stephane.

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

>
> Stephane> I also use the breakpoint structure as an extern variable. I
> Stephane> needed it to implement the set_hw_breakpoint routine for the
> Stephane> remote protocol.  I think it's not the nicer way to do but
> Stephane> ....
>
> Can you explain?  I don't see any use of struct breakpoint in the
> current arc-tdep.c, nor do I see a set_hw_breakpoint function?

The version at FSF is a very old version out of date.
I am currently extending gdb for ARC to support user extensions.

>
> >> Since you say you can remove the single step code, I assume that the
> >> ISS target and remote protocol agents can perform the single step by
> >> themselves?  If so, it would be advantageous to disable GDB's single
> >> step support.  GDB would then issue a single "step" command instead of
> >> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
> >> latency of a step should be greatly improved.
>
> Stephane> I will implement this for the remote target only since the
> Stephane> hardware supports single-stepping.  I'm not sure if this
> Stephane> feasible in the ISS - Yuri?
>
> Stephane> If not feasible I won't disable the GDB's single step
> Stephane> support (thus enabling a call to single_step()) but I will
> Stephane> implement a single_step call to gdbstub in remote_resume().
>
> If your ISS target cannot support single step, but the remote protocol
> can, perhaps the best thing is to make software single step a target
> specific option.

By creating a target specific command?
The single step mechanism is a not a step-over-calls mechanism.

>
> I don't think that the arc is the only processor that would benefit
> from such a change.  For example, the sparclet ROM monitor supports a
> single step command, but I do not know if it is ever issued because
> the SPARC target uses software single step.  Perhaps it inserts the
> trap instructions needed for single step and issues the monitor step
> command?
>
> I do not think it's possible for any change to remote_resume() to be
> the right solution.
>
You're right, I've tried and if does not work.
Thanks for your explanations anyway.

Stephane.

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

> > One issue that I'm not sure how to address is that there are several
> > places breakpoints are inserted where a breakpoint has not been
> > constructed.  Most of these occur in tdep code which implements single
> > step with breakpoints on processors without a trace mode.
>
> Aiee! Such code is evil and must be destroyed.

It seems to me that this single_step function is actually not in use.
I don't know about WindRiver, but here we have two targets: an ISS and a
remote target that use the generic breakpoint functions before stepping.
I don't mind to remove this code.

>
> One important value of the full breakpoint machinery is that it can help
> avoid the same location being patched twice. Any time we patch the same
> instruction twice, we must un-patch it in exactly reverse order or else
> we leave a breakpoint instruction sitting in the code -- Not Good.
>
> So I would have to argue that the singlestep logic must either happen at a
> really low level (this guarantees it will patch last and un-patch first) or
> must go through the real breakpoint logic which can do duplicate detection.
>

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:
Stephane> What do you mean by breakpoint without a trace mode?

Processors that support hardware single step often call it instruction
trace mode or something similar.  Processors without it have to resort
to inserting trap/breakpoint/invalid instructions.

>> However, if the breakpoint insert/remove API was changed to require a
>> struct breakpoint pointer, we couldn't call target_insert_breakpoint()
>> in the foo_single_step() functions without creating a real breakpoint
>> object to pass through that first pointer.

Stephane> I don't know what are the foo_single_step functions.

I was attempting to refer to those functions that implement single step
on all targets: arc_software_single_step(), rs6000_software_single_step,
and sparc_software_single_step().  It might have made more sense if I 
didn't forget "software_" and called it foo_software_single_step() or
maybe *_software_single_step().

Stephane> I also use the breakpoint structure as an extern variable. I
Stephane> needed it to implement the set_hw_breakpoint routine for the
Stephane> remote protocol.  I think it's not the nicer way to do but
Stephane> ....

Can you explain?  I don't see any use of struct breakpoint in the
current arc-tdep.c, nor do I see a set_hw_breakpoint function?

>> Another reason we might want a real breakpoint for this is that when
>> my memory region attribute code is complete we'll be able to say that
>> breakpoints in a region should be done with hardware breakpoints.
>> A user can debug his/her own code in read-only memory by using hbreak,
>> but there is no way to tell GDB to use hardware breakpoints for step,
>> next, continue, finish, until, etc.  One thing that surprised me was
>> that the single step code used multiple breakpoints.  That would put a
>> lot of pressure on hardware breakpoint registers.  I suspect that GDB
>> has all the information in order to insert a single breakpoint (by
>> evaluating which way a conditional branch will go, etc.).
>> 

Stephane> What would be a "real breakpoint"? a pointer to the
Stephane> breakpoint structure?

Yes, it's a a pointer to a breakpoint struct, but it's more than that.
I mean a breakpoint created by one of the breakpoint creation functions 
in breakpoint.c.

>> Since you say you can remove the single step code, I assume that the
>> ISS target and remote protocol agents can perform the single step by
>> themselves?  If so, it would be advantageous to disable GDB's single
>> step support.  GDB would then issue a single "step" command instead of
>> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
>> latency of a step should be greatly improved.

Stephane> I will implement this for the remote target only since the
Stephane> hardware supports single-stepping.  I'm not sure if this
Stephane> feasible in the ISS - Yuri?

Stephane> If not feasible I won't disable the GDB's single step
Stephane> support (thus enabling a call to single_step()) but I will
Stephane> implement a single_step call to gdbstub in remote_resume().

If your ISS target cannot support single step, but the remote protocol
can, perhaps the best thing is to make software single step a target
specific option.  

I don't think that the arc is the only processor that would benefit
from such a change.  For example, the sparclet ROM monitor supports a
single step command, but I do not know if it is ever issued because
the SPARC target uses software single step.  Perhaps it inserts the
trap instructions needed for single step and issues the monitor step
command?

I do not think it's possible for any change to remote_resume() to be
the right solution.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

>
> Stephane> I also use the breakpoint structure as an extern variable. I
> Stephane> needed it to implement the set_hw_breakpoint routine for the
> Stephane> remote protocol.  I think it's not the nicer way to do but
> Stephane> ....
>
> Can you explain?  I don't see any use of struct breakpoint in the
> current arc-tdep.c, nor do I see a set_hw_breakpoint function?

The version at FSF is a very old version out of date.
I am currently extending gdb for ARC to support user extensions.

>
> >> Since you say you can remove the single step code, I assume that the
> >> ISS target and remote protocol agents can perform the single step by
> >> themselves?  If so, it would be advantageous to disable GDB's single
> >> step support.  GDB would then issue a single "step" command instead of
> >> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
> >> latency of a step should be greatly improved.
>
> Stephane> I will implement this for the remote target only since the
> Stephane> hardware supports single-stepping.  I'm not sure if this
> Stephane> feasible in the ISS - Yuri?
>
> Stephane> If not feasible I won't disable the GDB's single step
> Stephane> support (thus enabling a call to single_step()) but I will
> Stephane> implement a single_step call to gdbstub in remote_resume().
>
> If your ISS target cannot support single step, but the remote protocol
> can, perhaps the best thing is to make software single step a target
> specific option.

By creating a target specific command?
The single step mechanism is a not a step-over-calls mechanism.

>
> I don't think that the arc is the only processor that would benefit
> from such a change.  For example, the sparclet ROM monitor supports a
> single step command, but I do not know if it is ever issued because
> the SPARC target uses software single step.  Perhaps it inserts the
> trap instructions needed for single step and issues the monitor step
> command?
>
> I do not think it's possible for any change to remote_resume() to be
> the right solution.
>
You're right, I've tried and if does not work.
Thanks for your explanations anyway.

Stephane.

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:
Stephane> What do you mean by breakpoint without a trace mode?

Processors that support hardware single step often call it instruction
trace mode or something similar.  Processors without it have to resort
to inserting trap/breakpoint/invalid instructions.

>> However, if the breakpoint insert/remove API was changed to require a
>> struct breakpoint pointer, we couldn't call target_insert_breakpoint()
>> in the foo_single_step() functions without creating a real breakpoint
>> object to pass through that first pointer.

Stephane> I don't know what are the foo_single_step functions.

I was attempting to refer to those functions that implement single step
on all targets: arc_software_single_step(), rs6000_software_single_step,
and sparc_software_single_step().  It might have made more sense if I 
didn't forget "software_" and called it foo_software_single_step() or
maybe *_software_single_step().

Stephane> I also use the breakpoint structure as an extern variable. I
Stephane> needed it to implement the set_hw_breakpoint routine for the
Stephane> remote protocol.  I think it's not the nicer way to do but
Stephane> ....

Can you explain?  I don't see any use of struct breakpoint in the
current arc-tdep.c, nor do I see a set_hw_breakpoint function?

>> Another reason we might want a real breakpoint for this is that when
>> my memory region attribute code is complete we'll be able to say that
>> breakpoints in a region should be done with hardware breakpoints.
>> A user can debug his/her own code in read-only memory by using hbreak,
>> but there is no way to tell GDB to use hardware breakpoints for step,
>> next, continue, finish, until, etc.  One thing that surprised me was
>> that the single step code used multiple breakpoints.  That would put a
>> lot of pressure on hardware breakpoint registers.  I suspect that GDB
>> has all the information in order to insert a single breakpoint (by
>> evaluating which way a conditional branch will go, etc.).
>> 

Stephane> What would be a "real breakpoint"? a pointer to the
Stephane> breakpoint structure?

Yes, it's a a pointer to a breakpoint struct, but it's more than that.
I mean a breakpoint created by one of the breakpoint creation functions 
in breakpoint.c.

>> Since you say you can remove the single step code, I assume that the
>> ISS target and remote protocol agents can perform the single step by
>> themselves?  If so, it would be advantageous to disable GDB's single
>> step support.  GDB would then issue a single "step" command instead of
>> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
>> latency of a step should be greatly improved.

Stephane> I will implement this for the remote target only since the
Stephane> hardware supports single-stepping.  I'm not sure if this
Stephane> feasible in the ISS - Yuri?

Stephane> If not feasible I won't disable the GDB's single step
Stephane> support (thus enabling a call to single_step()) but I will
Stephane> implement a single_step call to gdbstub in remote_resume().

If your ISS target cannot support single step, but the remote protocol
can, perhaps the best thing is to make software single step a target
specific option.  

I don't think that the arc is the only processor that would benefit
from such a change.  For example, the sparclet ROM monitor supports a
single step command, but I do not know if it is ever issued because
the SPARC target uses software single step.  Perhaps it inserts the
trap instructions needed for single step and issues the monitor step
command?

I do not think it's possible for any change to remote_resume() to be
the right solution.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

> >>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:
>
> jtc> One issue that I'm not sure how to address is that there are several
> jtc> places breakpoints are inserted where a breakpoint has not been
> jtc> constructed.  Most of these occur in tdep code which implements single
> jtc> step with breakpoints on processors without a trace mode.
>
> Todd> Aiee! Such code is evil and must be destroyed.
>
> Stephane> It seems to me that this single_step function is actually
> Stephane> not in use.  I don't know about WindRiver, but here we have
> Stephane> two targets: an ISS and a remote target that use the generic
> Stephane> breakpoint functions before stepping.  I don't mind to
> Stephane> remove this code.
>
> I assume you are refering to the ARC port?

yes. But I was actually wrong in my last email, the single step function in
arc-tdep is
used, I was thinking to another function, sorry.
What do you mean by breakpoint without a trace mode?

>
> I see three GDB ports that do single step in software: arc, rs6000,
> and sparc.  The arc and sparc ports, insert breakpoints with target-
> _insert_breakpoint().  The rs6000 port by reading and writing memory
> directly.  Of the two methods, I prefer target_insert_breakpoint().
>
> However, if the breakpoint insert/remove API was changed to require a
> struct breakpoint pointer, we couldn't call target_insert_breakpoint()
> in the foo_single_step() functions without creating a real breakpoint
> object to pass through that first pointer.

I don't know what are the foo_single_step functions.

I also use the breakpoint structure as an extern variable. I needed
it to implement the set_hw_breakpoint routine for the remote protocol.
I think it's not the nicer way to do but ....

> Another reason we might want a real breakpoint for this is that when
> my memory region attribute code is complete we'll be able to say that
> breakpoints in a region should be done with hardware breakpoints.
> A user can debug his/her own code in read-only memory by using hbreak,
> but there is no way to tell GDB to use hardware breakpoints for step,
> next, continue, finish, until, etc.  One thing that surprised me was
> that the single step code used multiple breakpoints.  That would put a
> lot of pressure on hardware breakpoint registers.  I suspect that GDB
> has all the information in order to insert a single breakpoint (by
> evaluating which way a conditional branch will go, etc.).
>

What would be a "real breakpoint"? a pointer to the breakpoint structure?

>
> Since you say you can remove the single step code, I assume that the
> ISS target and remote protocol agents can perform the single step by
> themselves?  If so, it would be advantageous to disable GDB's single
> step support.  GDB would then issue a single "step" command instead of
> "insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
> latency of a step should be greatly improved.

I will implement this for the remote target only since the hardware supports
single-stepping.
I'm not sure if this feasible in the ISS - Yuri?

If not feasible I won't disable the GDB's single step support (thus enabling a
call to single_step())
but I will implement a single_step call to gdbstub in remote_resume().

Stephane.

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[J.T. Conklin]

>>>>> "Stephane" == Stephane Bihan <Stephane.Bihan@arccores.com> writes:

jtc> One issue that I'm not sure how to address is that there are several
jtc> places breakpoints are inserted where a breakpoint has not been
jtc> constructed.  Most of these occur in tdep code which implements single
jtc> step with breakpoints on processors without a trace mode.

Todd> Aiee! Such code is evil and must be destroyed.

Stephane> It seems to me that this single_step function is actually
Stephane> not in use.  I don't know about WindRiver, but here we have
Stephane> two targets: an ISS and a remote target that use the generic
Stephane> breakpoint functions before stepping.  I don't mind to
Stephane> remove this code.

I assume you are refering to the ARC port?

I see three GDB ports that do single step in software: arc, rs6000,
and sparc.  The arc and sparc ports, insert breakpoints with target-
_insert_breakpoint().  The rs6000 port by reading and writing memory
directly.  Of the two methods, I prefer target_insert_breakpoint().

However, if the breakpoint insert/remove API was changed to require a
struct breakpoint pointer, we couldn't call target_insert_breakpoint()
in the foo_single_step() functions without creating a real breakpoint
object to pass through that first pointer.   

Another reason we might want a real breakpoint for this is that when
my memory region attribute code is complete we'll be able to say that
breakpoints in a region should be done with hardware breakpoints.  A
user can debug his/her own code in read-only memory by using hbreak,
but there is no way to tell GDB to use hardware breakpoints for step,
next, continue, finish, until, etc.  One thing that surprised me was
that the single step code used multiple breakpoints.  That would put a
lot of pressure on hardware breakpoint registers.  I suspect that GDB
has all the information in order to insert a single breakpoint (by
evaluating which way a conditional branch will go, etc.).


Since you say you can remove the single step code, I assume that the
ISS target and remote protocol agents can perform the single step by
themselves?  If so, it would be advantageous to disable GDB's single
step support.  GDB would then issue a single "step" command instead of
"insert breakpoint(s)", "continue", and "remove breakpoint(s)".  The
latency of a step should be greatly improved.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: breakpoint insert API (was: A patch for ia32 hardware watchpoint.)

[Stephane.Bihan]

> > One issue that I'm not sure how to address is that there are several
> > places breakpoints are inserted where a breakpoint has not been
> > constructed.  Most of these occur in tdep code which implements single
> > step with breakpoints on processors without a trace mode.
>
> Aiee! Such code is evil and must be destroyed.

It seems to me that this single_step function is actually not in use.
I don't know about WindRiver, but here we have two targets: an ISS and a
remote target that use the generic breakpoint functions before stepping.
I don't mind to remove this code.

>
> One important value of the full breakpoint machinery is that it can help
> avoid the same location being patched twice. Any time we patch the same
> instruction twice, we must un-patch it in exactly reverse order or else
> we leave a breakpoint instruction sitting in the code -- Not Good.
>
> So I would have to argue that the singlestep logic must either happen at a
> really low level (this guarantees it will patch last and un-patch first) or
> must go through the real breakpoint logic which can do duplicate detection.
>

----------------------------------------------------------------
Stephane Bihan
ARC Cores Ltd            http://www.arccores.com