Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [RFC] GDB patches for hw watchpoints - revised
@ 2005-12-06 19:54 Wu Zhou
  2005-12-06 22:46 ` Ulrich Weigand
  2005-12-06 23:05 ` Eli Zaretskii
  0 siblings, 2 replies; 34+ messages in thread
From: Wu Zhou @ 2005-12-06 19:54 UTC (permalink / raw)
  To: gdb-patches; +Cc: drow, eliz, mark.kettenis

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


^ permalink raw reply	[flat|nested] 34+ messages in thread
* Re: [RFC] GDB patches for hw watchpoints - revised
@ 2005-12-22 15:26 Wu Zhou
  2005-12-22 15:38 ` Wu Zhou
  0 siblings, 1 reply; 34+ messages in thread
From: Wu Zhou @ 2005-12-22 15:26 UTC (permalink / raw)
  To: anton; +Cc: drow, gdb-patches, mark, bje


> 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


^ permalink raw reply	[flat|nested] 34+ messages in thread
end of thread, other threads:[~2006-02-13  9:53 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-12-06 19:54 [RFC] GDB patches for hw watchpoints - revised Wu Zhou
2005-12-06 22:46 ` Ulrich Weigand
2005-12-09 12:00   ` Wu Zhou
2005-12-09 14:34     ` Ulrich Weigand
2005-12-06 23:05 ` Eli Zaretskii
2005-12-06 23:31   ` Daniel Jacobowitz
2005-12-09 12:04     ` Wu Zhou
2005-12-09 14:22       ` Daniel Jacobowitz
2005-12-09 18:58         ` Eli Zaretskii
2005-12-10 22:23         ` Wu Zhou
2005-12-11 11:12           ` Daniel Jacobowitz
2005-12-11 14:39             ` Wu Zhou
2005-12-13 22:47             ` Wu Zhou
2005-12-14 18:12               ` Eli Zaretskii
2005-12-14 18:13               ` Daniel Jacobowitz
2005-12-15 20:06                 ` Wu Zhou
2005-12-16  0:10                   ` Anton Blanchard
2005-12-22 15:26 Wu Zhou
2005-12-22 15:38 ` Wu Zhou
2005-12-22 15:57   ` Eli Zaretskii
2005-12-22 15:57     ` Wu Zhou
2005-12-23 20:52       ` Eli Zaretskii
2006-01-22 20:56       ` Daniel Jacobowitz
2006-01-24  3:40         ` Wu Zhou
2006-01-24  3:43           ` Daniel Jacobowitz
2006-01-24  4:33             ` Wu Zhou
2006-01-24 11:00               ` Wu Zhou
2006-01-24 21:20                 ` Daniel Jacobowitz
2006-01-25  3:19                   ` Wu Zhou
2006-01-25  8:34                     ` Replace to_region_size_ok_for_hw_watchpoint references with to_region_ok_for_hw_watchpoint ones Wu Zhou
2006-02-02  1:43                       ` [RFC] GDB patches for hw watchpoints - revised Daniel Jacobowitz
2006-02-08  5:35                         ` Wu Zhou
2006-02-09  5:44                           ` Wu Zhou
2006-02-09  7:44                             ` Eli Zaretskii
2006-02-13  9:53                               ` Wu Zhou

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox