[PATCH] Fix PR14617: New MI notification on tracepoint modified.

[PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Yao Qi]

Hi,
As PR gdb/14617 described, when the passcount of a tracepoint is
modified, GDB doesn't emit any notification.  This patch adds a
new notification '=tracepoint-modified' for the changes to
tracepoint-specific attributes.  The reason I don't use
'=breakpoint-modified' notification is that IMO, it is about
changes to general breakpoints (breakpoint, tracepoint, catchpoint,
etc), so I don't want to put tracepoint-specific stuff into
it.

Regression tested on x86_64-linux with both native and gdbserver,
OK to apply?

gdb/doc:

	* gdb.texinfo (GDB/MI Async Records): Document new MI notificaiton
	"=tracepoint-modified".
	* observer.texi (GDB Observers): Change the parameter of observer
	"tracepoint-modified".

gdb:

	Fix PR gdb/14617.
	* NEWS: Mention new MI notification "=tracepoint-modified".

	* breakpoint.c (trace_pass_set_count): Update caller of
	observer_notify_tracepoint_modified.
	* mi/mi-cmd-break.c (mi_cmd_break_passcount): Likewise.

	* mi/mi-interp.c: Declare 'mi_tracepoint_modified'.
	(mi_interpreter_init): Attach 'mi_tracepoint_modified'
	to 'tracepoint-modified' observer.
	(mi_breakpoint_modified): New.
	* observer.sh (struct objfile;): Declare 'struct tracepoint'.

gdb/testsuite:

	Fix PR gdb/14617.
	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
	Remove setup_kfail, and update test.
---
 gdb/NEWS                                       |    2 ++
 gdb/breakpoint.c                               |    2 +-
 gdb/doc/gdb.texinfo                            |    5 +++++
 gdb/doc/observer.texi                          |    6 +++---
 gdb/mi/mi-cmd-break.c                          |    2 +-
 gdb/mi/mi-interp.c                             |   19 +++++++++++++++++++
 gdb/observer.sh                                |    1 +
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp |    3 +--
 8 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 0d13c19..485c21b 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -66,6 +66,8 @@ py [command]
      "=memory-changed".
   ** Download of tracepoints are now notified using new async record
      "=tracepoint-downloaded".
+  ** Passcount of tracepoint changes are now notified using new async record
+     "=tracepoint-modified".
 
 *** Changes in GDB 7.5
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3d69d41..e233cd0 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15246,7 +15246,7 @@ static void
 trace_pass_set_count (struct tracepoint *tp, int count, int from_tty)
 {
   tp->pass_count = count;
-  observer_notify_tracepoint_modified (tp->base.number);
+  observer_notify_tracepoint_modified (tp);
   if (from_tty)
     printf_filtered (_("Setting tracepoint %d's passcount to %d\n"),
 		     tp->base.number, count);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index dbdf39b..64cda47 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27685,6 +27685,11 @@ breakpoint commands; @xref{GDB/MI Breakpoint Commands}.  The
 Note that if a breakpoint is emitted in the result record of a
 command, then it will not also be emitted in an async record.
 
+@item =tracepoint-modified,bkpt=@{number=@var{id},passcount=@var{count}@}
+Reports that a tracepoint-specific attribute was modified.  The @var{id}
+is the ordinal number of tracepoint and @var{count} is the passcount
+of the tracepoint.
+
 @item =tracepoint-downloaded,id="@var{number}",address="@var{addr}"
 Reports that a tracepoint was downloaded to target.  The @var{number}
 is the ordinal number of the tracepoint.  The @var{addr} is the
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 9fd92fb..e2033a6 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -189,9 +189,9 @@ A tracepoint has been destroyed.  The argument @var{tpnum} is the
 number of the newly-destroyed tracepoint.
 @end deftypefun
 
-@deftypefun void tracepoint_modified (int @var{tpnum})
-A tracepoint has been modified in some way.  The argument @var{tpnum}
-is the number of the modified tracepoint.
+@deftypefun void tracepoint_modified (struct tracepoint *@var{t})
+A tracepoint has been modified in some way.  The argument @var{t}
+is the pointer to the modified tracepoint.
 @end deftypefun
 
 @deftypefun void tracepoint_downloaded (struct bp_location *@var{loc})
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 2fe84dc..e3f709e 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -200,7 +200,7 @@ mi_cmd_break_passcount (char *command, char **argv, int argc)
   if (t)
     {
       t->pass_count = p;
-      observer_notify_tracepoint_modified (n);
+      observer_notify_tracepoint_modified (t);
     }
   else
     {
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index d83d493..ec14f03 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -76,6 +76,7 @@ static void mi_tsv_deleted (const char *name);
 static void mi_breakpoint_created (struct breakpoint *b);
 static void mi_breakpoint_deleted (struct breakpoint *b);
 static void mi_breakpoint_modified (struct breakpoint *b);
+static void mi_tracepoint_modified (struct tracepoint *t);
 static void mi_tracepoint_downloaded (struct bp_location *loc);
 static void mi_command_param_changed (const char *param, const char *value);
 static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
@@ -141,6 +142,7 @@ mi_interpreter_init (struct interp *interp, int top_level)
       observer_attach_breakpoint_created (mi_breakpoint_created);
       observer_attach_breakpoint_deleted (mi_breakpoint_deleted);
       observer_attach_breakpoint_modified (mi_breakpoint_modified);
+      observer_attach_tracepoint_modified (mi_tracepoint_modified);
       observer_attach_tracepoint_downloaded (mi_tracepoint_downloaded);
       observer_attach_command_param_changed (mi_command_param_changed);
       observer_attach_memory_changed (mi_memory_changed);
@@ -683,6 +685,23 @@ mi_breakpoint_modified (struct breakpoint *b)
   gdb_flush (mi->event_channel);
 }
 
+/* Emit notification about modified tracepoint.  */
+
+static void
+mi_tracepoint_modified (struct tracepoint *t)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+
+  target_terminal_ours ();
+
+  fprintf_unfiltered (mi->event_channel,
+		      "tracepoint-modified,bkpt={number=\"%d\""
+		      ",passcount=\"%d\"}\n",
+		      t->base.number, t->pass_count);
+
+  gdb_flush (mi->event_channel);
+}
+
 /* Emit notification about downloaded tracepoint.  MI frontends may
    check whether tracepoints are downloaded or not and display
    downloaded ones and un-downloaded ones differently.  */
