[ppc-linux-nat]: set access flag for h/w watchpoint even if it is only read or write

[ppc-linux-nat]: set access flag for h/w watchpoint even if it is only read or write

[Wu Zhou]

Hello all,

I found a bug in the current ppc-linux h/w watchpoint implementation:  
when we set read watchpoint to some expression, if there are any write 
operation to it before a read operation is hit, watchpoint_check will see 
that its value is changed. So user won't see the watchpoint is hit.

I make one change to the SET_DEBUGREG operation: even if it is only 
read or write watchpoint, we still set access flag.  Then, no matter 
what operation is on the watched address, a SIGTRAP will be triggered. 
The gdb code itself can determine if it is a write operation or read 
operation.  If it is write, watchpoint_check routine can update the 
bs->value to the latest.  

Here is the patch.  Thanks for reviewing.

2006-06-09  Wu Zhou  <woodzltc@cn.ibm.com>

	* ppc-linux-nat.c (ppc_linux_insert_watchpoint): Set access flag for
	all hardware watchpoint.

--- ppc-linux-nat.c.orig	2006-06-09 14:53:35.000000000 +0800
+++ ppc-linux-nat.c	2006-06-09 15:04:12.000000000 +0800
@@ -821,22 +821,7 @@ ppc_linux_insert_watchpoint (CORE_ADDR a
   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;
-    }
+  dabr_value = addr | 7;
 
   tid = TIDGET (ptid);
   if (tid == 0)


:ADDPATCH ppc:

Regards
- Wu Zhou

[RFC]: h/w watchpoint (r,w,a) for ppc440

[Wu Zhou]

I am also working on providing basic h/w watchpoint functionality to 
ppc440 chip.  It is requested that read/write/access watchpoints are all 
supported.  The bug talked about below is also found in this process. So I 
choose to merge them together.

Appended is the patch.  Please note that I am now using /proc/cpuinfo to 
know what chip we are running on.  I see that some code in gdb are using 
bfd_mach to differentiate between various chips in one cpu family.  But 
there is no ppc_440 (we have ppc_403, ppc_505, ppc_603 and so on) for 
bfd_mach yet.  So if you prefer this kind of consistent mechanism, we 
might need to convince bfd maintainers to get out a bfd_mach_ppc_440 
first. 

I had tested this on ppc440 and power4 machine with the testcase posted at 
http://sources.redhat.com/ml/gdb-patches/2006-06/msg00169.html, all 44 
tests pass.  Please review and comment.  Thanks.

diff -uNr src/gdb/ppc-linux-nat.c src.440/gdb/ppc-linux-nat.c
--- src/gdb/ppc-linux-nat.c	2006-04-11 03:53:24.000000000 +0800
+++ src.440/gdb/ppc-linux-nat.c	2006-06-22 08:35:20.947752072 +0800
@@ -796,16 +796,20 @@
   /* 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.  */
+  /* XXX: There are two watchpoint registers in ppc440.  But currrent kernel
+     only support one.  To support two watchpoint registers, both the kernel 
+     and gdb code need to make some change.  */
   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);
 
+  /* We need to know whether ptrace supports PTRACE_SET_DEBUGREG and whether
+     the target has DABR or DAC (ppc440 uses DAC register to store watchpoint
+     address).  If either answer is no, the ptrace call will return -1.  Fail
+     in that case.  */
   if (ptrace (PTRACE_SET_DEBUGREG, tid, 0, 0) == -1)
     return 0;
   return 1;
@@ -825,6 +829,37 @@
   return 1;
 }
 
+#define MAXLINE 256
+
+static char *
+get_cpu_name ()
+{
+  char *cpuinfo = "/proc/cpuinfo";
+  FILE *fp = fopen (cpuinfo, "r");
+  char *buf = malloc (MAXLINE);
+  char *saved;
+
+  if (fp == NULL)
+    {
+      perror ("file open failed:");
+      return (NULL);
+    }
+
+  while (fgets (buf, MAXLINE, fp) != NULL)
+    {
+      if (strstr (buf, "cpu") != NULL)
+        {
+//          printf ("cpu hit: %s\n", buf);
+          char *next = strtok_r (buf, ": ", &saved);
+          fclose (fp);
+          return (saved);
+        }
+    }
+
+  fclose (fp);
+  return (NULL);
+}
+
 /* Set a watchpoint of type TYPE at address ADDR.  */
 static int
 ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw)
@@ -833,22 +868,10 @@
   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;
-    }
+  if (strstr (get_cpu_name (), "440GP") != NULL)
+    dabr_value = addr | 3;
+  else
+    dabr_value = addr | 7;
 
   tid = TIDGET (ptid);
   if (tid == 0)


Regards
- Wu Zhou

On Fri, 9 Jun 2006, Wu Zhou wrote:

