Watchpoint conditions broken?

Watchpoint conditions broken?

[Marc Khouzam]

Hi,

when trying out a new version of GDB, we run a set of DSF JUnit tests to make sure
DSF interfaces with the new GDB properly.  Those tests are pretty detailed in their
attempt to exercise gdb functionality.

When running those tests with gdb 6.8, I found a failure with watchpoint conditions
which use to work in gdb 6.7

I tried to investigate, but I couldn't figure out where the watchpoint condition
was actually checked after the watchpoint was hit...  Or are hardware watchpoints
not supposed to be triggered at all, if the condition is not satisfied?

Below is a very small tests that shows the problem.
This is not normal, right?

Thanks

> gdb a.out
Wed Apr  9 16:20:30 EDT 2008
GNU gdb 6.8
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu"...
(gdb) l 1
1       int j;
2
3       int foo(int i)
4       {
5           j=i;
6           if (j==1) 
7               j=2;
8           else
9             j=3;
10          return j;
(gdb) 
11      }
12
13      int main() {
14          foo(1);
15          foo(2);
16      }
(gdb) b main
Breakpoint 1 at 0x8048447: file watch.cc, line 14.
(gdb) r
Starting program: /local/home/lmckhou/testing/a.out 

Breakpoint 1, main () at watch.cc:14
14          foo(1);
(gdb) watch j if j==3
Hardware watchpoint 2: j
(gdb) c
Continuing.
Hardware watchpoint 2: j

Old value = 0
New value = 1          <<<<<<<<<<<<<<< why did it stop here?
foo (i=1) at watch.cc:6
6           if (j==1) 
(gdb) c
Continuing.
Hardware watchpoint 2: j

Old value = 1
New value = 2
0x08048423 in foo (i=1) at watch.cc:7
7               j=2;
(gdb) p j
$1 = 2

(gdb) info b
Num     Type           Disp Enb Address    What
1       breakpoint     keep y   0x08048447 in main at watch.cc:14
        breakpoint already hit 1 time
2       hw watchpoint  keep y              j
        stop only if j==3
        breakpoint already hit 2 times
== 
Marc Khouzam

Re: Watchpoint conditions broken?

[Eli Zaretskii]

> Date: Wed, 9 Apr 2008 16:29:07 -0400
> From: "Marc Khouzam" <marc.khouzam@ericsson.com>
> 
> I tried to investigate, but I couldn't figure out where the watchpoint condition
> was actually checked after the watchpoint was hit...  Or are hardware watchpoints
> not supposed to be triggered at all, if the condition is not satisfied?

Hardware watchpoints _are_ supposed to trigger, but once they do, GDB
should see that the condition is false, and not announce the
watchpoint.

> Below is a very small tests that shows the problem.
> This is not normal, right?

No, it's a bug.

RE: Watchpoint conditions broken?

[Marc Khouzam]

> Hardware watchpoints _are_ supposed to trigger, but once they do, GDB
> should see that the condition is false, and not announce the
> watchpoint.

That is what I needed to know.  Thanks.

I believe the problem is in update_watchpoint().  
The below shows a fix that worked for me.
Funny thing is that this fix is the original way the code
was posted, as part of the patch about multiple locations 
for hardware breakpoints.

http://sourceware.org/ml/gdb-patches/2007-12/msg00008.html

Does it make sense?

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.307
diff -u -r1.307 breakpoint.c
--- gdb/breakpoint.c    14 Mar 2008 18:57:43 -0000      1.307
+++ gdb/breakpoint.c    10 Apr 2008 02:57:41 -0000
@@ -986,7 +986,7 @@
            value_free (v);
        }
 
-      if (reparse && b->cond_string != NULL)
+      if (b->cond_string != NULL)
        {
          char *s = b->cond_string;
          if (b->loc->cond)

==
Marc Khouzam

RE: Watchpoint conditions broken?

[Vladimir Prus]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]

Marc Khouzam wrote:

> 
>> Hardware watchpoints _are_ supposed to trigger, but once they do, GDB
>> should see that the condition is false, and not announce the
>> watchpoint.
> 
> That is what I needed to know.  Thanks.
> 
> I believe the problem is in update_watchpoint().
> The below shows a fix that worked for me.
> Funny thing is that this fix is the original way the code
> was posted, as part of the patch about multiple locations
> for hardware breakpoints.
> 
> http://sourceware.org/ml/gdb-patches/2007-12/msg00008.html
> 
> Does it make sense?
> 
> Index: gdb/breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.307
> diff -u -r1.307 breakpoint.c
> --- gdb/breakpoint.c    14 Mar 2008 18:57:43 -0000      1.307
> +++ gdb/breakpoint.c    10 Apr 2008 02:57:41 -0000
> @@ -986,7 +986,7 @@
>             value_free (v);
>         }
>  
> -      if (reparse && b->cond_string != NULL)
> +      if (b->cond_string != NULL)
>         {
>           char *s = b->cond_string;
>           if (b->loc->cond)

I think this patch is right. What happens if that update_watchpoint regenerates
the list of breakpoint locations. The condition is originally stored in the
first location, but first call to update_watchpoint creates new bp_location
instance, and does not have 'cond' field set to anything. To fix this,
we need to always reparse cond.

Alternatively, we can just store the current condition, and put it back. That would
be slightly more complex, and I'm not sure it will be much better.

I'm attaching a testcase for this problem. OK when Marc's code fix is in?

- Volodya


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: testsuite.diff --]
[-- Type: text/x-diff; name="testsuite.diff", Size: 1245 bytes --]

diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index bba97fa..f254773 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -120,6 +120,19 @@ func4 ()
   buf[0] = 7;
 }
 
+int global = 0;
+
+void test_watchpoint_condition ()
+{
+  global = 1;
+  global = 2;
+  global = 3;
+  global = 4;
+  global = 5;
+  global = 6;
+  global = 7;
+}
+
 int main ()
 {
 #ifdef usestubs
@@ -197,5 +210,7 @@ int main ()
 
   func4 ();
 
+  test_watchpoint_condition ();
+
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 3c20d10..7a4e567 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -833,6 +833,12 @@ if [initialize] then {
     }
 
     test_watchpoint_and_breakpoint
+
+    gdb_test "set can-use-hw-watchpoints 1\n" "" "enable hardware watchpoints again"
+    runto test_watchpoint_condition
+    gdb_test "watch global if global%3 == 0" ".Hardware watchpoint.*" "watchpoint on global"
+    gdb_test "continue" ".*New value = 3.*" "continue until global is 3"
+    gdb_test "continue" ".*New value = 6.*" "continue until global is 6"
 }
 
 # Restore old timeout

Re: Watchpoint conditions broken?

[Daniel Jacobowitz]

On Thu, Apr 10, 2008 at 01:15:34PM +0400, Vladimir Prus wrote:
> I'm attaching a testcase for this problem. OK when Marc's code fix is in?

Sorry, missed this since it wasn't on gdb-patches.  The only problem
is this bit:

> +    gdb_test "set can-use-hw-watchpoints 1\n" "" "enable hardware watchpoints again"

See the condition in proc initialize about when we can use hw
watchpoints in the testsuite.

-- 
Daniel Jacobowitz
CodeSourcery