diff --git a/gdb/observer.sh b/gdb/observer.sh
index 3df6578..00e3e90 100755
--- a/gdb/observer.sh
+++ b/gdb/observer.sh
@@ -65,6 +65,7 @@ struct objfile;
 struct thread_info;
 struct inferior;
 struct bp_location;
+struct tracepoint;
 EOF
         ;;
 esac
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index 773f588..2a5ab1a 100644
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -120,10 +120,9 @@ proc test_insert_delete_modify { } {
 	{.*=breakpoint-modified,bkpt=\{number="5",.*,ignore=\"1\".*\}.*\n\^done} \
 	$test
     # 5. when modifying pass count.
-    setup_kfail gdb/14617  *-*-*
     set test "passcount 1 4"
     mi_gdb_test $test \
-	{.*=breakpoint-modified,bkpt=\{number="4",.*\}.*\n\^done} \
+	{.*=tracepoint-modified,bkpt=\{number="4",passcount="1".*\}.*\n\^done} \
 	$test
 
     # Delete some breakpoints and verify that '=breakpoint-deleted
-- 
1.7.7.6

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Thu, 18 Oct 2012 18:36:26 +0800
> 
> As PR gdb/14617 described, when the passcount of a tracepoint is
> modified, GDB doesn't emit any notification.  This patch adds a
> new notification '=tracepoint-modified' for the changes to
> tracepoint-specific attributes.  The reason I don't use
> '=breakpoint-modified' notification is that IMO, it is about
> changes to general breakpoints (breakpoint, tracepoint, catchpoint,
> etc), so I don't want to put tracepoint-specific stuff into
> it.
> 
> Regression tested on x86_64-linux with both native and gdbserver,
> OK to apply?

The documentation parts are OK.  Thanks.

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Yao Qi]

On 10/18/2012 06:36 PM, Yao Qi wrote:
> gdb:
>
> 	Fix PR gdb/14617.
> 	* NEWS: Mention new MI notification "=tracepoint-modified".
>
> 	* breakpoint.c (trace_pass_set_count): Update caller of
> 	observer_notify_tracepoint_modified.
> 	* mi/mi-cmd-break.c (mi_cmd_break_passcount): Likewise.
>
> 	* mi/mi-interp.c: Declare 'mi_tracepoint_modified'.
> 	(mi_interpreter_init): Attach 'mi_tracepoint_modified'
> 	to 'tracepoint-modified' observer.
> 	(mi_breakpoint_modified): New.
> 	* observer.sh (struct objfile;): Declare 'struct tracepoint'.
>
> gdb/testsuite:
>
> 	Fix PR gdb/14617.
> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
> 	Remove setup_kfail, and update test.

Ping? http://sourceware.org/ml/gdb-patches/2012-10/msg00321.html

-- 
Yao

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Eli Zaretskii]

> Date: Tue, 30 Oct 2012 15:02:59 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 10/18/2012 06:36 PM, Yao Qi wrote:
> > gdb:
> >
> > 	Fix PR gdb/14617.
> > 	* NEWS: Mention new MI notification "=tracepoint-modified".
> >
> > 	* breakpoint.c (trace_pass_set_count): Update caller of
> > 	observer_notify_tracepoint_modified.
> > 	* mi/mi-cmd-break.c (mi_cmd_break_passcount): Likewise.
> >
> > 	* mi/mi-interp.c: Declare 'mi_tracepoint_modified'.
> > 	(mi_interpreter_init): Attach 'mi_tracepoint_modified'
> > 	to 'tracepoint-modified' observer.
> > 	(mi_breakpoint_modified): New.
> > 	* observer.sh (struct objfile;): Declare 'struct tracepoint'.
> >
> > gdb/testsuite:
> >
> > 	Fix PR gdb/14617.
> > 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
> > 	Remove setup_kfail, and update test.
> 
> Ping? http://sourceware.org/ml/gdb-patches/2012-10/msg00321.html

OK for the docs.

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> +@item =tracepoint-modified,bkpt=@{number=@var{id},passcount=@var{count}@}

print_one_breakpoint_location names the field "pass".
I think consistency would be good.  What do you think?

Tom

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Yao Qi]

