Don't delete local watchpoints just because a different thread stopped.

Don't delete local watchpoints just because a different thread stopped.

[Pedro Alves]

(I've actually written this with non-stop in mind, but it applies
equally to all-stop.)

Currently, this:

 "Hardware watchpoint foo deleted because the program has left the block"
 " in which its expression is valid".

triggers when a thread other than the thread the local watchpoint
had been set through, stops.  IMO, that's a bug.  The thread the
local watchpoint was set through could still be running in a
block where the watchpoint expression is valid.

When the local watchpoint is set, GDB stores the
selected frame id in the watchpoint (breakpoint) structure, but
the frame id alone is insuficient.  When a different thread
stops, GDB considers the program has left the watchpoint block,
because it can't find the watchpoint's frame id in this other
thread's frame chain ...

One could even come up with a test case where the current
frame id on this other thread does happen to be in the
frame chain.  Shouldn't be hard to trigger if the threads
are running the same function --- GDB can then mistakenly
try to extract a new current watchpoint value on the wrong
thread context.


Here's a patch to address this, and a new testcase.


2009-11-19  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.h (struct breakpoint) <watchpoint_thread>: New field.
	* breakpoint.c (watchpoint_thread_match): New.
	(update_watchpoint): Skip if the local watchpoint's thread doesn't
	match the current thread.
	(watchpoint_check): Ditto.
	(watch_command_1): Set the watchpoint's watchpoint_thread field.

2009-11-19  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* local-watch-wrong-thread.c, local-watch-wrong-thread.exp: New
	files.


WDYT?

Tested on x86_64-linux, native and gdbserver.


-- 
Pedro Alves

2009-11-19  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.h (struct breakpoint) <watchpoint_thread>: New field.
	* breakpoint.c (watchpoint_thread_match): New.
	(update_watchpoint): Skip if the local watchpoint's thread doesn't
	match the current thread.
	(watchpoint_check): Ditto.
	(watch_command_1): Set the watchpoint's watchpoint_thread field.

---
 gdb/breakpoint.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 gdb/breakpoint.h |    4 ++++
 2 files changed, 48 insertions(+), 2 deletions(-)

Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h	2009-11-19 01:10:05.000000000 +0000
+++ src/gdb/breakpoint.h	2009-11-19 01:10:16.000000000 +0000
@@ -457,6 +457,10 @@ struct breakpoint
        should be evaluated on the outermost frame.  */
     struct frame_id watchpoint_frame;
 
+    /* Holds the thread which identifies the frame this watchpoint
+       should be considered in scope for, or -1 if don't care.  */
+    int watchpoint_thread;
+
     /* For hardware watchpoints, the triggered status according to the
        hardware.  */
     enum watchpoint_triggered watchpoint_triggered;
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2009-11-19 01:10:05.000000000 +0000
+++ src/gdb/breakpoint.c	2009-11-19 01:11:21.000000000 +0000
@@ -985,6 +985,30 @@ fetch_watchpoint_value (struct expressio
     }
 }
 
+/* Assuming that B is a watchpoint: if B is a watchpoint on a local
+   expression, returns true if the current thread is the same as the
+   watchpoint's thread.  Always returns true for non-local
+   watchpoints.  */
+
+static int
+watchpoint_thread_match (struct breakpoint *b)
+{
+  if (b->watchpoint_thread != -1)
+    {
+      int thread;
+
+      if (!ptid_equal (inferior_ptid, null_ptid))
+	thread = inferior_thread ()->num;
+      else
+	thread = -1;
+
+      if (thread != b->watchpoint_thread)
+	return 0;
+    }
+
+  return 1;
+}
+
 /* Assuming that B is a watchpoint:
    - Reparse watchpoint expression, if REPARSE is non-zero
    - Evaluate expression and store the result in B->val
@@ -1004,6 +1028,12 @@ update_watchpoint (struct breakpoint *b,
   bpstat bs;
   struct program_space *frame_pspace;
 
+  /* If this is a local watchpoint, we only want to check if the
+     watchpoint frame is in scope if the current thread is the thread
+     that was used to create the watchpoint.  */
+  if (!watchpoint_thread_match (b))
+    return;
+
   /* We don't free locations.  They are stored in bp_location array and
      update_global_locations will eventually delete them and remove
      breakpoints if needed.  */
