[PATCH]: Make Linux use the new unified x86 watchpoint support

[PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Kettenis]

FYI, I checked this in.  HJ can finally be happy now (although things
probably won't work correctly for multithreaded programs).

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	Make Linux use the new unified support for hardware breakpoints
	and watchpoints on x86 targets.
	* i386-linux-nat.c: Doc fixes.  Include "gdb_assert.h".
	[HAVE_SYS_DEBUGREG_H]: Include <sys/debugreg.h>.
	(DR_FIRSTADDR, DR_LASTADDR, DR_STATUS, DR_CONTROL): Define to
	appropriate value if not already defined.
	(register_u_addr): New function.
	(kernel_u_size): New function.
	(i386_linux_dr_get, i386_linux_dr_set): New functions.
	(i386_linux_dr_set_control, i386_linux_dr_set_addr,
	i386_linux_reset_addr, i386_linux_dr_get_status): New functions.
	* config/i386/nm-linux.h: Don't include "nm-i386v.h".
	(I386_USE_GENERIC_WATCHPOINTS): Define and include "nm-i386.h".
	(TARGET_HAS_HARDWARE_WATCHPOINTS,
	TARGET_CAN_USE_HARDWARE_WATCHPOINTS, HAVE_CONTINUABLE_WATCHPOINT,
	STOPPED_BY_WATCHPOINT, target_insert_watchpoint,
	target_remove_watchpoint): Remove macros.
	(i386_stopped_by_watchpoint, i386_insert_watchpoint,
	i386_remove_watchpoint): Remove prototypes.
	(register_u_addr): New prototype.
	(REGISTER_U_ADDR): Define in terms of register_u_addr.
	(i386_linux_dr_set_control, i386_linux_dr_set_addr,
	i386_linux_reset_addr, i386_linux_dr_get_status): New prototypes.
	(I386_DR_LOW_SET_CONTROL, I386_DR_LOW_SET_ADDR,
	I386_DR_LOW_RESET_ADDR, I386_DR_LOW_GET_STATUS): New macros.
	* config/i386/linux.mh (NATDEPFILES): Replace i386v-nat.o with
	i386-nat.o.

Index: i386-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-linux-nat.c,v
retrieving revision 1.23
diff -u -p -r1.23 i386-linux-nat.c
--- i386-linux-nat.c 2001/03/13 23:31:13 1.23
+++ i386-linux-nat.c 2001/03/21 21:20:21
@@ -23,6 +23,7 @@
 #include "gdbcore.h"
 #include "regcache.h"
 
+#include "gdb_assert.h"
 #include <sys/ptrace.h>
 #include <sys/user.h>
 #include <sys/procfs.h>
@@ -31,6 +32,26 @@
 #include <sys/reg.h>
 #endif
 
+#ifdef HAVE_SYS_DEBUGREG_H
+#include <sys/debugreg.h>
+#endif
+
+#ifndef DR_FIRSTADDR
+#define DR_FIRSTADDR 0
+#endif
+
+#ifndef DR_LASTADDR
+#define DR_LASTADDR 3
+#endif
+
+#ifndef DR_STATUS
+#define DR_STATUS 6
+#endif
+
+#ifndef DR_CONTROL
+#define DR_CONTROL 7
+#endif
+
 /* Prototypes for supply_gregset etc.  */
 #include "gregset.h"
 
@@ -110,6 +131,26 @@ int have_ptrace_getfpxregs =
 ;
 \f
 
+/* Support for the user struct.  */
+
+/* Return the address of register REGNUM.  BLOCKEND is the value of
+   u.u_ar0, which should point to the registers.  */
+
+CORE_ADDR
+register_u_addr (CORE_ADDR blockend, int regnum)
+{
+  return (blockend + 4 * regmap[regnum]);
+}
+
+/* Return the size of the user struct.  */
+
+int
+kernel_u_size (void)
+{
+  return (sizeof (struct user));
+}
+\f
+
 /* Fetching registers directly from the U area, one at a time.  */
 
 /* FIXME: kettenis/2000-03-05: This duplicates code from `inptrace.c'.
@@ -657,6 +698,72 @@ store_inferior_registers (int regno)
 
   internal_error (__FILE__, __LINE__,
 		  "Got request to store bad register number %d.", regno);
+}
+\f
+
+static long
+i386_linux_dr_get (int regnum)
+{
+  int tid;
+  long value;
+
+  /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
+     multi-threaded processes here.  For now, pretend there is just
+     one thread.  */
+  tid = PIDGET (inferior_pid);
+
+  errno = 0;
+  value = ptrace (PT_READ_U, tid,
+		  offsetof (struct user, u_debugreg[regnum]), 0);
+  if (errno != 0)
+    perror_with_name ("Couldn't read debug register");
+
+  return value;
+}
+
+static void
+i386_linux_dr_set (int regnum, long value)
+{
+  int tid;
+
+  /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
+     multi-threaded processes here.  For now, pretend there is just
+     one thread.  */
+  tid = PIDGET (inferior_pid);
+
+  errno = 0;
+  ptrace (PT_WRITE_U, tid,
+	  offsetof (struct user, u_debugreg[regnum]), value);
+  if (errno != 0)
+    perror_with_name ("Couldn't write debug register");
+}
+
+void
+i386_linux_dr_set_control (long control)
+{
+  i386_linux_dr_set (DR_CONTROL, control);
+}
+
+void
+i386_linux_dr_set_addr (int regnum, CORE_ADDR addr)
+{
+  gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
+
+  i386_linux_dr_set (DR_FIRSTADDR + regnum, addr);
+}
+
+void
+i386_linux_dr_reset_addr (int regnum)
+{
+  gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
+
+  i386_linux_dr_set (DR_FIRSTADDR + regnum, 0L);
+}
+
+long
+i386_linux_dr_get_status (void)
+{
+  return i386_linux_dr_get (DR_STATUS);
 }
 \f
 
Index: config/i386/nm-linux.h
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/nm-linux.h,v
retrieving revision 1.8
diff -u -p -r1.8 nm-linux.h
--- config/i386/nm-linux.h 2001/03/06 08:21:28 1.8
+++ config/i386/nm-linux.h 2001/03/21 21:20:21
@@ -1,6 +1,6 @@
 /* Native support for Linux/x86.
    Copyright 1986, 1987, 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000
+   1999, 2000, 2001
    Free Software Foundation, Inc.
 
    This file is part of GDB.
@@ -23,52 +23,56 @@
 #ifndef NM_LINUX_H
 #define NM_LINUX_H
 
-#include "i386/nm-i386v.h"
+/* GNU/Linux supports the i386 hardware debugging registers.  */
+#define I386_USE_GENERIC_WATCHPOINTS
+
+#include "i386/nm-i386.h"
 #include "nm-linux.h"
 
-/* Return sizeof user struct to callers in less machine dependent routines */
+/* Return sizeof user struct to callers in less machine dependent
+   routines.  */
 
-#define KERNEL_U_SIZE kernel_u_size()
 extern int kernel_u_size (void);
+#define KERNEL_U_SIZE kernel_u_size()
 
 #define U_REGS_OFFSET 0
-
-/* GNU/Linux supports the 386 hardware debugging registers.  */
-
-#define TARGET_HAS_HARDWARE_WATCHPOINTS
-
-#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
 
-/* After a watchpoint trap, the PC points to the instruction after
-   the one that caused the trap.  Therefore we don't need to step over it.
-   But we do need to reset the status register to avoid another trap.  */
-#define HAVE_CONTINUABLE_WATCHPOINT
+extern CORE_ADDR register_u_addr (CORE_ADDR blockend, int regnum);
+#define REGISTER_U_ADDR(addr, blockend, regnum) \
+  (addr) = register_u_addr (blockend, regnum)
+
+/* Provide access to the i386 hardware debugging registers.  */
+
+extern void i386_linux_dr_set_control (long control);
+#define I386_DR_LOW_SET_CONTROL(control) \
+  i386_linux_dr_set_control (control)
+
+extern void i386_linux_dr_set_addr (int regnum, CORE_ADDR addr);
+#define I386_DR_LOW_SET_ADDR(regnum, addr) \
+  i386_linux_dr_set_addr (regnum, addr)
+
+extern void i386_linux_dr_reset_addr (int regnum);
+#define I386_DR_LOW_RESET_ADDR(regnum) \
+  i386_linux_dr_reset_addr (regnum)
+
+extern long i386_linux_dr_get_status (void);
+#define I386_DR_LOW_GET_STATUS() \
+  i386_linux_dr_get_status ()
 
-#define STOPPED_BY_WATCHPOINT(W)  \
-  i386_stopped_by_watchpoint (inferior_pid)
+/* We define this if link.h is available, because with ELF we use SVR4
+   style shared libraries.  */
 
-/* Use these macros for watchpoint insertion/removal.  */
-
-#define target_insert_watchpoint(addr, len, type)  \
-  i386_insert_watchpoint (inferior_pid, addr, len, type)
-
-#define target_remove_watchpoint(addr, len, type)  \
-  i386_remove_watchpoint (inferior_pid, addr, len)
-
-/* We define this if link.h is available, because with ELF we use SVR4 style
-   shared libraries. */
-
 #ifdef HAVE_LINK_H
 #define SVR4_SHARED_LIBS
-#include "solib.h"		/* Support for shared libraries. */
+#include "solib.h"		/* Support for shared libraries.  */
 #endif
 
 /* Override copies of {fetch,store}_inferior_registers in `infptrace.c'.  */
 #define FETCH_INFERIOR_REGISTERS
 
-/* Nevertheless, define CANNOT_{FETCH,STORE}_REGISTER, because we might fall
-   back on the code `infptrace.c' (well a copy of that code in
-   `i386-linux-nat.c' for now) and we can access only the
+/* Nevertheless, define CANNOT_{FETCH,STORE}_REGISTER, because we
+   might fall back on the code `infptrace.c' (well a copy of that code
+   in `i386-linux-nat.c' for now) and we can access only the
    general-purpose registers in that way.  */
 extern int cannot_fetch_register (int regno);
 extern int cannot_store_register (int regno);
