[RFC] GDB patches for hw watchpoints - revised

[RFC] GDB patches for hw watchpoints - revised

[Wu Zhou]

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

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

[Ulrich Weigand]

Wu Zhou wrote:

> 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? 

You should be setting the target vector's to_region_size_ok_for_hw_watchpoint
function instead.  See s390-nat.c where I now implement full watchpoint
support without any nm.h macro defined ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

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

[Eli Zaretskii]

> Date: Tue, 6 Dec 2005 14:12:26 +0800 (CST)
> From: Wu Zhou <woodzltc@cn.ibm.com>
> cc: drow@false.org, eliz@gnu.org, mark.kettenis@xs4all.nl
> 
> 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.  

Sorry, I'm not following: no matter how many debug registers the PPC
has, it can still return to GDB the data address that triggered the
watchpoint.  I don't think the higher levels of GDB (breakpoint.c)
should know or assume anything about the target capabilities or
resources.  Let's try keeping the code clean of such peculiarities.

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

[Daniel Jacobowitz]

On Tue, Dec 06, 2005 at 10:25:01PM +0200, Eli Zaretskii wrote:
> > Date: Tue, 6 Dec 2005 14:12:26 +0800 (CST)
> > From: Wu Zhou <woodzltc@cn.ibm.com>
> > cc: drow@false.org, eliz@gnu.org, mark.kettenis@xs4all.nl
> > 
> > 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.  
> 
> Sorry, I'm not following: no matter how many debug registers the PPC
> has, it can still return to GDB the data address that triggered the
> watchpoint.  I don't think the higher levels of GDB (breakpoint.c)
> should know or assume anything about the target capabilities or
> resources.  Let's try keeping the code clean of such peculiarities.

Wu, you're using GETSIGINFO to check for a watchpoint.  Then isn't the
faulting address stored in the siginfo anyway?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

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

[Wu Zhou]

On Tue, 6 Dec 2005, Ulrich Weigand wrote:

> You should be setting the target vector's to_region_size_ok_for_hw_watchpoint
> function instead.  See s390-nat.c where I now implement full watchpoint
> support without any nm.h macro defined ...
> 

Thanks a lot. That is a very good sample. But I see that you didn't 
implement to_stopped_data_address either. Do you think it is mandatory? It 
seems that it is mainly used in rwatch and awatch.

Regards
- Wu Zhou

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

[Wu Zhou]

On Tue, 6 Dec 2005, Daniel Jacobowitz wrote:

> On Tue, Dec 06, 2005 at 10:25:01PM +0200, Eli Zaretskii wrote:
> > > Date: Tue, 6 Dec 2005 14:12:26 +0800 (CST)
> > > From: Wu Zhou <woodzltc@cn.ibm.com>
> > > cc: drow@false.org, eliz@gnu.org, mark.kettenis@xs4all.nl
> > > 
> > > 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.  
> > 
> > Sorry, I'm not following: no matter how many debug registers the PPC
> > has, it can still return to GDB the data address that triggered the
> > watchpoint.  I don't think the higher levels of GDB (breakpoint.c)
> > should know or assume anything about the target capabilities or
> > resources.  Let's try keeping the code clean of such peculiarities.
> 
> Wu, you're using GETSIGINFO to check for a watchpoint.  Then isn't the
> faulting address stored in the siginfo anyway?

Currently what stored in the siginfo.si_addr is the address of next
instruction. Anton sent me a patch to change that to the data address 
instead. I am now playing with the patched kernel.

BTW. It seems that to_stopped_data_address is only used in rwatch and 
awatch, which is not that frequently used as watch itself.  So may we 
postpone its implementation till some later time?

Regards
- Wu Zhou

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

[Daniel Jacobowitz]

On Fri, Dec 09, 2005 at 10:25:33AM +0800, Wu Zhou wrote:
> Currently what stored in the siginfo.si_addr is the address of next
> instruction. Anton sent me a patch to change that to the data address 
> instead. I am now playing with the patched kernel.
> 
> BTW. It seems that to_stopped_data_address is only used in rwatch and 
> awatch, which is not that frequently used as watch itself.  So may we 
> postpone its implementation till some later time?

No, let's get it right the first time.  rwatch is extremely valuable,
and it sounds like you're having to play with ABI changes to get it to
work.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

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

[Ulrich Weigand]

Wu Zhou <woodzltc@cn.ibm.com> wrote on 09.12.2005 03:12:04:

> Thanks a lot. That is a very good sample. But I see that you didn't
> implement to_stopped_data_address either. Do you think it is mandatory?
It
> seems that it is mainly used in rwatch and awatch.

I don't implement this because our hardware simply does not provide
that information.  This is why a couple of the watchpoint testcases
are currently failing on s390 ...

(We don't have read/access watchpoints anyway.)


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com

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

[Eli Zaretskii]

> Date: Fri, 9 Dec 2005 00:01:32 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sources.redhat.com, mark.kettenis@xs4all.nl
> 
> On Fri, Dec 09, 2005 at 10:25:33AM +0800, Wu Zhou wrote:
> > Currently what stored in the siginfo.si_addr is the address of next
> > instruction. Anton sent me a patch to change that to the data address 
> > instead. I am now playing with the patched kernel.
> > 
> > BTW. It seems that to_stopped_data_address is only used in rwatch and 
> > awatch, which is not that frequently used as watch itself.  So may we 
> > postpone its implementation till some later time?
> 
> No, let's get it right the first time.  rwatch is extremely valuable

100% agreement.

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

[Wu Zhou]

On Fri, 9 Dec 2005, Daniel Jacobowitz wrote:

> On Fri, Dec 09, 2005 at 10:25:33AM +0800, Wu Zhou wrote:
> > Currently what stored in the siginfo.si_addr is the address of next
> > instruction. Anton sent me a patch to change that to the data address 
> > instead. I am now playing with the patched kernel.
> > 
> > BTW. It seems that to_stopped_data_address is only used in rwatch and 
> > awatch, which is not that frequently used as watch itself.  So may we 
> > postpone its implementation till some later time?
> 
> No, let's get it right the first time.  rwatch is extremely valuable,
> and it sounds like you're having to play with ABI changes to get it to
> work.

OK. I will try to make it right the first time. :-)

And can you elaborate on the statement that I am having to play with ABI 
changes to get it to work?  It seems that you must find something 
noticeable, right? If so, please point out. Thanks.

Regards
- Wu Zhou

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

[Daniel Jacobowitz]

On Sat, Dec 10, 2005 at 12:46:36PM +0800, Wu Zhou wrote:
> On Fri, 9 Dec 2005, Daniel Jacobowitz wrote:
> 
> > On Fri, Dec 09, 2005 at 10:25:33AM +0800, Wu Zhou wrote:
> > > Currently what stored in the siginfo.si_addr is the address of next
> > > instruction. Anton sent me a patch to change that to the data address 
> > > instead. I am now playing with the patched kernel.
> > > 
> > > BTW. It seems that to_stopped_data_address is only used in rwatch and 
> > > awatch, which is not that frequently used as watch itself.  So may we 
> > > postpone its implementation till some later time?
> > 
> > No, let's get it right the first time.  rwatch is extremely valuable,
> > and it sounds like you're having to play with ABI changes to get it to
> > work.
> 
> OK. I will try to make it right the first time. :-)
> 
> And can you elaborate on the statement that I am having to play with ABI 
> changes to get it to work?  It seems that you must find something 
> noticeable, right? If so, please point out. Thanks.

I was just talking about Anton's kernel patch.  If you're still
changing the kernel to make it work, it's not quite done yet.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

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

[Wu Zhou]

On Fri, 9 Dec 2005, Daniel Jacobowitz wrote:

> On Sat, Dec 10, 2005 at 12:46:36PM +0800, Wu Zhou wrote:
> > On Fri, 9 Dec 2005, Daniel Jacobowitz wrote:
> > 
> > > No, let's get it right the first time.  rwatch is extremely valuable,
> > > and it sounds like you're having to play with ABI changes to get it to
> > > work.
> > 
> > OK. I will try to make it right the first time. :-)
> > 
> > And can you elaborate on the statement that I am having to play with ABI 
> > changes to get it to work?  It seems that you must find something 
> > noticeable, right? If so, please point out. Thanks.
> 
> I was just talking about Anton's kernel patch.  If you're still
> changing the kernel to make it work, it's not quite done yet.

Sorry. I am not sure what you means by playing with ABI changes.  I had 
thought that you mean the changes in GDB's side.  But it seems that you 
are meaning the changes in kernel's side. 

And yes, Anton give me a patch to assign the faulting data address to 
siginfo.si_addr. But in my opinion, it is a small patch.  Just that I 
didn't get out the same value in gdb as that assigned to si_addr in 
kernel.  We are now believing that it might be a 32/64bit siginfo issue. 
But I don't have machine available at this time to track that down. 
(Another guy need to work with the shipped kernel.)

Regards
- Wu Zhou

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

[Wu Zhou]

On Fri, 9 Dec 2005, Daniel Jacobowitz wrote:

> On Sat, Dec 10, 2005 at 12:46:36PM +0800, Wu Zhou wrote:
> > On Fri, 9 Dec 2005, Daniel Jacobowitz wrote:
> > 
> > > On Fri, Dec 09, 2005 at 10:25:33AM +0800, Wu Zhou wrote:
> > > > 
> > > > BTW. It seems that to_stopped_data_address is only used in rwatch and 
> > > > awatch, which is not that frequently used as watch itself.  So may we 
> > > > postpone its implementation till some later time?
> > > 
> > > No, let's get it right the first time.  rwatch is extremely valuable,
> > > and it sounds like you're having to play with ABI changes to get it to
> > > work.
> > 
> > OK. I will try to make it right the first time. :-)
> > 
> > And can you elaborate on the statement that I am having to play with ABI 
> > changes to get it to work?  It seems that you must find something 
> > noticeable, right? If so, please point out. Thanks.
> 
> I was just talking about Anton's kernel patch.  If you're still
> changing the kernel to make it work, it's not quite done yet.

I am now trying three different method to get the stopped data address. 
But aach one seems to have its shortcoming , so I had to list them here to 
solicit comments and suggestions.  Thanks in advance.

1. The first one don't need any more change to kernel 2.6.14.3, I use 
GET_DEBUG_REG to get the content of DABR and assume it is the same as the 
stopped_data_address.  But the problem is that the content of DABR is not 
all the time the same as the data breakpoint.  What DABR monitor is an 
8-bytes region. The last three bits are used for setting read/write/translating
flag.

The code is something like this:
 
 tid = TIDGET (ptid);
  if (tid == 0)
    tid = PIDGET (ptid);

  ptrace (PTRACE_GET_DEBUGREG, tid, (PTRACE_TYPE_ARG3) 0, addr_p);
  *addr_p = *addr_p & ~7;

2. The second one need Anton's patch, which changed three lines in 
arch/ppc64/mm/fault.c:

Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6.orig/arch/powerpc/mm/fault.c		 2005-11-16 
03:21:49.000000000 +1100
+++ linux-2.6/arch/powerpc/mm/fault.c		 2005-12-08 
16:34:21.000000000 +1100
@@ -81,7 +81,8 @@
 }
 
 #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
-static void do_dabr(struct pt_regs *regs, unsigned long error_code)
+static void do_dabr(struct pt_regs *regs, unsigned long address,
+		 		     unsigned long error_code)
 {
 		 siginfo_t info;
 
@@ -99,7 +100,7 @@
 		 info.si_signo = SIGTRAP;
 		 info.si_errno = 0;
 		 info.si_code = TRAP_HWBKPT;
-		 info.si_addr = (void __user *)regs->nip;
+		 info.si_addr = (void __user *)address;
 		 force_sig_info(SIGTRAP, &info, current);
 }
 #endif /* !(CONFIG_4xx || CONFIG_BOOKE)*/
@@ -159,7 +160,7 @@
 #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
   		 if (error_code & DSISR_DABRMATCH) {
 		 		 /* DABR match */
-		 		 do_dabr(regs, error_code);
+		 		 do_dabr(regs, address, error_code);
 		 		 return 0;
 		 }
 #endif /* !(CONFIG_4xx || CONFIG_BOOKE)*/

With this patch, I can use PTRACE_GETSIGINFO to get the stopped data 
address (it is in siginfo.si_addr).  But one problem is that 
to_stopped_by_watchpoint will call PTRACE_GETSIGINFO first to determine if 
the stop is caused by watchpoint.  And another problem is that gdb need to 
single step the process to execute current instruction when a watchpoint 
is hit.  This will again drop into bpstat_stop_status, which will call 
stopped_by_watchpoint and thus call PTRACE_GETSIGINFO again.  

I take a look at IA64's code, it set the dd bit of IA64_PSR_REGNUM, which
will disable the watchpoint for the next instruction.  But it seems that 
ppc don't have such a way.  Do we have any workaround for this?  

3. The third one is a little tricky.  Now that ppc has at most 1 DABR. So 
I can set the stopped_data_address to the data address when we set the 
watchpoint (in ppc_linux_insert_watchpoint).  Everytime 
target_stopped_data_address is called, the breakpoint is either read or 
access, so it is already clear that it is stopped by watchpoint.  Then 
this trick seems to make sense, right?


I had tested the above three methods.  The first one works ok when the 
data breakpoint is aligned by 8 bytes.  The third one works ok for both 
aligned and non-aligned data breakpoint.  For the second one, I don't know 
how to work around the extra PTRACE_GETSIGINFO call caused by the single 
step yet.  But if I reserver the stopped_data_address when we first 
hit watchpoint, and store it back when I call ppc_linux_stopped_data_address. 
I can make rwatch and awatch to work as expected.

Any comments on the above three methods.  Thanks a lot in advance.

Regards
- Wu Zhou 

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

[Eli Zaretskii]

> Date: Tue, 13 Dec 2005 14:10:03 +0800 (CST)
> From: Wu Zhou <woodzltc@cn.ibm.com>
> cc: gdb-patches@sources.redhat.com, mark.kettenis@xs4all.nl, bje@au1.ibm.com,         anton@au1.ibm.com
> 
> 
> 3. The third one is a little tricky.  Now that ppc has at most 1 DABR. So 
> I can set the stopped_data_address to the data address when we set the 
> watchpoint (in ppc_linux_insert_watchpoint).  Everytime 
> target_stopped_data_address is called, the breakpoint is either read or 
> access, so it is already clear that it is stopped by watchpoint.  Then 
> this trick seems to make sense, right?

I think this third method should be good enough for the moment,
assuming that the target system allows only one active watchpoint at a
time.

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

[Daniel Jacobowitz]

On Tue, Dec 13, 2005 at 02:10:03PM +0800, Wu Zhou wrote:
> 2. The second one need Anton's patch, which changed three lines in 
> arch/ppc64/mm/fault.c:

> With this patch, I can use PTRACE_GETSIGINFO to get the stopped data 
> address (it is in siginfo.si_addr).  But one problem is that 
> to_stopped_by_watchpoint will call PTRACE_GETSIGINFO first to determine if 
> the stop is caused by watchpoint.  And another problem is that gdb need to 
> single step the process to execute current instruction when a watchpoint 
> is hit.  This will again drop into bpstat_stop_status, which will call 
> stopped_by_watchpoint and thus call PTRACE_GETSIGINFO again.  
> 
> I take a look at IA64's code, it set the dd bit of IA64_PSR_REGNUM, which
> will disable the watchpoint for the next instruction.  But it seems that 
> ppc don't have such a way.  Do we have any workaround for this?  

Could you cache the stopped data address when we are stopped by a
watchpoint, and then return the cached one if we aren't stopped by a
watchpoint any more but were the previous instruction?

I realize this is gross; the entire "step, then check the stopped data
address" is a bad idea, in my opinion.

> 3. The third one is a little tricky.  Now that ppc has at most 1 DABR. So 
> I can set the stopped_data_address to the data address when we set the 
> watchpoint (in ppc_linux_insert_watchpoint).  Everytime 
> target_stopped_data_address is called, the breakpoint is either read or 
> access, so it is already clear that it is stopped by watchpoint.  Then 
> this trick seems to make sense, right?

Weren't there other PPCs with more than one though?

> I had tested the above three methods.  The first one works ok when the 
> data breakpoint is aligned by 8 bytes.  The third one works ok for both 
> aligned and non-aligned data breakpoint.  For the second one, I don't know 
> how to work around the extra PTRACE_GETSIGINFO call caused by the single 
> step yet.  But if I reserver the stopped_data_address when we first 
> hit watchpoint, and store it back when I call ppc_linux_stopped_data_address. 
> I can make rwatch and awatch to work as expected.

... right, I guess I should have read your whole message first!  That's
my preferred option if we can find a clean way to do it.  I thought
there was a target with an example of this, but I can't find it.  I'm
not sure I understand why ia64 needs to disable the watchpoint, though.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

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

[Wu Zhou]

On Tue, 13 Dec 2005, Daniel Jacobowitz wrote:

> On Tue, Dec 13, 2005 at 02:10:03PM +0800, Wu Zhou wrote:
> > 2. The second one need Anton's patch, which changed three lines in 
> > arch/ppc64/mm/fault.c:
> 
> > With this patch, I can use PTRACE_GETSIGINFO to get the stopped data 
> > address (it is in siginfo.si_addr).  But one problem is that 
> > to_stopped_by_watchpoint will call PTRACE_GETSIGINFO first to determine if 
> > the stop is caused by watchpoint.  And another problem is that gdb need to 
> > single step the process to execute current instruction when a watchpoint 
> > is hit.  This will again drop into bpstat_stop_status, which will call 
> > stopped_by_watchpoint and thus call PTRACE_GETSIGINFO again.  
> > 
> > I take a look at IA64's code, it set the dd bit of IA64_PSR_REGNUM, which
> > will disable the watchpoint for the next instruction.  But it seems that 
> > ppc don't have such a way.  Do we have any workaround for this?  
> 
> Could you cache the stopped data address when we are stopped by a
> watchpoint, and then return the cached one if we aren't stopped by a
> watchpoint any more but were the previous instruction?

That is mostly the same as what I am doing with this method.  But what do 
you means by "were the previous instruction"?  Do you means to add another 
check on the instruction address when restoring the stopped_data_address?

The related code is like this:

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;
}

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;
}

last_stopped_data_address is a static CORE_ADDR in ppc-linux-nat.c, which 
is initialized to 0.

> I realize this is gross; the entire "step, then check the stopped data
> address" is a bad idea, in my opinion.

I am confused by this at the very first time too.  But it seems to 
desirable provided that quite a lot of h/w watchpoint is non-steppable.  
Maybe we can adopt another patch for data watchpoint, to say, "check 
data address, then single step"?

> 
> > 3. The third one is a little tricky.  Now that ppc has at most 1 DABR. So 
> > I can set the stopped_data_address to the data address when we set the 
> > watchpoint (in ppc_linux_insert_watchpoint).  Everytime 
> > target_stopped_data_address is called, the breakpoint is either read or 
> > access, so it is already clear that it is stopped by watchpoint.  Then 
> > this trick seems to make sense, right?
> 
> Weren't there other PPCs with more than one though?

Sorry, I don't know much about different PPC processors.  My knowledge 
about DABR is from the publically available PowerPC programming environment
manual. That book said that PowerPC has one optional DABR register. And 
the kernel is also coding like that.

Anton, do you have any comment on this question?

> 
> > I had tested the above three methods.  The first one works ok when the 
> > data breakpoint is aligned by 8 bytes.  The third one works ok for both 
> > aligned and non-aligned data breakpoint.  For the second one, I don't know 
> > how to work around the extra PTRACE_GETSIGINFO call caused by the single 
> > step yet.  But if I reserver the stopped_data_address when we first 
> > hit watchpoint, and store it back when I call ppc_linux_stopped_data_address. 
> > I can make rwatch and awatch to work as expected.
> 
> ... right, I guess I should have read your whole message first!  That's
> my preferred option if we can find a clean way to do it.  I thought
> there was a target with an example of this, but I can't find it.  I'm
> not sure I understand why ia64 needs to disable the watchpoint, though.