@@ -3085,6 +3115,12 @@ watchpoint_check (void *p)
 
   b = bs->breakpoint_at->owner;
 
+  /* If this is a local watchpoint, we only want to check if the
+     watchpoint frame is in scope if the current thread is the thread
+     that was used to create the watchpoint.  */
+  if (!watchpoint_thread_match (b))
+    return WP_VALUE_NOT_CHANGED;
+
   if (b->exp_valid_block == NULL)
     within_current_scope = 1;
   else
@@ -7151,9 +7187,15 @@ watch_command_1 (char *arg, int accessfl
     b->cond_string = 0;
 
   if (frame)
-    b->watchpoint_frame = get_frame_id (frame);
+    {
+      b->watchpoint_frame = get_frame_id (frame);
+      b->watchpoint_thread = inferior_thread ()->num;
+    }
   else
-    b->watchpoint_frame = null_frame_id;
+    {
+      b->watchpoint_frame = null_frame_id;
+      b->watchpoint_thread = -1;
+    }
 
   if (scope_breakpoint != NULL)
     {

===================================================================

2009-11-19  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* local-watch-wrong-thread.c, local-watch-wrong-thread.exp: New
	files.

---
 gdb/testsuite/gdb.threads/local-watch-wrong-thread.c   |   80 +++++++++++
 gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp |  118 +++++++++++++++++
 2 files changed, 198 insertions(+)

Index: src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.c	2009-11-19 01:10:15.000000000 +0000
@@ -0,0 +1,80 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2002, 2003, 2004, 2007, 2008, 2009 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+*/
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+unsigned int args[2];
+int trigger = 0;
+
+void *
+thread_function0 (void *arg)
+{
+  int my_number =  (long) arg;
+  volatile int *myp = (volatile int *) &args[my_number];
+
+  while (*myp > 0)
+    {
+      (*myp) ++;
+      usleep (1);  /* Loop increment 1.  */
+    }
+
+  return NULL;
+}
+
+void *
+thread_function1 (void *arg)
+{
+  int my_number =  (long) arg;
+
+  volatile int *myp = (volatile int *) &args[my_number];
+
+  while (*myp > 0)
+    {
+      (*myp) ++;
+      usleep (1);  /* Loop increment 2.  */
+    }
+
+  return NULL;
+}
+
+int
+main ()
+{
+  int res;
+  pthread_t threads[2];
+  void *thread_result;
+  long i = 0;
+
+  args[i] = 1; /* Init value.  */
+  res = pthread_create (&threads[i], NULL,
+			thread_function0,
+			(void *) i);
+
+  i++;
+  args[i] = 1; /* Init value.  */
+  res = pthread_create(&threads[i], NULL,
+		       thread_function1,
+		       (void *) i);
+
+  pthread_join (threads[0], &thread_result);
+  pthread_join (threads[1], &thread_result);
+  exit(EXIT_SUCCESS);
+}
Index: src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp	2009-11-19 02:02:12.000000000 +0000
@@ -0,0 +1,118 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2004, 2007, 2008, 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that GDB can support multiple watchpoints across threads.
+
+# This test verifies that a local watchpoint isn't deleted when a
+# thread other than the thread the local watchpoint was set in stops
+# for a breakpoint.
+if [target_info exists gdb,no_hardware_watchpoints] {
+    return 0;
+}
+
+set testfile "local-watch-wrong-thread"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if {[gdb_compile_pthreads \
+	 "${srcdir}/${subdir}/${srcfile}" \
+	 "${binfile}" executable {debug} ] != "" } {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+gdb_test "set can-use-hw-watchpoints 1" "" ""
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return
+}
+
+set inc_line_1 [gdb_get_line_number "Loop increment 1"]
+set inc_line_2 [gdb_get_line_number "Loop increment 2"]
+
+# Run to the loop within thread_function0, so we can set our local
+# watchpoint.
+gdb_test "break ${srcfile}:${inc_line_1}" \
+    "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
+    "breakpoint on thread_function0"
+
+gdb_test "continue" \
+    ".*Breakpoint 2.*thread_function0.*" \
+    "continue to thread_function0"
+
+delete_breakpoints
+
+# Set the local watchpoint, and confirm that it traps as expected.
+gdb_test "watch *myp" \
+    "Hardware watchpoint 3\: \\*myp.*" \
+    "set local watchpoint on *myp"
+
+gdb_test "continue" \
+    "Hardware watchpoint.*Old value.*New value.*thread_function0.*" \
+    "local watchpoint triggers"
+
+delete_breakpoints
+
+# Recreate the watchpoint, but this time with a condition we know
+# won't trigger.  This is so the watchpoint is inserted, and the
+# target reports events, but GDB doesn't stop for them.  We want to
+# hit the breakpoints on the other thread, and make sure this
+# watchpoint isn't deleted then.
+gdb_test "watch *myp if trigger != 0" \
+    "Hardware watchpoint 4\: \\*myp.*" \
+    "set local watchpoint on *myp, with false conditional"
+
+# Run to a breakpoint on a different thread.  The previous local
+# watchpoint should not be deleted with the standard 'the program has
+# left the block in which its expression is valid', because the
+# thread_function0 thread should still be running in the loop.
+gdb_test "break ${srcfile}:${inc_line_2}" \
+    "Breakpoint 5 at .*: file .*${srcfile}, line .*" \
+    "breakpoint on the other thread"
+
+gdb_test "continue" \
+    "Breakpoint 5, thread_function1.*" \
+    "the other thread stopped on breakpoint"
+
+# Delete the new breakpoint, we don't need it anymore.
+gdb_test "delete 5" "" ""
+
+# Check if the local watchpoint hasn't been deleted (is still listed).
+# This is simpler to check than expecting 'the program has left ...',
+# and, is immune to string changes in that warning.
+gdb_test "info breakpoints" \
+    ".*4.*hw watchpoint.*keep.*y.*\\*myp.*" \
+    "local watchpoint is still in breakpoint list"
+
+# Make the watchpoint condition eval true.
+gdb_test "set trigger=1" "" "let local watchpoint trigger"
+
+gdb_test "continue" \
+    "Hardware watchpoint.*Old value.*New value.*thread_function0.*" \
+    "local watchpoint still triggers"
+
+# Confirm that the local watchpoint is indeed deleted if
+# thread_function0 returns.
+gdb_test "set *myp=0" "" "let thread_function0 return"
+
+gdb_test "continue" \
+    ".*Watchpoint.*deleted.*" \
+    "local watchpoint automatically deleted"

Re: Don't delete local watchpoints just because a different thread stopped.

[Daniel Jacobowitz]

On Thu, Nov 19, 2009 at 02:07:01AM +0000, Pedro Alves wrote:
> One could even come up with a test case where the current
> frame id on this other thread does happen to be in the
> frame chain.  Shouldn't be hard to trigger if the threads
> are running the same function --- GDB can then mistakenly
> try to extract a new current watchpoint value on the wrong
> thread context.

I hope your threads have different stacks :-)  So I don't think this
case happens.

> Index: src/gdb/breakpoint.h
> ===================================================================
> --- src.orig/gdb/breakpoint.h	2009-11-19 01:10:05.000000000 +0000
> +++ src/gdb/breakpoint.h	2009-11-19 01:10:16.000000000 +0000
> @@ -457,6 +457,10 @@ struct breakpoint
>         should be evaluated on the outermost frame.  */
>      struct frame_id watchpoint_frame;
>  
> +    /* Holds the thread which identifies the frame this watchpoint
> +       should be considered in scope for, or -1 if don't care.  */
> +    int watchpoint_thread;
> +
>      /* For hardware watchpoints, the triggered status according to the
>         hardware.  */
>      enum watchpoint_triggered watchpoint_triggered;

Why not just use a ptid?  If the answer has to do with ptid reuse,
then we ought to delete the watchpoint before that comes up - anyway,
at least deserves a comment of what units this is in.

> @@ -1004,6 +1028,12 @@ update_watchpoint (struct breakpoint *b,
>    bpstat bs;
>    struct program_space *frame_pspace;
>  
> +  /* If this is a local watchpoint, we only want to check if the
> +     watchpoint frame is in scope if the current thread is the thread
> +     that was used to create the watchpoint.  */
> +  if (!watchpoint_thread_match (b))
> +    return;
> +
>    /* We don't free locations.  They are stored in bp_location array and
>       update_global_locations will eventually delete them and remove
>       breakpoints if needed.  */

For all-stop, do we want to check whenever the watchpoint's thread is
stopped?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Don't delete local watchpoints just because a different thread stopped.

[Pedro Alves]

On Thursday 19 November 2009 17:45:39, Daniel Jacobowitz wrote:
> On Thu, Nov 19, 2009 at 02:07:01AM +0000, Pedro Alves wrote:
> > One could even come up with a test case where the current
> > frame id on this other thread does happen to be in the
> > frame chain.  Shouldn't be hard to trigger if the threads
> > are running the same function --- GDB can then mistakenly
> > try to extract a new current watchpoint value on the wrong
> > thread context.
> 
> I hope your threads have different stacks :-)  So I don't think this
> case happens.

Urgh, talk about braino!  :-P  :-)