@@ -77,10 +81,6 @@ extern int cannot_store_register (int re
 
 /* Override child_resume in `infptrace.c'.  */
 #define CHILD_RESUME
-
-extern CORE_ADDR i386_stopped_by_watchpoint (int);
-extern int i386_insert_watchpoint (int pid, CORE_ADDR addr, int len, int rw);
-extern int i386_remove_watchpoint (int pid, CORE_ADDR addr, int len);
 
 /* FIXME: kettenis/2000-09-03: This should be moved to ../nm-linux.h
    once we have converted all Linux targets to use the new threads
Index: config/i386/linux.mh
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/linux.mh,v
retrieving revision 1.6
diff -u -p -r1.6 linux.mh
--- config/i386/linux.mh 2000/10/30 22:33:32 1.6
+++ config/i386/linux.mh 2001/03/21 21:20:21
@@ -5,7 +5,7 @@ XDEPFILES=
 
 NAT_FILE= nm-linux.h
 NATDEPFILES= infptrace.o inftarg.o fork-child.o corelow.o \
-	core-aout.o i386v-nat.o i386-linux-nat.o i387-nat.o \
+	core-aout.o i386-nat.o i386-linux-nat.o i387-nat.o \
 	proc-service.o thread-db.o lin-lwp.o
 
 LOADLIBES = -ldl -rdynamic

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Mark Kettenis wrote:
> 
> FYI, I checked this in.  HJ can finally be happy now (although things
> probably won't work correctly for multithreaded programs).

Mark, this breaks remote i386 targets debugged from linux hosts.
STOPPED_BY_WATCHPOINT is unconditionally defined to a function in
i386-nat.c, but we may not be debugging a native target.

> 
> Mark
> 
> Index: ChangeLog
> from  Mark Kettenis  <kettenis@gnu.org>
> 
>         Make Linux use the new unified support for hardware breakpoints
>         and watchpoints on x86 targets.
>         * i386-linux-nat.c: Doc fixes.  Include "gdb_assert.h".
>         [HAVE_SYS_DEBUGREG_H]: Include <sys/debugreg.h>.
>         (DR_FIRSTADDR, DR_LASTADDR, DR_STATUS, DR_CONTROL): Define to
>         appropriate value if not already defined.
>         (register_u_addr): New function.
>         (kernel_u_size): New function.
>         (i386_linux_dr_get, i386_linux_dr_set): New functions.
>         (i386_linux_dr_set_control, i386_linux_dr_set_addr,
>         i386_linux_reset_addr, i386_linux_dr_get_status): New functions.
>         * config/i386/nm-linux.h: Don't include "nm-i386v.h".
>         (I386_USE_GENERIC_WATCHPOINTS): Define and include "nm-i386.h".
>         (TARGET_HAS_HARDWARE_WATCHPOINTS,
>         TARGET_CAN_USE_HARDWARE_WATCHPOINTS, HAVE_CONTINUABLE_WATCHPOINT,
>         STOPPED_BY_WATCHPOINT, target_insert_watchpoint,
>         target_remove_watchpoint): Remove macros.
>         (i386_stopped_by_watchpoint, i386_insert_watchpoint,
>         i386_remove_watchpoint): Remove prototypes.
>         (register_u_addr): New prototype.
>         (REGISTER_U_ADDR): Define in terms of register_u_addr.
>         (i386_linux_dr_set_control, i386_linux_dr_set_addr,
>         i386_linux_reset_addr, i386_linux_dr_get_status): New prototypes.
>         (I386_DR_LOW_SET_CONTROL, I386_DR_LOW_SET_ADDR,
>         I386_DR_LOW_RESET_ADDR, I386_DR_LOW_GET_STATUS): New macros.
>         * config/i386/linux.mh (NATDEPFILES): Replace i386v-nat.o with
>         i386-nat.o.
> 
> Index: i386-linux-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-linux-nat.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 i386-linux-nat.c
> --- i386-linux-nat.c 2001/03/13 23:31:13 1.23
> +++ i386-linux-nat.c 2001/03/21 21:20:21
> @@ -23,6 +23,7 @@
>  #include "gdbcore.h"
>  #include "regcache.h"
> 
> +#include "gdb_assert.h"
>  #include <sys/ptrace.h>
>  #include <sys/user.h>
>  #include <sys/procfs.h>
> @@ -31,6 +32,26 @@
>  #include <sys/reg.h>
>  #endif
> 
> +#ifdef HAVE_SYS_DEBUGREG_H
> +#include <sys/debugreg.h>
> +#endif
> +
> +#ifndef DR_FIRSTADDR
> +#define DR_FIRSTADDR 0
> +#endif
> +
> +#ifndef DR_LASTADDR
> +#define DR_LASTADDR 3
> +#endif
> +
> +#ifndef DR_STATUS
> +#define DR_STATUS 6
> +#endif
> +
> +#ifndef DR_CONTROL
> +#define DR_CONTROL 7
> +#endif
> +
>  /* Prototypes for supply_gregset etc.  */
>  #include "gregset.h"
> 
> @@ -110,6 +131,26 @@ int have_ptrace_getfpxregs =
>  ;
> 
> 
> +/* Support for the user struct.  */
> +
> +/* Return the address of register REGNUM.  BLOCKEND is the value of
> +   u.u_ar0, which should point to the registers.  */
> +
> +CORE_ADDR
> +register_u_addr (CORE_ADDR blockend, int regnum)
> +{
> +  return (blockend + 4 * regmap[regnum]);
> +}
> +
> +/* Return the size of the user struct.  */
> +
> +int
> +kernel_u_size (void)
> +{
> +  return (sizeof (struct user));
> +}
> +
> +
>  /* Fetching registers directly from the U area, one at a time.  */
> 
>  /* FIXME: kettenis/2000-03-05: This duplicates code from `inptrace.c'.
> @@ -657,6 +698,72 @@ store_inferior_registers (int regno)
> 
>    internal_error (__FILE__, __LINE__,
>                   "Got request to store bad register number %d.", regno);
> +}
> +
> +
> +static long
> +i386_linux_dr_get (int regnum)
> +{
> +  int tid;
> +  long value;
> +
> +  /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
> +     multi-threaded processes here.  For now, pretend there is just
> +     one thread.  */
> +  tid = PIDGET (inferior_pid);
> +
> +  errno = 0;
> +  value = ptrace (PT_READ_U, tid,
> +                 offsetof (struct user, u_debugreg[regnum]), 0);
> +  if (errno != 0)
> +    perror_with_name ("Couldn't read debug register");
> +
> +  return value;
> +}
> +
> +static void
> +i386_linux_dr_set (int regnum, long value)
> +{
> +  int tid;
> +
> +  /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
> +     multi-threaded processes here.  For now, pretend there is just
> +     one thread.  */
> +  tid = PIDGET (inferior_pid);
> +
> +  errno = 0;
> +  ptrace (PT_WRITE_U, tid,
> +         offsetof (struct user, u_debugreg[regnum]), value);
> +  if (errno != 0)
> +    perror_with_name ("Couldn't write debug register");
> +}
> +
> +void
> +i386_linux_dr_set_control (long control)
> +{
> +  i386_linux_dr_set (DR_CONTROL, control);
> +}
> +
> +void
> +i386_linux_dr_set_addr (int regnum, CORE_ADDR addr)
> +{
> +  gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
> +
> +  i386_linux_dr_set (DR_FIRSTADDR + regnum, addr);
> +}
> +
> +void
> +i386_linux_dr_reset_addr (int regnum)
> +{
> +  gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
> +
> +  i386_linux_dr_set (DR_FIRSTADDR + regnum, 0L);
> +}
> +
> +long
> +i386_linux_dr_get_status (void)
> +{
> +  return i386_linux_dr_get (DR_STATUS);
>  }
> 
> 
> Index: config/i386/nm-linux.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/i386/nm-linux.h,v
> retrieving revision 1.8
> diff -u -p -r1.8 nm-linux.h
> --- config/i386/nm-linux.h 2001/03/06 08:21:28 1.8
> +++ config/i386/nm-linux.h 2001/03/21 21:20:21
> @@ -1,6 +1,6 @@
>  /* Native support for Linux/x86.
>     Copyright 1986, 1987, 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
> -   1999, 2000
> +   1999, 2000, 2001
>     Free Software Foundation, Inc.
> 
>     This file is part of GDB.
> @@ -23,52 +23,56 @@
>  #ifndef NM_LINUX_H
>  #define NM_LINUX_H
> 
> -#include "i386/nm-i386v.h"
> +/* GNU/Linux supports the i386 hardware debugging registers.  */
> +#define I386_USE_GENERIC_WATCHPOINTS
> +
> +#include "i386/nm-i386.h"
>  #include "nm-linux.h"
> 
> -/* Return sizeof user struct to callers in less machine dependent routines */
> +/* Return sizeof user struct to callers in less machine dependent
> +   routines.  */
> 
> -#define KERNEL_U_SIZE kernel_u_size()
>  extern int kernel_u_size (void);
> +#define KERNEL_U_SIZE kernel_u_size()
> 
>  #define U_REGS_OFFSET 0
> -
> -/* GNU/Linux supports the 386 hardware debugging registers.  */
> -
> -#define TARGET_HAS_HARDWARE_WATCHPOINTS
> -
> -#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
> 
> -/* After a watchpoint trap, the PC points to the instruction after
> -   the one that caused the trap.  Therefore we don't need to step over it.
> -   But we do need to reset the status register to avoid another trap.  */
> -#define HAVE_CONTINUABLE_WATCHPOINT
> +extern CORE_ADDR register_u_addr (CORE_ADDR blockend, int regnum);
> +#define REGISTER_U_ADDR(addr, blockend, regnum) \
> +  (addr) = register_u_addr (blockend, regnum)
> +
> +/* Provide access to the i386 hardware debugging registers.  */
> +
> +extern void i386_linux_dr_set_control (long control);
> +#define I386_DR_LOW_SET_CONTROL(control) \
> +  i386_linux_dr_set_control (control)
> +
> +extern void i386_linux_dr_set_addr (int regnum, CORE_ADDR addr);
> +#define I386_DR_LOW_SET_ADDR(regnum, addr) \
> +  i386_linux_dr_set_addr (regnum, addr)
> +
> +extern void i386_linux_dr_reset_addr (int regnum);
> +#define I386_DR_LOW_RESET_ADDR(regnum) \
> +  i386_linux_dr_reset_addr (regnum)
> +
> +extern long i386_linux_dr_get_status (void);
> +#define I386_DR_LOW_GET_STATUS() \
> +  i386_linux_dr_get_status ()
> 
> -#define STOPPED_BY_WATCHPOINT(W)  \
> -  i386_stopped_by_watchpoint (inferior_pid)
> +/* We define this if link.h is available, because with ELF we use SVR4
> +   style shared libraries.  */
> 
> -/* Use these macros for watchpoint insertion/removal.  */
> -
> -#define target_insert_watchpoint(addr, len, type)  \
> -  i386_insert_watchpoint (inferior_pid, addr, len, type)
> -
> -#define target_remove_watchpoint(addr, len, type)  \
> -  i386_remove_watchpoint (inferior_pid, addr, len)
> -
> -/* We define this if link.h is available, because with ELF we use SVR4 style
> -   shared libraries. */
> -
>  #ifdef HAVE_LINK_H
>  #define SVR4_SHARED_LIBS
> -#include "solib.h"             /* Support for shared libraries. */
> +#include "solib.h"             /* Support for shared libraries.  */
>  #endif
> 
>  /* Override copies of {fetch,store}_inferior_registers in `infptrace.c'.  */
>  #define FETCH_INFERIOR_REGISTERS
> 
> -/* Nevertheless, define CANNOT_{FETCH,STORE}_REGISTER, because we might fall
> -   back on the code `infptrace.c' (well a copy of that code in
> -   `i386-linux-nat.c' for now) and we can access only the
> +/* Nevertheless, define CANNOT_{FETCH,STORE}_REGISTER, because we
> +   might fall back on the code `infptrace.c' (well a copy of that code
> +   in `i386-linux-nat.c' for now) and we can access only the
>     general-purpose registers in that way.  */
>  extern int cannot_fetch_register (int regno);
>  extern int cannot_store_register (int regno);
> @@ -77,10 +81,6 @@ extern int cannot_store_register (int re
> 
>  /* Override child_resume in `infptrace.c'.  */
>  #define CHILD_RESUME
> -
> -extern CORE_ADDR i386_stopped_by_watchpoint (int);
> -extern int i386_insert_watchpoint (int pid, CORE_ADDR addr, int len, int rw);
> -extern int i386_remove_watchpoint (int pid, CORE_ADDR addr, int len);
> 
>  /* FIXME: kettenis/2000-09-03: This should be moved to ../nm-linux.h
>     once we have converted all Linux targets to use the new threads
> Index: config/i386/linux.mh
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/i386/linux.mh,v
> retrieving revision 1.6
> diff -u -p -r1.6 linux.mh
> --- config/i386/linux.mh 2000/10/30 22:33:32 1.6
> +++ config/i386/linux.mh 2001/03/21 21:20:21
> @@ -5,7 +5,7 @@ XDEPFILES=
> 
>  NAT_FILE= nm-linux.h
>  NATDEPFILES= infptrace.o inftarg.o fork-child.o corelow.o \
> -       core-aout.o i386v-nat.o i386-linux-nat.o i387-nat.o \
> +       core-aout.o i386-nat.o i386-linux-nat.o i387-nat.o \
>         proc-service.o thread-db.o lin-lwp.o
> 
>  LOADLIBES = -ldl -rdynamic

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Mark Kettenis wrote:
> 
> FYI, I checked this in.  HJ can finally be happy now (although things
> probably won't work correctly for multithreaded programs).

Guys, this implementation has problems.  You have it hard-coded so that on a 
linux host, it unconditionally calls native linux methods involving ptrace
to get the debug registers.  This breaks very badly if you're using a native
linux host to debug a remote i386 target.

Seems to me, what you need to do is add these debug registers to the
reg cache, and treat them like ordinary registers.  Then you can just
use the ordinary read_register interface to get them, and remote.c
will do the right thing for you (assuming the target knows about these
extra registers).

Failing that, what you've got now is certainly not right.


> 
> Mark
> 
> Index: ChangeLog
> from  Mark Kettenis  <kettenis@gnu.org>
> 
>         Make Linux use the new unified support for hardware breakpoints
>         and watchpoints on x86 targets.
>         * i386-linux-nat.c: Doc fixes.  Include "gdb_assert.h".
>         [HAVE_SYS_DEBUGREG_H]: Include <sys/debugreg.h>.
>         (DR_FIRSTADDR, DR_LASTADDR, DR_STATUS, DR_CONTROL): Define to
>         appropriate value if not already defined.
>         (register_u_addr): New function.
>         (kernel_u_size): New function.
>         (i386_linux_dr_get, i386_linux_dr_set): New functions.
>         (i386_linux_dr_set_control, i386_linux_dr_set_addr,
>         i386_linux_reset_addr, i386_linux_dr_get_status): New functions.
>         * config/i386/nm-linux.h: Don't include "nm-i386v.h".
>         (I386_USE_GENERIC_WATCHPOINTS): Define and include "nm-i386.h".
>         (TARGET_HAS_HARDWARE_WATCHPOINTS,
>         TARGET_CAN_USE_HARDWARE_WATCHPOINTS, HAVE_CONTINUABLE_WATCHPOINT,
>         STOPPED_BY_WATCHPOINT, target_insert_watchpoint,
>         target_remove_watchpoint): Remove macros.
>         (i386_stopped_by_watchpoint, i386_insert_watchpoint,
>         i386_remove_watchpoint): Remove prototypes.
>         (register_u_addr): New prototype.
>         (REGISTER_U_ADDR): Define in terms of register_u_addr.
>         (i386_linux_dr_set_control, i386_linux_dr_set_addr,
>         i386_linux_reset_addr, i386_linux_dr_get_status): New prototypes.
>         (I386_DR_LOW_SET_CONTROL, I386_DR_LOW_SET_ADDR,
>         I386_DR_LOW_RESET_ADDR, I386_DR_LOW_GET_STATUS): New macros.
>         * config/i386/linux.mh (NATDEPFILES): Replace i386v-nat.o with
>         i386-nat.o.
> 
> Index: i386-linux-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-linux-nat.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 i386-linux-nat.c
> --- i386-linux-nat.c 2001/03/13 23:31:13 1.23
> +++ i386-linux-nat.c 2001/03/21 21:20:21
> @@ -23,6 +23,7 @@
>  #include "gdbcore.h"
>  #include "regcache.h"
> 
> +#include "gdb_assert.h"
>  #include <sys/ptrace.h>
>  #include <sys/user.h>
>  #include <sys/procfs.h>
> @@ -31,6 +32,26 @@
>  #include <sys/reg.h>
>  #endif
> 
> +#ifdef HAVE_SYS_DEBUGREG_H
> +#include <sys/debugreg.h>
> +#endif
> +
> +#ifndef DR_FIRSTADDR
> +#define DR_FIRSTADDR 0
> +#endif
> +
> +#ifndef DR_LASTADDR
> +#define DR_LASTADDR 3
> +#endif
> +
> +#ifndef DR_STATUS
> +#define DR_STATUS 6
> +#endif
> +
> +#ifndef DR_CONTROL
> +#define DR_CONTROL 7
> +#endif
> +
>  /* Prototypes for supply_gregset etc.  */
>  #include "gregset.h"
> 
> @@ -110,6 +131,26 @@ int have_ptrace_getfpxregs =
>  ;
> 
> 
> +/* Support for the user struct.  */
> +
> +/* Return the address of register REGNUM.  BLOCKEND is the value of
> +   u.u_ar0, which should point to the registers.  */
> +
> +CORE_ADDR
> +register_u_addr (CORE_ADDR blockend, int regnum)
> +{
> +  return (blockend + 4 * regmap[regnum]);
> +}
> +
> +/* Return the size of the user struct.  */
> +
> +int
> +kernel_u_size (void)
> +{
> +  return (sizeof (struct user));
> +}
> +
> +
>  /* Fetching registers directly from the U area, one at a time.  */
> 
>  /* FIXME: kettenis/2000-03-05: This duplicates code from `inptrace.c'.
> @@ -657,6 +698,72 @@ store_inferior_registers (int regno)
> 
>    internal_error (__FILE__, __LINE__,
>                   "Got request to store bad register number %d.", regno);
> +}
> +
> +
> +static long
> +i386_linux_dr_get (int regnum)
> +{
> +  int tid;
> +  long value;
> +
> +  /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
> +     multi-threaded processes here.  For now, pretend there is just
> +     one thread.  */
> +  tid = PIDGET (inferior_pid);
> +
> +  errno = 0;
> +  value = ptrace (PT_READ_U, tid,
> +                 offsetof (struct user, u_debugreg[regnum]), 0);
> +  if (errno != 0)
> +    perror_with_name ("Couldn't read debug register");
> +
> +  return value;
> +}
> +
> +static void
> +i386_linux_dr_set (int regnum, long value)
> +{
> +  int tid;
> +
> +  /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
> +     multi-threaded processes here.  For now, pretend there is just
> +     one thread.  */
> +  tid = PIDGET (inferior_pid);
> +
> +  errno = 0;
> +  ptrace (PT_WRITE_U, tid,
> +         offsetof (struct user, u_debugreg[regnum]), value);
> +  if (errno != 0)
> +    perror_with_name ("Couldn't write debug register");
> +}
> +
> +void
> +i386_linux_dr_set_control (long control)
> +{
> +  i386_linux_dr_set (DR_CONTROL, control);
> +}
> +
> +void
> +i386_linux_dr_set_addr (int regnum, CORE_ADDR addr)
> +{
> +  gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
> +
> +  i386_linux_dr_set (DR_FIRSTADDR + regnum, addr);
> +}
> +
> +void
> +i386_linux_dr_reset_addr (int regnum)
> +{
> +  gdb_assert (regnum >= 0 && regnum <= DR_LASTADDR - DR_FIRSTADDR);
> +
> +  i386_linux_dr_set (DR_FIRSTADDR + regnum, 0L);
> +}
> +
> +long
> +i386_linux_dr_get_status (void)
> +{
> +  return i386_linux_dr_get (DR_STATUS);
>  }
> 
> 
> Index: config/i386/nm-linux.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/i386/nm-linux.h,v
> retrieving revision 1.8
> diff -u -p -r1.8 nm-linux.h
> --- config/i386/nm-linux.h 2001/03/06 08:21:28 1.8
> +++ config/i386/nm-linux.h 2001/03/21 21:20:21
> @@ -1,6 +1,6 @@
>  /* Native support for Linux/x86.
>     Copyright 1986, 1987, 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
> -   1999, 2000
> +   1999, 2000, 2001
>     Free Software Foundation, Inc.
> 
>     This file is part of GDB.
> @@ -23,52 +23,56 @@
>  #ifndef NM_LINUX_H
>  #define NM_LINUX_H
> 
> -#include "i386/nm-i386v.h"
> +/* GNU/Linux supports the i386 hardware debugging registers.  */
> +#define I386_USE_GENERIC_WATCHPOINTS
> +
> +#include "i386/nm-i386.h"
>  #include "nm-linux.h"
> 
> -/* Return sizeof user struct to callers in less machine dependent routines */
> +/* Return sizeof user struct to callers in less machine dependent
> +   routines.  */
> 
> -#define KERNEL_U_SIZE kernel_u_size()
>  extern int kernel_u_size (void);
> +#define KERNEL_U_SIZE kernel_u_size()
> 
>  #define U_REGS_OFFSET 0
> -
> -/* GNU/Linux supports the 386 hardware debugging registers.  */
> -
> -#define TARGET_HAS_HARDWARE_WATCHPOINTS
> -
> -#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(type, cnt, ot) 1
> 
> -/* After a watchpoint trap, the PC points to the instruction after
> -   the one that caused the trap.  Therefore we don't need to step over it.
> -   But we do need to reset the status register to avoid another trap.  */
> -#define HAVE_CONTINUABLE_WATCHPOINT
> +extern CORE_ADDR register_u_addr (CORE_ADDR blockend, int regnum);
> +#define REGISTER_U_ADDR(addr, blockend, regnum) \
> +  (addr) = register_u_addr (blockend, regnum)
> +
> +/* Provide access to the i386 hardware debugging registers.  */
> +
> +extern void i386_linux_dr_set_control (long control);
> +#define I386_DR_LOW_SET_CONTROL(control) \
> +  i386_linux_dr_set_control (control)
> +
> +extern void i386_linux_dr_set_addr (int regnum, CORE_ADDR addr);
> +#define I386_DR_LOW_SET_ADDR(regnum, addr) \
> +  i386_linux_dr_set_addr (regnum, addr)
> +
> +extern void i386_linux_dr_reset_addr (int regnum);
> +#define I386_DR_LOW_RESET_ADDR(regnum) \
> +  i386_linux_dr_reset_addr (regnum)
> +
> +extern long i386_linux_dr_get_status (void);
> +#define I386_DR_LOW_GET_STATUS() \
> +  i386_linux_dr_get_status ()
> 
> -#define STOPPED_BY_WATCHPOINT(W)  \
> -  i386_stopped_by_watchpoint (inferior_pid)
> +/* We define this if link.h is available, because with ELF we use SVR4
> +   style shared libraries.  */
> 
> -/* Use these macros for watchpoint insertion/removal.  */
> -
> -#define target_insert_watchpoint(addr, len, type)  \
> -  i386_insert_watchpoint (inferior_pid, addr, len, type)
> -
> -#define target_remove_watchpoint(addr, len, type)  \
> -  i386_remove_watchpoint (inferior_pid, addr, len)
> -
> -/* We define this if link.h is available, because with ELF we use SVR4 style
> -   shared libraries. */
> -
>  #ifdef HAVE_LINK_H
>  #define SVR4_SHARED_LIBS
> -#include "solib.h"             /* Support for shared libraries. */
> +#include "solib.h"             /* Support for shared libraries.  */
>  #endif
> 
>  /* Override copies of {fetch,store}_inferior_registers in `infptrace.c'.  */
>  #define FETCH_INFERIOR_REGISTERS
> 
> -/* Nevertheless, define CANNOT_{FETCH,STORE}_REGISTER, because we might fall
> -   back on the code `infptrace.c' (well a copy of that code in
> -   `i386-linux-nat.c' for now) and we can access only the
> +/* Nevertheless, define CANNOT_{FETCH,STORE}_REGISTER, because we
> +   might fall back on the code `infptrace.c' (well a copy of that code
> +   in `i386-linux-nat.c' for now) and we can access only the
>     general-purpose registers in that way.  */
>  extern int cannot_fetch_register (int regno);
>  extern int cannot_store_register (int regno);
> @@ -77,10 +81,6 @@ extern int cannot_store_register (int re
> 
>  /* Override child_resume in `infptrace.c'.  */
>  #define CHILD_RESUME
> -
> -extern CORE_ADDR i386_stopped_by_watchpoint (int);
> -extern int i386_insert_watchpoint (int pid, CORE_ADDR addr, int len, int rw);
> -extern int i386_remove_watchpoint (int pid, CORE_ADDR addr, int len);
> 
>  /* FIXME: kettenis/2000-09-03: This should be moved to ../nm-linux.h
>     once we have converted all Linux targets to use the new threads
> Index: config/i386/linux.mh
> ===================================================================
> RCS file: /cvs/src/src/gdb/config/i386/linux.mh,v
> retrieving revision 1.6
> diff -u -p -r1.6 linux.mh
> --- config/i386/linux.mh 2000/10/30 22:33:32 1.6
> +++ config/i386/linux.mh 2001/03/21 21:20:21
> @@ -5,7 +5,7 @@ XDEPFILES=
> 
>  NAT_FILE= nm-linux.h
>  NATDEPFILES= infptrace.o inftarg.o fork-child.o corelow.o \
> -       core-aout.o i386v-nat.o i386-linux-nat.o i387-nat.o \
> +       core-aout.o i386-nat.o i386-linux-nat.o i387-nat.o \
>         proc-service.o thread-db.o lin-lwp.o
> 
>  LOADLIBES = -ldl -rdynamic

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Mon, 26 Mar 2001, Michael Snyder wrote:

> Guys, this implementation has problems.  You have it hard-coded so that on a 
> linux host, it unconditionally calls native linux methods involving ptrace
> to get the debug registers.  This breaks very badly if you're using a native
> linux host to debug a remote i386 target.

Sorry, I'm not following.  The watchpoint-related macros are defined
on files which should be used only with native debugging (i386-nat.c
and nm-i386.h).  On top of that, the macros only get exposed if the
port defines I386_USE_GENERIC_WATCHPOINTS.  A port which doesn't want
that should get rid of the watchpoints for free, by simply not
defining I386_USE_GENERIC_WATCHPOINTS.

Which part of the above misfires, and why?

> Seems to me, what you need to do is add these debug registers to the
> reg cache, and treat them like ordinary registers.

This possibility has been discussed back in November, but the
conclusion was that it's not a good idea.  I don't remember the
details, but the reasons had something to do with threads and how
the register cache is used in conjunction with threads.  (I can dig
out the URLs of the relevant messages, if you want to read them.)

> Then you can just use the ordinary read_register interface to get
> them, and remote.c will do the right thing for you (assuming the
> target knows about these extra registers).

Mark explicitly didn't want the watchpoint code to be in the
target-depenent files, so watchpoints cannot currently work for remote
targets.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Kettenis]

   Date: Mon, 26 Mar 2001 18:14:00 -0800
   From: Michael Snyder <msnyder@cygnus.com>

   Mark Kettenis wrote:
   > 
   > FYI, I checked this in.  HJ can finally be happy now (although things
   > probably won't work correctly for multithreaded programs).

   Mark, this breaks remote i386 targets debugged from linux hosts.
   STOPPED_BY_WATCHPOINT is unconditionally defined to a function in
   i386-nat.c, but we may not be debugging a native target.

STOPPED_BY_WATCHPOINT always has been unconditionally defined.  The
only difference I can see is that with the old stuff is that I now
call perror_with_name if the ptrace call fails.

If I'm right, the attached patch should fix your problems.  I cannot
test it right now (no access to a Linux/x86 box), but if it works for
you, feel free to check it in.

More details in the reply to your other message.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* i386-linux-nat.c (i386_linux_dr_get): Return 0 if ptrace call
	fails instead of calling perror_with_name.  This should fix
	debugging remote i386 targets with a native Linux/x86 GDB.  Add
	FIXME for this hack.

Index: i386-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-linux-nat.c,v
retrieving revision 1.24
diff -u -r1.24 i386-linux-nat.c
--- i386-linux-nat.c 2001/03/21 21:22:48 1.24
+++ i386-linux-nat.c 2001/03/27 08:40:06
@@ -712,11 +712,20 @@
      one thread.  */
   tid = PIDGET (inferior_pid);
 
+  /* FIXME: kettenis/2001-03-27: Calling perror_with_name if the
+     ptrace call fails breaks debugging remote targets.  The correct
+     way to fix this is to add the hardware breakpoint and watchpoint
+     stuff to the target vectore.  For now, just return zero if the
+     ptrace call fails.  */
   errno = 0;
   value = ptrace (PT_READ_U, tid,
 		  offsetof (struct user, u_debugreg[regnum]), 0);
   if (errno != 0)
+#if 0
     perror_with_name ("Couldn't read debug register");
+#else
+    return 0;
+#endif
 
   return value;
 }

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Kettenis]

   Date: Tue, 27 Mar 2001 08:55:00 +0200 (IST)
   From: Eli Zaretskii <eliz@is.elta.co.il>

   On Mon, 26 Mar 2001, Michael Snyder wrote:

   > Guys, this implementation has problems.  You have it hard-coded
   > so that on a linux host, it unconditionally calls native linux
   > methods involving ptrace to get the debug registers.  This breaks
   > very badly if you're using a native linux host to debug a remote
   > i386 target.

