FYI: add some breakpoint setter methods

FYI: add some breakpoint setter methods

[Tom Tromey]

I plan to check this in.

I noticed that modifications to breakpoints via Python do not cause the
breakpoint-modified observer to fire.  This is important to make the
(future) MI async notifications work correctly.

This patch fixes the problem by introducing some setter methods, then
changing Python to use these methods.  I looked for other places that
set these fields, but didn't find any.

I removed make_breakpoint_silent in favor of the new setter, since
having multiple setters seemed strange.

I opted to have the observers only fire if the field value actually
changed.  This seemed best to me.

As an aside, it seems to me that the breakpoint observers should
probably take a 'struct breakpoint *' as an argument, rather than a
breakpoint number.  This would be more efficient: I think most users
will want to have access to the actual breakpoint anyhow, and this would
avoid a search through all breakpoints for the number.  Perhaps I will
implement this.

Bootstrapped and regtested on x86-64.

Tom

2011-01-27  Tom Tromey  <tromey@redhat.com>

	* infcmd.c (finish_backward): Use breakpoint_set_silent.
	* python/py-breakpoint.c (bppy_set_silent): Use
	breakpoint_set_silent.
	(bppy_set_thread): Use breakpoint_set_thread.
	(bppy_set_task): Use breakpoint_set_task.
	* breakpoint.h (breakpoint_set_silent, breakpoint_set_thread)
	(breakpoint_set_task): Declare.
	(make_breakpoint_silent): Remove.
	* breakpoint.c (breakpoint_set_silent): New function.
	(breakpoint_set_thread): Likewise.
	(breakpoint_set_task): Likewise.
	(make_breakpoint_silent): Remove.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f82607b..027491a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -940,6 +940,43 @@ breakpoint_set_commands (struct breakpoint *b,
   observer_notify_breakpoint_modified (b->number);
 }
 
+/* Set the internal `silent' flag on the breakpoint.  Note that this
+   is not the same as the "silent" that may appear in the breakpoint's
+   commands.  */
+
+void
+breakpoint_set_silent (struct breakpoint *b, int silent)
+{
+  int old_silent = b->silent;
+  b->silent = silent;
+  if (old_silent != silent)
+    observer_notify_breakpoint_modified (b->number);
+}
+
+/* Set the thread for this breakpoint.  If THREAD is -1, make the
+   breakpoint work for any thread.  */
+
+void
+breakpoint_set_thread (struct breakpoint *b, int thread)
+{
+  int old_thread = b->thread;
+  b->thread = thread;
+  if (old_thread != thread)
+    observer_notify_breakpoint_modified (b->number);
+}
+
+/* Set the task for this breakpoint.  If TASK is 0, make the
+   breakpoint work for any task.  */
+
+void
+breakpoint_set_task (struct breakpoint *b, int task)
+{
+  int old_task = b->task;
+  b->task = task;
+  if (old_task != task)
+    observer_notify_breakpoint_modified (b->number);
+}
+
 void
 check_tracepoint_command (char *line, void *closure)
 {
@@ -10739,13 +10776,6 @@ set_ignore_count (int bptnum, int count, int from_tty)
   error (_("No breakpoint number %d."), bptnum);
 }
 
-void
-make_breakpoint_silent (struct breakpoint *b)
-{
-  /* Silence the breakpoint.  */
-  b->silent = 1;
-}
-
 /* Command to set ignore-count of breakpoint N to COUNT.  */
 
 static void
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 6eed2cd..a7d0c61 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1063,6 +1063,12 @@ extern void enable_breakpoint (struct breakpoint *);
 extern void breakpoint_set_commands (struct breakpoint *b, 
 				     struct command_line *commands);
 
+extern void breakpoint_set_silent (struct breakpoint *b, int silent);
+
+extern void breakpoint_set_thread (struct breakpoint *b, int thread);
+
+extern void breakpoint_set_task (struct breakpoint *b, int task);
+
 /* Clear the "inserted" flag in all breakpoints.  */
 extern void mark_breakpoints_out (void);
 
@@ -1140,9 +1146,6 @@ extern int catch_syscall_enabled (void);
    Returns 0 if not, greater than 0 if we are.  */
 extern int catching_syscall_number (int syscall_number);
 
-/* Tell a breakpoint to be quiet.  */
-extern void make_breakpoint_silent (struct breakpoint *);
-
 /* Return a tracepoint with the given number if found.  */
 extern struct breakpoint *get_tracepoint (int num);
 
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 5efbf39..2755fd0 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1545,7 +1545,7 @@ finish_backward (struct symbol *function)
 				  bp_breakpoint);
       /* Tell the breakpoint to keep quiet.  We won't be done
          until we've done another reverse single-step.  */