I had a search in the current gdb source.  Don't see it either. As for the 
IA64 mechanism, I don't understand that either. :-) But it seems that it 
can handle the problem above. (If no, how can these code got into the 
source? ) 

Regards
- Wu Zhou

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

[Anton Blanchard]

> Sorry, I don't know much about different PPC processors.  My knowledge 
> about DABR is from the publically available PowerPC programming environment
> manual. That book said that PowerPC has one optional DABR register. And 
> the kernel is also coding like that.
> 
> Anton, do you have any comment on this question?

Yeah, we cant assume we have only one data breakpoint register - I think
some of the 32bit cpus have multiple ones.

Anton

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

[Wu Zhou]

On Thu, 9 Feb 2006, Eli Zaretskii wrote:

> > Date: Thu, 9 Feb 2006 13:43:14 +0800 (CST)
> > From: Wu Zhou <woodzltc@cn.ibm.com>
> > cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com,
> >         bje@au1.ibm.com, anton@au1.ibm.com, pgilliam@us.ibm.com
> > 
> > Daniel,
> > 
> > I committed the two patches you approved.  Here is a seperate patch for 
> > gdbint.texinfo:
> > 
> > 2006-02-08  Wu Zhou  <woodzltc@cn.ibm.com>
> > 
> > 	* gdbint.texinfo (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> 
> The patch to gdb.texinfo is approved, but this ChangeLog entry is
> incorrect.  What should be in the parentheses is the name of the node
> where the changes were made.  The text then should say "Delete
> TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT.".  Okay?
> 

Thanks.  I committed this:

2006-02-13  Wu Zhou  <woodzltc@cn.ibm.com>

	* gdbint.texinfo (Watchpoints): Delete
	TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT.

Index: gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.238
diff -c -p -r1.238 gdbint.texinfo
*** gdbint.texinfo	6 Feb 2006 22:14:31 -0000	1.238
--- gdbint.texinfo	9 Feb 2006 05:32:27 -0000
*************** the same time).
*** 465,477 ****
  Return non-zero if hardware watchpoints can be used to watch a region
  whose address is @var{addr} and whose length in bytes is @var{len}.
  
- @findex TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT
- @item TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (@var{size})
- Return non-zero if hardware watchpoints can be used to watch a region
- whose size is @var{size}.  @value{GDBN} only uses this macro as a
- fall-back, in case @code{TARGET_REGION_OK_FOR_HW_WATCHPOINT} is not
- defined.
- 
  @cindex insert or remove hardware watchpoint
  @findex target_insert_watchpoint
  @findex target_remove_watchpoint
--- 465,470 ----

Regards
- Wu Zhou 

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

[Eli Zaretskii]

> Date: Thu, 9 Feb 2006 13:43:14 +0800 (CST)
> From: Wu Zhou <woodzltc@cn.ibm.com>
> cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com,
>         bje@au1.ibm.com, anton@au1.ibm.com, pgilliam@us.ibm.com
> 
> Daniel,
> 
> I committed the two patches you approved.  Here is a seperate patch for 
> gdbint.texinfo:
> 
> 2006-02-08  Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* gdbint.texinfo (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.

The patch to gdb.texinfo is approved, but this ChangeLog entry is
incorrect.  What should be in the parentheses is the name of the node
where the changes were made.  The text then should say "Delete
TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT.".  Okay?

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

[Wu Zhou]

Daniel,

I committed the two patches you approved.  Here is a seperate patch for 
gdbint.texinfo:

2006-02-08  Wu Zhou  <woodzltc@cn.ibm.com>

	* gdbint.texinfo (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.

Index: gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.238
diff -c -p -r1.238 gdbint.texinfo
*** gdbint.texinfo	6 Feb 2006 22:14:31 -0000	1.238
--- gdbint.texinfo	9 Feb 2006 05:32:27 -0000
*************** the same time).
*** 465,477 ****
  Return non-zero if hardware watchpoints can be used to watch a region
  whose address is @var{addr} and whose length in bytes is @var{len}.
  
- @findex TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT
- @item TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (@var{size})
- Return non-zero if hardware watchpoints can be used to watch a region
- whose size is @var{size}.  @value{GDBN} only uses this macro as a
- fall-back, in case @code{TARGET_REGION_OK_FOR_HW_WATCHPOINT} is not
- defined.
- 
  @cindex insert or remove hardware watchpoint
  @findex target_insert_watchpoint
  @findex target_remove_watchpoint
--- 465,470 ----


Besides this, I am also thinking of a little more cleanup work in 
config/i386/nm-i386.h: macro TARGET_REGION_OK_FOR_HW_WATCHPOINT
can be replaced by a target vector initialization in i386-nat.c or 
somewhere else.

Maybe some other macros can also be replaced, such as 
STOPPED_BY_WATCHPOINT, target_stopped_data_address(target, x) and so on...

What is your thought on this?

Regards
- Wu Zhou

On Wed, 8 Feb 2006, Wu Zhou wrote:

> Hi Daniel,
> 
> Sorry for the delayed reply.  I am just back from a vacation.
> 
> On Wed, 1 Feb 2006, Daniel Jacobowitz wrote:
> 
> > On Wed, Jan 25, 2006 at 04:34:14PM +0800, Wu Zhou wrote:
> > > I am also thinking of replace the macro 
> > > TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (SIZE) with 
> > > TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR, SIZE).  Thus the code will seem 
> > > to be more clean: we will only have one macro to see if the target region 
> > > is ok for watchpoint monitoring.  Following this way, function 
> > > default_region_ok_for_hw_watchpoint will also return back to its original 
> > > implementation.  What is your thought on this?
> > 
> > I'm not sure what you mean.  After these patches, the only reference to
> > TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT is in gdbint.texinfo (which I'd
> > appreciate if you fixed, in a separate patch - thanks in advance).
> > What are the two ways now?
> 
> What I mean is the second patch below, which you had said ok. :-) 
> That is the only way I can thought of at that time.  Now that you had said 
> ok, I don't need to find a second way.  :-)
> 
> BTW.  I will fix the reference to TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT 
> in gdbint.texinfo in a separate patch after commiting this.
> 
> > 
> > On Wed, Jan 25, 2006 at 11:18:49AM +0800, Wu Zhou wrote:
> > > OK to commit?
> > > 
> > > 2006-01-22  Ben Elliston  <bje@au1.ibm.com>
> > > 	    Wu Zhou  <woodzltc@cn.ibm.com>
> > > 
> > > 	* ppc-linux-nat.c (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG,
> > > 	PTRACE_GETSIGINFO): Define.
> > > 	(last_stopped_data_address): New.
> > > 	(ppc_linux_check_watch_resources): New function.
> > > 	(ppc_linux_region_ok_for_hw_watchpoint): New function.
> > > 	(ppc_linux_insert_watchpoint): New function.
> > > 	(ppc_linux_remove_watchpoint): New function.
> > > 	(ppc_linux_stopped_data_address): New function.
> > > 	(ppc_linux_stopped_by_watchpoint): New function.
> > > 	(_initialize_ppc_linux_nat): Set the above hardware watchpoint
> > > 	related target vectors.
> > > 	* rs6000-tdep.c (rs6000_gdbarch_init): Set PPC architectures
> > > 	to have nonsteppable watchpoint.
> > > 	* target.c (default_region_ok_for_hw_watchpoint,
> > > 	debug_to_region_ok_for_hw_watchpoint): New prototypes.
> > > 	(update_current_target): Inherit to_region_ok_for_hw_watchpoint
> > > 	and set default to_region_ok_for_hw_watchpoint.
> > > 	(default_region_ok_for_hw_watchpoint): New function.
> > > 	(debug_to_region_ok_for_hw_watchpoint): New function.
> > > 	(setup_target_debug): Set to_region_ok_for_hw_watchpoint of 
> > > 	debug_target.
> > > 	* target.h (struct target_ops): Add a new target vector 
> > > 	to_region_ok_for_hw_watchpoint.
> > > 	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Define this if it is not
> > > 	defined anyplace else.
> > 
> > On Wed, Jan 25, 2006 at 04:34:14PM +0800, Wu Zhou wrote:
> > > 2006-01-25  Wu Zhou  <woodzltc@cn.ibm.com>
> > > 
> > > 	* breakpoint.c (TARGET_REGION_OK_FOR_HW_WATCHPOINT): Delete.
> > > 	* config/i386/nm-i386sol2.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > > 	* config/mips/nm-irix5.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > > 	* config/sparc/nm-sol2.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > > 	* inf-ttrace.c (inf_ttrace_region_ok_for_hw_watchpoint): New.
> > > 	(inf_ttrace_region_size_ok_for_hw_watchpoint): Delete.
> > > 	(inf_ttrace_target): Delete to_region_size_ok_for_hw_watchpoint and
> > > 	add to_region_ok_for_hw_watchpoint.
> > > 	* s390-nat.c (s390_region_size_ok_for_hw_watchpoint): Delete.
> > > 	(s390_region_ok_for_hw_watchpoint): New.
> > > 	(_initialize_s390_nat): Delete to_region_size_ok_for_hw_watchpoint
> > > 	and add to_region_ok_for_hw_watchpoint.
> > > 	* target.c (default_region_size_ok_for_hw_watchpoint, 
> > > 	debug_to_region_size_ok_for_hw_watchpoint): Delete prototype.
> > > 	(update_current_target): Delete to_region_size_ok_for_hw_watchpoint
> > > 	inheritance and default_region_size_ok_for_hw_watchpoint.
> > > 	(default_region_ok_for_hw_watchpoint): If len is less than or equal
> > > 	the length of void pointer, return ok.
> > > 	(default_region_size_ok_for_hw_watchpoint): Delete.
> > > 	(debug_to_region_size_ok_for_hw_watchpoint): Delete.
> > > 	(setup_target_debug): Delete to_region_size_ok_for_hw_watchpoint.
> > > 	* target.h (struct target_ops): Delete 
> > > 	to_region_size_ok_for_hw_watchpoint.
> > > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > 
> > These patches are both OK.  You might want to combine them - it's a
> > much smaller diff :-)  But it doesn't matter since you've already got
> > them separated out.
> 
> Thanks for reviewing that.  I would like to commit them one by one, thus I 
> don't need to think about how to re-describe them.  :-)
> 
> I also think that it is clearer to differentiate the purpose of these two 
> patches.  Combining them together seems a little confusing to me.
> 
> Best Regards
> - Wu Zhou

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