Just like the old implementation.  I suspect the old implementation
just happened to work because it didn't do any strict error checking.

The fundamental problem is that the watpoint-stuff isn't part of the
target vector.

   Sorry, I'm not following.  The watchpoint-related macros are defined
   on files which should be used only with native debugging (i386-nat.c
   and nm-i386.h).  On top of that, the macros only get exposed if the
   port defines I386_USE_GENERIC_WATCHPOINTS.  A port which doesn't want
   that should get rid of the watchpoints for free, by simply not
   defining I386_USE_GENERIC_WATCHPOINTS.

   Which part of the above misfires, and why?

It used to be possible to debug a remote i386 target with a native
Linux/x86 debugger.

   > Seems to me, what you need to do is add these debug registers to the
   > reg cache, and treat them like ordinary registers.

   This possibility has been discussed back in November, but the
   conclusion was that it's not a good idea.  I don't remember the
   details, but the reasons had something to do with threads and how
   the register cache is used in conjunction with threads.  (I can dig
   out the URLs of the relevant messages, if you want to read them.)

I suggested doing this, but several people objected to exposing the
debug registers in this way.  Threads have nothing to do with it
(actually doing the correct thing for threads would make it easier if
the debug registers would be part of the register cache).

   > Then you can just use the ordinary read_register interface to get
   > them, and remote.c will do the right thing for you (assuming the
   > target knows about these extra registers).

   Mark explicitly didn't want the watchpoint code to be in the
   target-depenent files, so watchpoints cannot currently work for remote
   targets.