-      make_breakpoint_silent (breakpoint);
+      breakpoint_set_silent (breakpoint, 1);
       old_chain = make_cleanup_delete_breakpoint (breakpoint);
       proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
       /* We will be stopped when proceed returns.  */
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index b6d0088..3474b71 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -201,7 +201,7 @@ bppy_set_silent (PyObject *self, PyObject *newvalue, void *closure)
   if (cmp < 0)
     return -1;
   else
-    self_bp->bp->silent = cmp;
+    breakpoint_set_silent (self_bp->bp, cmp);
 
   return 0;
 }
@@ -240,7 +240,7 @@ bppy_set_thread (PyObject *self, PyObject *newvalue, void *closure)
       return -1;
     }
 
-  self_bp->bp->thread = id;
+  breakpoint_set_thread (self_bp->bp, id);
 
   return 0;
 }
@@ -279,7 +279,7 @@ bppy_set_task (PyObject *self, PyObject *newvalue, void *closure)
       return -1;
     }
 
-  self_bp->bp->task = id;
+  breakpoint_set_task (self_bp->bp, id);
 
   return 0;
 }

Re: FYI: add some breakpoint setter methods

[Joel Brobecker]

> As an aside, it seems to me that the breakpoint observers should
> probably take a 'struct breakpoint *' as an argument, rather than a
> breakpoint number.  This would be more efficient: I think most users
> will want to have access to the actual breakpoint anyhow, and this would
> avoid a search through all breakpoints for the number.  Perhaps I will
> implement this.

Seems like a good idea. I had a discussion like that with one of the
contributors a while ago, trying to convince him to use a breakpoint
struct instead of the breakpoint number in his observer. I think it was
for annotations.  In any case, he argued that this was sufficient for
his needs so far, so I let it go, but it came back to "bite" me later on
("bite" <=> I had to change it to use struct breakpoint).

> 2011-01-27  Tom Tromey  <tromey@redhat.com>
> 
> 	* infcmd.c (finish_backward): Use breakpoint_set_silent.
> 	* python/py-breakpoint.c (bppy_set_silent): Use
> 	breakpoint_set_silent.
> 	(bppy_set_thread): Use breakpoint_set_thread.
> 	(bppy_set_task): Use breakpoint_set_task.
> 	* breakpoint.h (breakpoint_set_silent, breakpoint_set_thread)
> 	(breakpoint_set_task): Declare.
> 	(make_breakpoint_silent): Remove.
> 	* breakpoint.c (breakpoint_set_silent): New function.
> 	(breakpoint_set_thread): Likewise.
> 	(breakpoint_set_task): Likewise.
> 	(make_breakpoint_silent): Remove.

Just a nit I noticed:

> +/* Set the internal `silent' flag on the breakpoint.  Note that this
> +   is not the same as the "silent" that may appear in the breakpoint's
> +   commands.  */
> +
> +void
> +breakpoint_set_silent (struct breakpoint *b, int silent)
> +{
> +  int old_silent = b->silent;
> +  b->silent = silent;

Missing empty line after variable declaration (it's happening multiple
times throughout the patch).

-- 
Joel

Re: FYI: add some breakpoint setter methods

[Vladimir Prus]

Joel Brobecker wrote:

>> As an aside, it seems to me that the breakpoint observers should
>> probably take a 'struct breakpoint *' as an argument, rather than a
>> breakpoint number.  This would be more efficient: I think most users
>> will want to have access to the actual breakpoint anyhow, and this would
>> avoid a search through all breakpoints for the number.  Perhaps I will
>> implement this.
> 
> Seems like a good idea.

I will be submitting a patch for that very soon.

- Volodya


-- 
Vladimir Prus
Mentor Graphics
+7 (812) 677-68-40

Re: FYI: add some breakpoint setter methods

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Tom> As an aside, it seems to me that the breakpoint observers should
Tom> probably take a 'struct breakpoint *' as an argument, rather than a
Tom> breakpoint number.

Joel> Seems like a good idea.

It turns out that Volodya also wrote a patch to do async breakpoint
notifications.  I think he is going to post his soon; his includes this
observer change.

[...]
Joel> Missing empty line after variable declaration (it's happening multiple
Joel> times throughout the patch).

Thanks.  Here is what I am checking in.

Tom

2011-01-31  Tom Tromey  <tromey@redhat.com>

	* infcmd.c (finish_backward): Use breakpoint_set_silent.
	* python/py-breakpoint.c (bppy_set_silent): Use
	breakpoint_set_silent.
	(bppy_set_thread): Use breakpoint_set_thread.
	(bppy_set_task): Use breakpoint_set_task.
	* breakpoint.h (breakpoint_set_silent, breakpoint_set_thread)
	(breakpoint_set_task): Declare.
	(make_breakpoint_silent): Remove.
	* breakpoint.c (breakpoint_set_silent): New function.
	(breakpoint_set_thread): Likewise.
	(breakpoint_set_task): Likewise.
	(make_breakpoint_silent): Remove.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.530
diff -u -r1.530 breakpoint.c
--- breakpoint.c	31 Jan 2011 15:07:49 -0000	1.530
+++ breakpoint.c	31 Jan 2011 15:10:42 -0000
@@ -940,6 +940,46 @@
   observer_notify_breakpoint_modified (b->number);
 }
 