On 11/02/2012 04:27 AM, Tom Tromey wrote:
> Yao> +@item =tracepoint-modified,bkpt=@{number=@var{id},passcount=@var{count}@}
> 
> print_one_breakpoint_location names the field "pass".
> I think consistency would be good.  What do you think?


Tom,
Inspired by your comments, 'breakpoint-modified' notification has covered
tracepoint modification, which I didn't realize.  This version is much
simpler because we can put tracepoint object to function
observer_notify_breakpoint_modified, and "pass" field appears in the
MI notification.  On the other hand, existing observers
tracepoint_{created,deleted,modified} are useless, so they are removed
in this patch as well.

Regression tested on x86_64-linux.  Is it OK?

-- 
Yao

gdb:

2012-11-02  Yao Qi  <yao@codesourcery.com>

	Fix PR gdb/14617.
	* breakpoint.c (trace_pass_set_count): Call
	observer_notify_breakpoint_modified instead of
	observer_notify_tracepoint_modified.
	* mi/mi-cmd-break.c (mi_cmd_break_passcount): Likewise.

gdb/doc:

2012-11-02  Yao Qi  <yao@codesourcery.com>

	* observer.texi (GDB Observers): Remove observer
	'tracepoint_modified', 'tracepoint_created' and
	'tracepoint_deleted'.

gdb/testsuite:

2012-11-02  Yao Qi  <yao@codesourcery.com>

	Fix PR gdb/14617.
	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
	Remove setup_kfail, and update test.
