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

[Michael Snyder <msnyder at cygnus dot 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).

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