> 
> > Index: src/gdb/breakpoint.h
> > ===================================================================
> > --- src.orig/gdb/breakpoint.h	2009-11-19 01:10:05.000000000 +0000
> > +++ src/gdb/breakpoint.h	2009-11-19 01:10:16.000000000 +0000
> > @@ -457,6 +457,10 @@ struct breakpoint
> >         should be evaluated on the outermost frame.  */
> >      struct frame_id watchpoint_frame;
> >  
> > +    /* Holds the thread which identifies the frame this watchpoint
> > +       should be considered in scope for, or -1 if don't care.  */
> > +    int watchpoint_thread;
> > +
> >      /* For hardware watchpoints, the triggered status according to the
> >         hardware.  */
> >      enum watchpoint_triggered watchpoint_triggered;
> 
> Why not just use a ptid?  If the answer has to do with ptid reuse,
> then we ought to delete the watchpoint before that comes up - anyway,
> at least deserves a comment of what units this is in.

No reason.  I started out with a hack using the bkpt->thread field, then
decided to keep user visible things distinct from internal things, and
just copy/pasted the field --- the int'ness sticked.  Indeed a ptid
makes things simpler.  Thanks.

> > @@ -1004,6 +1028,12 @@ update_watchpoint (struct breakpoint *b,
> >    bpstat bs;
> >    struct program_space *frame_pspace;
> >  
> > +  /* If this is a local watchpoint, we only want to check if the
> > +     watchpoint frame is in scope if the current thread is the thread
> > +     that was used to create the watchpoint.  */
> > +  if (!watchpoint_thread_match (b))
> > +    return;
> > +
> >    /* We don't free locations.  They are stored in bp_location array and
> >       update_global_locations will eventually delete them and remove
> >       breakpoints if needed.  */
> 
> For all-stop, do we want to check whenever the watchpoint's thread is
> stopped?

Yep (non-stop), good catch.  Usually we'd get here with a stopped
thread, but there's always say,
'(gdb) file' -> breakpoint_re_set -> update_watchpoint.
I'm actually thinking that path may need further
tweaking if the current thread is running, but I'll defer
putting more brain cycles on that until more non-stop watchpoint
support is in place.


Here's the updated patch.  New test unchanged.  Tested on x86_64-linux.

2009-11-19  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.h (struct breakpoint) <watchpoint_thread>: New field.
	* breakpoint.c (watchpoint_in_thread_scope): New.
	(update_watchpoint): Skip if the local watchpoint's thread doesn't
	match the current thread, or if the current thread is running.
	(watchpoint_check): Ditto.
	(watch_command_1): Set the watchpoint's watchpoint_thread field.

2009-11-19  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* local-watch-wrong-thread.c, local-watch-wrong-thread.exp: New
	files.


More comments?

-- 
Pedro Alves

2009-11-19  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.h (struct breakpoint) <watchpoint_thread>: New field.
	* breakpoint.c (watchpoint_in_thread_scope): New.
	(update_watchpoint): Skip if the local watchpoint's thread doesn't
	match the current thread, or if the current thread is running.
	(watchpoint_check): Ditto.
	(watch_command_1): Set the watchpoint's watchpoint_thread field.

---
 gdb/breakpoint.c |   39 +++++++++++++++++++++++++++++++++++++--
 gdb/breakpoint.h |    5 +++++
 2 files changed, 42 insertions(+), 2 deletions(-)

Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h	2009-11-20 00:07:15.000000000 +0000
+++ src/gdb/breakpoint.h	2009-11-20 00:15:39.000000000 +0000
@@ -457,6 +457,11 @@ struct breakpoint
        should be evaluated on the outermost frame.  */
     struct frame_id watchpoint_frame;
 
+    /* Holds the thread which identifies the frame this watchpoint
+       should be considered in scope for, or `null_ptid' if the
+       watchpoint should be evaluated in all threads.  */
+    ptid_t watchpoint_thread;
+
     /* For hardware watchpoints, the triggered status according to the
        hardware.  */
     enum watchpoint_triggered watchpoint_triggered;
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2009-11-20 00:07:15.000000000 +0000
+++ src/gdb/breakpoint.c	2009-11-20 00:26:07.000000000 +0000
@@ -985,6 +985,23 @@ fetch_watchpoint_value (struct expressio
     }
 }
 