Actually, our problems would only be bigger if the watchpoint code was
part of the target-dependent files.  But if we add the debug registers
to the register cache and add the watchpoint stuff to the target
vector, only a few changes are necessary to make the watchpoint code
ready for a move to i386-tdep.c.

Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Tue, 27 Mar 2001, Mark Kettenis wrote:

>    Sorry, I'm not following.  The watchpoint-related macros are defined
>    on files which should be used only with native debugging (i386-nat.c
>    and nm-i386.h).  On top of that, the macros only get exposed if the
>    port defines I386_USE_GENERIC_WATCHPOINTS.  A port which doesn't want
>    that should get rid of the watchpoints for free, by simply not
>    defining I386_USE_GENERIC_WATCHPOINTS.
> 
>    Which part of the above misfires, and why?
> 
> It used to be possible to debug a remote i386 target with a native
> Linux/x86 debugger.

This sounds as The Source of All Evil, doesn't it?  Can GDB really be 
tweaked into debugging a remote target with a native debug code?  I'm 
surprised that it even works, unless there are some extra-special hacks 
lying around, just for this.

>    This possibility has been discussed back in November, but the
>    conclusion was that it's not a good idea.  I don't remember the
>    details, but the reasons had something to do with threads and how
>    the register cache is used in conjunction with threads.  (I can dig
>    out the URLs of the relevant messages, if you want to read them.)
> 
> I suggested doing this, but several people objected to exposing the
> debug registers in this way.  Threads have nothing to do with it

