[RFC] GDB patches for hw watchpoints - revised

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

Hello, maintainers

I am taking over Manoj's work of submitting Ben Elliston's ppc64 h/w 
watchpoint patch.  When Manoj submitted that patch a few monthes ago, some 
of you (Daniel, Eli and Mark...) expressed some concerns about that patch.
We are now making some changes to that patch, and wish that this could 
address (or partially address) your concerns.

Appended is the revised patch.  And here is some explanation about it.

1. Daniel ever express the concern that some PPC variations don't have any 
DABR. Yes, that is true. So I added a runtime test to check this. If the arch 
don't have DABR or that the kernel don't support PTRACE_SET_DEBUGREG, then 
  ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0)
will return -1.  We will return 0 for to_can_use_hardware_watchpoint.  See 
the revised implementation of ppc_linux_check_watch_resources

2. PTRACE_SET_DEBUGREG and PTRACE_GETSIGINFO for ppc64 is now in kernel 
2.6.14.3 from kernel.org.  You can also check 
  http://patchwork.ozlabs.org/linuxppc64/patch?id=2341
and related thread on the patch.

3. Eli ever expressed a concern that the PPC doesn't have a way to return 
the data address that triggered the watchpoint?  As far as I think, the 
reason is that PPC will only have one DABR (if it does have). So maybe we 
don't need to have such a method.  

4. Mark said that we can't add anything more to nm.h.  The revised patch 
changed that and add most of them to target vector.  But we had to find a 
way to set TARGET_REGION_OK_FOR_HW_WATCHPOINT for ppc64 arch. I am now 
setting that in nm-ppc64-linux.h.  Is that okay?  Maybe we need to set 
TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT instead? 

5. I had built a 2.6.14.3 kernel and test the following patch on a P630 
box.  It works.  Test with gdb.base/recurse.exp report 12 PASS and 7 FAIL, 
which is better than 6 PASS and 13 FAIL with un-patched GDB. What is more,
The 7 failure is around the second watchpoint, which is a software one. So 
I am believing that this is a progress.  I also tried gdb.base/watchpoint.exp,
no regression is found.  The third test I made is against Paul's testcase 
to verify "rs6000_in_function_epilogue_p". Hardware watchpoint didn't 
encounter the problem software watchpoint met. Any suggestion for more 
test to be taken?  Thanks in advance.

Here goes the patch. Please review and comment.

diff -cpr gdb-6.3.90/gdb/config/powerpc/nm-ppc64-linux.h gdb-6.3.90.new/gdb/config/powerpc/nm-ppc64-linux.h
*** gdb-6.3.90/gdb/config/powerpc/nm-ppc64-linux.h	2003-06-12 19:30:39.000000000 -0400
--- gdb-6.3.90.new/gdb/config/powerpc/nm-ppc64-linux.h	2005-11-30 18:13:04.000000000 -0500
*************** Foundation, Inc., 675 Mass Ave, Cambridg
*** 24,27 ****
--- 24,31 ----
  #define PTRACE_ARG3_TYPE void *
  #define PTRACE_XFER_TYPE long
  
+ /* Return true for region ok for hardware watchpoint.  */
+ 
+ #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, len) 1
+ 
  #endif /* NM_PPC64_LINUX_H */
diff -cpr gdb-6.3.90/gdb/ppc-linux-nat.c gdb-6.3.90.new/gdb/ppc-linux-nat.c
*** gdb-6.3.90/gdb/ppc-linux-nat.c	2005-09-10 14:11:04.000000000 -0400
--- gdb-6.3.90.new/gdb/ppc-linux-nat.c	2005-12-06 18:36:07.000000000 -0500
***************
*** 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.
*************** store_ppc_registers (int tid)
*** 777,782 ****
--- 787,890 ----
      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;
+ }
+ 
+ /* 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_by_watchpoint (void)
+ {
+   int tid;
+   struct siginfo siginfo;
+   ptid_t ptid = inferior_ptid;
+ 
+   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;
+ 
+   return 1;
+ }
+ 
  static void
  ppc_linux_store_inferior_registers (int regno)
  {
*************** _initialize_ppc_linux_nat (void)
*** 897,904 ****
--- 1005,1017 ----
    t = linux_target ();
  
    /* Add our register access methods.  */
+   t->to_can_use_hw_breakpoint = ppc_linux_check_watch_resources;
    t->to_fetch_registers = ppc_linux_fetch_inferior_registers;
+   t->to_insert_watchpoint = ppc_linux_insert_watchpoint;
+   t->to_remove_watchpoint = ppc_linux_remove_watchpoint;
    t->to_store_registers = ppc_linux_store_inferior_registers;
+   t->to_stopped_by_watchpoint = ppc_linux_stopped_by_watchpoint;
+   
  
    /* Register the target.  */
    add_target (t);
diff -cpr gdb-6.3.90/gdb/rs6000-tdep.c gdb-6.3.90.new/gdb/rs6000-tdep.c
*** gdb-6.3.90/gdb/rs6000-tdep.c	2005-10-05 20:22:57.000000000 -0400
--- gdb-6.3.90.new/gdb/rs6000-tdep.c	2005-12-06 20:24:43.000000000 -0500
*************** rs6000_gdbarch_init (struct gdbarch_info
*** 3277,3282 ****
--- 3277,3290 ----
                      _("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);
  
Regards
- Wu Zhou