+/* Assuming that B is a watchpoint: returns true if the current thread
+   and its running state are safe to evaluate or update watchpoint B.
+   Watchpoints on local expressions need to evaluated in the context
+   of the thread that was current when the watchpoint was created,
+   and, that thread needs to be stopped to be able to select the
+   correct frame context.  Watchpoints on global expressions can be
+   evaluated on any thread, and in any state.  It is presently left to
+   the target allowing memory accesses when threads are running.  */
+
+static int
+watchpoint_in_thread_scope (struct breakpoint *b)
+{
+  return (ptid_equal (b->watchpoint_thread, null_ptid)
+	  || (ptid_equal (inferior_ptid, b->watchpoint_thread)
+	      && !is_executing (inferior_ptid)));
+}
+
 /* Assuming that B is a watchpoint:
    - Reparse watchpoint expression, if REPARSE is non-zero
    - Evaluate expression and store the result in B->val
@@ -1004,6 +1021,12 @@ update_watchpoint (struct breakpoint *b,
   bpstat bs;
   struct program_space *frame_pspace;
 
+  /* If this is a local watchpoint, we only want to check if the
+     watchpoint frame is in scope if the current thread is the thread
+     that was used to create the watchpoint.  */
+  if (!watchpoint_in_thread_scope (b))
+    return;
+
   /* We don't free locations.  They are stored in bp_location array and
      update_global_locations will eventually delete them and remove
      breakpoints if needed.  */