I'm quite sure they did.  IIRC, the issue was that debug registers are 
global, whereas normal registers are per thread.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Kettenis]

   Date: Tue, 27 Mar 2001 11:29:19 +0200 (IST)
   From: Eli Zaretskii <eliz@is.elta.co.il>

   On Tue, 27 Mar 2001, Mark Kettenis wrote:

   >    Sorry, I'm not following.  The watchpoint-related macros are defined
   >    on files which should be used only with native debugging (i386-nat.c
   >    and nm-i386.h).  On top of that, the macros only get exposed if the
   >    port defines I386_USE_GENERIC_WATCHPOINTS.  A port which doesn't want
   >    that should get rid of the watchpoints for free, by simply not
   >    defining I386_USE_GENERIC_WATCHPOINTS.
   > 
   >    Which part of the above misfires, and why?
   > 
   > It used to be possible to debug a remote i386 target with a native
   > Linux/x86 debugger.

   This sounds as The Source of All Evil, doesn't it?  Can GDB really be 
   tweaked into debugging a remote target with a native debug code?  I'm 
   surprised that it even works, unless there are some extra-special hacks 
   lying around, just for this.

AFAIK it has been possible for a long time.  As long as the ISA and
ABI match, there shouldn't be any problems.  Except that the
implementation of hardware breakpoints/watchpoints in core GDB is
really shoddy.

   >    This possibility has been discussed back in November, but the
   >    conclusion was that it's not a good idea.  I don't remember the
   >    details, but the reasons had something to do with threads and how
   >    the register cache is used in conjunction with threads.  (I can dig
   >    out the URLs of the relevant messages, if you want to read them.)
   > 
   > I suggested doing this, but several people objected to exposing the
   > debug registers in this way.  Threads have nothing to do with it

   I'm quite sure they did.  IIRC, the issue was that debug registers are 
   global, whereas normal registers are per thread.

I guess that depends on the underlying OS.  In Linux they are
per-process, which implies they are per-thread too.  On bare hardware,
they might be global.  With the current implementation of GDB's
register cache (which only caches registers for a single
process/thread) the difference shouldn't matter.  It's just that any
code that writes to these registers (or a user manipulating them)
should realize that it (they) might be changing a global property.

Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Tue, 27 Mar 2001, Mark Kettenis wrote:

> Except that the implementation of hardware breakpoints/watchpoints in 
> core GDB is really shoddy.

True.  It clearly shows that it was originally written for a very special 
platform (Sparclet, I think) and was designed accordingly.  Certain 
aspects of how breakpoint.c handles watchpoints cannot be understood 
without having this in mind.  For example, why on Earth are 
software watchpoints and hardware watchpoints handled differently from 
read and access watchpoints? why are hardware breakpoints treated like 
normal breakpoints instead of being akin to watchpoints? etc., etc.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Mark Kettenis wrote:
> 
>    Date: Mon, 26 Mar 2001 18:14:00 -0800
>    From: Michael Snyder <msnyder@cygnus.com>
> 
>    Mark Kettenis wrote:
>    >
>    > FYI, I checked this in.  HJ can finally be happy now (although things
>    > probably won't work correctly for multithreaded programs).
> 
>    Mark, this breaks remote i386 targets debugged from linux hosts.
>    STOPPED_BY_WATCHPOINT is unconditionally defined to a function in
>    i386-nat.c, but we may not be debugging a native target.
> 
> STOPPED_BY_WATCHPOINT always has been unconditionally defined.  The
> only difference I can see is that with the old stuff is that I now
> call perror_with_name if the ptrace call fails.
> 
> If I'm right, the attached patch should fix your problems.  I cannot
> test it right now (no access to a Linux/x86 box), but if it works for
> you, feel free to check it in.

No help.  When I'm debugging a remote target, the ptrace call
will fail.

> 
> More details in the reply to your other message.
> 
> Mark
> 
> Index: ChangeLog
> from  Mark Kettenis  <kettenis@gnu.org>
> 
>         * i386-linux-nat.c (i386_linux_dr_get): Return 0 if ptrace call
>         fails instead of calling perror_with_name.  This should fix
>         debugging remote i386 targets with a native Linux/x86 GDB.  Add
>         FIXME for this hack.
> 
> Index: i386-linux-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-linux-nat.c,v
> retrieving revision 1.24
> diff -u -r1.24 i386-linux-nat.c
> --- i386-linux-nat.c 2001/03/21 21:22:48 1.24
> +++ i386-linux-nat.c 2001/03/27 08:40:06
> @@ -712,11 +712,20 @@
>       one thread.  */
>    tid = PIDGET (inferior_pid);
> 
> +  /* FIXME: kettenis/2001-03-27: Calling perror_with_name if the
> +     ptrace call fails breaks debugging remote targets.  The correct
> +     way to fix this is to add the hardware breakpoint and watchpoint
> +     stuff to the target vectore.  For now, just return zero if the
> +     ptrace call fails.  */
>    errno = 0;
>    value = ptrace (PT_READ_U, tid,
>                   offsetof (struct user, u_debugreg[regnum]), 0);
>    if (errno != 0)
> +#if 0
>      perror_with_name ("Couldn't read debug register");
> +#else
> +    return 0;
> +#endif
> 
>    return value;
>  }

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Eli Zaretskii wrote:
> 
> On Mon, 26 Mar 2001, Michael Snyder wrote:
> 
> > Guys, this implementation has problems.  You have it hard-coded so that on a
> > linux host, it unconditionally calls native linux methods involving ptrace
> > to get the debug registers.  This breaks very badly if you're using a native
> > linux host to debug a remote i386 target.
> 
> Sorry, I'm not following.  The watchpoint-related macros are defined
> on files which should be used only with native debugging (i386-nat.c
> and nm-i386.h).  On top of that, the macros only get exposed if the
> port defines I386_USE_GENERIC_WATCHPOINTS.  A port which doesn't want
> that should get rid of the watchpoints for free, by simply not
> defining I386_USE_GENERIC_WATCHPOINTS.
> 
> Which part of the above misfires, and why?

The gdb was configured for native linux, but until this change, 
it was also able to be used to debug a remote embedded i386
target.  This change breaks that, because it makes ptrace calls.
If debugging target remote, there is no process on the host machine
on which to make a ptrace call.

> 
> > Seems to me, what you need to do is add these debug registers to the
> > reg cache, and treat them like ordinary registers.
> 
> This possibility has been discussed back in November, but the
> conclusion was that it's not a good idea.  I don't remember the
> details, but the reasons had something to do with threads and how
> the register cache is used in conjunction with threads.  (I can dig
> out the URLs of the relevant messages, if you want to read them.)
> 
> > Then you can just use the ordinary read_register interface to get
> > them, and remote.c will do the right thing for you (assuming the
> > target knows about these extra registers).
> 
> Mark explicitly didn't want the watchpoint code to be in the
> target-depenent files, so watchpoints cannot currently work for remote
> targets.

I don't mind if watchpoints don't work for remote targets --
but the code as written prevents me from debugging a remote
target at all.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Mark Kettenis wrote:
> 
>    Date: Tue, 27 Mar 2001 08:55:00 +0200 (IST)
>    From: Eli Zaretskii <eliz@is.elta.co.il>
> 
>    On Mon, 26 Mar 2001, Michael Snyder wrote:
> 
>    > Guys, this implementation has problems.  You have it hard-coded
>    > so that on a linux host, it unconditionally calls native linux
>    > methods involving ptrace to get the debug registers.  This breaks
>    > very badly if you're using a native linux host to debug a remote
>    > i386 target.
> 
> Just like the old implementation.  I suspect the old implementation
> just happened to work because it didn't do any strict error checking.

No, you're on the wrong track.  I haven't looked at how the old
implementation worked, but the problem with the new implementation
is _not_ the perror call.  It's the ptrace call that preceeds it.
You can't call ptrace if the process you're debugging is remote.

 
> The fundamental problem is that the watpoint-stuff isn't part of the
> target vector.