> Hello all,
> 
> I found a bug in the current ppc-linux h/w watchpoint implementation:  
> when we set read watchpoint to some expression, if there are any write 
> operation to it before a read operation is hit, watchpoint_check will see 
> that its value is changed. So user won't see the watchpoint is hit.
> 
> I make one change to the SET_DEBUGREG operation: even if it is only 
> read or write watchpoint, we still set access flag.  Then, no matter 
> what operation is on the watched address, a SIGTRAP will be triggered. 
> The gdb code itself can determine if it is a write operation or read 
> operation.  If it is write, watchpoint_check routine can update the 
> bs->value to the latest.  
> 
> Here is the patch.  Thanks for reviewing.
> 
> 2006-06-09  Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* ppc-linux-nat.c (ppc_linux_insert_watchpoint): Set access flag for
> 	all hardware watchpoint.
> 
> --- ppc-linux-nat.c.orig	2006-06-09 14:53:35.000000000 +0800
> +++ ppc-linux-nat.c	2006-06-09 15:04:12.000000000 +0800
> @@ -821,22 +821,7 @@ ppc_linux_insert_watchpoint (CORE_ADDR a
>    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;
> -    }
> +  dabr_value = addr | 7;
>  
>    tid = TIDGET (ptid);
>    if (tid == 0)
> 
> 
> :ADDPATCH ppc:
> 
> Regards
> - Wu Zhou
> 

Re: [ppc-linux-nat]: set access flag for h/w watchpoint even if it is only read or write

[Daniel Jacobowitz]

On Fri, Jun 09, 2006 at 11:40:26PM +0800, Wu Zhou wrote:
> Hello all,
> 
> I found a bug in the current ppc-linux h/w watchpoint implementation:  
> when we set read watchpoint to some expression, if there are any write 
> operation to it before a read operation is hit, watchpoint_check will see 
> that its value is changed. So user won't see the watchpoint is hit.
> 
> I make one change to the SET_DEBUGREG operation: even if it is only 
> read or write watchpoint, we still set access flag.  Then, no matter 
> what operation is on the watched address, a SIGTRAP will be triggered. 
> The gdb code itself can determine if it is a write operation or read 
> operation.  If it is write, watchpoint_check routine can update the 
> bs->value to the latest.  

Eli, you're the most familiar with watchpoint support; do you have any
comment on this?

I know that we've encountered the same problem on other architectures
before (I can't recall exactly when, but I definitely remember Ulrich
fixing it for S/390, in the same way).  It seems strange to me.  Many
targets can tell us what sort of watchpoint was hit; we could do
something like extend target_stopped_data_address to also return what
sort of watchpoint was triggered, if it knows.

> -    case hw_write:
> -      /* Set write and translate bits.  */
> -      dabr_value |= 6;
> -      break;

Wu, I bet you don't really want to change this case.  Write watchpoints
are the most common, and I think they'll work fine even if they don't
trigger on reads - since some platforms can't even trigger on reads.
This would be a big slowdown.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [ppc-linux-nat]: set access flag for h/w watchpoint even if it is only read or write

[Eli Zaretskii]

> Date: Thu, 6 Jul 2006 09:20:20 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> On Fri, Jun 09, 2006 at 11:40:26PM +0800, Wu Zhou wrote:
> > Hello all,
> > 
> > I found a bug in the current ppc-linux h/w watchpoint implementation:  
> > when we set read watchpoint to some expression, if there are any write 
> > operation to it before a read operation is hit, watchpoint_check will see 
> > that its value is changed. So user won't see the watchpoint is hit.
> > 
> > I make one change to the SET_DEBUGREG operation: even if it is only 
> > read or write watchpoint, we still set access flag.  Then, no matter 
> > what operation is on the watched address, a SIGTRAP will be triggered. 
> > The gdb code itself can determine if it is a write operation or read 
> > operation.  If it is write, watchpoint_check routine can update the 
> > bs->value to the latest.  
> 
> Eli, you're the most familiar with watchpoint support; do you have any
> comment on this?

I'm sorry, I missed it somehow.

Yes, this problem is known on x86 and elsewhere.  The problem is
extremely rare, as reading and writing to the same address in the same
instruction is a hard-to-accomplish treat.  Wu, could you show a
real-life example of where this matters?

The solution you suggest, in my experience, is worse than the problem:
it will cause a significant slow-down of the most frequent case, as
Daniel points out.

> Many targets can tell us what sort of watchpoint was hit; we could
> do something like extend target_stopped_data_address to also return
> what sort of watchpoint was triggered, if it knows.

Yes, that would be a welcome addition, I agree.

Re: [ppc-linux-nat]: set access flag for h/w watchpoint even if it is only read or write

[Daniel Jacobowitz]

On Thu, Jul 06, 2006 at 11:58:58PM +0300, Eli Zaretskii wrote:
> Yes, this problem is known on x86 and elsewhere.  The problem is
> extremely rare, as reading and writing to the same address in the same
> instruction is a hard-to-accomplish treat.  Wu, could you show a
> real-life example of where this matters?

I thought, though I may be misremembering, that it was actually a
different problem.  Something like this:

- We set a read watchpoint.  It does not trigger on writes.

- An instruction writes to the location.

- GDB stops, sees that it stopped at a watchpoint at the given address,
  tries to determine what sort of watchpoint it was, determines that
  the value had changed, and ignores the read watchpoint - the value
  has changed since we last checked so this "must" have been a write
  watchpoint.

Is that plausible or nonsensical?

-- 
Daniel Jacobowitz
CodeSourcery