@@ -3085,6 +3108,12 @@ watchpoint_check (void *p)
 
   b = bs->breakpoint_at->owner;
 
+  /* If this is a local watchpoint, we only want to check if the
+     watchpoint frame is in scope if the current thread is the thread
+     that was used to create the watchpoint.  */
+  if (!watchpoint_in_thread_scope (b))
+    return WP_VALUE_NOT_CHANGED;
+
   if (b->exp_valid_block == NULL)
     within_current_scope = 1;
   else
@@ -7151,9 +7180,15 @@ watch_command_1 (char *arg, int accessfl
     b->cond_string = 0;
 
   if (frame)
-    b->watchpoint_frame = get_frame_id (frame);
+    {
+      b->watchpoint_frame = get_frame_id (frame);
+      b->watchpoint_thread = inferior_ptid;
+    }
   else
-    b->watchpoint_frame = null_frame_id;
+    {
+      b->watchpoint_frame = null_frame_id;
+      b->watchpoint_thread = null_ptid;
+    }
 
   if (scope_breakpoint != NULL)
     {

===================================================================

2009-11-19  Pedro Alves  <pedro@codesourcery.com>

	gdb/testsuite/
	* local-watch-wrong-thread.c, local-watch-wrong-thread.exp: New
	files.

---
 gdb/testsuite/gdb.threads/local-watch-wrong-thread.c   |   80 +++++++++++
 gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp |  118 +++++++++++++++++
 2 files changed, 198 insertions(+)

Index: src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.c	2009-11-19 01:10:15.000000000 +0000
@@ -0,0 +1,80 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2002, 2003, 2004, 2007, 2008, 2009 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+*/
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+
+unsigned int args[2];
+int trigger = 0;
+
+void *
+thread_function0 (void *arg)
+{
+  int my_number =  (long) arg;
+  volatile int *myp = (volatile int *) &args[my_number];
+
+  while (*myp > 0)
+    {
+      (*myp) ++;
+      usleep (1);  /* Loop increment 1.  */
+    }
+
+  return NULL;
+}
+
+void *
+thread_function1 (void *arg)
+{
+  int my_number =  (long) arg;
+
+  volatile int *myp = (volatile int *) &args[my_number];
+
+  while (*myp > 0)
+    {
+      (*myp) ++;
+      usleep (1);  /* Loop increment 2.  */
+    }
+
+  return NULL;
+}
+
+int
+main ()
+{
+  int res;
+  pthread_t threads[2];
+  void *thread_result;
+  long i = 0;
+
+  args[i] = 1; /* Init value.  */
+  res = pthread_create (&threads[i], NULL,
+			thread_function0,
+			(void *) i);
+
+  i++;
+  args[i] = 1; /* Init value.  */
+  res = pthread_create(&threads[i], NULL,
+		       thread_function1,
+		       (void *) i);
+
+  pthread_join (threads[0], &thread_result);
+  pthread_join (threads[1], &thread_result);
+  exit(EXIT_SUCCESS);
+}
Index: src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.threads/local-watch-wrong-thread.exp	2009-11-19 02:02:12.000000000 +0000
@@ -0,0 +1,118 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2004, 2007, 2008, 2009 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that GDB can support multiple watchpoints across threads.
+
+# This test verifies that a local watchpoint isn't deleted when a
+# thread other than the thread the local watchpoint was set in stops
+# for a breakpoint.
+if [target_info exists gdb,no_hardware_watchpoints] {
+    return 0;
+}
+
+set testfile "local-watch-wrong-thread"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if {[gdb_compile_pthreads \
+	 "${srcdir}/${subdir}/${srcfile}" \
+	 "${binfile}" executable {debug} ] != "" } {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+gdb_test "set can-use-hw-watchpoints 1" "" ""
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return
+}
+
+set inc_line_1 [gdb_get_line_number "Loop increment 1"]
+set inc_line_2 [gdb_get_line_number "Loop increment 2"]
+
+# Run to the loop within thread_function0, so we can set our local
+# watchpoint.
+gdb_test "break ${srcfile}:${inc_line_1}" \
+    "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
+    "breakpoint on thread_function0"
+
+gdb_test "continue" \
+    ".*Breakpoint 2.*thread_function0.*" \
+    "continue to thread_function0"
+
+delete_breakpoints
+
+# Set the local watchpoint, and confirm that it traps as expected.
+gdb_test "watch *myp" \
+    "Hardware watchpoint 3\: \\*myp.*" \
+    "set local watchpoint on *myp"
+
+gdb_test "continue" \
+    "Hardware watchpoint.*Old value.*New value.*thread_function0.*" \
+    "local watchpoint triggers"
+
+delete_breakpoints
+
+# Recreate the watchpoint, but this time with a condition we know
+# won't trigger.  This is so the watchpoint is inserted, and the
+# target reports events, but GDB doesn't stop for them.  We want to
+# hit the breakpoints on the other thread, and make sure this
+# watchpoint isn't deleted then.
+gdb_test "watch *myp if trigger != 0" \
+    "Hardware watchpoint 4\: \\*myp.*" \
+    "set local watchpoint on *myp, with false conditional"
+
+# Run to a breakpoint on a different thread.  The previous local
+# watchpoint should not be deleted with the standard 'the program has
+# left the block in which its expression is valid', because the
+# thread_function0 thread should still be running in the loop.
+gdb_test "break ${srcfile}:${inc_line_2}" \
+    "Breakpoint 5 at .*: file .*${srcfile}, line .*" \
+    "breakpoint on the other thread"
+
+gdb_test "continue" \
+    "Breakpoint 5, thread_function1.*" \
+    "the other thread stopped on breakpoint"
+
+# Delete the new breakpoint, we don't need it anymore.
+gdb_test "delete 5" "" ""
+
+# Check if the local watchpoint hasn't been deleted (is still listed).
+# This is simpler to check than expecting 'the program has left ...',
+# and, is immune to string changes in that warning.
+gdb_test "info breakpoints" \
+    ".*4.*hw watchpoint.*keep.*y.*\\*myp.*" \
+    "local watchpoint is still in breakpoint list"
+
+# Make the watchpoint condition eval true.
+gdb_test "set trigger=1" "" "let local watchpoint trigger"
+
+gdb_test "continue" \
+    "Hardware watchpoint.*Old value.*New value.*thread_function0.*" \
+    "local watchpoint still triggers"
+
+# Confirm that the local watchpoint is indeed deleted if
+# thread_function0 returns.
+gdb_test "set *myp=0" "" "let thread_function0 return"
+
+gdb_test "continue" \
+    ".*Watchpoint.*deleted.*" \
+    "local watchpoint automatically deleted"

Re: Don't delete local watchpoints just because a different thread stopped.

[Daniel Jacobowitz]

On Fri, Nov 20, 2009 at 12:30:50AM +0000, Pedro Alves wrote:
> On Thursday 19 November 2009 17:45:39, Daniel Jacobowitz wrote:
> > For all-stop, do we want to check whenever the watchpoint's thread is
> > stopped?
> 
> Yep (non-stop), good catch.  Usually we'd get here with a stopped
> thread, but there's always say,

No, I really meant all-stop.

Suppose one thread hits a breakpoint, and another thread hits a
watchpoint while we're trying to stop it.  Is there any reason not to
temporarily switch threads to check?  Or any reason to, for that
matter - I'm not sure.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Don't delete local watchpoints just because a different thread stopped.

[Pedro Alves]

On Friday 20 November 2009 01:13:06, Daniel Jacobowitz wrote:
> watchpoint while we're trying to stop it.  Is there any reason not to
> temporarily switch threads to check?  Or any reason to, for that
> matter - I'm not sure.

At least on linux, the other watchpoint hit will be left
pending, and so will be reported on the next resume,
so it doesn't seem worth it to bother much with that.

-- 
Pedro Alves

Re: Don't delete local watchpoints just because a different thread stopped.

[Daniel Jacobowitz]

On Fri, Nov 20, 2009 at 01:32:35AM +0000, Pedro Alves wrote:
> On Friday 20 November 2009 01:13:06, Daniel Jacobowitz wrote:
> > watchpoint while we're trying to stop it.  Is there any reason not to
> > temporarily switch threads to check?  Or any reason to, for that
> > matter - I'm not sure.
> 
> At least on linux, the other watchpoint hit will be left
> pending, and so will be reported on the next resume,
> so it doesn't seem worth it to bother much with that.

Looks fine to me then.

I think you get any credit that needs getting for the non-stop change;
I had no idea we could get there while the current thread was running.
Seems strange.

-- 
Daniel Jacobowitz
CodeSourcery