RE: Watchpoint conditions broken?

[Vladimir Prus <ghost@cs.msu.su>]

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