[Wu Zhou]

Hi Daniel,

Sorry for the delayed reply.  I am just back from a vacation.

On Wed, 1 Feb 2006, Daniel Jacobowitz wrote:

> On Wed, Jan 25, 2006 at 04:34:14PM +0800, Wu Zhou wrote:
> > I am also thinking of replace the macro 
> > TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (SIZE) with 
> > TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR, SIZE).  Thus the code will seem 
> > to be more clean: we will only have one macro to see if the target region 
> > is ok for watchpoint monitoring.  Following this way, function 
> > default_region_ok_for_hw_watchpoint will also return back to its original 
> > implementation.  What is your thought on this?
> 
> I'm not sure what you mean.  After these patches, the only reference to
> TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT is in gdbint.texinfo (which I'd
> appreciate if you fixed, in a separate patch - thanks in advance).
> What are the two ways now?

What I mean is the second patch below, which you had said ok. :-) 
That is the only way I can thought of at that time.  Now that you had said 
ok, I don't need to find a second way.  :-)

BTW.  I will fix the reference to TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT 
in gdbint.texinfo in a separate patch after commiting this.

> 
> On Wed, Jan 25, 2006 at 11:18:49AM +0800, Wu Zhou wrote:
> > OK to commit?
> > 
> > 2006-01-22  Ben Elliston  <bje@au1.ibm.com>
> > 	    Wu Zhou  <woodzltc@cn.ibm.com>
> > 
> > 	* ppc-linux-nat.c (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG,
> > 	PTRACE_GETSIGINFO): Define.
> > 	(last_stopped_data_address): New.
> > 	(ppc_linux_check_watch_resources): New function.
> > 	(ppc_linux_region_ok_for_hw_watchpoint): New function.
> > 	(ppc_linux_insert_watchpoint): New function.
> > 	(ppc_linux_remove_watchpoint): New function.
> > 	(ppc_linux_stopped_data_address): New function.
> > 	(ppc_linux_stopped_by_watchpoint): New function.
> > 	(_initialize_ppc_linux_nat): Set the above hardware watchpoint
> > 	related target vectors.
> > 	* rs6000-tdep.c (rs6000_gdbarch_init): Set PPC architectures
> > 	to have nonsteppable watchpoint.
> > 	* target.c (default_region_ok_for_hw_watchpoint,
> > 	debug_to_region_ok_for_hw_watchpoint): New prototypes.
> > 	(update_current_target): Inherit to_region_ok_for_hw_watchpoint
> > 	and set default to_region_ok_for_hw_watchpoint.
> > 	(default_region_ok_for_hw_watchpoint): New function.
> > 	(debug_to_region_ok_for_hw_watchpoint): New function.
> > 	(setup_target_debug): Set to_region_ok_for_hw_watchpoint of 
> > 	debug_target.
> > 	* target.h (struct target_ops): Add a new target vector 
> > 	to_region_ok_for_hw_watchpoint.
> > 	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Define this if it is not
> > 	defined anyplace else.
> 
> On Wed, Jan 25, 2006 at 04:34:14PM +0800, Wu Zhou wrote:
> > 2006-01-25  Wu Zhou  <woodzltc@cn.ibm.com>
> > 
> > 	* breakpoint.c (TARGET_REGION_OK_FOR_HW_WATCHPOINT): Delete.
> > 	* config/i386/nm-i386sol2.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > 	* config/mips/nm-irix5.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > 	* config/sparc/nm-sol2.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> > 	* inf-ttrace.c (inf_ttrace_region_ok_for_hw_watchpoint): New.
> > 	(inf_ttrace_region_size_ok_for_hw_watchpoint): Delete.
> > 	(inf_ttrace_target): Delete to_region_size_ok_for_hw_watchpoint and
> > 	add to_region_ok_for_hw_watchpoint.
> > 	* s390-nat.c (s390_region_size_ok_for_hw_watchpoint): Delete.
> > 	(s390_region_ok_for_hw_watchpoint): New.
> > 	(_initialize_s390_nat): Delete to_region_size_ok_for_hw_watchpoint
> > 	and add to_region_ok_for_hw_watchpoint.
> > 	* target.c (default_region_size_ok_for_hw_watchpoint, 
> > 	debug_to_region_size_ok_for_hw_watchpoint): Delete prototype.
> > 	(update_current_target): Delete to_region_size_ok_for_hw_watchpoint
> > 	inheritance and default_region_size_ok_for_hw_watchpoint.
> > 	(default_region_ok_for_hw_watchpoint): If len is less than or equal
> > 	the length of void pointer, return ok.
> > 	(default_region_size_ok_for_hw_watchpoint): Delete.
> > 	(debug_to_region_size_ok_for_hw_watchpoint): Delete.
> > 	(setup_target_debug): Delete to_region_size_ok_for_hw_watchpoint.
> > 	* target.h (struct target_ops): Delete 
> > 	to_region_size_ok_for_hw_watchpoint.
> > 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> 
> These patches are both OK.  You might want to combine them - it's a
> much smaller diff :-)  But it doesn't matter since you've already got
> them separated out.

Thanks for reviewing that.  I would like to commit them one by one, thus I 
don't need to think about how to re-describe them.  :-)

I also think that it is clearer to differentiate the purpose of these two 
patches.  Combining them together seems a little confusing to me.

Best Regards
- Wu Zhou

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

[Daniel Jacobowitz]

On Wed, Jan 25, 2006 at 04:34:14PM +0800, Wu Zhou wrote:
> I am also thinking of replace the macro 
> TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (SIZE) with 
> TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR, SIZE).  Thus the code will seem 
> to be more clean: we will only have one macro to see if the target region 
> is ok for watchpoint monitoring.  Following this way, function 
> default_region_ok_for_hw_watchpoint will also return back to its original 
> implementation.  What is your thought on this?

I'm not sure what you mean.  After these patches, the only reference to
TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT is in gdbint.texinfo (which I'd
appreciate if you fixed, in a separate patch - thanks in advance).
What are the two ways now?

On Wed, Jan 25, 2006 at 11:18:49AM +0800, Wu Zhou wrote:
> OK to commit?
> 
> 2006-01-22  Ben Elliston  <bje@au1.ibm.com>
> 	    Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* ppc-linux-nat.c (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG,
> 	PTRACE_GETSIGINFO): Define.
> 	(last_stopped_data_address): New.
> 	(ppc_linux_check_watch_resources): New function.
> 	(ppc_linux_region_ok_for_hw_watchpoint): New function.
> 	(ppc_linux_insert_watchpoint): New function.
> 	(ppc_linux_remove_watchpoint): New function.
> 	(ppc_linux_stopped_data_address): New function.
> 	(ppc_linux_stopped_by_watchpoint): New function.
> 	(_initialize_ppc_linux_nat): Set the above hardware watchpoint
> 	related target vectors.
> 	* rs6000-tdep.c (rs6000_gdbarch_init): Set PPC architectures
> 	to have nonsteppable watchpoint.
> 	* target.c (default_region_ok_for_hw_watchpoint,
> 	debug_to_region_ok_for_hw_watchpoint): New prototypes.
> 	(update_current_target): Inherit to_region_ok_for_hw_watchpoint
> 	and set default to_region_ok_for_hw_watchpoint.
> 	(default_region_ok_for_hw_watchpoint): New function.
> 	(debug_to_region_ok_for_hw_watchpoint): New function.
> 	(setup_target_debug): Set to_region_ok_for_hw_watchpoint of 
> 	debug_target.
> 	* target.h (struct target_ops): Add a new target vector 
> 	to_region_ok_for_hw_watchpoint.
> 	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Define this if it is not
> 	defined anyplace else.

On Wed, Jan 25, 2006 at 04:34:14PM +0800, Wu Zhou wrote:
> 2006-01-25  Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* breakpoint.c (TARGET_REGION_OK_FOR_HW_WATCHPOINT): Delete.
> 	* config/i386/nm-i386sol2.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> 	* config/mips/nm-irix5.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> 	* config/sparc/nm-sol2.h (TARGET_REGION_OK_FOR_HW_WATCHPOINT): New.
> 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.
> 	* inf-ttrace.c (inf_ttrace_region_ok_for_hw_watchpoint): New.
> 	(inf_ttrace_region_size_ok_for_hw_watchpoint): Delete.
> 	(inf_ttrace_target): Delete to_region_size_ok_for_hw_watchpoint and
> 	add to_region_ok_for_hw_watchpoint.
> 	* s390-nat.c (s390_region_size_ok_for_hw_watchpoint): Delete.
> 	(s390_region_ok_for_hw_watchpoint): New.
> 	(_initialize_s390_nat): Delete to_region_size_ok_for_hw_watchpoint
> 	and add to_region_ok_for_hw_watchpoint.
> 	* target.c (default_region_size_ok_for_hw_watchpoint, 
> 	debug_to_region_size_ok_for_hw_watchpoint): Delete prototype.
> 	(update_current_target): Delete to_region_size_ok_for_hw_watchpoint
> 	inheritance and default_region_size_ok_for_hw_watchpoint.
> 	(default_region_ok_for_hw_watchpoint): If len is less than or equal
> 	the length of void pointer, return ok.
> 	(default_region_size_ok_for_hw_watchpoint): Delete.
> 	(debug_to_region_size_ok_for_hw_watchpoint): Delete.
> 	(setup_target_debug): Delete to_region_size_ok_for_hw_watchpoint.
> 	* target.h (struct target_ops): Delete 
> 	to_region_size_ok_for_hw_watchpoint.
> 	(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT): Delete.