Yes, you're absolutely right about this.  This is why I don't
like ptrace calls being made from outside of the target vector.

 
>    Sorry, I'm not following.  The watchpoint-related macros are defined
>    on files which should be used only with native debugging (i386-nat.c
>    and nm-i386.h).  On top of that, the macros only get exposed if the
>    port defines I386_USE_GENERIC_WATCHPOINTS.  A port which doesn't want
>    that should get rid of the watchpoints for free, by simply not
>    defining I386_USE_GENERIC_WATCHPOINTS.
> 
>    Which part of the above misfires, and why?
> 
> It used to be possible to debug a remote i386 target with a native
> Linux/x86 debugger.
> 
>    > Seems to me, what you need to do is add these debug registers to the
>    > reg cache, and treat them like ordinary registers.
> 
>    This possibility has been discussed back in November, but the
>    conclusion was that it's not a good idea.  I don't remember the
>    details, but the reasons had something to do with threads and how
>    the register cache is used in conjunction with threads.  (I can dig
>    out the URLs of the relevant messages, if you want to read them.)
> 
> I suggested doing this, but several people objected to exposing the
> debug registers in this way.  Threads have nothing to do with it
> (actually doing the correct thing for threads would make it easier if
> the debug registers would be part of the register cache).

Yes.  But it's OK, 'cause breakpoints don't really work on a
per-thread basis either.  The process will stop no matter which
thread hits the breakpoint, and then GDB will decide whether 
the breakpoint was hit by the thread-of-interest.  Can do the
same for watchpoints.

>    > Then you can just use the ordinary read_register interface to get
>    > them, and remote.c will do the right thing for you (assuming the
>    > target knows about these extra registers).
> 
>    Mark explicitly didn't want the watchpoint code to be in the
>    target-depenent files, so watchpoints cannot currently work for remote
>    targets.
> 
> Actually, our problems would only be bigger if the watchpoint code was
> part of the target-dependent files.  But if we add the debug registers
> to the register cache and add the watchpoint stuff to the target
> vector, only a few changes are necessary to make the watchpoint code
> ready for a move to i386-tdep.c.
> 
> Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Eli Zaretskii wrote:
> 
> On Tue, 27 Mar 2001, Mark Kettenis wrote:
> 
> >    Sorry, I'm not following.  The watchpoint-related macros are defined
> >    on files which should be used only with native debugging (i386-nat.c
> >    and nm-i386.h).  On top of that, the macros only get exposed if the
> >    port defines I386_USE_GENERIC_WATCHPOINTS.  A port which doesn't want
> >    that should get rid of the watchpoints for free, by simply not
> >    defining I386_USE_GENERIC_WATCHPOINTS.
> >
> >    Which part of the above misfires, and why?
> >
> > It used to be possible to debug a remote i386 target with a native
> > Linux/x86 debugger.
> 
> This sounds as The Source of All Evil, doesn't it? 

No, it's quite common.  A sparc-solaris gdb can debug
embedded sparc boards, for instance.

> Can GDB really be
> tweaked into debugging a remote target with a native debug code?  I'm
> surprised that it even works, unless there are some extra-special hacks
> lying around, just for this.

It works.  Or it used to...  and it's quite important that it
continue to work, for debugging embedded Linux.

> 
> >    This possibility has been discussed back in November, but the
> >    conclusion was that it's not a good idea.  I don't remember the
> >    details, but the reasons had something to do with threads and how
> >    the register cache is used in conjunction with threads.  (I can dig
> >    out the URLs of the relevant messages, if you want to read them.)
> >
> > I suggested doing this, but several people objected to exposing the
> > debug registers in this way.  Threads have nothing to do with it
> 
> I'm quite sure they did.  IIRC, the issue was that debug registers are
> global, whereas normal registers are per thread.

Mmmmm... that's probably true.  However the effect would just be
that all threads appear to have the same values for debug regs.
If you change them in one, they'll change in all.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

> Date: Tue, 27 Mar 2001 08:47:16 -0800
> From: Michael Snyder <msnyder@redhat.com>
> 
> The gdb was configured for native linux, but until this change, 
> it was also able to be used to debug a remote embedded i386
> target.  This change breaks that, because it makes ptrace calls.

How does GDB know if the target is local or remote?  Can the Linux
implementation of I386_DR_LOW_* macros test that and avoid calling
ptrace in that case?

Also, since I see that the Linux port of GDB was linking in
i386v-nat.c, and i386v-nat.c defined those same watchpoint-related
functions which called ptrace, and did that unconditionally, how did
that work before the last changes?

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

> Date: Tue, 27 Mar 2001 08:54:14 -0800
> From: Michael Snyder <msnyder@redhat.com>
> 
> > >    This possibility has been discussed back in November, but the
> > >    conclusion was that it's not a good idea.  I don't remember the
> > >    details, but the reasons had something to do with threads and how
> > >    the register cache is used in conjunction with threads.  (I can dig
> > >    out the URLs of the relevant messages, if you want to read them.)
> > >
> > > I suggested doing this, but several people objected to exposing the
> > > debug registers in this way.  Threads have nothing to do with it
> > 
> > I'm quite sure they did.  IIRC, the issue was that debug registers are
> > global, whereas normal registers are per thread.
> 
> Mmmmm... that's probably true.  However the effect would just be
> that all threads appear to have the same values for debug regs.
> If you change them in one, they'll change in all.

As far as I understood, GDB's regcache cannot do such magic.  It
assumes that all the registers are per thread, so if you change them
in one thread, the other threads' registers remain unchanged in the
cache.

In other words, this kind of global registers is (or was when we
discussed it) incompatible with how regcache works.  That is why I was
advised not to introduce the debug registers into the regcache.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Fernando Nasser]

Eli Zaretskii wrote:
> 
> In other words, this kind of global registers is (or was when we
> discussed it) incompatible with how regcache works.  That is why I was
> advised not to introduce the debug registers into the regcache.

I agree.  The user has no reason to be meddling with those (unless
he/she is debugging a debugger, but he/she could still check the values
that are loaded there while they are still in GPS registers).

I think we should consider these debug registers as some piece of
debugger hardware similar to one of those JTAG devices, a kind of
CPU-embeded logic analyzer or something of a sort -- some debugger
device that GDB uses, not as an inferior resource.


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Andrew Cagney]

Mark Kettenis wrote:
> 
>    Date: Tue, 27 Mar 2001 08:55:00 +0200 (IST)
>    From: Eli Zaretskii <eliz@is.elta.co.il>
> 
>    On Mon, 26 Mar 2001, Michael Snyder wrote:
> 
>    > Guys, this implementation has problems.  You have it hard-coded
>    > so that on a linux host, it unconditionally calls native linux
>    > methods involving ptrace to get the debug registers.  This breaks
>    > very badly if you're using a native linux host to debug a remote
>    > i386 target.
> 
> Just like the old implementation.  I suspect the old implementation
> just happened to work because it didn't do any strict error checking.
> 
> The fundamental problem is that the watpoint-stuff isn't part of the
> target vector.

I think it is a combination of both the architecture and the target
vector.  It is the same problem as software single step.  A layer of the
target is asked to implement a memory watch.  Since that can't be
implemented directly, it asks the architecture vector to implement it
for it.

	Andrew

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Salter]

>>>>> Eli Zaretskii writes:

> On Tue, 27 Mar 2001, Mark Kettenis wrote:

>> Except that the implementation of hardware breakpoints/watchpoints in 
>> core GDB is really shoddy.

> True.  It clearly shows that it was originally written for a very special 
> platform (Sparclet, I think) and was designed accordingly.  Certain 
> aspects of how breakpoint.c handles watchpoints cannot be understood 
> without having this in mind.  For example, why on Earth are 
> software watchpoints and hardware watchpoints handled differently from 
> read and access watchpoints? why are hardware breakpoints treated like 
> normal breakpoints instead of being akin to watchpoints? etc., etc.

Not just shoddy, but plain broken for a number of targets.

When a read wathcpoint is triggered, the target stops and informs gdb.
In breakpoint.c, gdb sees that there are read or access watchpoints set
and that the data address reported by the target matches. This causes
watchpoint_check() to be called. The problem is that watchpoint_check()
will try to  read from the watched data area to see if it changed, but
this is done before gdb has removed watchpoints from the target. This
causes the target to respond with an error when gdb tries to access the
watched area.

--Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Eli Zaretskii wrote:
> 
> > Date: Tue, 27 Mar 2001 08:47:16 -0800
> > From: Michael Snyder <msnyder@redhat.com>
> >
> > The gdb was configured for native linux, but until this change,
> > it was also able to be used to debug a remote embedded i386
> > target.  This change breaks that, because it makes ptrace calls.
> 
> How does GDB know if the target is local or remote?  Can the Linux
> implementation of I386_DR_LOW_* macros test that and avoid calling
> ptrace in that case?

If you say "run" it's native -- if you say "target remote /dev/tty0"
it's remote.  Or did you mean how can you tell programmatically?
There is no clean way, other than to test "current_target", eg.
	if (strcmp (current_target.to_shortname, "remote") == 0)
and that's obviously not the nicest approach.


> Also, since I see that the Linux port of GDB was linking in
> i386v-nat.c, and i386v-nat.c defined those same watchpoint-related
> functions which called ptrace, and did that unconditionally, how did
> that work before the last changes?

I've no idea, I just know that it suddenly stopped working
with these changes.  And I'm not talking about watchpoints
working -- I'm talking about _anything_ working.  There is a
call to STOPPED_BY_WATCHPOINT in wait_for_inferior, and
you can't even connect to the target without wait_for_inferior
being called.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Eli Zaretskii wrote:
> 
> > Date: Tue, 27 Mar 2001 08:54:14 -0800
> > From: Michael Snyder <msnyder@redhat.com>
> >
> > > >    This possibility has been discussed back in November, but the
> > > >    conclusion was that it's not a good idea.  I don't remember the
> > > >    details, but the reasons had something to do with threads and how
> > > >    the register cache is used in conjunction with threads.  (I can dig
> > > >    out the URLs of the relevant messages, if you want to read them.)
> > > >
> > > > I suggested doing this, but several people objected to exposing the
> > > > debug registers in this way.  Threads have nothing to do with it
> > >
> > > I'm quite sure they did.  IIRC, the issue was that debug registers are
> > > global, whereas normal registers are per thread.
> >
> > Mmmmm... that's probably true.  However the effect would just be
> > that all threads appear to have the same values for debug regs.
> > If you change them in one, they'll change in all.
> 
> As far as I understood, GDB's regcache cannot do such magic.  It
> assumes that all the registers are per thread, so if you change them
> in one thread, the other threads' registers remain unchanged in the
> cache.

Currently there is only one cache (not one per thread).
When you change threads, the cache is flushed.  However, 
there are plans for a per-thread cache, so your concern
is not ill-placed.