+/* Set the internal `silent' flag on the breakpoint.  Note that this
+   is not the same as the "silent" that may appear in the breakpoint's
+   commands.  */
+
+void
+breakpoint_set_silent (struct breakpoint *b, int silent)
+{
+  int old_silent = b->silent;
+
+  b->silent = silent;
+  if (old_silent != silent)
+    observer_notify_breakpoint_modified (b->number);
+}
+
+/* Set the thread for this breakpoint.  If THREAD is -1, make the
+   breakpoint work for any thread.  */
+
+void
+breakpoint_set_thread (struct breakpoint *b, int thread)
+{
+  int old_thread = b->thread;
+
+  b->thread = thread;
+  if (old_thread != thread)
+    observer_notify_breakpoint_modified (b->number);
+}
+
+/* Set the task for this breakpoint.  If TASK is 0, make the
+   breakpoint work for any task.  */
+
+void
+breakpoint_set_task (struct breakpoint *b, int task)
+{
+  int old_task = b->task;
+
+  b->task = task;
+  if (old_task != task)
+    observer_notify_breakpoint_modified (b->number);
+}
+
 void
 check_tracepoint_command (char *line, void *closure)
 {
@@ -10738,13 +10778,6 @@
   error (_("No breakpoint number %d."), bptnum);
 }
 
-void
-make_breakpoint_silent (struct breakpoint *b)
-{
-  /* Silence the breakpoint.  */
-  b->silent = 1;
-}
-
 /* Command to set ignore-count of breakpoint N to COUNT.  */
 
 static void
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.133
diff -u -r1.133 breakpoint.h
--- breakpoint.h	31 Jan 2011 15:07:49 -0000	1.133
+++ breakpoint.h	31 Jan 2011 15:10:42 -0000
@@ -1063,6 +1063,12 @@
 extern void breakpoint_set_commands (struct breakpoint *b, 
 				     struct command_line *commands);
 
+extern void breakpoint_set_silent (struct breakpoint *b, int silent);
+
+extern void breakpoint_set_thread (struct breakpoint *b, int thread);
+
+extern void breakpoint_set_task (struct breakpoint *b, int task);
+
 /* Clear the "inserted" flag in all breakpoints.  */
 extern void mark_breakpoints_out (void);
 
@@ -1140,9 +1146,6 @@
    Returns 0 if not, greater than 0 if we are.  */
 extern int catching_syscall_number (int syscall_number);
 
-/* Tell a breakpoint to be quiet.  */
-extern void make_breakpoint_silent (struct breakpoint *);
-
 /* Return a tracepoint with the given number if found.  */
 extern struct breakpoint *get_tracepoint (int num);
 
Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.276
diff -u -r1.276 infcmd.c
--- infcmd.c	25 Jan 2011 17:58:59 -0000	1.276
+++ infcmd.c	31 Jan 2011 15:10:42 -0000
@@ -1545,7 +1545,7 @@
 				  bp_breakpoint);
       /* Tell the breakpoint to keep quiet.  We won't be done
          until we've done another reverse single-step.  */
-      make_breakpoint_silent (breakpoint);
+      breakpoint_set_silent (breakpoint, 1);
       old_chain = make_cleanup_delete_breakpoint (breakpoint);
       proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
       /* We will be stopped when proceed returns.  */
Index: python/py-breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-breakpoint.c,v
retrieving revision 1.12
diff -u -r1.12 py-breakpoint.c
--- python/py-breakpoint.c	26 Jan 2011 20:53:45 -0000	1.12
+++ python/py-breakpoint.c	31 Jan 2011 15:10:42 -0000
@@ -201,7 +201,7 @@
   if (cmp < 0)
     return -1;
   else
-    self_bp->bp->silent = cmp;
+    breakpoint_set_silent (self_bp->bp, cmp);
 
   return 0;
 }
@@ -242,7 +242,7 @@
       return -1;
     }
 
-  self_bp->bp->thread = id;
+  breakpoint_set_thread (self_bp->bp, id);
 
   return 0;
 }
@@ -283,7 +283,7 @@
       return -1;
     }
 
-  self_bp->bp->task = id;
+  breakpoint_set_task (self_bp->bp, id);
 
   return 0;
 }