Re: [RFC] GDB patches for hw watchpoints - revised

[Wu Zhou <woodzltc@cn.ibm.com>]

Here is the reworked patch for latest 6.4 tree.  Any comments?  Thanks a 
lot!

2005-12-22  Ben Elliston  <bje@au1.ibm.com>
	    Wu Zhou  <woodzltc@cn.ibm.com>

	* rs6000-tdep.c (rs6000_gdbarch_init): If the macn is p630, set
	gdbarch to have nonsteppable watchpoint.
	* ppc-linux-nat.c: Defind PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG
	and PTRACE_GETSIGINFO.
	(ppc_linux_check_watch_resources): New function to check whether 
	the target have available hardware watchpoint resource.
	(ppc_linux_insert_watchpoint): New function to insert a hardware
	watchpoint.
	(ppc_linux_remove_watchpoint): New function to remove a hardware
	watchpoint.
	(ppc_linux_stopped_data_address): New function to get the stopped
	data address of the watchpoint.
	(ppc_linux_stopped_by_watchpoint): New function to see if the 
	inferior is stopped by watchpoint.
	(_initialize_ppc_linux_nat): Set the above hardware watchpoint
	related target vectors.
	
Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.248
diff -c -3 -p -r1.248 rs6000-tdep.c
*** rs6000-tdep.c	1 Nov 2005 19:32:36 -0000	1.248
--- rs6000-tdep.c	22 Dec 2005 03:52:09 -0000
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 3278,3283 ****
--- 3278,3291 ----
                      _("rs6000_gdbarch_init: "
                      "received unexpected BFD 'arch' value"));
  
+   /* If the MACH is p630, set have_nonsteppable_watchpoint to 1.
+ 
+      FIXME: not quite sure if all ppc64 mach support nonsteppable watchpoint.
+      But p630 do support nonsteppable h/w watchpoint.  */
+ 
+   if (bfd_mach_ppc_630)
+     set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
+ 
    /* Sanity check on registers.  */
    gdb_assert (strcmp (tdep->regs[tdep->ppc_gp0_regnum].name, "r0") == 0);
  
Index: ppc-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-linux-nat.c,v
retrieving revision 1.55
diff -c -3 -p -r1.55 ppc-linux-nat.c
*** ppc-linux-nat.c	10 Sep 2005 18:11:04 -0000	1.55
--- ppc-linux-nat.c	22 Dec 2005 03:52:10 -0000
***************
*** 81,86 ****
--- 81,96 ----
  #define PTRACE_SETEVRREGS 21
  #endif
  
+ /* Similarly for the hardware watchpoint support.  */
+ #ifndef PTRACE_GET_DEBUGREG
+ #define PTRACE_GET_DEBUGREG    25
+ #endif
+ #ifndef PTRACE_SET_DEBUGREG
+ #define PTRACE_SET_DEBUGREG    26
+ #endif
+ #ifndef PTRACE_GETSIGINFO
+ #define PTRACE_GETSIGINFO    0x4202
+ #endif
  
  /* This oddity is because the Linux kernel defines elf_vrregset_t as
     an array of 33 16 bytes long elements.  I.e. it leaves out vrsave.
*************** struct gdb_evrregset_t
*** 146,151 ****
--- 156,162 ----
     error.  */
  int have_ptrace_getvrregs = 1;
  
+ static CORE_ADDR last_stopped_data_address;
  
  /* Non-zero if our kernel may support the PTRACE_GETEVRREGS and
     PTRACE_SETEVRREGS requests, for reading and writing the SPE
*************** int have_ptrace_getvrregs = 1;
*** 153,159 ****
     error.  */
  int have_ptrace_getsetevrregs = 1;
  
- 
  int
  kernel_u_size (void)
  {
--- 164,169 ----
*************** store_ppc_registers (int tid)
*** 777,782 ****
--- 787,931 ----
      store_spe_register (tid, -1);
  }
  