These patches are both OK.  You might want to combine them - it's a
much smaller diff :-)  But it doesn't matter since you've already got
them separated out.


-- 
Daniel Jacobowitz
CodeSourcery

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

[Wu Zhou]

On Tue, 24 Jan 2006, Daniel Jacobowitz wrote:

> On Tue, Jan 24, 2006 at 06:59:53PM +0800, Wu Zhou wrote:
> > Hi Daniel,
> > 
> > Because there are quite a few places in different arch/target of GDB which 
> > use region_size_ok_for_hw_watchpoint, so I am prefering to get this done 
> > in two steps: first add to_region_ok_for_hw_watchpoint into struct 
> > target_ops, then replace these to_region_size_ok_for_hw_watchpoint 
> > reference with to_region_ok_for_hw_watchpoint ones.  IMHO, it is easier to 
> > not confuse the original intention of this patch with this replacement. 
> > What is your thought on this?
> 
> Sure.  Couple small things but we're almost done.

Thanks!  Here is the revised patch to address these small things.  

OK to commit?

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

	* ppc-linux-nat.c (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG,
	PTRACE_GETSIGINFO): Define.
	(last_stopped_data_address): New.
	(ppc_linux_check_watch_resources): New function.
	(ppc_linux_region_ok_for_hw_watchpoint): New function.
	(ppc_linux_insert_watchpoint): New function.
	(ppc_linux_remove_watchpoint): New function.
	(ppc_linux_stopped_data_address): New function.
	(ppc_linux_stopped_by_watchpoint): New function.
	(_initialize_ppc_linux_nat): Set the above hardware watchpoint
	related target vectors.
	* rs6000-tdep.c (rs6000_gdbarch_init): Set PPC architectures
	to have nonsteppable watchpoint.
	* target.c (default_region_ok_for_hw_watchpoint,
	debug_to_region_ok_for_hw_watchpoint): New prototypes.
	(update_current_target): Inherit to_region_ok_for_hw_watchpoint
	and set default to_region_ok_for_hw_watchpoint.
	(default_region_ok_for_hw_watchpoint): New function.
	(debug_to_region_ok_for_hw_watchpoint): New function.
	(setup_target_debug): Set to_region_ok_for_hw_watchpoint of 
	debug_target.
	* target.h (struct target_ops): Add a new target vector 
	to_region_ok_for_hw_watchpoint.
	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Define this if it is not
	defined anyplace else.

Index: ppc-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-linux-nat.c,v
retrieving revision 1.55
diff -u -p -r1.55 ppc-linux-nat.c
--- ppc-linux-nat.c	10 Sep 2005 18:11:04 -0000	1.55
+++ ppc-linux-nat.c	25 Jan 2006 02:20:49 -0000
@@ -81,6 +81,16 @@
 #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.
@@ -146,6 +156,7 @@ struct gdb_evrregset_t
    error.  */
 int have_ptrace_getvrregs = 1;
 
+static CORE_ADDR last_stopped_data_address = 0;
 
 /* Non-zero if our kernel may support the PTRACE_GETEVRREGS and
    PTRACE_SETEVRREGS requests, for reading and writing the SPE
@@ -153,7 +164,6 @@ int have_ptrace_getvrregs = 1;
    error.  */
 int have_ptrace_getsetevrregs = 1;
 
-
 int
 kernel_u_size (void)
 {
@@ -777,6 +787,124 @@ store_ppc_registers (int tid)
     store_spe_register (tid, -1);
 }
 
+static 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 variants.
+     Some variants 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 supports PTRACE_SET_DEBUGREG and whether
+     the target has DABR.  If either answer is no, the ptrace call will
+     return -1.  Fail in that case.  */
+  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_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+  /* Handle sub-8-byte quantities.  */
+  if (len <= 0)
+    return 0;
+
+  /* addr+len must fall in the 8 byte watchable region.  */
+  if ((addr + len) > (addr & ~7) + 8)
+    return 0;
+
+  return 1;
+}
+
+/* Set a watchpoint of type TYPE at address ADDR.  */
+static long
+ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw)
+{
+  int tid;
+  long dabr_value;
+  ptid_t ptid = inferior_ptid;
+
+  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);
+}
+
+static 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);
+}
+
+static 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;
+      last_stopped_data_address = 0;
+      return 1;
+    }
+  return 0;
+}
+
+static 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)
 {
@@ -900,6 +1028,14 @@ _initialize_ppc_linux_nat (void)
   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_ok_for_hw_watchpoint = ppc_linux_region_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);
 }
Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.248
diff -u -p -r1.248 rs6000-tdep.c
--- rs6000-tdep.c	1 Nov 2005 19:32:36 -0000	1.248
+++ rs6000-tdep.c	25 Jan 2006 02:20:51 -0000
@@ -3278,6 +3278,8 @@ rs6000_gdbarch_init (struct gdbarch_info
                     _("rs6000_gdbarch_init: "
                     "received unexpected BFD 'arch' value"));
 
+  set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
+
   /* Sanity check on registers.  */
   gdb_assert (strcmp (tdep->regs[tdep->ppc_gp0_regnum].name, "r0") == 0);
 
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.111
diff -u -p -r1.111 target.c
--- target.c	4 Sep 2005 16:18:20 -0000	1.111
+++ target.c	25 Jan 2006 02:20:51 -0000
@@ -47,6 +47,8 @@ static void kill_or_be_killed (int);
 
 static void default_terminal_info (char *, int);
 
+static int default_region_ok_for_hw_watchpoint (CORE_ADDR, int);
+
 static int default_region_size_ok_for_hw_watchpoint (int);
 
 static int nosymbol (char *, CORE_ADDR *);
@@ -128,6 +130,8 @@ static int debug_to_stopped_by_watchpoin
 
 static int debug_to_stopped_data_address (struct target_ops *, CORE_ADDR *);
 
+static int debug_to_region_ok_for_hw_watchpoint (CORE_ADDR, int);
+
 static int debug_to_region_size_ok_for_hw_watchpoint (int);
 
 static void debug_to_terminal_init (void);
@@ -405,6 +409,7 @@ update_current_target (void)
       INHERIT (to_stopped_data_address, t);
       INHERIT (to_stopped_by_watchpoint, t);
       INHERIT (to_have_continuable_watchpoint, t);
+      INHERIT (to_region_ok_for_hw_watchpoint, t);
       INHERIT (to_region_size_ok_for_hw_watchpoint, t);
       INHERIT (to_terminal_init, t);
       INHERIT (to_terminal_inferior, t);
@@ -531,6 +536,8 @@ update_current_target (void)
   de_fault (to_stopped_data_address,
 	    (int (*) (struct target_ops *, CORE_ADDR *))
 	    return_zero);
+  de_fault (to_region_ok_for_hw_watchpoint,
+	    default_region_ok_for_hw_watchpoint);
   de_fault (to_region_size_ok_for_hw_watchpoint,
 	    default_region_size_ok_for_hw_watchpoint);
   de_fault (to_terminal_init, 
@@ -1575,6 +1582,12 @@ find_default_create_inferior (char *exec
 }
 
 static int
+default_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+  return TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (len);
+}
+
+static int
 default_region_size_ok_for_hw_watchpoint (int byte_count)
 {
   return (byte_count <= TYPE_LENGTH (builtin_type_void_data_ptr));
@@ -2115,6 +2128,21 @@ debug_to_can_use_hw_breakpoint (int type
 }
 
 static int
+debug_to_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+  CORE_ADDR retval;
+
+  retval = debug_target.to_region_ok_for_hw_watchpoint (addr, len);
+
+  fprintf_unfiltered (gdb_stdlog,
+		      "TARGET_REGION_OK_FOR_HW_WATCHPOINT (%ld, %ld) = 0x%lx\n",
+		      (unsigned long) addr,
+		      (unsigned long) len,
+		      (unsigned long) retval);
+  return retval;
+}
+
+static int
 debug_to_region_size_ok_for_hw_watchpoint (int byte_count)
 {
   CORE_ADDR retval;
@@ -2533,6 +2561,7 @@ setup_target_debug (void)
   current_target.to_remove_watchpoint = debug_to_remove_watchpoint;
   current_target.to_stopped_by_watchpoint = debug_to_stopped_by_watchpoint;
   current_target.to_stopped_data_address = debug_to_stopped_data_address;
+  current_target.to_region_ok_for_hw_watchpoint = debug_to_region_ok_for_hw_watchpoint;
   current_target.to_region_size_ok_for_hw_watchpoint = debug_to_region_size_ok_for_hw_watchpoint;
   current_target.to_terminal_init = debug_to_terminal_init;
   current_target.to_terminal_inferior = debug_to_terminal_inferior;
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.76
diff -u -p -r1.76 target.h
--- target.h	4 Sep 2005 16:18:20 -0000	1.76
+++ target.h	25 Jan 2006 02:20:51 -0000
@@ -345,6 +345,7 @@ struct target_ops
     int (*to_stopped_by_watchpoint) (void);
     int to_have_continuable_watchpoint;
     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
+    int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, int);
     int (*to_region_size_ok_for_hw_watchpoint) (int);
     void (*to_terminal_init) (void);
     void (*to_terminal_inferior) (void);
