read watchpoints ignored?

read watchpoints ignored?

[Vladimir Prus]

Hi,
I found a situation when read watchpoints are ignored by gdb. Consider this
program:

   #include <stdio.h>

   int a, b, c;

   int main()
   {
       a = 10;
       printf("Here I am\n");

       b = a;
       c = a;

       return 0;
   }

and this gdb session:

    (gdb) b main
    Breakpoint 1 at 0x80483a4: file rw.cpp, line 8.
    (gdb) r
    Starting program: /tmp/a.out

    Breakpoint 1, main () at rw.cpp:8
    8           a = 10;
    (gdb) rwatch a
    Hardware read watchpoint 2: a
    (gdb) c
    Continuing.
    Hardware read watchpoint 2: a

    Value = 10
    0x080483bd in main () at rw.cpp:11
    11          c = a;

Expected result: gdb stops on "b = a" line.
Actual result: gdb stops on "c = a".

Here's why it happens. When breakpoint is set, gdb reads the current value
of 'a', which is zero. After continue, a is assigned the value of '10', and
then first read watchpoint fires (on "b = a"). We arrive to
"bpstat_stop_status" (breakpoint.c), which has the following code:


  else if (b->type == bp_read_watchpoint || 
             b->type == bp_access_watchpoint)
      {
        ...
        if (found)
          {
            ...
            int e = catch_errors (watchpoint_check, bs, message,
                                  RETURN_MASK_ALL);
            switch (e)
              {
              .......
              case WP_VALUE_CHANGED:
                if (b->type == bp_read_watchpoint)
                  {
                    /* Don't stop: read watchpoints shouldn't fire if
                       the value has changed.  This is for targets
                       which cannot set read-only watchpoints.  */
                    bs->print_it = print_it_noop;
                    bs->stop = 0;
                    continue;
                  }


Since value of 'a' was changed by the "a = 10" line, "watchpoint_check"
returns "WP_VALUE_CHANGED", and reporting of watchpoint is completely
suppressed.

Questions:
1. Is this a bug?
2. Can this bug be fixed by removing this inner

      if (b->type == bp_read_wathcpoint)

   statement? This will cause watchpoints to trigger more often
   on those target that don't have "read-only watchpoints", but 
   there will be no risk of missing watchpoint. And I think missed
   watchpoint is more harm then spurious watchpoint.

Opinions?

- Volodya

Re: read watchpoints ignored?

[Eli Zaretskii]

> From: Vladimir Prus <ghost@cs.msu.su>
> Date: Fri, 11 Nov 2005 16:22:00 +0300
> 
>     (gdb) b main
>     Breakpoint 1 at 0x80483a4: file rw.cpp, line 8.
>     (gdb) r
>     Starting program: /tmp/a.out
> 
>     Breakpoint 1, main () at rw.cpp:8
>     8           a = 10;
>     (gdb) rwatch a
>     Hardware read watchpoint 2: a
>     (gdb) c
>     Continuing.
>     Hardware read watchpoint 2: a
> 
>     Value = 10
>     0x080483bd in main () at rw.cpp:11
>     11          c = a;
> 
> Expected result: gdb stops on "b = a" line.
> Actual result: gdb stops on "c = a".

On what platform was that, and with which version of GDB?  Also, what
debug info format was used by the compiler in your case, and what
compiler switches were used to compile and link the program?

I will assume that this is on some x86 platform; if not, the rest of
this message might not be useful.

First, I cannot reproduce this problem with the MS-Windows port of
GCC; the session transcript I saw is at the end of this message.

On Debian GNU/Linux, I can reproduce the problem, but it seems that it
is a side effect of another problem: the breakpoint set by "b main"
stops the program too late.  Observe:

  ~$ gdb ./rwt
  GNU gdb 6.1-debian
  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 "i386-linux"...Using host libthread_db library "/lib/tls/libthread_db.so.1".
  (gdb) info address main
  Symbol "main" is a function at address 0x8048364.
  (gdb) b *0x8048364
  Breakpoint 1 at 0x8048364: file rwt.c, line 6.
  (gdb) r
  Starting program: /home/e/eliz/rwt

  Breakpoint 1, main () at rwt.c:6
  6       {
  (gdb) rwatch a
  Hardware read watchpoint 2: a
  (gdb) c
  Continuing.
  Here I am
  Hardware read watchpoint 2: a

  Value = 10
  0x0804838f in main () at rwt.c:10
  10         b = a;
  (gdb) c
  Continuing.
  Hardware read watchpoint 2: a

  Value = 10
  0x08048399 in main () at rwt.c:11
  11         c = a;

Note that a breakpoint set by address now stops the program at the
right place: the opening brace of the main function.  In your session
(and also here, if I use "b main"), it stopped on line 7, not line 6.

Can someone please tell why "b main" stops the program one line too
late?

So I'm guessing that the problem happens because GDB misses the data
write into a in line 7, and thus doesn't take notice that a was
assigned the value 10.  That's why it gets confused when "b = a;"
reads from a.

> Here's why it happens. When breakpoint is set, gdb reads the current value
> of 'a', which is zero. After continue, a is assigned the value of '10', and
> then first read watchpoint fires (on "b = a"). We arrive to
> "bpstat_stop_status" (breakpoint.c), which has the following code:
> 
> 
>   else if (b->type == bp_read_watchpoint || 
>              b->type == bp_access_watchpoint)
>       {
>         ...
>         if (found)
>           {
>             ...
>             int e = catch_errors (watchpoint_check, bs, message,
>                                   RETURN_MASK_ALL);
>             switch (e)
>               {
>               .......
>               case WP_VALUE_CHANGED:
>                 if (b->type == bp_read_watchpoint)
>                   {
>                     /* Don't stop: read watchpoints shouldn't fire if
>                        the value has changed.  This is for targets
>                        which cannot set read-only watchpoints.  */
>                     bs->print_it = print_it_noop;
>                     bs->stop = 0;
>                     continue;
>                   }
> 
> 
> Since value of 'a' was changed by the "a = 10" line, "watchpoint_check"
> returns "WP_VALUE_CHANGED", and reporting of watchpoint is completely
> suppressed.
> 
> Questions:
> 1. Is this a bug?

No, I don't think so.  The theory behind the read watchpoint support
on x86 is that GDB should also see the data writes into the watched
variable, and thus always track its current value.  This is because
x86 debug hardware doesn't support read watchpoints, it only supports
data-write watchpoints and access (read or write) watchpoints.  So GDB
_emulates_ read watchpoints on x86 by setting an access watchpoint,
and monitoring value changes with the code you presented above.
Access watchpoint should trigger when the address is written by the
"a = 10;" line, at which point GDB should get control, record the new
value, then continue the debuggee without announcing the watchpoint
(since the value changed).  Later, when "b = a;" reads from a, the
access watchpoint fires again, but this time the value didn't change,
so the watchpoint should be announced.

Can you see which part of the above doesn't work, and why?

> 2. Can this bug be fixed by removing this inner
> 
>       if (b->type == bp_read_wathcpoint)
> 
>    statement?

I hope the description above explains why not.

>    This will cause watchpoints to trigger more often
>    on those target that don't have "read-only watchpoints", but 
>    there will be no risk of missing watchpoint.

Not only does it cause extra read watchpoints, it wreaks a complete
havoc in all but the most simple situations.  For example, consider
the case that two or more watchpoints are set on the same address,
each one with a different condition.  Without the above code, read
watchpoints are unusable on x86, and, worse, any use of read
watchpoints completely disrupts all other hardware watchpoints,
because the facilities used by GDB to find out which watchpoints fired
are very limited, so it gets completely confused when more than one
watchpoint seems to fire.

>    And I think missed
>    watchpoint is more harm then spurious watchpoint.

Both of these harmful, so we should fix this problem.  But not at a
price of going back to the mess we had back before this condition was
added; that's a price which is too heavy, IMO.

Here's the GDB session on MS-Windows.  As you see, "b main" stops at
line 6, the opening brace of the main function, and then the
watchpoint works as you expected.

  D:\usr\eli>gdb ./rwt.exe
  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) b main
  Breakpoint 1 at 0x4012b5: file rwt.c, line 6.
  (gdb) r
  Starting program: D:\usr\eli/./rwt.exe

  Breakpoint 1, main () at rwt.c:6
  6       {
  (gdb) rwatch a
  Hardware read watchpoint 2: a
  (gdb) c
  Continuing.
  Here I am
  Hardware read watchpoint 2: a

  Value = 10
  0x004012d5 in main () at rwt.c:10
  10         b = a;
  (gdb) c
  Continuing.
  Hardware read watchpoint 2: a

  Value = 10
  0x004012df in main () at rwt.c:11
  11         c = a;
  (gdb)

Re: read watchpoints ignored?

[Daniel Jacobowitz]

On Fri, Nov 11, 2005 at 08:19:41PM +0200, Eli Zaretskii wrote:
> >     (gdb) b main
> >     Breakpoint 1 at 0x80483a4: file rw.cpp, line 8.
> >     (gdb) r
> >     Starting program: /tmp/a.out
> > 
> >     Breakpoint 1, main () at rw.cpp:8
> >     8           a = 10;
> >     (gdb) rwatch a
> >     Hardware read watchpoint 2: a
> >     (gdb) c
> >     Continuing.
> >     Hardware read watchpoint 2: a
> > 
> >     Value = 10
> >     0x080483bd in main () at rw.cpp:11
> >     11          c = a;
> > 
> > Expected result: gdb stops on "b = a" line.
> > Actual result: gdb stops on "c = a".

I can reproduce this on Debian/unstable.

> On Debian GNU/Linux, I can reproduce the problem, but it seems that it
> is a side effect of another problem: the breakpoint set by "b main"
> stops the program too late.  Observe:

You included an example of setting the breakpoint by address, but not
one of setting by "b main", so I can't guess from your transcript what
happened.  You also didn't show the disassembly of the function.  To
answer your question, GDB sets breakpoints on functions based on a
combination of prologue analysis and debug info.

Here's what mine looks like (note, this is x86_64, not i386):

0x00000000004004a8 <main+0>:    push   %rbp
0x00000000004004a9 <main+1>:    mov    %rsp,%rbp
0x00000000004004ac <main+4>:    movl   $0xa,1049662(%rip)        # 0x5008f4 <a>
0x00000000004004b6 <main+14>:   mov    $0x40061c,%edi
0x00000000004004bb <main+19>:   callq  0x4003e0 <puts@plt>
0x00000000004004c0 <main+24>:   mov    1049646(%rip),%eax        # 0x5008f4 <a>
0x00000000004004c6 <main+30>:   mov    %eax,1049632(%rip)        # 0x5008ec <b>
0x00000000004004cc <main+36>:   mov    1049634(%rip),%eax        # 0x5008f4 <a>
0x00000000004004d2 <main+42>:   mov    %eax,1049624(%rip)        # 0x5008f0 <c>
0x00000000004004d8 <main+48>:   mov    $0x0,%eax
0x00000000004004dd <main+53>:   leaveq 
0x00000000004004de <main+54>:   retq   

"break main" sets the breakpoint at main+4.  Which I'd expect, really -
it's after the stack frame is created.

>   ~$ gdb ./rwt
>   GNU gdb 6.1-debian
>   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 "i386-linux"...Using host libthread_db library "/lib/tls/libthread_db.so.1".

This is a relatively old GDB, and so probably a relatively old GCC.  On
my installation it behaves as reported.

> So I'm guessing that the problem happens because GDB misses the data
> write into a in line 7, and thus doesn't take notice that a was
> assigned the value 10.  That's why it gets confused when "b = a;"
> reads from a.

It misses it, but shouldn't.  When GDB single steps over the
temporarily removed breakpoint at "main", it is not updating the values
of watchpoints before resuming the instruction.  That instruction
happened to modify the value behind a read watchpoint.

Presumably because trap_expected is set, infrun never calls
bpstat_stop_status, so the watchpoint is never checked.  Maybe we
should separate that code out into a separate function, or maybe we
should call it more rigorously.

> No, I don't think so.  The theory behind the read watchpoint support
> on x86 is that GDB should also see the data writes into the watched
> variable, and thus always track its current value.  This is because
> x86 debug hardware doesn't support read watchpoints, it only supports
> data-write watchpoints and access (read or write) watchpoints.  So GDB
> _emulates_ read watchpoints on x86 by setting an access watchpoint,
> and monitoring value changes with the code you presented above.
> Access watchpoint should trigger when the address is written by the
> "a = 10;" line, at which point GDB should get control, record the new
> value, then continue the debuggee without announcing the watchpoint
> (since the value changed).  Later, when "b = a;" reads from a, the
> access watchpoint fires again, but this time the value didn't change,
> so the watchpoint should be announced.

Eli, am I reading breakpoint.c right?  It looks like we _always_ ignore
changed values for bp_read_watchpoint, which means that the core of GDB
does not work on targets which support true read watchpoints.  Which
IIRC includes S/390, which disabled them for this reason.

This should be controlled by an architecture method indicating that
there are only access watchpoints.

> Here's the GDB session on MS-Windows.  As you see, "b main" stops at
> line 6, the opening brace of the main function, and then the
> watchpoint works as you expected.

Probably Windows code requires an additional instruction before "a" can
be written to, so you are not stopped on the store instruction as in my
example above.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: read watchpoints ignored?

[Eli Zaretskii]

> Date: Sun, 13 Nov 2005 11:45:48 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Vladimir Prus <ghost@cs.msu.su>, gdb@sources.redhat.com
> 
> I can reproduce this on Debian/unstable.

So can I, as I wrote in my reply.

> You included an example of setting the breakpoint by address, but not
> one of setting by "b main", so I can't guess from your transcript what
> happened.

You don't need to guess: since you reproduced the problem on your
machine (and I on mine), you could just look yourself.  I didn't show
such a transcript because it was identical to what Vladimir reported,
so instead I just said that I see the same on Debian.  I don't see why
I need to waste bandwidth by posting redundant information, given that
the OP's problem is easily reproducible on any similar platform.

> You also didn't show the disassembly of the function.

Again, there's no need to do that, as your own disassembly clearly
shows.  I don't understand why are you nitpicking at me.

> This is a relatively old GDB, and so probably a relatively old GCC.  On
> my installation it behaves as reported.

So it is on mine, thus the ``relatively old version'' is irrelevant.

> > So I'm guessing that the problem happens because GDB misses the data
> > write into a in line 7, and thus doesn't take notice that a was
> > assigned the value 10.  That's why it gets confused when "b = a;"
> > reads from a.
> 
> It misses it, but shouldn't.  When GDB single steps over the
> temporarily removed breakpoint at "main",
                      ^^^^^^^^^^
Did you mean ``watchpoint''?  If not, I'm afraid I don't follow.

>                                           it is not updating the values
> of watchpoints before resuming the instruction.  That instruction
> happened to modify the value behind a read watchpoint.

Then that's the bug, I'd say.  Do you agree that this is what we
should fix to solve this bug?  Because the following discussion of
problems with read watchpoints is only remotely related to the bug
reported by Vladimir.

> Presumably because trap_expected is set, infrun never calls
> bpstat_stop_status, so the watchpoint is never checked.  Maybe we
> should separate that code out into a separate function, or maybe we
> should call it more rigorously.

Sorry, I don't see how the suggestions in your second sentence would
solve the problem identified in the first.  Could you please explain?

> Eli, am I reading breakpoint.c right?

Right, and so am I.

> It looks like we _always_ ignore changed values for
> bp_read_watchpoint, which means that the core of GDB does not work
> on targets which support true read watchpoints.  Which IIRC includes
> S/390, which disabled them for this reason.

We've been through this: it's inaccurate to say that ``GDB does not
work on targets which support true read watchpoints.''  A more
accurate statement would be ``on targets which support true read
watchpoints, GDB does not announce read watchpoints when the watched
value changes,'' which is quite a different story, and a fairly rare
case.

If someone finds a way to announce a read watchpoint when the value
changed on a target that supports that, without losing support for
read watchpoints on x86 (which is by far the most popular platform
these days), I'll gladly agree to such a change.  IIRC, until now no
one did find how to do that.  I object to fixing read watchpoints in
such rare situations (i.e., when the data is only read, not written,
but the value still changes as a side effect) at a price of losing
read watchpoints on x86.  The solution proposed by Vladimir would do
precisely that, so I don't think we should accept it.

> This should be controlled by an architecture method indicating that
> there are only access watchpoints.

And then what? disable `rwatch'?  I object to such ``solution'', since
the emulated read watchpoints proved to be most useful to me since the
code we discuss was introduced.

Re: read watchpoints ignored?

[Daniel Jacobowitz]

On Mon, Nov 14, 2005 at 12:38:46AM +0200, Eli Zaretskii wrote:
> > I can reproduce this on Debian/unstable.
> 
> So can I, as I wrote in my reply.

Eli, why do you sound so offended?  The first part of your message was
a question about why GDB behaved a particular way for you, completely
independent of the original problem.  I tried my best to explain.  My
questions were based on the knowledge that this depends heavily on:
  - The version of GDB
  - The code generated for the function
  - The debug info generated by your compiler.

You reproduced it on a Debian system at least a year out of date.  I
have learned painfully over time that trying to generalize from the
behavior of one toolchain to the behavior of another is not time well
spent.  So I asked for some more information.  Between then, Debian
switched from GCC3 to GCC4.

Anyway, it doesn't matter.  Moving on.

> > > So I'm guessing that the problem happens because GDB misses the data
> > > write into a in line 7, and thus doesn't take notice that a was
> > > assigned the value 10.  That's why it gets confused when "b = a;"
> > > reads from a.
> > 
> > It misses it, but shouldn't.  When GDB single steps over the
> > temporarily removed breakpoint at "main",
>                       ^^^^^^^^^^
> Did you mean ``watchpoint''?  If not, I'm afraid I don't follow.

No, I mean breakpoint.  This is why I wanted to see the disassembly of
the function for you.

I'm talking about the breakpoint you set at main.  It's only while
single-stepping over that breakpoint, in order to "continue", that this
problem will manifest.  The breakpoint was set on the same instruction
which stored to "a", i.e. would have triggered the access watchpoint.

> >                                           it is not updating the values
> > of watchpoints before resuming the instruction.  That instruction
> > happened to modify the value behind a read watchpoint.
> 
> Then that's the bug, I'd say.  Do you agree that this is what we
> should fix to solve this bug?  Because the following discussion of
> problems with read watchpoints is only remotely related to the bug
> reported by Vladimir.

Yes, certainly this is the bug, I just don't know which way to fix it.

> > Presumably because trap_expected is set, infrun never calls
> > bpstat_stop_status, so the watchpoint is never checked.  Maybe we
> > should separate that code out into a separate function, or maybe we
> > should call it more rigorously.
> 
> Sorry, I don't see how the suggestions in your second sentence would
> solve the problem identified in the first.  Could you please explain?

I'm talking about watchpoint_check.  If that had been called, we would
have updated the value stored in the watchpoint for "a", and at the
next read watchpoint, it would not appear to have changed.

> > It looks like we _always_ ignore changed values for
> > bp_read_watchpoint, which means that the core of GDB does not work
> > on targets which support true read watchpoints.  Which IIRC includes
> > S/390, which disabled them for this reason.
> 
> We've been through this: it's inaccurate to say that ``GDB does not
> work on targets which support true read watchpoints.''  A more
> accurate statement would be ``on targets which support true read
> watchpoints, GDB does not announce read watchpoints when the watched
> value changes,'' which is quite a different story, and a fairly rare
> case.
> 
> If someone finds a way to announce a read watchpoint when the value
> changed on a target that supports that, without losing support for
> read watchpoints on x86 (which is by far the most popular platform
> these days), I'll gladly agree to such a change.  IIRC, until now no
> one did find how to do that.  I object to fixing read watchpoints in
> such rare situations (i.e., when the data is only read, not written,
> but the value still changes as a side effect) at a price of losing
> read watchpoints on x86.  The solution proposed by Vladimir would do
> precisely that, so I don't think we should accept it.

I didn't suggest that we should.

I also happen to disagree with you about the severity of this problem.
Fortunately any platform with read watchpoints probably also has access
watchpoints, so we can get by with using those instead.  But the first
read after the value changes has a high chance of being the important
one while debugging.

> > This should be controlled by an architecture method indicating that
> > there are only access watchpoints.
> 
> And then what? disable `rwatch'?  I object to such ``solution'', since
> the emulated read watchpoints proved to be most useful to me since the
> code we discuss was introduced.

I thought this was fairly straightforward, so I didn't go into detail.
Certainly there's no reason to disable rwatch.  Conditionalize "skip
this watchpoint if the value has changed" on "this is really an access
watchpoint because the target does not support read watchpoints".

It should be trivial to fix if you had a platform with read watchpoints
handy.  Which I don't easily.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: read watchpoints ignored?

[Eli Zaretskii]

> Date: Sun, 13 Nov 2005 21:43:45 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb@sources.redhat.com, Vladimir Prus <ghost@cs.msu.su>
> 
> > >                                           it is not updating the values
> > > of watchpoints before resuming the instruction.  That instruction
> > > happened to modify the value behind a read watchpoint.
> > 
> > Then that's the bug, I'd say.  Do you agree that this is what we
> > should fix to solve this bug?  Because the following discussion of
> > problems with read watchpoints is only remotely related to the bug
> > reported by Vladimir.
> 
> Yes, certainly this is the bug, I just don't know which way to fix it.

We need to find a way to get GDB to check whether the watchpoint fired
during such stepping.

> I'm talking about watchpoint_check.  If that had been called, we would
> have updated the value stored in the watchpoint for "a", and at the
> next read watchpoint, it would not appear to have changed.

I think we need call watchpoint_check only if
target_stopped_data_address says the watchpoint fired.

> Fortunately any platform with read watchpoints probably also has access
> watchpoints, so we can get by with using those instead.

Exactly.

> But the first read after the value changes has a high chance of
> being the important one while debugging.

I think the current code should handle this well.  But perhaps you are
thinking about some particular situation where it won't, so please
show the code you had in mind.

> Certainly there's no reason to disable rwatch.  Conditionalize "skip
> this watchpoint if the value has changed" on "this is really an access
> watchpoint because the target does not support read watchpoints".

That's fine with me.

Re: read watchpoints ignored?

[Vladimir Prus]

> > >                                           it is not updating the values
> > > of watchpoints before resuming the instruction.  That instruction
> > > happened to modify the value behind a read watchpoint.
> >
> > Then that's the bug, I'd say.  Do you agree that this is what we
> > should fix to solve this bug?  Because the following discussion of
> > problems with read watchpoints is only remotely related to the bug
> > reported by Vladimir.
>
> Yes, certainly this is the bug, I just don't know which way to fix it.

Well, there are two distinct bugs there. First is that "breakpoint on store 
instruction", where watched value should be re-read by gdb, but is not. 
Second is that for targets with real read watchpoints, the problem is present 
anyway. When I do "target remote :1234" the current PC is before assignment,
since watchpoint is read-only, gdb does not stop on write, and ignores first 
read of variable.

I can't fix the first problem yet, because I'm not familiar with the code.
About the second...

> > > This should be controlled by an architecture method indicating that
> > > there are only access watchpoints.
> >
> > And then what? disable `rwatch'?  I object to such ``solution'', since
> > the emulated read watchpoints proved to be most useful to me since the
> > code we discuss was introduced.
>
> I thought this was fairly straightforward, so I didn't go into detail.
> Certainly there's no reason to disable rwatch.  Conditionalize "skip
> this watchpoint if the value has changed" on "this is really an access
> watchpoint because the target does not support read watchpoints".
>
> It should be trivial to fix if you had a platform with read watchpoints
> handy.  Which I don't easily.

.. I can try adding extra method to target ops, and for remote target, 
implement it by returning the value of 
 
   remote_protocol_Z[Z_PACKET_READ_WP].support

Should I do this?

- Volodya

Re: read watchpoints ignored?

[Johan Rydberg]

Daniel Jacobowitz wrote:

> I thought this was fairly straightforward, so I didn't go into detail.
> Certainly there's no reason to disable rwatch.  Conditionalize "skip
> this watchpoint if the value has changed" on "this is really an access
> watchpoint because the target does not support read watchpoints".
> 
> It should be trivial to fix if you had a platform with read watchpoints
> handy.  Which I don't easily.

I happen to have access to a target that supports true read watchpoints,
and the following patch seems to do the trick, at least for the testcase
that Vladimir provided.

I also quickly tested it against the i386 target, and for that simple
test it does not seem to introduce any regressions.

Though I can not say that "this is the (right) way to do it", but you
could take a peek at it.  I suppose the comment in breakpoint.h should
be a bit more verbose.


2005-11-14  Johan Rydberg  <jrydberg@virtutech.com>

         * breakpoint.h: Document why hw_access should be used instead of
         hw_read for read watchpoints.

         * breakpoint.c (insert_bp_location): Use hw_access instead of
         hw_read for read watchpoints.



Index: breakpoint.h
===================================================================
RCS file: /repository/gdb/gdb/breakpoint.h,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 breakpoint.h
--- breakpoint.h        22 Aug 2005 15:45:43 -0000      1.1.1.1
+++ breakpoint.h        14 Nov 2005 13:01:54 -0000
@@ -177,6 +177,11 @@
      disp_donttouch             /* Leave it alone */
    };

+/* Most targets does not support true read watchpoints so GDB instead
+   uses access watchpoints, which almost every known target supports,
+   to provide the same functionality as true read watchpoints.  For
+   this reason, GDB will never insert a real read watchpoint (hw_read). */
+
  enum target_hw_bp_type
    {
      hw_write   = 0,            /* Common  HW watchpoint */
Index: breakpoint.c
===================================================================
RCS file: /repository/gdb/gdb/breakpoint.c,v
retrieving revision 1.2
diff -u -r1.2 breakpoint.c
--- breakpoint.c        20 Oct 2005 16:17:59 -0000      1.2
+++ breakpoint.c        14 Nov 2005 13:08:47 -0000
@@ -978,7 +978,11 @@
                       len = TYPE_LENGTH (VALUE_TYPE (v));
                       type = hw_write;
                       if (bpt->owner->type == bp_read_watchpoint)
-                       type = hw_read;
+                       {
+                         /* Use hw_access instead of hw_read for read
+                            watchpoints.  Why? See breakpoint.h */
+                         type = hw_access;
+                       }
                       else if (bpt->owner->type == bp_access_watchpoint)
                         type = hw_access;

@@ -1508,7 +1512,11 @@
                   len = TYPE_LENGTH (VALUE_TYPE (v));
                   type   = hw_write;
                   if (b->owner->type == bp_read_watchpoint)
-                   type = hw_read;
+                   {
+                     /* Use hw_access instead of hw_read for read
+                        watchpoints.  Why? See breakpoint.h */
+                     type = hw_access;
+                   }
                   else if (b->owner->type == bp_access_watchpoint)
                     type = hw_access;

Re: read watchpoints ignored?

[Vladimir Prus]

On Monday 14 November 2005 16:16, Johan Rydberg wrote:
> Daniel Jacobowitz wrote:
> > I thought this was fairly straightforward, so I didn't go into detail.
> > Certainly there's no reason to disable rwatch.  Conditionalize "skip
> > this watchpoint if the value has changed" on "this is really an access
> > watchpoint because the target does not support read watchpoints".
> >
> > It should be trivial to fix if you had a platform with read watchpoints
> > handy.  Which I don't easily.
>
> I happen to have access to a target that supports true read watchpoints,
> and the following patch seems to do the trick, at least for the testcase
> that Vladimir provided.

This essentially disables read watchpoints. What's the point in first 
implementing read watchpoints in a target, and then disabling them completely 
in gdb? Should be then disallow "read watchpoint" packet in the remote 
protocol?

I don't yet have specific *real-world* examples, but using "is the value 
different from the value last time we stopped" as a heuristic to decide if we 
hit write watchpoint or read watchpoint seems fragile.

- Volodya

Re: read watchpoints ignored?

[Johan Rydberg]

Vladimir Prus wrote:

> This essentially disables read watchpoints. What's the point in first 
> implementing read watchpoints in a target, and then disabling them completely 
> in gdb? Should be then disallow "read watchpoint" packet in the remote 
> protocol?

I wouldn't say it disables read watchpoints, since the functionality GDB
provides is exactly that; read watchpoints.  The difference is that it
uses access watchpoints to accomplish this, because a large set of targets
does not support true read watchpoints.

Which is more important; providing the functionality, or providing some
functionality for some targets, but using hardware read watchpoints instead
of hardware access watchpoints?

~j

Re: read watchpoints ignored?

[Vladimir Prus]

On Monday 14 November 2005 16:59, you wrote:
> Vladimir Prus wrote:
> > This essentially disables read watchpoints. What's the point in first
> > implementing read watchpoints in a target, and then disabling them
> > completely in gdb? Should be then disallow "read watchpoint" packet in
> > the remote protocol?
>
> I wouldn't say it disables read watchpoints, since the functionality GDB
> provides is exactly that; read watchpoints.  The difference is that it
> uses access watchpoints to accomplish this, because a large set of targets
> does not support true read watchpoints.
>
> Which is more important; providing the functionality, or providing some
> functionality for some targets, but using hardware read watchpoints instead
> of hardware access watchpoints?

I hope we can have both. The only thing needed is a way to detect that remote 
targets supports read watchpoints. Using 

   remote_protocol_Z[Z_PACKET_READ_WP].support

from remote.c (under cover of new target operation) is such a way, and I think 
it should work fine. But comments from more knowledgable persons are welcome.

- Volodya

Re: read watchpoints ignored?

[Daniel Jacobowitz]

On Mon, Nov 14, 2005 at 01:10:37PM +0300, Vladimir Prus wrote:
> > > > This should be controlled by an architecture method indicating that
> > > > there are only access watchpoints.
> > >
> > > And then what? disable `rwatch'?  I object to such ``solution'', since
> > > the emulated read watchpoints proved to be most useful to me since the
> > > code we discuss was introduced.
> >
> > I thought this was fairly straightforward, so I didn't go into detail.
> > Certainly there's no reason to disable rwatch.  Conditionalize "skip
> > this watchpoint if the value has changed" on "this is really an access
> > watchpoint because the target does not support read watchpoints".
> >
> > It should be trivial to fix if you had a platform with read watchpoints
> > handy.  Which I don't easily.
> 
> .. I can try adding extra method to target ops, and for remote target, 
> implement it by returning the value of 
>  
>    remote_protocol_Z[Z_PACKET_READ_WP].support
> 
> Should I do this?

Hmm, hmm.  For remote targets we want to conditionalize this on the
read watchpoint packet.  For native targets I was thinking we would
need an architecture (gdbarch) method, but now that we have target
vector inheritance going, we probably don't - a target method would
work OK too.  Or we can just do it this way, even better.

Could you give this patch a try?  It will need some tweaking to build
for any native target but i386 but i386 native and remote should work. 
And I've tested that it fixes the bug for i386 native.

If it works for you I'll finish the mechanical conversion.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.760
diff -u -p -r1.760 Makefile.in
--- Makefile.in	7 Nov 2005 15:27:06 -0000	1.760
+++ Makefile.in	14 Nov 2005 14:39:18 -0000
@@ -2090,7 +2090,7 @@ i386v4-nat.o: i386v4-nat.c $(defs_h) $(v
 	$(i386_tdep_h) $(i387_tdep_h) $(gregset_h)
 i386v-nat.o: i386v-nat.c $(defs_h) $(frame_h) $(inferior_h) $(language_h) \
 	$(gdbcore_h) $(gdb_stat_h) $(floatformat_h) $(target_h) \
-	$(i386_tdep_h)
+	$(i386_tdep_h) $(breakpoint_h)
 i387-tdep.o: i387-tdep.c $(defs_h) $(doublest_h) $(floatformat_h) $(frame_h) \
 	$(gdbcore_h) $(inferior_h) $(language_h) $(regcache_h) $(value_h) \
 	$(gdb_assert_h) $(gdb_string_h) $(i386_tdep_h) $(i387_tdep_h)
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.217
diff -u -p -r1.217 breakpoint.c
--- breakpoint.c	29 May 2005 03:13:17 -0000	1.217
+++ breakpoint.c	14 Nov 2005 14:39:19 -0000
@@ -992,7 +992,8 @@ insert_bp_location (struct bp_location *
 		      else if (bpt->owner->type == bp_access_watchpoint)
 			type = hw_access;
 
-		      val = target_insert_watchpoint (addr, len, type);
+		      val = target_insert_watchpoint (addr, len, &type);
+		      bpt->hw_type = type;
 		      if (val == -1)
 			{
 			  /* Don't exit the loop, try to insert
@@ -1511,17 +1512,12 @@ remove_breakpoint (struct bp_location *b
 		      && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
 		{
 		  CORE_ADDR addr;
-		  int len, type;
+		  int len;
 
 		  addr = VALUE_ADDRESS (v) + value_offset (v);
 		  len = TYPE_LENGTH (value_type (v));
-		  type   = hw_write;
-		  if (b->owner->type == bp_read_watchpoint)
-		    type = hw_read;
-		  else if (b->owner->type == bp_access_watchpoint)
-		    type = hw_access;
 
-		  val = target_remove_watchpoint (addr, len, type);
+		  val = target_remove_watchpoint (addr, len, b->hw_type);
 		  if (val == -1)
 		    b->inserted = 1;
 		  val = 0;
@@ -2793,7 +2789,8 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 		/* Stop.  */
 		break;
 	      case WP_VALUE_CHANGED:
-		if (b->type == bp_read_watchpoint)
+		if (b->type == bp_read_watchpoint
+		    && b->loc->hw_type == hw_access)
 		  {
 		    /* Don't stop: read watchpoints shouldn't fire if
 		       the value has changed.  This is for targets
@@ -4029,6 +4026,7 @@ allocate_bp_location (struct breakpoint 
     case bp_read_watchpoint:
     case bp_access_watchpoint:
       loc->loc_type = bp_loc_hardware_watchpoint;
+      loc->hw_type = hw_none;
       break;
     case bp_watchpoint:
     case bp_catch_fork:
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.35
diff -u -p -r1.35 breakpoint.h
--- breakpoint.h	26 May 2005 20:48:57 -0000	1.35
+++ breakpoint.h	14 Nov 2005 14:39:19 -0000
@@ -182,7 +182,8 @@ enum target_hw_bp_type
     hw_write   = 0, 		/* Common  HW watchpoint */
     hw_read    = 1, 		/* Read    HW watchpoint */
     hw_access  = 2, 		/* Access  HW watchpoint */
-    hw_execute = 3		/* Execute HW breakpoint */
+    hw_execute = 3,		/* Execute HW breakpoint */
+    hw_none			/* Sentinel value.  */
   };
 
 /* GDB maintains two types of information about each breakpoint (or
@@ -256,6 +257,12 @@ struct bp_location
      which to place the breakpoint in order to comply with a
      processor's architectual constraints.  */
   CORE_ADDR requested_address;
+
+  /* For hardware breakpoints, the type of hardware breakpoint currently
+     used to implement it.  For instance, a read watchpoint may be
+     implemented by an access (read/write) watchpoint.  This is only
+     valid while the breakpoint is inserted.  */
+  enum target_hw_bp_type hw_type;
 };
 
 /* This structure is a collection of function pointers that, if available,
Index: i386-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-nat.c,v
retrieving revision 1.12
diff -u -p -r1.12 i386-nat.c
--- i386-nat.c	10 Sep 2005 18:11:02 -0000	1.12
+++ i386-nat.c	14 Nov 2005 14:39:19 -0000
@@ -502,22 +502,26 @@ Invalid value %d of operation in i386_ha
    of the type TYPE.  Return 0 on success, -1 on failure.  */
 
 int
-i386_insert_watchpoint (CORE_ADDR addr, int len, int type)
+i386_insert_watchpoint (CORE_ADDR addr, int len, int *type)
 {
   int retval;
 
+  if (*type == hw_read)
+    /* The i386 does not support read watchpoints.  */
+    *type = hw_access;
+
   if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
-    retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type);
+    retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, *type);
   else
     {
-      unsigned len_rw = i386_length_and_rw_bits (len, type);
+      unsigned len_rw = i386_length_and_rw_bits (len, *type);
 
       retval = i386_insert_aligned_watchpoint (addr, len_rw);
     }
 
   if (maint_show_dr)
-    i386_show_dr ("insert_watchpoint", addr, len, type);
+    i386_show_dr ("insert_watchpoint", addr, len, *type);
 
   return retval;
 }
Index: i386v-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386v-nat.c,v
retrieving revision 1.15
diff -u -p -r1.15 i386v-nat.c
--- i386v-nat.c	7 Sep 2004 21:55:10 -0000	1.15
+++ i386v-nat.c	14 Nov 2005 14:39:19 -0000
@@ -34,6 +34,7 @@
 #include "inferior.h"
 #include "language.h"
 #include "gdbcore.h"
+#include "breakpoint.h"
 
 #include <sys/param.h>
 #include <sys/dir.h>
@@ -125,9 +126,13 @@ static int i386_insert_nonaligned_watchp
 /* Insert a watchpoint.  */
 
 int
-i386_insert_watchpoint (int pid, CORE_ADDR addr, int len, int rw)
+i386_insert_watchpoint (int pid, CORE_ADDR addr, int len, int *rw)
 {
-  return i386_insert_aligned_watchpoint (pid, addr, addr, len, rw);
+  /* The i386 does not support read watchpoints.  */
+  if (*rw == hw_read)
+    *rw = hw_access;
+
+  return i386_insert_aligned_watchpoint (pid, addr, addr, len, *rw);
 }
 
 static int
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.195
diff -u -p -r1.195 remote.c
--- remote.c	20 Jul 2005 02:56:43 -0000	1.195
+++ remote.c	14 Nov 2005 14:39:20 -0000
@@ -4609,12 +4609,21 @@ watchpoint_to_Z_packet (int type)
 }
 
 static int
-remote_insert_watchpoint (CORE_ADDR addr, int len, int type)
+remote_insert_watchpoint (CORE_ADDR addr, int len, int *type)
 {
   struct remote_state *rs = get_remote_state ();
   char *buf = alloca (rs->remote_packet_size);
   char *p;
-  enum Z_packet_type packet = watchpoint_to_Z_packet (type);
+  enum Z_packet_type packet = watchpoint_to_Z_packet (*type);
+
+  /* If this target does not support read watchpoints, try access watchpoints
+     instead.  */
+  if (remote_protocol_Z[packet].support == PACKET_DISABLE
+      && *type == hw_read)
+    {
+      *type = hw_access;
+      packet = watchpoint_to_Z_packet (*type);
+    }
 
   if (remote_protocol_Z[packet].support == PACKET_DISABLE)
     error (_("Can't set hardware watchpoints without the '%s' (%s) packet."),
@@ -4632,8 +4641,15 @@ remote_insert_watchpoint (CORE_ADDR addr
 
   switch (packet_ok (buf, &remote_protocol_Z[packet]))
     {
-    case PACKET_ERROR:
     case PACKET_UNKNOWN:
+      if (*type == hw_read)
+	{
+	  *type = hw_access;
+	  return remote_insert_watchpoint (addr, len, type);
+	}
+      else
+	return -1;
+    case PACKET_ERROR:
       return -1;
     case PACKET_OK:
       return 0;
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	14 Nov 2005 14:39:20 -0000
@@ -341,7 +341,7 @@ struct target_ops
     int (*to_insert_hw_breakpoint) (CORE_ADDR, gdb_byte *);
     int (*to_remove_hw_breakpoint) (CORE_ADDR, gdb_byte *);
     int (*to_remove_watchpoint) (CORE_ADDR, int, int);
-    int (*to_insert_watchpoint) (CORE_ADDR, int, int);
+    int (*to_insert_watchpoint) (CORE_ADDR, int, int *);
     int (*to_stopped_by_watchpoint) (void);
     int to_have_continuable_watchpoint;
     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
@@ -1036,7 +1036,7 @@ extern void (*deprecated_target_new_objf
 #endif
 
 
-/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.  TYPE is 0
+/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.  *TYPE is 0
    for write, 1 for read, and 2 for read/write accesses.  Returns 0 for
    success, non-zero for failure.  */
 
Index: config/i386/nm-i386.h
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/nm-i386.h,v
retrieving revision 1.7
diff -u -p -r1.7 nm-i386.h
--- config/i386/nm-i386.h	8 Oct 2004 17:30:48 -0000	1.7
+++ config/i386/nm-i386.h	14 Nov 2005 14:39:20 -0000
@@ -31,8 +31,8 @@ extern void i386_cleanup_dregs (void);
 
 /* Insert a watchpoint to watch a memory region which starts at
    address ADDR and whose length is LEN bytes.  Watch memory accesses
-   of the type TYPE.  Return 0 on success, -1 on failure.  */
-extern int i386_insert_watchpoint (CORE_ADDR addr, int len, int type);
+   of the type *TYPE.  Return 0 on success, -1 on failure.  */
+extern int i386_insert_watchpoint (CORE_ADDR addr, int len, int *type);
 
 /* Remove a watchpoint that watched the memory region which starts at
    address ADDR, whose length is LEN bytes, and for accesses of the

Re: read watchpoints ignored?

[Daniel Jacobowitz]

On Mon, Nov 14, 2005 at 06:38:20AM +0200, Eli Zaretskii wrote:
> We need to find a way to get GDB to check whether the watchpoint fired
> during such stepping.
> 
> > I'm talking about watchpoint_check.  If that had been called, we would
> > have updated the value stored in the watchpoint for "a", and at the
> > next read watchpoint, it would not appear to have changed.
> 
> I think we need call watchpoint_check only if
> target_stopped_data_address says the watchpoint fired.

I'm not sure if we can call watchpoint_check when not checking bpstats;
too much context.  In any case, for now we only need to fix one of the
two bugs; what do you think of the patch I just posted?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: read watchpoints ignored?

[Daniel Jacobowitz]

On Mon, Nov 14, 2005 at 09:41:18AM -0500, Daniel Jacobowitz wrote:
> Could you give this patch a try?  It will need some tweaking to build
> for any native target but i386 but i386 native and remote should work. 
> And I've tested that it fixes the bug for i386 native.

and

On Mon, Nov 14, 2005 at 09:42:44AM -0500, Daniel Jacobowitz wrote:
> I'm not sure if we can call watchpoint_check when not checking bpstats;
> too much context.  In any case, for now we only need to fix one of the
> two bugs; what do you think of the patch I just posted?

Sorry folks, too early on Monday morning, don't know what I was
thinking.  It does _not_ fix the bug for i386 native, it simply fails
to break the existing read watchpoint support.  We need to fix both
bugs.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: read watchpoints ignored?

[Eli Zaretskii]

> From: Vladimir Prus <ghost@cs.msu.su>
> Date: Mon, 14 Nov 2005 16:42:20 +0300
> Cc: Daniel Jacobowitz <drow@false.org>,
>  Eli Zaretskii <eliz@gnu.org>,
>  gdb@sources.redhat.com
> 
> This essentially disables read watchpoints. What's the point in first 
> implementing read watchpoints in a target, and then disabling them completely 
> in gdb?

I agree, this is not a good solution.

> I don't yet have specific *real-world* examples, but using "is the value 
> different from the value last time we stopped" as a heuristic to decide if we 
> hit write watchpoint or read watchpoint seems fragile.

It is a vast improvement on what we had before.

Re: read watchpoints ignored?

[Eli Zaretskii]

> Date: Mon, 14 Nov 2005 09:45:40 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> Sorry folks, too early on Monday morning, don't know what I was
> thinking.  It does _not_ fix the bug for i386 native, it simply fails
> to break the existing read watchpoint support.  We need to fix both
> bugs.

Yes, true.

Re: read watchpoints ignored?

[Eli Zaretskii]

> Date: Mon, 14 Nov 2005 09:42:44 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb@sources.redhat.com, Vladimir Prus <ghost@cs.msu.su>
> 
> > I think we need call watchpoint_check only if
> > target_stopped_data_address says the watchpoint fired.
> 
> I'm not sure if we can call watchpoint_check when not checking bpstats;
> too much context.

I will look at this some more when I have more time.