+ int
+ ppc_linux_check_watch_resources (int type, int cnt, int ot)
+ {
+   int tid;
+   ptid_t ptid = inferior_ptid;
+ 
+   /* DABR (data address breakpoint register) is optional for PPC variations.
+      Some variation have one DABR, others have none.  So CNT can't be larger
+      than 1.  */
+   if (cnt > 1)
+     return 0;
+ 
+   /* We need to know whether ptrace syscall support PTRACE_SET_DEBUGREG and 
+      whether the ppc arch have DABR.  If either answer is no, the ptrace call
+      will return -1.  Return 0 for that.  */
+   tid = TIDGET (ptid);
+   if (tid == 0)
+     tid = PIDGET (ptid);
+ 
+   if (ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0) == -1)
+     return 0;
+   return 1;
+ }
+ 
+ static int
+ ppc_linux_region_size_ok_for_hw_watchpoint (int cnt)
+ {
+   return 1;
+ }
+ 
+ /* Set a watchpoint of type TYPE at address ADDR.  */
+ long
+ ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw)
+ {
+   int tid;
+   long dabr_value;
+   ptid_t ptid = inferior_ptid;
+ 
+   /* Handle sub-8-byte quantities.  */
+   if (len <= 0)
+     return -1;
+ 
+   /* addr+len must fall in the 8 byte watchable region.  */
+   if ((addr + len) > (addr & ~7) + 8)
+     return -1;
+ 
+   dabr_value = addr & ~7;
+   switch (rw)
+     {
+     case hw_read:
+       /* Set read and translate bits.  */
+       dabr_value |= 5;
+       break;
+     case hw_write:
+       /* Set write and translate bits.  */
+       dabr_value |= 6;
+       break;
+     case hw_access:
+       /* Set read, write and translate bits.  */
+       dabr_value |= 7;
+       break;
+     }
+ 
+   tid = TIDGET (ptid);
+   if (tid == 0)
+     tid = PIDGET (ptid);
+ 
+   return ptrace (PTRACE_SET_DEBUGREG, tid, 0, dabr_value);
+ }
+ 
+ long
+ ppc_linux_remove_watchpoint (CORE_ADDR addr, int len)
+ {
+   int tid;
+   ptid_t ptid = inferior_ptid;
+ 
+   tid = TIDGET (ptid);
+   if (tid == 0)
+     tid = PIDGET (ptid);
+ 
+   return ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0);
+ }
+ 
+ int
+ ppc_linux_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p)
+ {
+   if (last_stopped_data_address)
+     {
+       *addr_p = last_stopped_data_address;
+       return 1;
+     }
+   return 0;
+ }
+ 
+ /*
+ int
+ ppc_linux_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p)
+ {
+   int tid;
+   struct siginfo siginfo;
+   ptid_t ptid = inferior_ptid;
+ 
+   tid = TIDGET (ptid);
+   if (tid == 0)
+     tid = PIDGET (ptid);
+ 
+   errno = 0;
+   ptrace (PTRACE_GET_DEBUGREG, tid, (PTRACE_TYPE_ARG3) 0, addr_p);
+   if (errno)
+     return 0;
+ 
+   *addr_p = *addr_p & ~7;
+   return 1;
+ }
+ */
+ 
+ int
+ ppc_linux_stopped_by_watchpoint (void)
+ {
+   int tid;
+   struct siginfo siginfo;
+   ptid_t ptid = inferior_ptid;
+   CORE_ADDR *addr_p;
+ 
+   tid = TIDGET(ptid);
+   if (tid == 0)
+     tid = PIDGET (ptid);
+ 
+   errno = 0;
+   ptrace (PTRACE_GETSIGINFO, tid, (PTRACE_TYPE_ARG3) 0, &siginfo);
+ 
+   if (errno != 0 || siginfo.si_signo != SIGTRAP ||
+       (siginfo.si_code & 0xffff) != 0x0004)
+     return 0;
+ 
+   last_stopped_data_address = (CORE_ADDR) siginfo.si_addr;
+   return 1;
+ }
+ 
  static void
  ppc_linux_store_inferior_registers (int regno)
  {
*************** _initialize_ppc_linux_nat (void)
*** 900,905 ****
--- 1049,1062 ----
    t->to_fetch_registers = ppc_linux_fetch_inferior_registers;
    t->to_store_registers = ppc_linux_store_inferior_registers;
  
+   /* Add our watchpoint methods.  */
+   t->to_can_use_hw_breakpoint = ppc_linux_check_watch_resources;
+   t->to_region_size_ok_for_hw_watchpoint = ppc_linux_region_size_ok_for_hw_watchpoint;
+   t->to_insert_watchpoint = ppc_linux_insert_watchpoint;
+   t->to_remove_watchpoint = ppc_linux_remove_watchpoint;
+   t->to_stopped_by_watchpoint = ppc_linux_stopped_by_watchpoint;
+   t->to_stopped_data_address = ppc_linux_stopped_data_address;
+ 
    /* Register the target.  */
    add_target (t);
  }


On Thu, 22 Dec 2005, Wu Zhou wrote:

> 
> > Yeah, we cant assume we have only one data breakpoint register - I think
> > some of the 32bit cpus have multiple ones.
> 
> Can you recall which cpus have multiple ones? I am now reading related 
> processor document and find that Book E seems to use a different debug 
> facility.  It has three debug control registers, one debug status 
> register, two insruction address compare registers and two data address 
> compare registers (maybe the same as DABR in other POWER/PowerPC arch).
> 
> That is in <<Book E: Enhanced PowerPC Architecture>>.  So maybe the "at 
> most one DABR" assertion still hold for most ppc arches.  And provided 
> that the current kernel only support at most one DABR, so this patch still 
> make sense for GDB, right?  Any objection?  :-)
> 
> What is more, in function ppc_linux_check_watch_resources, there is a run 
> time check to see whether the command PTRACE_SET_DEBUGREG of ptrace can 
> succeed.  I believe that will make these archs which don't have DABR not 
> impacted by this patch.
> 
> So I am thinking this patch still make sense.  What is your thought?  
> Very happy to know your comments.
> 
> Regards
> - Wu Zhou
> 
>