> In other words, this kind of global registers is (or was when we
> discussed it) incompatible with how regcache works.  That is why I was
> advised not to introduce the debug registers into the regcache.

They could be pseudo-registers, which means the debugger would
have the opportunity to handle them specially.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Fernando Nasser wrote:
> 
> Eli Zaretskii wrote:
> >
> > In other words, this kind of global registers is (or was when we
> > discussed it) incompatible with how regcache works.  That is why I was
> > advised not to introduce the debug registers into the regcache.
> 
> I agree.  The user has no reason to be meddling with those (unless
> he/she is debugging a debugger, but he/she could still check the values
> that are loaded there while they are still in GPS registers).
> 
> I think we should consider these debug registers as some piece of
> debugger hardware similar to one of those JTAG devices, a kind of
> CPU-embeded logic analyzer or something of a sort -- some debugger
> device that GDB uses, not as an inferior resource.

Then you need some target-vector-dependent way to fetch them.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Fernando Nasser]

Michael Snyder wrote:
> 
> Then you need some target-vector-dependent way to fetch them.

We all seem to agree that we need some target vector entries (and
multiarch) for dealing with this.  It is just that nobody found time to
do it yet.


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Kettenis]

   Date: Tue, 27 Mar 2001 08:51:10 -0800
   From: Michael Snyder <msnyder@redhat.com>

   Mark Kettenis wrote:
   > 
   >    Date: Tue, 27 Mar 2001 08:55:00 +0200 (IST)
   >    From: Eli Zaretskii <eliz@is.elta.co.il>
   > 
   >    On Mon, 26 Mar 2001, Michael Snyder wrote:
   > 
   >    > Guys, this implementation has problems.  You have it hard-coded
   >    > so that on a linux host, it unconditionally calls native linux
   >    > methods involving ptrace to get the debug registers.  This breaks
   >    > very badly if you're using a native linux host to debug a remote
   >    > i386 target.
   > 
   > Just like the old implementation.  I suspect the old implementation
   > just happened to work because it didn't do any strict error checking.

   No, you're on the wrong track.  I haven't looked at how the old
   implementation worked, but the problem with the new implementation
   is _not_ the perror call.  It's the ptrace call that preceeds it.
   You can't call ptrace if the process you're debugging is remote.

Did you actually try my patch?  I just did, using gdbserver (which by
the way needs some fixing for Linux/x86, I'll see what I can do), and
it seems to fix the problems for me.  Note that it isn't really a
problem to call ptrace if the process you're debugging is remote.
You'll just call ptrace on untraced (and possibly non-existing)
process, and it will fail with ESRCH.

   > The fundamental problem is that the watpoint-stuff isn't part of the
   > target vector.

   Yes, you're absolutely right about this.  This is why I don't
   like ptrace calls being made from outside of the target vector.

Me neither, but this seems a reasonable temporary workaround for the
problem, especially since GDB has been doing something similar for
quite some time.

Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Tue, 27 Mar 2001, Mark Salter wrote:

> When a read wathcpoint is triggered, the target stops and informs gdb.
> In breakpoint.c, gdb sees that there are read or access watchpoints set
> and that the data address reported by the target matches. This causes
> watchpoint_check() to be called. The problem is that watchpoint_check()
> will try to  read from the watched data area to see if it changed, but
> this is done before gdb has removed watchpoints from the target. This
> causes the target to respond with an error when gdb tries to access the
> watched area.

Sorry, I don't understand: why does reading a watched region generate
an error?  At least in the x86 implementation, watchpoints are set to
be task-local, so reading the data from GDB, which is another process,
should not produce any errors.  Am I missing something?

Could you please post a reproducible test case where this happens, and
tell what host/target do you see that with?

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Tue, 27 Mar 2001, Michael Snyder wrote:

> > How does GDB know if the target is local or remote?  Can the Linux
> > implementation of I386_DR_LOW_* macros test that and avoid calling
> > ptrace in that case?
> 
> If you say "run" it's native -- if you say "target remote /dev/tty0"
> it's remote.  Or did you mean how can you tell programmatically?

Programmatically, yes.

> There is no clean way, other than to test "current_target", eg.
> 	if (strcmp (current_target.to_shortname, "remote") == 0)
> and that's obviously not the nicest approach.

It's not nice, but I386_DR_LOW_* macros (or the Linux-specific
functions they call) could make such a test and avoid ptrace, if that
is the source of trouble.

However, Mark seems to tell that his simpler band-aid also works.

In the longer run, we could make watchpoints work even in remote
debugging, provided that there's a facility in the remote protocol to
fiddle with debug registers (sorry, I don't know anything about the
remote protocol, so I might be saying something stupid).  Just make
the Linux-specific implementations of I386_DR_LOW_* macros send the
right packet if the target's name is "remote".

> I've no idea, I just know that it suddenly stopped working
> with these changes.  And I'm not talking about watchpoints
> working -- I'm talking about _anything_ working.  There is a
> call to STOPPED_BY_WATCHPOINT in wait_for_inferior, and
> you can't even connect to the target without wait_for_inferior
> being called.

Actually, that call to STOPPED_BY_WATCHPOINT is IMHO totally bogus: no
one looks at the result it returns, and I have yet to see any
implementation of watchpoints that needs that call.

I'm guessing that this call goes back to the implementation for 
Sparclite, which uses this call to produce a side effect (to be able
to step over the instruction which caused the watchpoint to trigger),
see config/sparc/tm-sparclite.h.  Since this seems to be the only 
platform which needs a side effect, and since we already have an 
equivalent mechanism for doing the same, with the HAVE_*_WATCHPOINT 
macros, I think this call can be removed.

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Tue, 27 Mar 2001, Michael Snyder wrote:

> > As far as I understood, GDB's regcache cannot do such magic.  It
> > assumes that all the registers are per thread, so if you change them
> > in one thread, the other threads' registers remain unchanged in the
> > cache.
> 
> Currently there is only one cache (not one per thread).
> When you change threads, the cache is flushed.  However, 
> there are plans for a per-thread cache, so your concern
> is not ill-placed.
> 
> > In other words, this kind of global registers is (or was when we
> > discussed it) incompatible with how regcache works.  That is why I was
> > advised not to introduce the debug registers into the regcache.
> 
> They could be pseudo-registers, which means the debugger would
> have the opportunity to handle them specially.

I agree that, once the register cache transition is complete, we
should probably revisit this issue and consider including the debug
registers in the cache.  At the time I wrote the code, and even now,
this issue seemed to be too fluid to rely on it.

Something to put into TODO for 5.2, perhaps?

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Kettenis]

   Date: Tue, 27 Mar 2001 11:58:34 -0800
   From: Michael Snyder <msnyder@cygnus.com>

   > As far as I understood, GDB's regcache cannot do such magic.  It
   > assumes that all the registers are per thread, so if you change them
   > in one thread, the other threads' registers remain unchanged in the
   > cache.

   Currently there is only one cache (not one per thread).
   When you change threads, the cache is flushed.  However, 
   there are plans for a per-thread cache, so your concern
   is not ill-placed.

Hmm, I doubt that a per-thread cache is feasable.  As soon as a single
thread is running, it might be changing another thread's registers
even if that thread isn't running.  I know for a fact that this can
happen on the Hurd.  And I suspect that many-to-many threads
implementations do similar things.

Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Salter]

>>>>> Eli Zaretskii writes:

> On Tue, 27 Mar 2001, Mark Salter wrote:

>> When a read wathcpoint is triggered, the target stops and informs gdb.
>> In breakpoint.c, gdb sees that there are read or access watchpoints set
>> and that the data address reported by the target matches. This causes
>> watchpoint_check() to be called. The problem is that watchpoint_check()
>> will try to  read from the watched data area to see if it changed, but
>> this is done before gdb has removed watchpoints from the target. This
>> causes the target to respond with an error when gdb tries to access the
>> watched area.

> Sorry, I don't understand: why does reading a watched region generate
> an error?  At least in the x86 implementation, watchpoints are set to
> be task-local, so reading the data from GDB, which is another process,
> should not produce any errors.  Am I missing something?

I'm using a remote target which has global hw watchpoints and no OS.
The remote target still has the watchpoints installed when gdb tries
to read the area being watched. I don't know. Maybe the target stub
should take it upon itself to remove the watchpoints before telling
gdb it stopped?

--Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Wed, 28 Mar 2001, Mark Salter wrote:

> > Sorry, I don't understand: why does reading a watched region generate
> > an error?  At least in the x86 implementation, watchpoints are set to
> > be task-local, so reading the data from GDB, which is another process,
> > should not produce any errors.  Am I missing something?
> 
> I'm using a remote target which has global hw watchpoints and no OS.
> The remote target still has the watchpoints installed when gdb tries
> to read the area being watched. I don't know. Maybe the target stub
> should take it upon itself to remove the watchpoints before telling
> gdb it stopped?

Is there any reason that you can't define HAVE_NONSTEPPABLE_WATCHPOINT 
(or some of its other brethren; see infrun.c) and get GDB to DTRT for 
your target?

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Salter]

>>>>> Eli Zaretskii writes:

> On Wed, 28 Mar 2001, Mark Salter wrote:

>> > Sorry, I don't understand: why does reading a watched region generate
>> > an error?  At least in the x86 implementation, watchpoints are set to
>> > be task-local, so reading the data from GDB, which is another process,
>> > should not produce any errors.  Am I missing something?
>> 
>> I'm using a remote target which has global hw watchpoints and no OS.
>> The remote target still has the watchpoints installed when gdb tries
>> to read the area being watched. I don't know. Maybe the target stub
>> should take it upon itself to remove the watchpoints before telling
>> gdb it stopped?

> Is there any reason that you can't define HAVE_NONSTEPPABLE_WATCHPOINT 
> (or some of its other brethren; see infrun.c) and get GDB to DTRT for 
> your target?

Hmm. So, what are the meanings of HAVE_NONSTEPPABLE_WATCHPOINT and
HAVE_STEPPABLE_WATCHPOINT? It seems if you have one, you don't have
the other, but infrun.c doesn't see it that way. The thing is that
the hw watchpoints on this architecture will not trap until the insn
which triggers the watchpoint finishes execution. That is, the PC 
reported to GDB is for the insn just past the insn which triggered
the watchpoint.

Anyway, if I don't define either, and debug this app:

  int foo = 3;

  main()
  {
     printf("%d\n", foo);
  }