---
 gdb/breakpoint.c                               |    2 +-
 gdb/doc/observer.texi                          |   15 ---------------
 gdb/mi/mi-cmd-break.c                          |    2 +-
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp |    3 +--
 4 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 76e3e89..1d79d6a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15245,7 +15245,7 @@ static void
 trace_pass_set_count (struct tracepoint *tp, int count, int from_tty)
 {
   tp->pass_count = count;
-  observer_notify_tracepoint_modified (tp->base.number);
+  observer_notify_breakpoint_modified (&tp->base);
   if (from_tty)
     printf_filtered (_("Setting tracepoint %d's passcount to %d\n"),
 		     tp->base.number, count);
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 106475b..50038ac 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -179,21 +179,6 @@ A breakpoint has been modified in some way.  The argument @var{b}
 is the modified breakpoint.
 @end deftypefun
 
-@deftypefun void tracepoint_created (int @var{tpnum})
-A new tracepoint has been created.  The argument @var{tpnum} is the
-number of the newly-created tracepoint.
-@end deftypefun
-
-@deftypefun void tracepoint_deleted (int @var{tpnum})
-A tracepoint has been destroyed.  The argument @var{tpnum} is the
-number of the newly-destroyed tracepoint.
-@end deftypefun
-
-@deftypefun void tracepoint_modified (int @var{tpnum})
-A tracepoint has been modified in some way.  The argument @var{tpnum}
-is the number of the modified tracepoint.
-@end deftypefun
-
 @deftypefun void traceframe_changed (int @var{tfnum}, int @var{tpnum})
 The trace frame is changed to @var{tfnum} (e.g., by using the
 @code{tfind} command).  If @var{tfnum} is negative, it means
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 2fe84dc..487d42d 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -200,7 +200,7 @@ mi_cmd_break_passcount (char *command, char **argv, int argc)
   if (t)
     {
       t->pass_count = p;
-      observer_notify_tracepoint_modified (n);
+      observer_notify_breakpoint_modified (&t->base);
     }
   else
     {
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index 773f588..ec10032 100644
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -120,10 +120,9 @@ proc test_insert_delete_modify { } {
 	{.*=breakpoint-modified,bkpt=\{number="5",.*,ignore=\"1\".*\}.*\n\^done} \
 	$test
     # 5. when modifying pass count.
-    setup_kfail gdb/14617  *-*-*
     set test "passcount 1 4"
     mi_gdb_test $test \
-	{.*=breakpoint-modified,bkpt=\{number="4",.*\}.*\n\^done} \
+	{.*=breakpoint-modified,bkpt=\{number="4",.*pass="1".*\}.*\n\^done} \
 	$test
 
     # Delete some breakpoints and verify that '=breakpoint-deleted
-- 
1.7.7.6

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> Inspired by your comments, 'breakpoint-modified' notification has covered
Yao> tracepoint modification, which I didn't realize.  This version is much
Yao> simpler because we can put tracepoint object to function
Yao> observer_notify_breakpoint_modified, and "pass" field appears in the
Yao> MI notification.  On the other hand, existing observers
Yao> tracepoint_{created,deleted,modified} are useless, so they are removed
Yao> in this patch as well.

Yao> Regression tested on x86_64-linux.  Is it OK?

It seems reasonable to me.
Ok.

Tom

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Pedro Alves]

On 10/18/2012 11:36 AM, Yao Qi wrote:
> Hi,
> As PR gdb/14617 described, when the passcount of a tracepoint is
> modified, GDB doesn't emit any notification.  This patch adds a
> new notification '=tracepoint-modified' for the changes to
> tracepoint-specific attributes.  The reason I don't use
> '=breakpoint-modified' notification is that IMO, it is about
> changes to general breakpoints (breakpoint, tracepoint, catchpoint,
> etc), so I don't want to put tracepoint-specific stuff into
> it.

For better or worse, we call everything "breakpoint".  The number space
of tracepoints is the same as of all other "breakpoint kinds".
This didn't use to be the case.  Tracepoints used to be a separate
entity.  They were merged back as a kind of breakpoint a couple years
ago.  It was seen as a step forward.

We emit =breakpoint-created, =breakpoint-deleted for tracepoints:

trace main
&"trace main\n"
~"Note: breakpoint 6 also set at pc 0x457aab.\n"
~"Tracepoint 7 at 0x457aab: file ../../src/gdb/gdb.c, line 29.\n"
=breakpoint-created,bkpt={number="7",type="tracepoint",disp="keep",enabled="y",addr="0x0000000000457aab",func="main",file="../../src/gdb/gdb.c",fullname="/home/pedro/gdb/mygit/src/gdb/gdb.c",line="29",times="0",original-location="main"}
^done
(gdb)

delete 7
&"delete 7\n"
=breakpoint-deleted,id="7"
^done

As also emit =breakpoint-modified for tracepoints, all other kinds
of breakpoints (watchpoints, catchpoints, etc.).  E.g.,

(gdb)
condition 6 0
&"condition 6 0\n"
=breakpoint-modified,bkpt={number="6",type="tracepoint",disp="keep",enabled="y",addr="0x0000000000457aab",func="main",file="../../src/gdb/gdb.c",fullname="/home/pedro/gdb/mygit/src/gdb/gdb.c",line="29",cond="0",times="0",original-location="main"}
^done

This notification dumps the whole breakpoint, instead of trying
to second guess what might be useful for the frontend.

If you follow the tracepoint_modified observer introduction in
breakpoint.c, you'll find that it was originally moved from
tracepoint.c back into breakpoint.c, by Stan's patch that made
tracepoints be a kind of breakpoint (23d4c598):

    2009-03-30  Stan Shebs  <stan@codesourcery.com>

        Make tracepoints into a type of breakpoint.
    ...

It just seems like an oversight that this wasn't converted
that well.

For all of this, I'm against this new notification.  Just make changing
the tracepoint emit the existing =breakpoint-modified notification.
Why doesn't that work?

-- 
Pedro Alves

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Yao Qi]

On 11/02/2012 11:17 PM, Pedro Alves wrote:
> For all of this, I'm against this new notification.  Just make changing
> the tracepoint emit the existing =breakpoint-modified notification.
> Why doesn't that work?

Pedro, I realized this issue during patch review.  I posted a new one 
which uses 'breakpoint-modified' notification for tracepoint as well.

   http://sourceware.org/ml/gdb-patches/2012-11/msg00033.html

-- 
Yao

Re: [PATCH] Fix PR14617: New MI notification on tracepoint modified.

[Pedro Alves]

On 11/02/2012 03:21 PM, Yao Qi wrote:
> On 11/02/2012 11:17 PM, Pedro Alves wrote:
>> For all of this, I'm against this new notification.  Just make changing
>> the tracepoint emit the existing =breakpoint-modified notification.
>> Why doesn't that work?
> 
> Pedro, I realized this issue during patch review.  I posted a new one which uses 'breakpoint-modified' notification for tracepoint as well.
> 
>   http://sourceware.org/ml/gdb-patches/2012-11/msg00033.html

Ah awesome.  Sorry for missing it.

-- 
Pedro Alves