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

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

[Wu Zhou]

Eli,

Just as Daniel said, my problem is not the same as yours. given the
following code:

   int var1 = 0, var2;
   var1 = 10;
   var2 = var1;

if we set read watchpoint on var1, it should stop at "var2 = var1", but
gdb are using "watchpoint_check" to see if the watched variable are
changed or not, if it is changed, then it assumes the watched variable is
writed; if not, it assumes the watched variable is read.

Because when execution get to "var1=10", it won't trigger a watchpoint,
the old value stored in var1 is still 0, so watchpoint_check will get a
wrong conclusion that var1 is changed to 10 by this instruction.  So it
won't treat it as a read watchpoint hit.

My solution is to let gdb update the value stored in the watched variable,
so that it always get the fresh value for comparison.

Another solution might be to change the verify logic of read watchpoint
hit in watchpoint_check.  Maybe we can just trust the underlying os will
only trigger in read hit?

Still another is like what Daniel and you said, we can let the target to
tell what kind of hit it is on the watched variable.  As for this, I can
figure out two methods at this time: the kernel should transfer the
watchpoint type information to user space (maybe through siginfo), or
gdb iterate through all the watched variable to determine what kind of
watchpoint this is.  Both of these changes also involve the change in
watchpoint_check routine.

Comparing all these solution, my solution is the most simple one.  I mean,
it makes little change to the current code.  To address the slowdown, we
can still use the original flags for write hit.

What is your idea on this?

On Thu, 6 Jul 2006, Daniel Jacobowitz wrote:

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

Regards
- Wu Zhou

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

[Eli Zaretskii]

> Date: Fri,  7 Jul 2006 00:35:21 -0400
> From: Wu Zhou <woodzltc@cn.ibm.com>
> Cc: gdb-patches@sourceware.org
> 
> Just as Daniel said, my problem is not the same as yours. given the
> following code:
> 
>    int var1 = 0, var2;
>    var1 = 10;
>    var2 = var1;
> 
> if we set read watchpoint on var1, it should stop at "var2 = var1", but
> gdb are using "watchpoint_check" to see if the watched variable are
> changed or not, if it is changed, then it assumes the watched variable is
> writed; if not, it assumes the watched variable is read.
> 
> Because when execution get to "var1=10", it won't trigger a watchpoint,
> the old value stored in var1 is still 0, so watchpoint_check will get a
> wrong conclusion that var1 is changed to 10 by this instruction.  So it
> won't treat it as a read watchpoint hit.

This doesn't happen on x86, with this test program:

    #include <stdio.h>

    int main (void)
    {
      int var1 = 0, var2;
      var1 = 10;
      var2 = var1;

      printf ("%d %d\n", var1, var2);
      return 0;
    }

I think the reason it works for me is that on x86, there's no real
support for read watchpoints, so we actually set a read-write
watchpoint, and then the logic of watchpoint_check does TRT.

What is the situation with this on a PPC?  What kinds of data
breakpoints does it support, and what associated functionality can we
use in the ptrace call or its PPC equivalent?

> My solution is to let gdb update the value stored in the watched variable,
> so that it always get the fresh value for comparison.

That will work, but the question is: is this the optimal solution for
the PPC?  Maybe there's a better solution, one that doesn't slow down
the normal case.

> Another solution might be to change the verify logic of read watchpoint
> hit in watchpoint_check.  Maybe we can just trust the underlying os will
> only trigger in read hit?

The question is, how to do that without breaking other platforms, like
x86, which we cannot trust.  If you can come up with a design that
accommodates both types of situations, I will be happy to review it.

Daniel, could you please point me to Ulrich's change, either in
ChangeLogs or in the sources?  I cannot find it forf some reason.

> Comparing all these solution, my solution is the most simple one.  I mean,
> it makes little change to the current code.  To address the slowdown, we
> can still use the original flags for write hit.
> 
> What is your idea on this?

I think the cleanest solution is for breakpoint.c to get the idea of
the kind of support it can get from the target.  If the target
implements real read watchpoints, it should propagate that knowledge
to breakpoint.c, where it examines the watchpoints and decides which
one to announce.