I get this:

  (gdb) rwatch foo
  Hardware read watchpoint 1: foo
  (gdb) c
  Continuing.
  Error evaluating expression for watchpoint 1
  Cannot access memory at address 0xa0028000
  Watchpoint 1 deleted.
  0xa0020028 in main () at wtest.c:6
  (gdb) 

Note that gdb stopped at the line that referenced the watched variable.
The only problem is the error message.

If I define HAVE_NONSTEPPABLE_WATCHPOINT, I get:

  (gdb) c
  Continuing.
  Hardware read watchpoint 1: foo

  Value = 3
  0xa0020c4c in printf () at ....
  (gdb)

Note that gdb stopped inside printf. This seems suboptimal but at least
there's not error.

I didn't try HAVE_CONTINUABLE_WATCHPOINT or HAVE_STEPPABLE_WATCHPOINT because
I can't step or continue past insns that trigger watchpoints.

--Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Eli Zaretskii wrote:
> 
> On Tue, 27 Mar 2001, Michael Snyder wrote:
> > There is a
> > call to STOPPED_BY_WATCHPOINT in wait_for_inferior, and
> > you can't even connect to the target without wait_for_inferior
> > being called.
> 
> Actually, that call to STOPPED_BY_WATCHPOINT is IMHO totally bogus: no
> one looks at the result it returns, and I have yet to see any
> implementation of watchpoints that needs that call.

I agree, I think it's bogus too, but we would need to trace back
its history or at least analyze it more carefully than I have done.


> I'm guessing that this call goes back to the implementation for
> Sparclite, which uses this call to produce a side effect (to be able
> to step over the instruction which caused the watchpoint to trigger),
> see config/sparc/tm-sparclite.h.  Since this seems to be the only
> platform which needs a side effect, and since we already have an
> equivalent mechanism for doing the same, with the HAVE_*_WATCHPOINT
> macros, I think this call can be removed.

I'd like to see it removed, but I'm chicken.  ;-)

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Salter]

>>>>> Eli Zaretskii writes:

> On Wed, 28 Mar 2001, Mark Salter wrote:

>> > Is there any reason that you can't define HAVE_NONSTEPPABLE_WATCHPOINT 
>> > (or some of its other brethren; see infrun.c) and get GDB to DTRT for 
>> > your target?
>> 
>> Hmm. So, what are the meanings of HAVE_NONSTEPPABLE_WATCHPOINT and
>> HAVE_STEPPABLE_WATCHPOINT? It seems if you have one, you don't have
>> the other, but infrun.c doesn't see it that way.

> IMVHO, infrun.c is somewhat messy in this area; see the comments around 
> that code.  A target should have either of these two or none, but not 
> both.  However, in practice, if you don't define any of these constants, 
> it automatically gets defined to zero by infrun.c (see the beginning of 
> the file), so the compiler will simply optimize the wrong if clauses out 
> of existence when you build GDB.

I looked at that a little further and it seems that if one of those is
defined, then GDB will step the target. It looks like one of those
should be defined iff GDB needs to step past the insn causing the
watchpoint. That is the case for architectures where the watchpoint
is triggered before the insn executes. On the architecture I'm working
with, the insn executes before the watchpoint triggers, so GDB doesn't
need to step.

>> int foo = 3;
>> 
>> main()
>> {
>> printf("%d\n", foo);
>> }
> [...]
>> If I define HAVE_NONSTEPPABLE_WATCHPOINT, I get:
>> 
>> (gdb) c
>> Continuing.
>> Hardware read watchpoint 1: foo
>> 
>> Value = 3
>> 0xa0020c4c in printf () at ....
>> (gdb)

> This looks like what you should get.

>> Note that gdb stopped inside printf. This seems suboptimal but at least
>> there's not error.

> Why suboptimal?  Does disassembly show that the debuggee stopped on a 
> wrong insn?  That is, do you indeed see an instruction before 0x0020c4c 
> which accesses the value of `foo'?  Can you show the instructions near 
> this point?

0xa0020024 <main+24>:	ldr	r1, [r3]
0xa0020028 <main+28>:	bl	0xa0020c4c <printf>

So here, the 'ldr' insn reads the foo variable, then the bl insn calls printf.
The PC is at 0xa0020028 when the wathcpoint is reported to GDB. If I don't
define HAVE_NONSTEPPABLE_WATCHPOINT, then gdb reports the read error and the
target PC is left at 0xa0020028. If I define HAVE_NONSTEPPABLE_WATCHPOINT,
then GDB steps the target which causes it to branch into printf. No read
error is reported, but the PC is in printf.

--Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Wed, 28 Mar 2001, Michael Snyder wrote:

> > I'm guessing that this call goes back to the implementation for
> > Sparclite, which uses this call to produce a side effect (to be able
> > to step over the instruction which caused the watchpoint to trigger),
> > see config/sparc/tm-sparclite.h.  Since this seems to be the only
> > platform which needs a side effect, and since we already have an
> > equivalent mechanism for doing the same, with the HAVE_*_WATCHPOINT
> > macros, I think this call can be removed.
> 
> I'd like to see it removed, but I'm chicken.  ;-)

What do others think about removing this call to STOPPED_BY_WATCHPOINT?  
Andrew?

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Eli Zaretskii]

On Wed, 28 Mar 2001, Mark Salter wrote:

> > Is there any reason that you can't define HAVE_NONSTEPPABLE_WATCHPOINT 
> > (or some of its other brethren; see infrun.c) and get GDB to DTRT for 
> > your target?
> 
> Hmm. So, what are the meanings of HAVE_NONSTEPPABLE_WATCHPOINT and
> HAVE_STEPPABLE_WATCHPOINT? It seems if you have one, you don't have
> the other, but infrun.c doesn't see it that way.

IMVHO, infrun.c is somewhat messy in this area; see the comments around 
that code.  A target should have either of these two or none, but not 
both.  However, in practice, if you don't define any of these constants, 
it automatically gets defined to zero by infrun.c (see the beginning of 
the file), so the compiler will simply optimize the wrong if clauses out 
of existence when you build GDB.

>   int foo = 3;
> 
>   main()
>   {
>      printf("%d\n", foo);
>   }
[...]
> If I define HAVE_NONSTEPPABLE_WATCHPOINT, I get:
> 
>   (gdb) c
>   Continuing.
>   Hardware read watchpoint 1: foo
> 
>   Value = 3
>   0xa0020c4c in printf () at ....
>   (gdb)

This looks like what you should get.

> Note that gdb stopped inside printf. This seems suboptimal but at least
> there's not error.

Why suboptimal?  Does disassembly show that the debuggee stopped on a 
wrong insn?  That is, do you indeed see an instruction before 0x0020c4c 
which accesses the value of `foo'?  Can you show the instructions near 
this point?

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Michael Snyder]

Mark, this change did in fact fix the problem.
Would you like to check it in?

				Thanks,
				Michael Snyder

Mark Kettenis wrote:
> 
>    Date: Mon, 26 Mar 2001 18:14:00 -0800
>    From: Michael Snyder <msnyder@cygnus.com>
> 
>    Mark Kettenis wrote:
>    >
>    > FYI, I checked this in.  HJ can finally be happy now (although things
>    > probably won't work correctly for multithreaded programs).
> 
>    Mark, this breaks remote i386 targets debugged from linux hosts.
>    STOPPED_BY_WATCHPOINT is unconditionally defined to a function in
>    i386-nat.c, but we may not be debugging a native target.
> 
> STOPPED_BY_WATCHPOINT always has been unconditionally defined.  The
> only difference I can see is that with the old stuff is that I now
> call perror_with_name if the ptrace call fails.
> 
> If I'm right, the attached patch should fix your problems.  I cannot
> test it right now (no access to a Linux/x86 box), but if it works for
> you, feel free to check it in.
> 
> More details in the reply to your other message.
> 
> Mark
> 
> Index: ChangeLog
> from  Mark Kettenis  <kettenis@gnu.org>
> 
>         * i386-linux-nat.c (i386_linux_dr_get): Return 0 if ptrace call
>         fails instead of calling perror_with_name.  This should fix
>         debugging remote i386 targets with a native Linux/x86 GDB.  Add
>         FIXME for this hack.
> 
> Index: i386-linux-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-linux-nat.c,v
> retrieving revision 1.24
> diff -u -r1.24 i386-linux-nat.c
> --- i386-linux-nat.c 2001/03/21 21:22:48 1.24
> +++ i386-linux-nat.c 2001/03/27 08:40:06
> @@ -712,11 +712,20 @@
>       one thread.  */
>    tid = PIDGET (inferior_pid);
> 
> +  /* FIXME: kettenis/2001-03-27: Calling perror_with_name if the
> +     ptrace call fails breaks debugging remote targets.  The correct
> +     way to fix this is to add the hardware breakpoint and watchpoint
> +     stuff to the target vectore.  For now, just return zero if the
> +     ptrace call fails.  */
>    errno = 0;
>    value = ptrace (PT_READ_U, tid,
>                   offsetof (struct user, u_debugreg[regnum]), 0);
>    if (errno != 0)
> +#if 0
>      perror_with_name ("Couldn't read debug register");
> +#else
> +    return 0;
> +#endif
> 
>    return value;
>  }

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Kettenis]

   Date: Tue, 17 Apr 2001 17:26:43 -0700
   From: Michael Snyder <msnyder@cygnus.com>

   Mark, this change did in fact fix the problem.
   Would you like to check it in?

I already did!

Mark

Re: [PATCH]: Make Linux use the new unified x86 watchpoint support

[Mark Salter]

>>>>> Eli Zaretskii writes:

> On Thu, 29 Mar 2001, Mark Salter wrote:

>> I looked at that a little further and it seems that if one of those is
>> defined, then GDB will step the target. It looks like one of those
>> should be defined iff GDB needs to step past the insn causing the
>> watchpoint. That is the case for architectures where the watchpoint
>> is triggered before the insn executes. On the architecture I'm working
>> with, the insn executes before the watchpoint triggers, so GDB doesn't
>> need to step.

> Yes, it seems like there's a similar code fragment which doesn't step the 
> target.  I will look into it.

> In any case, given that there's a way to cause GDB to remove watchpoints 
> without stepping the target, it will solve your problem, yes?

Yes, if GDB can be made to remove watchpoints without stepping and before
watchpoint_check reads from the watched area, then my problem is solved.

--Mark