@@ -1030,6 +1031,11 @@ extern void (*deprecated_target_new_objf
  (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
 #endif
 
+#ifndef TARGET_REGION_OK_FOR_HW_WATCHPOINT
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, len) \
+    (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
+#endif
+
 #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
 #define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \
     (*current_target.to_region_size_ok_for_hw_watchpoint) (byte_count)

Regards
- Wu Zhou

> 
> > 	(ppc_linux_check_watch_resources): New function to check whether
> > 	the target has available hardware watchpoint resource.
> > 	(ppc_linux_region_ok_for_hw_watchpoint): New function to check if
> > 	the region is ok for hardware watchpoint.
> > 	(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.
> 
> "New function" is sufficient; if you feel that the function needs an
> explanation, it should go in the code, not in the changelog.
> 
> > 	target.h (struct target_ops): Add a new target vector 
> 
> Missed a "* " there.
> 
> > +  /* P630 has nonsteppable watchpoint.  So we are assuming that all PowerPC
> > +     targets have nonsteppable watchpoint.  */
> > +  set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
> 
> Please skip this comment.
> 
> > @@ -1575,6 +1582,12 @@ find_default_create_inferior (char *exec
> >  }
> >  
> >  static int
> > +default_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
> > +{
> > +  return (len <= TYPE_LENGTH (builtin_type_void_data_ptr));
> > +}
> 
> TARGET_REGION_OK_FOR_HW_WATCHPOINT will now always be defined, because
> of the #ifdef in target.h.  Therefore this won't trigger (from
> breakpoint.c):
> 
> #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
>      (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN))
> #endif
> 
> So you need to call TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (len) here,
> in case the target has overridden that.
> 
> 
> -- 
> Daniel Jacobowitz
> CodeSourcery
> 
> 

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

[Daniel Jacobowitz]

On Tue, Jan 24, 2006 at 06:59:53PM +0800, Wu Zhou wrote:
> Hi Daniel,
> 
> Because there are quite a few places in different arch/target of GDB which 
> use region_size_ok_for_hw_watchpoint, so I am prefering to get this done 
> in two steps: first add to_region_ok_for_hw_watchpoint into struct 
> target_ops, then replace these to_region_size_ok_for_hw_watchpoint 
> reference with to_region_ok_for_hw_watchpoint ones.  IMHO, it is easier to 
> not confuse the original intention of this patch with this replacement. 
> What is your thought on this?

Sure.  Couple small things but we're almost done.

> 	(ppc_linux_check_watch_resources): New function to check whether
> 	the target has available hardware watchpoint resource.
> 	(ppc_linux_region_ok_for_hw_watchpoint): New function to check if
> 	the region is ok for hardware watchpoint.
> 	(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.

"New function" is sufficient; if you feel that the function needs an
explanation, it should go in the code, not in the changelog.

> 	target.h (struct target_ops): Add a new target vector 

Missed a "* " there.

> +  /* P630 has nonsteppable watchpoint.  So we are assuming that all PowerPC
> +     targets have nonsteppable watchpoint.  */
> +  set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);

Please skip this comment.

> @@ -1575,6 +1582,12 @@ find_default_create_inferior (char *exec
>  }
>  
>  static int
> +default_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
> +{
> +  return (len <= TYPE_LENGTH (builtin_type_void_data_ptr));
> +}

TARGET_REGION_OK_FOR_HW_WATCHPOINT will now always be defined, because
of the #ifdef in target.h.  Therefore this won't trigger (from
breakpoint.c):

#if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
     (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN))
#endif

So you need to call TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT (len) here,
in case the target has overridden that.


-- 
Daniel Jacobowitz
CodeSourcery

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

[Wu Zhou]

Hi Daniel,

Because there are quite a few places in different arch/target of GDB which 
use region_size_ok_for_hw_watchpoint, so I am prefering to get this done 
in two steps: first add to_region_ok_for_hw_watchpoint into struct 
target_ops, then replace these to_region_size_ok_for_hw_watchpoint 
reference with to_region_ok_for_hw_watchpoint ones.  IMHO, it is easier to 
not confuse the original intention of this patch with this replacement. 
What is your thought on this?

Here is the modified patch following this point.  Please review.

P.S: I tested it on ppc64 with 2.6.15-git8 kernel (without any patch this 
time, Anton had check the needed code into the official kernel source
tree).  It works ok.  I can see six more PASS on gdb.base/recurse.exp; the 
other seven failure are around the second watchpoint, which is S/W one. 
No regression is found on other testcases. I also tested this on old 
PPC kernel which don't support DABR, don't find any regression with the 
introduction of this patch.

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

	* ppc-linux-nat.c (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG,
	PTRACE_GETSIGINFO): Define.
	(last_stopped_data_address): New.
	(ppc_linux_check_watch_resources): New function to check whether
	the target has available hardware watchpoint resource.
	(ppc_linux_region_ok_for_hw_watchpoint): New function to check if
	the region is ok for hardware watchpoint.
	(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.
	* rs6000-tdep.c (rs6000_gdbarch_init): Set PPC architectures
	to have nonsteppable watchpoint.
	* target.c (default_region_ok_for_hw_watchpoint,
	debug_to_region_ok_for_hw_watchpoint): New prototypes.
	(update_current_target): Inherit to_region_ok_for_hw_watchpoint
	and set default to_region_ok_for_hw_watchpoint.
	(default_region_ok_for_hw_watchpoint): New function.
	(debug_to_region_ok_for_hw_watchpoint): New function.
	(setup_target_debug): Set to_region_ok_for_hw_watchpoint of 
	debug_target.
	target.h (struct target_ops): Add a new target vector 
	to_region_ok_for_hw_watchpoint.
	(TARGET_REGION_OK_FOR_HW_WATCHPOINT): Define this if it is not
	defined anyplace else.

Index: ppc-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-linux-nat.c,v
retrieving revision 1.55
diff -u -p -r1.55 ppc-linux-nat.c
--- ppc-linux-nat.c	10 Sep 2005 18:11:04 -0000	1.55
+++ ppc-linux-nat.c	24 Jan 2006 09:19:06 -0000
@@ -81,6 +81,16 @@
 #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.
@@ -146,6 +156,7 @@ struct gdb_evrregset_t
    error.  */
 int have_ptrace_getvrregs = 1;
 
+static CORE_ADDR last_stopped_data_address = 0;
 
 /* Non-zero if our kernel may support the PTRACE_GETEVRREGS and
    PTRACE_SETEVRREGS requests, for reading and writing the SPE
@@ -153,7 +164,6 @@ int have_ptrace_getvrregs = 1;
    error.  */
 int have_ptrace_getsetevrregs = 1;
 
-
 int
 kernel_u_size (void)
 {
@@ -777,6 +787,124 @@ store_ppc_registers (int tid)
     store_spe_register (tid, -1);
 }
 
+static 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 variants.
+     Some variants 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 supports PTRACE_SET_DEBUGREG and whether
+     the target has DABR.  If either answer is no, the ptrace call will
+     return -1.  Fail in that case.  */
+  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_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+  /* Handle sub-8-byte quantities.  */
+  if (len <= 0)
+    return 0;
+
+  /* addr+len must fall in the 8 byte watchable region.  */
+  if ((addr + len) > (addr & ~7) + 8)
+    return 0;
+
+  return 1;
+}
+
+/* Set a watchpoint of type TYPE at address ADDR.  */
+static long
+ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw)
+{
+  int tid;
+  long dabr_value;
+  ptid_t ptid = inferior_ptid;
+
+  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);
+}
+
+static 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);
+}
+
+static 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;
+      last_stopped_data_address = 0;
+      return 1;
+    }
+  return 0;
+}
+
+static 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)
 {
@@ -900,6 +1028,14 @@ _initialize_ppc_linux_nat (void)
   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_ok_for_hw_watchpoint = ppc_linux_region_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);
 }
Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.248
diff -u -p -r1.248 rs6000-tdep.c
--- rs6000-tdep.c	1 Nov 2005 19:32:36 -0000	1.248
+++ rs6000-tdep.c	24 Jan 2006 09:19:08 -0000
@@ -3278,6 +3278,10 @@ rs6000_gdbarch_init (struct gdbarch_info
                     _("rs6000_gdbarch_init: "
                     "received unexpected BFD 'arch' value"));
 
+  /* P630 has nonsteppable watchpoint.  So we are assuming that all PowerPC
+     targets have nonsteppable watchpoint.  */
+  set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
+
   /* Sanity check on registers.  */
   gdb_assert (strcmp (tdep->regs[tdep->ppc_gp0_regnum].name, "r0") == 0);
 
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.111
diff -u -p -r1.111 target.c
--- target.c	4 Sep 2005 16:18:20 -0000	1.111
+++ target.c	24 Jan 2006 09:19:09 -0000
@@ -47,6 +47,8 @@ static void kill_or_be_killed (int);
 
 static void default_terminal_info (char *, int);
 
+static int default_region_ok_for_hw_watchpoint (CORE_ADDR, int);
+
 static int default_region_size_ok_for_hw_watchpoint (int);
 
 static int nosymbol (char *, CORE_ADDR *);
@@ -128,6 +130,8 @@ static int debug_to_stopped_by_watchpoin
 
 static int debug_to_stopped_data_address (struct target_ops *, CORE_ADDR *);
 
+static int debug_to_region_ok_for_hw_watchpoint (CORE_ADDR, int);
+
 static int debug_to_region_size_ok_for_hw_watchpoint (int);
 
 static void debug_to_terminal_init (void);
@@ -405,6 +409,7 @@ update_current_target (void)
       INHERIT (to_stopped_data_address, t);
       INHERIT (to_stopped_by_watchpoint, t);
       INHERIT (to_have_continuable_watchpoint, t);
+      INHERIT (to_region_ok_for_hw_watchpoint, t);
       INHERIT (to_region_size_ok_for_hw_watchpoint, t);
       INHERIT (to_terminal_init, t);
       INHERIT (to_terminal_inferior, t);
@@ -531,6 +536,8 @@ update_current_target (void)
   de_fault (to_stopped_data_address,
 	    (int (*) (struct target_ops *, CORE_ADDR *))
 	    return_zero);