Please note that there are complications in this area: a user could
have legitimately set several different watchpoints on the same
address, each one with its own type (read, write, access) and its own
set of conditions and/or commands.  Whatever new design we come up
with, it has to support these complicated situations, because the user
will expect GDB to announce only those watchpoints which meet the
conditions and type constraints.  For example, what happens with your
solution if I put both read and write watchpoints on the same
variable?  The current code does TRT on x86 in that case.  As you see
below, it correctly announces each one of the two watchpoints where
expected (except for a slight off-by-one shift in line numbers):

    GNU gdb 6.3
    Copyright 2004 Free Software Foundation, Inc.
    GDB is free software, covered by the GNU General Public License, and you are
    welcome to change it and/or distribute copies of it under certain conditions.
    Type "show copying" to see the conditions.
    There is absolutely no warranty for GDB.  Type "show warranty" for details.
    This GDB was configured as "i686-pc-mingw32"...
    (gdb) start
    Breakpoint 1 at 0x4012b5: file wpt.c, line 4.
    Starting program: D:\usr\eli\data/./wpt.exe
    main () at wpt.c:4
    4       {
    (gdb) watch var1
    Hardware watchpoint 2: var1
    (gdb) rwatch var1
    Hardware read watchpoint 3: var1
    (gdb) c
    Continuing.
    Hardware watchpoint 2: var1

    Old value = 2
    New value = 0
    main () at wpt.c:6
    6         var1 = 10;
    (gdb)
    Continuing.
    Hardware watchpoint 2: var1

    Old value = 0
    New value = 10
    main () at wpt.c:7
    7         var2 = var1;
    (gdb)
    Continuing.
    Hardware read watchpoint 3: var1

    Value = 10
    0x004012cb in main () at wpt.c:7
    7         var2 = var1;
    (gdb)
    Continuing.
    Hardware read watchpoint 3: var1

    Value = 10
    0x004012d8 in main () at wpt.c:9
    9         printf ("%d %d\n", var1, var2);
    (gdb)
    Continuing.
    10 10

    Watchpoint 2 deleted because the program has left the block in
    which its expression is valid.

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

[Daniel Jacobowitz]

On Fri, Jul 07, 2006 at 01:04:36PM +0300, Eli Zaretskii wrote:
> I think the reason it works for me is that on x86, there's no real
> support for read watchpoints, so we actually set a read-write
> watchpoint, and then the logic of watchpoint_check does TRT.

Precisely.

> The question is, how to do that without breaking other platforms, like
> x86, which we cannot trust.  If you can come up with a design that
> accommodates both types of situations, I will be happy to review it.
> 
> Daniel, could you please point me to Ulrich's change, either in
> ChangeLogs or in the sources?  I cannot find it forf some reason.

I must be mistaken; our S/390 support doesn't have any read watchpoints
(I don't know if the architecture does or not).  In fact I can't find
any architecture that does this.  But I immediately recognized
the description of the problem... so it must have happened somewhere.

I can't find the discussion of it, but the gdbserver crisv32 port does
the same thing:

  /* Read watchpoints are set as access watchpoints, because of GDB's
     inability to deal with pure read watchpoints.  */
  if (type == '3')
    type = '4';

Here's some more about it:
  http://sources.redhat.com/ml/gdb/2005-11/msg00231.html

In which I also claimed S/390 did it, which doesn't appear to be true,
but at least I've had the same misconception for a while now.


-- 
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: Fri, 7 Jul 2006 09:17:54 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Wu Zhou <woodzltc@cn.ibm.com>, gdb-patches@sourceware.org
> 
> > Daniel, could you please point me to Ulrich's change, either in
> > ChangeLogs or in the sources?  I cannot find it forf some reason.
> 
> I must be mistaken; our S/390 support doesn't have any read watchpoints
> (I don't know if the architecture does or not).  In fact I can't find
> any architecture that does this.  But I immediately recognized
> the description of the problem... so it must have happened somewhere.

You are not dreaming, because I remember that as well.  Perhaps Ulrich
just suggested a change, but it wasn't accepted eventually.

> I can't find the discussion of it, but the gdbserver crisv32 port does
> the same thing:
> 
>   /* Read watchpoints are set as access watchpoints, because of GDB's
>      inability to deal with pure read watchpoints.  */
>   if (type == '3')
>     type = '4';
> 
> Here's some more about it:
>   http://sources.redhat.com/ml/gdb/2005-11/msg00231.html
> 
> In which I also claimed S/390 did it, which doesn't appear to be true,
> but at least I've had the same misconception for a while now.

Thanks for the pointers.  I think if we wish to change this, someone
will need to step forward and volunteer to submit a clean design of
how a back end could tell breakpoint.c that read watchpoints are
really supported, and how breakpoint.c could use that info to DTRT.

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

[Ulrich Weigand]

Eli Zaretskii wrote:
> > Date: Fri, 7 Jul 2006 09:17:54 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Wu Zhou <woodzltc@cn.ibm.com>, gdb-patches@sourceware.org
> > 
> > > Daniel, could you please point me to Ulrich's change, either in
> > > ChangeLogs or in the sources?  I cannot find it forf some reason.
> > 
> > I must be mistaken; our S/390 support doesn't have any read watchpoints
> > (I don't know if the architecture does or not).  In fact I can't find
> > any architecture that does this.  But I immediately recognized
> > the description of the problem... so it must have happened somewhere.
> 
> You are not dreaming, because I remember that as well.  Perhaps Ulrich
> just suggested a change, but it wasn't accepted eventually.

I'm not sure which conversation you're referring to.  In any case, S/390
does not support read watchpoints at all, neither by themselves nor in
combination with a write watchpoint.  The only type of hardware watchpoint
it supports is the pure write watchpoint.

I do recall some discussion about watchpoints on S/390, but that had to
do with the fact that on our platform, the hardware does not identify
the data address that triggered the watchpoint, and there were some
changes to GDB common code that implicitly assumed that the target 
should be able to do that ...

Bye,
Ulrich

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

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

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

[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