+  de_fault (to_region_ok_for_hw_watchpoint,
+	    default_region_ok_for_hw_watchpoint);
   de_fault (to_region_size_ok_for_hw_watchpoint,
 	    default_region_size_ok_for_hw_watchpoint);
   de_fault (to_terminal_init, 
@@ -1575,6 +1582,12 @@ find_default_create_inferior (char *exec
 }
 
 static int
+default_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+  return (len <= TYPE_LENGTH (builtin_type_void_data_ptr));
+}
+
+static int
 default_region_size_ok_for_hw_watchpoint (int byte_count)
 {
   return (byte_count <= TYPE_LENGTH (builtin_type_void_data_ptr));
@@ -2115,6 +2128,21 @@ debug_to_can_use_hw_breakpoint (int type
 }
 
 static int
+debug_to_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
+{
+  CORE_ADDR retval;
+
+  retval = debug_target.to_region_ok_for_hw_watchpoint (addr, len);
+
+  fprintf_unfiltered (gdb_stdlog,
+		      "TARGET_REGION_OK_FOR_HW_WATCHPOINT (%ld, %ld) = 0x%lx\n",
+		      (unsigned long) addr,
+		      (unsigned long) len,
+		      (unsigned long) retval);
+  return retval;
+}
+
+static int
 debug_to_region_size_ok_for_hw_watchpoint (int byte_count)
 {
   CORE_ADDR retval;
@@ -2533,6 +2561,7 @@ setup_target_debug (void)
   current_target.to_remove_watchpoint = debug_to_remove_watchpoint;
   current_target.to_stopped_by_watchpoint = debug_to_stopped_by_watchpoint;
   current_target.to_stopped_data_address = debug_to_stopped_data_address;
+  current_target.to_region_ok_for_hw_watchpoint = debug_to_region_ok_for_hw_watchpoint;
   current_target.to_region_size_ok_for_hw_watchpoint = debug_to_region_size_ok_for_hw_watchpoint;
   current_target.to_terminal_init = debug_to_terminal_init;
   current_target.to_terminal_inferior = debug_to_terminal_inferior;
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.76
diff -u -p -r1.76 target.h
--- target.h	4 Sep 2005 16:18:20 -0000	1.76
+++ target.h	24 Jan 2006 09:19:09 -0000
@@ -345,6 +345,7 @@ struct target_ops
     int (*to_stopped_by_watchpoint) (void);
     int to_have_continuable_watchpoint;
     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
+    int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, int);
     int (*to_region_size_ok_for_hw_watchpoint) (int);
     void (*to_terminal_init) (void);
     void (*to_terminal_inferior) (void);
@@ -1030,6 +1031,11 @@ extern void (*deprecated_target_new_objf
  (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
 #endif
 
+#ifndef TARGET_REGION_OK_FOR_HW_WATCHPOINT
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, len) \
+    (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
+#endif
+
 #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
 #define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \
     (*current_target.to_region_size_ok_for_hw_watchpoint) (byte_count)



Regards
- Wu Zhou

On Tue, 24 Jan 2006, Wu Zhou wrote:

> 
> On Mon, 23 Jan 2006, Daniel Jacobowitz wrote:
> 
> > On Tue, Jan 24, 2006 at 11:40:16AM +0800, Wu Zhou wrote:
> > > p630 is one kind of POWER4 based pSeriese server. It is currently the only 
> > > available ppc machine I can get.  :-)
> > > 
> > > In fact, I am not sure before if the ppc arch has nonsteppable watchpoints 
> > > or not. But testing on my p630 box, it did had nonsteppable ones.  Now 
> > > that an architecture either have or doesn't have nonsteppable watchpoints, 
> > > can we get from this test a result that ppc architecture has nonsteppable 
> > > watchpoints?
> > > 
> > > If so, maybe I can just remove the stupid conditional statement below.  
> > > (my original intention is to verify that v->mach equals bfd_mach_ppc_630 :-)
> > 
> > Well, it'd be nice to have some architectural reference for this.  But
> > it's probably a safe bet to assume that this is generally true for all
> > PowerPC targets, so let's just assume it.
> 
> OK.  I will assume this then.
> 
> > > Function to_region_ok_for_hw_watchpoint is not in the current target 
> > > vector (I means struct target_ops).  Maybe we can add it into
> > > target_ops? There are a few other archs also use this.  But they had to 
> > > include it in nm-xxx-yyy.h.  If not, the only method I can think of is 
> > > also include its definition in nm-ppc64-linux.h.  So what about the 
> > > following patch section?
> > > 
> > >       int (*to_region_size_ok_for_hw_watchpoint) (int);
> > > +     int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
> > >       void (*to_terminal_init) (void);
> > 
> > I would recommend replacing to_region_size_ok_for_hw_watchpoint
> > with to_region_ok_for_hw_watchpoint.  You'll have to update the
> > callers, including the non-multi-arch ones, to ignore the first
> > argument; shouldn't be hard?
> 
> Do you means to make the following change in gdb/target.h?
> 
> --- gdb/target.h        4 Sep 2005 16:18:20 -0000       1.76
> +++ gdb/target.h        24 Jan 2006 04:17:13 -0000
> @@ -345,7 +345,7 @@
>      int (*to_stopped_by_watchpoint) (void);
>      int to_have_continuable_watchpoint;
>      int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
> -    int (*to_region_size_ok_for_hw_watchpoint) (int);
> +    int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
>      void (*to_terminal_init) (void);
>      void (*to_terminal_inferior) (void);
>      void (*to_terminal_ours_for_output) (void);
> @@ -1030,9 +1030,9 @@
>   (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
>  #endif
> 
> -#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \
> -    (*current_target.to_region_size_ok_for_hw_watchpoint) (byte_count)
> +#if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, byte_count) \
> +    (*current_target.to_region_ok_for_hw_watchpoint) (addr, byte_count)
>  #endif
> 
> Then make responsive changes to the code where is referenced?
> 
> such as (in config/i386/nm-i386sol2.h):
> 
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(SIZE) 1
> +#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR, SIZE) 1
> 
> or (in s390-nat.c): 
> 
>  static int
> -s390_region_size_ok_for_hw_watchpoint (int cnt)
> +s390_region_ok_for_hw_watchpoint (CORE_ADDR *, int cnt)
>  {
>    return 1;
>  }
> @@ -380,7 +380,7 @@
> 
>    /* Add our watchpoint methods.  */
>    t->to_can_use_hw_breakpoint = s390_can_use_hw_breakpoint;
> -  t->to_region_size_ok_for_hw_watchpoint = s390_region_size_ok_for_hw_watchpoint;
> +  t->to_region_ok_for_hw_watchpoint = s390_region_ok_for_hw_watchpoint;
>    t->to_have_continuable_watchpoint = 1;
>    t->to_stopped_by_watchpoint = s390_stopped_by_watchpoint;
> 
> and so on.
> 
> P.S: If I can understand and also I understand correctly, it is not hard.
> Sometimes I just need  a little more time to understand the precise 
> meaning of your words.  Most of the time I attribute it to my english.
> 
> ;-)
> 
> Regards
> - Wu Zhou
> 
> 

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

[Wu Zhou]

On Mon, 23 Jan 2006, Daniel Jacobowitz wrote:

> On Tue, Jan 24, 2006 at 11:40:16AM +0800, Wu Zhou wrote:
> > p630 is one kind of POWER4 based pSeriese server. It is currently the only 
> > available ppc machine I can get.  :-)
> > 
> > In fact, I am not sure before if the ppc arch has nonsteppable watchpoints 
> > or not. But testing on my p630 box, it did had nonsteppable ones.  Now 
> > that an architecture either have or doesn't have nonsteppable watchpoints, 
> > can we get from this test a result that ppc architecture has nonsteppable 
> > watchpoints?
> > 
> > If so, maybe I can just remove the stupid conditional statement below.  
> > (my original intention is to verify that v->mach equals bfd_mach_ppc_630 :-)
> 
> Well, it'd be nice to have some architectural reference for this.  But
> it's probably a safe bet to assume that this is generally true for all
> PowerPC targets, so let's just assume it.

OK.  I will assume this then.

> > Function to_region_ok_for_hw_watchpoint is not in the current target 
> > vector (I means struct target_ops).  Maybe we can add it into
> > target_ops? There are a few other archs also use this.  But they had to 
> > include it in nm-xxx-yyy.h.  If not, the only method I can think of is 
> > also include its definition in nm-ppc64-linux.h.  So what about the 
> > following patch section?
> > 
> >       int (*to_region_size_ok_for_hw_watchpoint) (int);
> > +     int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
> >       void (*to_terminal_init) (void);
> 
> I would recommend replacing to_region_size_ok_for_hw_watchpoint
> with to_region_ok_for_hw_watchpoint.  You'll have to update the
> callers, including the non-multi-arch ones, to ignore the first
> argument; shouldn't be hard?

Do you means to make the following change in gdb/target.h?

--- gdb/target.h        4 Sep 2005 16:18:20 -0000       1.76
+++ gdb/target.h        24 Jan 2006 04:17:13 -0000
@@ -345,7 +345,7 @@
     int (*to_stopped_by_watchpoint) (void);
     int to_have_continuable_watchpoint;
     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
-    int (*to_region_size_ok_for_hw_watchpoint) (int);
+    int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
     void (*to_terminal_init) (void);
     void (*to_terminal_inferior) (void);
     void (*to_terminal_ours_for_output) (void);
@@ -1030,9 +1030,9 @@
  (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
 #endif

-#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
-#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \
-    (*current_target.to_region_size_ok_for_hw_watchpoint) (byte_count)
+#if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(addr, byte_count) \
+    (*current_target.to_region_ok_for_hw_watchpoint) (addr, byte_count)
 #endif

Then make responsive changes to the code where is referenced?

such as (in config/i386/nm-i386sol2.h):

-#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(SIZE) 1
+#define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR, SIZE) 1

or (in s390-nat.c): 

 static int
-s390_region_size_ok_for_hw_watchpoint (int cnt)
+s390_region_ok_for_hw_watchpoint (CORE_ADDR *, int cnt)
 {
   return 1;
 }
@@ -380,7 +380,7 @@

   /* Add our watchpoint methods.  */
   t->to_can_use_hw_breakpoint = s390_can_use_hw_breakpoint;
-  t->to_region_size_ok_for_hw_watchpoint = s390_region_size_ok_for_hw_watchpoint;
+  t->to_region_ok_for_hw_watchpoint = s390_region_ok_for_hw_watchpoint;
   t->to_have_continuable_watchpoint = 1;
   t->to_stopped_by_watchpoint = s390_stopped_by_watchpoint;

and so on.

P.S: If I can understand and also I understand correctly, it is not hard.
Sometimes I just need  a little more time to understand the precise 
meaning of your words.  Most of the time I attribute it to my english.

;-)

Regards
- Wu Zhou

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

[Daniel Jacobowitz]

On Tue, Jan 24, 2006 at 11:40:16AM +0800, Wu Zhou wrote:
> p630 is one kind of POWER4 based pSeriese server. It is currently the only 
> available ppc machine I can get.  :-)
> 
> In fact, I am not sure before if the ppc arch has nonsteppable watchpoints 
> or not. But testing on my p630 box, it did had nonsteppable ones.  Now 
> that an architecture either have or doesn't have nonsteppable watchpoints, 
> can we get from this test a result that ppc architecture has nonsteppable 
> watchpoints?
> 
> If so, maybe I can just remove the stupid conditional statement below.  
> (my original intention is to verify that v->mach equals bfd_mach_ppc_630 :-)

Well, it'd be nice to have some architectural reference for this.  But
it's probably a safe bet to assume that this is generally true for all
PowerPC targets, so let's just assume it.
> Function to_region_ok_for_hw_watchpoint is not in the current target 
> vector (I means struct target_ops).  Maybe we can add it into
> target_ops? There are a few other archs also use this.  But they had to 
> include it in nm-xxx-yyy.h.  If not, the only method I can think of is 
> also include its definition in nm-ppc64-linux.h.  So what about the 
> following patch section?
> 
>       int (*to_region_size_ok_for_hw_watchpoint) (int);
> +     int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
>       void (*to_terminal_init) (void);

I would recommend replacing to_region_size_ok_for_hw_watchpoint
with to_region_ok_for_hw_watchpoint.  You'll have to update the
callers, including the non-multi-arch ones, to ignore the first
argument; shouldn't be hard?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Wu Zhou]

Hi Daniel,

On Sun, 22 Jan 2006, Daniel Jacobowitz wrote:

> Most of this looks good.  A couple bits don't though.

Thanks for reviewing this patch.  I will try to address your concerns.

> On Thu, Dec 22, 2005 at 12:47:18PM +0800, Wu Zhou wrote:
> > 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.
> 
> First, please don't abbreviate in changelogs.  Second, this code
> doesn't make sense.  It sounds like you've only tested on p630,
> whatever that is, which is fine - but watchpoints have nothing to do
> with bfd_mach_ppc_p630.  Either the architecture has nonsteppable
> watchpoints, or it doesn't.

p630 is one kind of POWER4 based pSeriese server. It is currently the only 
available ppc machine I can get.  :-)

In fact, I am not sure before if the ppc arch has nonsteppable watchpoints 
or not. But testing on my p630 box, it did had nonsteppable ones.  Now 
that an architecture either have or doesn't have nonsteppable watchpoints, 
can we get from this test a result that ppc architecture has nonsteppable 
watchpoints?

If so, maybe I can just remove the stupid conditional statement below.  
(my original intention is to verify that v->mach equals bfd_mach_ppc_630 :-)

> 
> > +   if (bfd_mach_ppc_630)
> > +     set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
> 
> BTW:
> 
> ../bfd/bfd-in2.h:#define bfd_mach_ppc_630       630
> 
> So I don't think this is testing what you wanted to, anyway :-)
> 
> > 	* ppc-linux-nat.c: Define three macro: PTRACE_GET_DEBUGREG,
> > 	PTRACE_SET_DEBUGREG and PTRACE_GETSIGINFO. Define one static
> > 	variable last_stopped_data_address.
> 
> Please use:
> 
> (PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG, PTRACE_GETSIGINFO): Define.
> (last_stopped_data_address): New.

OK.  
 
> Can all the new functions in ppc-linux-nat.c be static?
> 
> > +   /* 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.  */
> 
> I believe you want "variants" in both places.

Yes.  You are right.  Will update this with a new patch.
  
> > +   /* 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.  */
> 
>   /* We need to know whether ptrace supports PTRACE_SET_DEBUGREG and whether the
>      target has DABR.  If either answer is no, the ptrace call will return -1.
>      Fail in that case.  */

Sorry for the bad english.  :-)

Will update this with the new patch.

> 
> > + static int
> > + ppc_linux_region_size_ok_for_hw_watchpoint (int cnt)
> > + {
> > +   return 1;
> > + }
> 
> The argument is LEN, not CNT.  It would be nice to do a useful check
> here; I think that to do that, you'd need to move
> TARGET_REGION_OK_FOR_HW_WATCHPOINT into the target vector.  You could
> probably replace TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT and have the
> current implementations ignore the address.

Function to_region_ok_for_hw_watchpoint is not in the current target 
vector (I means struct target_ops).  Maybe we can add it into
target_ops? There are a few other archs also use this.  But they had to 
include it in nm-xxx-yyy.h.  If not, the only method I can think of is 
also include its definition in nm-ppc64-linux.h.  So what about the 
following patch section?

      int (*to_region_size_ok_for_hw_watchpoint) (int);
+     int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR *, int);
      void (*to_terminal_init) (void);

> That would let you remove some failure cases from
> target_insert_watchpoint.

You said it.

> Also, please remove the commented out version of
> ppc_linux_stopped_data_address.

OK. I will do this in a new patch a while later.

Regards
- Wu Zhou

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

[Daniel Jacobowitz]

Most of this looks good.  A couple bits don't though.

On Thu, Dec 22, 2005 at 12:47:18PM +0800, Wu Zhou wrote:
> 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.

First, please don't abbreviate in changelogs.  Second, this code
doesn't make sense.  It sounds like you've only tested on p630,
whatever that is, which is fine - but watchpoints have nothing to do
with bfd_mach_ppc_p630.  Either the architecture has nonsteppable
watchpoints, or it doesn't.

> +   if (bfd_mach_ppc_630)
> +     set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);

BTW:

../bfd/bfd-in2.h:#define bfd_mach_ppc_630       630

So I don't think this is testing what you wanted to, anyway :-)

> 	* ppc-linux-nat.c: Define three macro: PTRACE_GET_DEBUGREG,
> 	PTRACE_SET_DEBUGREG and PTRACE_GETSIGINFO. Define one static
> 	variable last_stopped_data_address.

Please use:

(PTRACE_GET_DEBUGREG, PTRACE_SET_DEBUGREG, PTRACE_GETSIGINFO): Define.
(last_stopped_data_address): New.

Can all the new functions in ppc-linux-nat.c be static?

> +   /* 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.  */

I believe you want "variants" in both places.

> +   /* 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.  */

  /* We need to know whether ptrace supports PTRACE_SET_DEBUGREG and whether the
     target has DABR.  If either answer is no, the ptrace call will return -1.
     Fail in that case.  */

> + static int
> + ppc_linux_region_size_ok_for_hw_watchpoint (int cnt)
> + {
> +   return 1;
> + }

The argument is LEN, not CNT.  It would be nice to do a useful check
here; I think that to do that, you'd need to move
TARGET_REGION_OK_FOR_HW_WATCHPOINT into the target vector.  You could
probably replace TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT and have the
current implementations ignore the address.

That would let you remove some failure cases from
target_insert_watchpoint.

Also, please remove the commented out version of
ppc_linux_stopped_data_address.


-- 
Daniel Jacobowitz
CodeSourcery

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

[Eli Zaretskii]

> Date: Thu, 22 Dec 2005 12:47:18 +0800 (CST)
> From: Wu Zhou <woodzltc@cn.ibm.com>
> cc: gdb-patches@sources.redhat.com, drow@false.org, mark@xs4all.nl,         bje@au1.ibm.com, anton@au1.ibm.com
> 
> 
> After we get stopped data address, we can set it back to 0, because it is 
> not used any more if not hit another time by a h/w watchpoint.  Did it 
> make sense? 

Yes, I think so.

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

[Wu Zhou]

On Thu, 22 Dec 2005, Eli Zaretskii wrote:

> > Date: Thu, 22 Dec 2005 12:05:47 +0800 (CST)
> > From: Wu Zhou <woodzltc@cn.ibm.com>
> > cc: drow@false.org, mark@xs4all.nl, bje@au1.ibm.com, anton@au1.ibm.com
> > 
> > Here is the reworked patch for latest 6.4 tree.  Any comments?  Thanks a 
> > lot!
> 
> I don't see in your patch where last_stopped_data_address is reset to
> zero.  It seems like it always stores the last address, so GDB might
> think a watchpoint was hit where in fact it wasn't.
> 
> Am I missing something?

Sorry, it is me who missed something.  :-)

After we get stopped data address, we can set it back to 0, because it is 
not used any more if not hit another time by a h/w watchpoint.  Did it 
make sense? 

Here is the modified patch:

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: Define three macro: PTRACE_GET_DEBUGREG,
	PTRACE_SET_DEBUGREG and PTRACE_GETSIGINFO. Define one static
	variable last_stopped_data_address.
	(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 04:39:40 -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 04:39:40 -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 = 0;
  
  /* 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,932 ----
      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;
+       last_stopped_data_address = 0;
+       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 ****
--- 1050,1063 ----
    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);
  }
 
Regards
- Wu Zhou 

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

[Eli Zaretskii]

> Date: Thu, 22 Dec 2005 12:05:47 +0800 (CST)
> From: Wu Zhou <woodzltc@cn.ibm.com>
> cc: drow@false.org, mark@xs4all.nl, bje@au1.ibm.com, anton@au1.ibm.com
> 
> Here is the reworked patch for latest 6.4 tree.  Any comments?  Thanks a 
> lot!

I don't see in your patch where last_stopped_data_address is reset to
zero.  It seems like it always stores the last address, so GDB might
think a watchpoint was hit where in fact it wasn't.

Am I missing something?

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

[Wu Zhou]

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
> 
> 

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

[Wu Zhou]

> 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