[RFC 0/2] Two new MI notifications

[RFC 0/2] Two new MI notifications

[Yao Qi]

Hi,
This patch series is about adding two new MI notifications,
memory-changed notification and tracepoint-downloaded notification.
They are quite independent of each other.

[PATCH 1/2] new memory-changed MI notification.

[Yao Qi]

Hi,
There are usually two views in MI front-end, 'memory view' and 'code
view', which displays the contents of 'data' and 'code'.  If user
modified memory in console, both views should be updated to show the
latest contents.  In order to achieve this, this patch adds a new MI
notification '=memory-changed', so that MI front-ends can refresh its
'memory view' and 'code view' once this notification arrives.

The existing observer 'memory-changed' doesn't have a parameter on
inferior, and thinks memory-change happens on 'current inferior'.  It
is better not to propagate 'current inferior' everywhere, so this
patch adds a parameter 'inferior' in observer 'memory-changed'.

Memory can be modified in two ways through MI, 'var-assign' and
'data-write-memory[-btes]', so we add two flags for suppression
'var_assign' and 'data_write_memory'.

gdb:

2012-09-27  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (invalidate_bp_value_on_memory_change): Add one
	more parameter 'inferior'.
	* corefile.c (write_memory_with_notification): Caller update.

	* mi/mi-cmds.c (mi_cmd mi_cmds): Update for "var-assign",
	"data-write-memory", and "data-write-memory-bytes.
	* mi/mi-interp.c: Include objfiles.h.
	(mi_interpreter_init): Call observer_attach_memory_changed.
	(mi_memory_changed): New.
	* mi/mi-main.h (struct mi_suppress_notification) <var_assign>:
	<data_write_memory>: New fields.

	* NEWS: Mention new MI notification "memory-changed".

gdb/doc:

2012-09-27  Yao Qi  <yao@codesourcery.com>

	* observer.texi (GDB Observers): Update observer
	'memory_changed'.

	* gdb.texinfo (GDB/MI Async Records): Document for
	"memory-changed" notification.

gdb/testsuite:

2012-09-27  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-memory-changed.exp: New.
---
 gdb/NEWS                                   |    2 +
 gdb/breakpoint.c                           |    4 +-
 gdb/corefile.c                             |    2 +-
 gdb/doc/gdb.texinfo                        |    7 +++
 gdb/doc/observer.texi                      |    4 +-
 gdb/mi/mi-cmds.c                           |    9 ++-
 gdb/mi/mi-interp.c                         |   46 ++++++++++++++++
 gdb/mi/mi-main.h                           |    5 ++
 gdb/testsuite/gdb.mi/mi-memory-changed.exp |   81 ++++++++++++++++++++++++++++
 9 files changed, 152 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-memory-changed.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index abd0932..798f050 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -57,6 +57,8 @@ py [command]
      using new async records "=tsv-created" and "=tsv-deleted".
   ** The start and stop of process record are now notified using new
      async record "=record-started" and "=record-stopped".
+  ** Memory changes are now notified using new async record
+     "=memory-changed".
 
 *** Changes in GDB 7.5
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4e7c145..3d0f2c2 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14718,8 +14718,8 @@ show_breakpoint_cmd (char *args, int from_tty)
    GDB itself.  */
 
 static void
-invalidate_bp_value_on_memory_change (CORE_ADDR addr, ssize_t len,
-				      const bfd_byte *data)
+invalidate_bp_value_on_memory_change (struct inferior *inferior, CORE_ADDR addr,
+				      ssize_t len, const bfd_byte *data)
 {
   struct breakpoint *bp;
 
diff --git a/gdb/corefile.c b/gdb/corefile.c
index ac8eff5..a2ed24f 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -369,7 +369,7 @@ write_memory_with_notification (CORE_ADDR memaddr, const bfd_byte *myaddr,
 				ssize_t len)
 {
   write_memory (memaddr, myaddr, len);
-  observer_notify_memory_changed (memaddr, len, myaddr);
+  observer_notify_memory_changed (current_inferior (), memaddr, len, myaddr);
 }
 
 /* Store VALUE at ADDR in the inferior as a LEN-byte unsigned
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5fcbada..fa925d7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27670,6 +27670,13 @@ changed to @var{value}.  In the multi-word @code{set} command,
 the @var{param} is the whole parameter list to @code{set} command.
 For example, In command @code{set check type on}, @var{param}
 is @code{check type} and @var{value} is @code{on}.
+
+@item =memory-changed,thread-group=@var{id},addr=@var{addr},len=@var{len}"[,type=@var{type}"]
+Reports that bytes from @var{addr} to @var{data} + @var{len} were
+written in an inferior.  The @var{id} is the identifier of the
+thread group corresponding to the affected inferior.  @var{type}
+is the type of the section written to, @code{code}; it exists only
+when the type of the section is known.
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 3fdc90a..106475b 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -230,9 +230,9 @@ The inferior @var{inf} has been removed from the list of inferiors.
 This method is called immediately before freeing @var{inf}.
 @end deftypefun
 
-@deftypefun void memory_changed (CORE_ADDR @var{addr}, ssize_t @var{len}, const bfd_byte *@var{data})
+@deftypefun void memory_changed (struct inferior *@var{inferior}, CORE_ADDR @var{addr}, ssize_t @var{len}, const bfd_byte *@var{data})
 Bytes from @var{data} to @var{data} + @var{len} have been written
-to the current inferior at @var{addr}.
+to the @var{inferior} at @var{addr}.
 @end deftypefun
 
 @deftypefun void before_prompt (const char *@var{current_prompt})
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 2ed1905..8faf1a9 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -75,8 +75,10 @@ static struct mi_cmd mi_cmds[] =
   DEF_MI_CMD_MI ("data-list-register-values", mi_cmd_data_list_register_values),
   DEF_MI_CMD_MI ("data-read-memory", mi_cmd_data_read_memory),
   DEF_MI_CMD_MI ("data-read-memory-bytes", mi_cmd_data_read_memory_bytes),
-  DEF_MI_CMD_MI ("data-write-memory", mi_cmd_data_write_memory),
-  DEF_MI_CMD_MI ("data-write-memory-bytes", mi_cmd_data_write_memory_bytes),
+  DEF_MI_CMD_MI_1 ("data-write-memory", mi_cmd_data_write_memory,
+		   &mi_suppress_notification.data_write_memory),
+  DEF_MI_CMD_MI_1 ("data-write-memory-bytes", mi_cmd_data_write_memory_bytes,
+		   &mi_suppress_notification.data_write_memory),
   DEF_MI_CMD_MI ("data-write-register-values",
 		 mi_cmd_data_write_register_values),
   DEF_MI_CMD_MI ("enable-timings", mi_cmd_enable_timings),
@@ -144,7 +146,8 @@ static struct mi_cmd mi_cmds[] =
   DEF_MI_CMD_MI ("trace-start", mi_cmd_trace_start),
   DEF_MI_CMD_MI ("trace-status", mi_cmd_trace_status),
   DEF_MI_CMD_MI ("trace-stop", mi_cmd_trace_stop),
-  DEF_MI_CMD_MI ("var-assign", mi_cmd_var_assign),
+  DEF_MI_CMD_MI_1 ("var-assign", mi_cmd_var_assign,
+		   &mi_suppress_notification.var_assign),
   DEF_MI_CMD_MI ("var-create", mi_cmd_var_create),
   DEF_MI_CMD_MI ("var-delete", mi_cmd_var_delete),
   DEF_MI_CMD_MI ("var-evaluate-expression", mi_cmd_var_evaluate_expression),
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index d383958..e58d9e0 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -35,6 +35,7 @@
 #include "gdbthread.h"
 #include "solist.h"
 #include "gdb.h"
+#include "objfiles.h"
 
 /* These are the interpreter setup, etc. functions for the MI
    interpreter.  */
@@ -76,6 +77,8 @@ 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_command_param_changed (const char *param, const char *value);
+static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
+			       ssize_t len, const bfd_byte *myaddr);
 
 static int report_initial_inferior (struct inferior *inf, void *closure);
 
@@ -138,6 +141,7 @@ mi_interpreter_init (struct interp *interp, int top_level)
       observer_attach_breakpoint_deleted (mi_breakpoint_deleted);
       observer_attach_breakpoint_modified (mi_breakpoint_modified);
       observer_attach_command_param_changed (mi_command_param_changed);
+      observer_attach_memory_changed (mi_memory_changed);
 
       /* The initial inferior is created before this function is
 	 called, so we need to report it explicitly.  Use iteration in
@@ -840,6 +844,48 @@ mi_command_param_changed (const char *param, const char *value)
   gdb_flush (mi->event_channel);
 }
 
+/* Emit notification about the target memory change.  */
+
+static void
+mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
+		   ssize_t len, const bfd_byte *myaddr)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
+  struct obj_section *sec;
+
+  if (mi_suppress_notification.var_assign
+      || mi_suppress_notification.data_write_memory)
+    return;
+
+  target_terminal_ours ();
+
+  fprintf_unfiltered (mi->event_channel,
+		      "memory-changed");
+
+  ui_out_redirect (mi_uiout, mi->event_channel);
+
+  ui_out_field_fmt (mi_uiout, "thread-group", "i%d", inferior->num);
+  ui_out_field_core_addr (mi_uiout, "addr", target_gdbarch, memaddr);
+  ui_out_field_fmt (mi_uiout, "len", "0x%zx", len);
+
+  /* Append 'type=code' into notification if MEMADDR falls in the range of
+     sections contain code.  */
+  sec = find_pc_section (memaddr);
+  if (sec != NULL && sec->objfile != NULL)
+    {
+      flagword flags = bfd_get_section_flags (sec->objfile->obfd,
+					      sec->the_bfd_section);
+
+      if (flags & SEC_CODE)
+	ui_out_field_string (mi_uiout, "type", "code");
+    }
+
+  ui_out_redirect (mi_uiout, NULL);
+
+  gdb_flush (mi->event_channel);
+}
+
 static int
 report_initial_inferior (struct inferior *inf, void *closure)
 {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index aad7eeb..260d906 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -41,6 +41,11 @@ struct mi_suppress_notification
   int cmd_param_changed;
   /* Traceframe changed notification suppressed?  */
   int traceframe;
+  /* Notifications triggered by -var-assign suppressed?  */
+  int var_assign;
+  /* Notifications triggered by -data-write-memory and
+     -data-write-meory-bytes suppressed?  */
+  int data_write_memory;
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
diff --git a/gdb/testsuite/gdb.mi/mi-memory-changed.exp b/gdb/testsuite/gdb.mi/mi-memory-changed.exp
new file mode 100644
index 0000000..f290992
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-memory-changed.exp
@@ -0,0 +1,81 @@
+# Copyright 2012 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/>.
+
+standard_testfile basics.c
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
+	  executable {debug nowarnings}] != "" } {
+    untested mi-record-changed.exp
+    return -1
+}
+
+load_lib mi-support.exp
+
+if [mi_gdb_start] {
+    return
+}
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+
+mi_gdb_test "-break-insert -t ${srcfile}:[gdb_get_line_number "C = A + B"]" \
+    "\\^done,bkpt=\{number=\"1\".*" \
+    "insert breakpoint"
+mi_run_cmd
+mi_expect_stop "breakpoint-hit" "callee4" "" ".*" ".*" {"" "disp=\"del\""} \
+    "continue to callee4"
+
+mi_gdb_test "set var C = 4" \
+    ".*=memory-changed,thread-group=\"i${decimal}\".addr=\"${hex}\",len=\"${hex}\".*\\^done" \
+    "set var C = 4"
+
+# Write memory through MI commands shouldn't trigger MI notification.
+mi_gdb_test "-var-create var_c * C" \
+    "\\^done,name=\"var_c\",numchild=\"0\",value=\"4\",type=\"int\",thread-id=\"1\",has_more=\"0\"" \
+    "create objvar for C"
+
+mi_gdb_test "-var-assign var_c 5" \
+    "-var-assign var_c 5\r\n\\^done,value=\"5\"" \
+    "change C thru. varobj"
+
+mi_gdb_test  "-data-write-memory-bytes &C \"00\"" \
+    {\^done} \
+    "change C thru. -data-write-memory-bytes"
+
+# Modify code section also triggers MI notification.
+
+# Get the instruction content of function main and its address.
+set main_addr ""
+set main_insn ""
+set test "get address of main"
+send_gdb "x/x main\n"
+gdb_expect {
+    -re ".*(${hex}) <main>:.*(${hex}).*$mi_gdb_prompt$" {
+	set main_addr $expect_out(1,string)
+	set main_insn $expect_out(2,string)
+	pass $test
+    }
+    -re ".*$mi_gdb_prompt$" {
+	fail $test
+	return
+    }
+    timeout {
+	fail "$test (timeout)"
+	return
+    }
+}
+
+mi_gdb_test "set var *(unsigned int *) ${main_addr} = ${main_insn}" \
+    ".*=memory-changed,thread-group=\"i${decimal}\".addr=\"${main_addr}\",len=\"0x4\",type=\"code\".*\\^done"
+mi_gdb_exit
+return 0
-- 
1.7.7.6

Re: [PATCH 1/2] new memory-changed MI notification.

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Fri, 28 Sep 2012 08:49:06 +0800
> 
> Hi,
> There are usually two views in MI front-end, 'memory view' and 'code
> view', which displays the contents of 'data' and 'code'.  If user
> modified memory in console, both views should be updated to show the
> latest contents.  In order to achieve this, this patch adds a new MI
> notification '=memory-changed', so that MI front-ends can refresh its
> 'memory view' and 'code view' once this notification arrives.

Thanks.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -57,6 +57,8 @@ py [command]
>       using new async records "=tsv-created" and "=tsv-deleted".
>    ** The start and stop of process record are now notified using new
>       async record "=record-started" and "=record-stopped".
> +  ** Memory changes are now notified using new async record
> +     "=memory-changed".

This part is OK.

> +@item =memory-changed,thread-group=@var{id},addr=@var{addr},len=@var{len}"[,type=@var{type}"]

I think the quotes should be deleted; I see no sign of them in the
code that produces the "type=..." part.

> +Reports that bytes from @var{addr} to @var{data} + @var{len} were
> +written in an inferior.  The @var{id} is the identifier of the
> +thread group corresponding to the affected inferior.  @var{type}
> +is the type of the section written to, @code{code}; it exists only
> +when the type of the section is known.

If @var{type} can only be "code", then I suggest to say

  ...[,type=code]

explicitly.

Btw, why "code"?  If this is the name of the section, it should be
".text", not code.

> --- a/gdb/doc/observer.texi
> +++ b/gdb/doc/observer.texi
> @@ -230,9 +230,9 @@ The inferior @var{inf} has been removed from the list of inferiors.
>  This method is called immediately before freeing @var{inf}.
>  @end deftypefun
>  
> -@deftypefun void memory_changed (CORE_ADDR @var{addr}, ssize_t @var{len}, const bfd_byte *@var{data})
> +@deftypefun void memory_changed (struct inferior *@var{inferior}, CORE_ADDR @var{addr}, ssize_t @var{len}, const bfd_byte *@var{data})
>  Bytes from @var{data} to @var{data} + @var{len} have been written
> -to the current inferior at @var{addr}.
> +to the @var{inferior} at @var{addr}.
>  @end deftypefun

This part is OK.

Thanks.

Re: [PATCH 1/2] new memory-changed MI notification.

[Yao Qi]

On 09/28/2012 03:36 PM, Eli Zaretskii wrote:
> If @var{type} can only be "code", then I suggest to say
>
>    ...[,type=code]
>
> explicitly.
>

@var{type} can only "code" nowadays, but I am wondering we may have 
other types, such as "data", in the future.  I don't want to give the 
consumer of this notification an impression that "type=code" is 
hard-coded into notification.  If we don't have to worry about it at 
all, "[,type=code]" is fine to me.

> Btw, why "code"?  If this is the name of the section, it should be
> ".text", not code.

 From the consumer of this notification's point of view, 'type' is more 
useful than 'name', because the consumer may don't know what ".text" 
section is, or on some ports, text section is not named as ".text", such 
as section ".text_vle" for VLE.

-- 
Yao

Re: [PATCH 1/2] new memory-changed MI notification.

[Eli Zaretskii]

> Date: Fri, 28 Sep 2012 15:57:59 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 09/28/2012 03:36 PM, Eli Zaretskii wrote:
> > If @var{type} can only be "code", then I suggest to say
> >
> >    ...[,type=code]
> >
> > explicitly.
> >
> 
> @var{type} can only "code" nowadays, but I am wondering we may have 
> other types, such as "data", in the future.  I don't want to give the 
> consumer of this notification an impression that "type=code" is 
> hard-coded into notification.  If we don't have to worry about it at 
> all, "[,type=code]" is fine to me.

We don't have to worry about this at this time.  The explanatory text
says that type can only be "code" anyway, and will have to be revised
when other types can be emitted.  So not saying 'code" saves us
nothing.

> > Btw, why "code"?  If this is the name of the section, it should be
> > ".text", not code.
> 
>  From the consumer of this notification's point of view, 'type' is more 
> useful than 'name', because the consumer may don't know what ".text" 
> section is, or on some ports, text section is not named as ".text", such 
> as section ".text_vle" for VLE.

In that case, saying that this identifies a section is inaccurate.

But if we are going to put "code" explicitly in the notification line,
then this is perhaps a moot point.  The explanatory text should say
something like

  The optional @code{type="code"} part is reported if the memory
  written to holds executable code.

Re: [PATCH 1/2] new memory-changed MI notification.

[Tom Tromey]

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

Yao> There are usually two views in MI front-end, 'memory view' and 'code
Yao> view', which displays the contents of 'data' and 'code'.

Thanks.

Yao> Memory can be modified in two ways through MI, 'var-assign' and
Yao> 'data-write-memory[-btes]', so we add two flags for suppression
Yao> 'var_assign' and 'data_write_memory'.

Is there a particular reason to have two flags?
It seems one would do.

Usually I think it would be preferable to have a flag correspond to a
notification and not a command; but this would not work so well if a
command needed to suppress two different messages.  (Though if that
happens then maybe we should have a slightly different approach based on
bitmasks.)

Yao> +invalidate_bp_value_on_memory_change (struct inferior *inferior, CORE_ADDR addr,

This line is too long now.

Tom

Re: [PATCH 1/2] new memory-changed MI notification.

[Yao Qi]

On 09/29/2012 01:17 AM, Tom Tromey wrote:
> Is there a particular reason to have two flags?
> It seems one would do.
>
> Usually I think it would be preferable to have a flag correspond to a
> notification and not a command; but this would not work so well if a
> command needed to suppress two different messages.  (Though if that
> happens then maybe we should have a slightly different approach based on
> bitmasks.)

Flag 'var_assign' would suppress two notifications, 'memory-changed' and 
'register-changed' (to be posted later), because I feel hard to 
differentiate 'memory write' and 'register write' inside 
mi_cmd_var_assign and its callees.

Presently, looks only "-var-assign' may trigger two notifications, so I 
am not sure we have to switch to the new approach based on bitmask.

-- 
Yao

Re: [PATCH 1/2] new memory-changed MI notification.

[Yao Qi]

On 09/29/2012 01:17 AM, Tom Tromey wrote:
> Usually I think it would be preferable to have a flag correspond to a
> notification and not a command; but this would not work so well if a
> command needed to suppress two different messages.  (Though if that
> happens then maybe we should have a slightly different approach based on
> bitmasks.)
> 

I agree with you that one flag should correspond to a notification.  I
revised my patch a little bit to get rid of suppression flag
'var_assign'.

> Yao> +invalidate_bp_value_on_memory_change (struct inferior *inferior, CORE_ADDR addr,
> 
> This line is too long now.

The length of this line is 80, so my editor doesn't remind me to
shorten it.  Anyway, this line is shortened in the new patch.  What do
you think?

-- 
Yao

gdb:

2012-10-09  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (invalidate_bp_value_on_memory_change): Add one
	more parameter 'inferior'.
	* corefile.c (write_memory_with_notification): Caller update.

	* mi/mi-cmd-var.c: Include "mi-main.h".
	(mi_cmd_var_assign): Set mi_suppress_notification.data_write_memory
	to 1 and restore it later.
	* mi/mi-cmds.c (mi_cmd mi_cmds): Update for "data-write-memory"
	and "data-write-memory-bytes.
	* mi/mi-interp.c: Include objfiles.h.
	(mi_interpreter_init): Call observer_attach_memory_changed.
	(mi_memory_changed): New.
	* mi/mi-main.h (struct mi_suppress_notification) <memory>:
	New field.

	* NEWS: Mention new MI notification "memory-changed".

gdb/doc:

2012-10-09  Yao Qi  <yao@codesourcery.com>

	* observer.texi (GDB Observers): Update observer
	'memory_changed'.

	* gdb.texinfo (GDB/MI Async Records): Document for
	"memory-changed" notification.

gdb/testsuite:

2012-10-09  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-memory-changed.exp: New.
---
 gdb/NEWS                                   |    2 +
 gdb/breakpoint.c                           |    3 +-
 gdb/corefile.c                             |    2 +-
 gdb/doc/gdb.texinfo                        |    7 +++
 gdb/doc/observer.texi                      |    4 +-
 gdb/mi/mi-cmd-var.c                        |   10 ++++
 gdb/mi/mi-cmds.c                           |    6 ++-
 gdb/mi/mi-interp.c                         |   45 +++++++++++++++
 gdb/mi/mi-main.h                           |    2 +
 gdb/testsuite/gdb.mi/mi-memory-changed.exp |   81 ++++++++++++++++++++++++++++
 10 files changed, 156 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-memory-changed.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index abd0932..798f050 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -57,6 +57,8 @@ py [command]
      using new async records "=tsv-created" and "=tsv-deleted".
   ** The start and stop of process record are now notified using new
      async record "=record-started" and "=record-stopped".
+  ** Memory changes are now notified using new async record
+     "=memory-changed".
 
 *** Changes in GDB 7.5
 
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4e7c145..8eeeacf 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14718,7 +14718,8 @@ show_breakpoint_cmd (char *args, int from_tty)
    GDB itself.  */
 
 static void
-invalidate_bp_value_on_memory_change (CORE_ADDR addr, ssize_t len,
+invalidate_bp_value_on_memory_change (struct inferior *inferior,
+				      CORE_ADDR addr, ssize_t len,
 				      const bfd_byte *data)
 {
   struct breakpoint *bp;
diff --git a/gdb/corefile.c b/gdb/corefile.c
index ac8eff5..a2ed24f 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -369,7 +369,7 @@ write_memory_with_notification (CORE_ADDR memaddr, const bfd_byte *myaddr,
 				ssize_t len)
 {
   write_memory (memaddr, myaddr, len);
-  observer_notify_memory_changed (memaddr, len, myaddr);
+  observer_notify_memory_changed (current_inferior (), memaddr, len, myaddr);
 }
 
 /* Store VALUE at ADDR in the inferior as a LEN-byte unsigned
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5fcbada..19d0868 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27670,6 +27670,13 @@ changed to @var{value}.  In the multi-word @code{set} command,
 the @var{param} is the whole parameter list to @code{set} command.
 For example, In command @code{set check type on}, @var{param}
 is @code{check type} and @var{value} is @code{on}.
+
+@item =memory-changed,thread-group=@var{id},addr=@var{addr},len=@var{len}[,type="code"]
+Reports that bytes from @var{addr} to @var{data} + @var{len} were
+written in an inferior.  The @var{id} is the identifier of the
+thread group corresponding to the affected inferior.  The optional
+@code{type="code"} part is reported if the memory written to holds
+executable code.
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 3fdc90a..106475b 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -230,9 +230,9 @@ The inferior @var{inf} has been removed from the list of inferiors.
 This method is called immediately before freeing @var{inf}.
 @end deftypefun
 
-@deftypefun void memory_changed (CORE_ADDR @var{addr}, ssize_t @var{len}, const bfd_byte *@var{data})
+@deftypefun void memory_changed (struct inferior *@var{inferior}, CORE_ADDR @var{addr}, ssize_t @var{len}, const bfd_byte *@var{data})
 Bytes from @var{data} to @var{data} + @var{len} have been written
-to the current inferior at @var{addr}.
+to the @var{inferior} at @var{addr}.
 @end deftypefun
 
 @deftypefun void before_prompt (const char *@var{current_prompt})
diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c
index 5d7081f..dc47bc1 100644
--- a/gdb/mi/mi-cmd-var.c
+++ b/gdb/mi/mi-cmd-var.c
@@ -21,6 +21,7 @@
 
 #include "defs.h"
 #include "mi-cmds.h"
+#include "mi-main.h"
 #include "ui-out.h"
 #include "mi-out.h"
 #include "varobj.h"
@@ -616,6 +617,7 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
   struct ui_out *uiout = current_uiout;
   struct varobj *var;
   char *expression, *val;
+  struct cleanup *cleanup;
 
   if (argc != 2)
     error (_("-var-assign: Usage: NAME EXPRESSION."));
@@ -628,6 +630,12 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
 
   expression = xstrdup (argv[1]);
 
+  /* MI command '-var-assign' may write memory, so suppress memory
+     changed notification if it does.  */
+  cleanup
+    = make_cleanup_restore_integer (&mi_suppress_notification.memory);
+  mi_suppress_notification.memory = 1;
+
   if (!varobj_set_value (var, expression))
     error (_("-var-assign: Could not assign "
 	     "expression to variable object"));
@@ -635,6 +643,8 @@ mi_cmd_var_assign (char *command, char **argv, int argc)
   val = varobj_get_value (var);
   ui_out_field_string (uiout, "value", val);
   xfree (val);
+
+  do_cleanups (cleanup);
 }
 
 /* Type used for parameters passing to mi_cmd_var_update_iter.  */
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index 2ed1905..572625f 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -75,8 +75,10 @@ static struct mi_cmd mi_cmds[] =
   DEF_MI_CMD_MI ("data-list-register-values", mi_cmd_data_list_register_values),
   DEF_MI_CMD_MI ("data-read-memory", mi_cmd_data_read_memory),
   DEF_MI_CMD_MI ("data-read-memory-bytes", mi_cmd_data_read_memory_bytes),
-  DEF_MI_CMD_MI ("data-write-memory", mi_cmd_data_write_memory),
-  DEF_MI_CMD_MI ("data-write-memory-bytes", mi_cmd_data_write_memory_bytes),
+  DEF_MI_CMD_MI_1 ("data-write-memory", mi_cmd_data_write_memory,
+		   &mi_suppress_notification.memory),
+  DEF_MI_CMD_MI_1 ("data-write-memory-bytes", mi_cmd_data_write_memory_bytes,
+		   &mi_suppress_notification.memory),
   DEF_MI_CMD_MI ("data-write-register-values",
 		 mi_cmd_data_write_register_values),
   DEF_MI_CMD_MI ("enable-timings", mi_cmd_enable_timings),
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index d383958..d3c3d81 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -35,6 +35,7 @@
 #include "gdbthread.h"
 #include "solist.h"
 #include "gdb.h"
+#include "objfiles.h"
 
 /* These are the interpreter setup, etc. functions for the MI
    interpreter.  */
@@ -76,6 +77,8 @@ 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_command_param_changed (const char *param, const char *value);
+static void mi_memory_changed (struct inferior *inf, CORE_ADDR memaddr,
+			       ssize_t len, const bfd_byte *myaddr);
 
 static int report_initial_inferior (struct inferior *inf, void *closure);
 
@@ -138,6 +141,7 @@ mi_interpreter_init (struct interp *interp, int top_level)
       observer_attach_breakpoint_deleted (mi_breakpoint_deleted);
       observer_attach_breakpoint_modified (mi_breakpoint_modified);
       observer_attach_command_param_changed (mi_command_param_changed);
+      observer_attach_memory_changed (mi_memory_changed);
 
       /* The initial inferior is created before this function is
 	 called, so we need to report it explicitly.  Use iteration in
@@ -840,6 +844,47 @@ mi_command_param_changed (const char *param, const char *value)
   gdb_flush (mi->event_channel);
 }
 
+/* Emit notification about the target memory change.  */
+
+static void
+mi_memory_changed (struct inferior *inferior, CORE_ADDR memaddr,
+		   ssize_t len, const bfd_byte *myaddr)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
+  struct obj_section *sec;
+
+  if (mi_suppress_notification.memory)
+    return;
+
+  target_terminal_ours ();
+
+  fprintf_unfiltered (mi->event_channel,
+		      "memory-changed");
+
+  ui_out_redirect (mi_uiout, mi->event_channel);
+
+  ui_out_field_fmt (mi_uiout, "thread-group", "i%d", inferior->num);
+  ui_out_field_core_addr (mi_uiout, "addr", target_gdbarch, memaddr);
+  ui_out_field_fmt (mi_uiout, "len", "0x%zx", len);
+
+  /* Append 'type=code' into notification if MEMADDR falls in the range of
+     sections contain code.  */
+  sec = find_pc_section (memaddr);
+  if (sec != NULL && sec->objfile != NULL)
+    {
+      flagword flags = bfd_get_section_flags (sec->objfile->obfd,
+					      sec->the_bfd_section);
+
+      if (flags & SEC_CODE)
+	ui_out_field_string (mi_uiout, "type", "code");
+    }
+
+  ui_out_redirect (mi_uiout, NULL);
+
+  gdb_flush (mi->event_channel);
+}
+
 static int
 report_initial_inferior (struct inferior *inf, void *closure)
 {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index aad7eeb..a815fbe 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -41,6 +41,8 @@ struct mi_suppress_notification
   int cmd_param_changed;
   /* Traceframe changed notification suppressed?  */
   int traceframe;
+  /* Memory changed notification suppressed?  */
+  int memory;
 };
 extern struct mi_suppress_notification mi_suppress_notification;
 
diff --git a/gdb/testsuite/gdb.mi/mi-memory-changed.exp b/gdb/testsuite/gdb.mi/mi-memory-changed.exp
new file mode 100644
index 0000000..f290992
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-memory-changed.exp
@@ -0,0 +1,81 @@
+# Copyright 2012 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/>.
+
+standard_testfile basics.c
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
+	  executable {debug nowarnings}] != "" } {
+    untested mi-record-changed.exp
+    return -1
+}
+
+load_lib mi-support.exp
+
+if [mi_gdb_start] {
+    return
+}
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+
+mi_gdb_test "-break-insert -t ${srcfile}:[gdb_get_line_number "C = A + B"]" \
+    "\\^done,bkpt=\{number=\"1\".*" \
+    "insert breakpoint"
+mi_run_cmd
+mi_expect_stop "breakpoint-hit" "callee4" "" ".*" ".*" {"" "disp=\"del\""} \
+    "continue to callee4"
+
+mi_gdb_test "set var C = 4" \
+    ".*=memory-changed,thread-group=\"i${decimal}\".addr=\"${hex}\",len=\"${hex}\".*\\^done" \
+    "set var C = 4"
+
+# Write memory through MI commands shouldn't trigger MI notification.
+mi_gdb_test "-var-create var_c * C" \
+    "\\^done,name=\"var_c\",numchild=\"0\",value=\"4\",type=\"int\",thread-id=\"1\",has_more=\"0\"" \
+    "create objvar for C"
+
+mi_gdb_test "-var-assign var_c 5" \
+    "-var-assign var_c 5\r\n\\^done,value=\"5\"" \
+    "change C thru. varobj"
+
+mi_gdb_test  "-data-write-memory-bytes &C \"00\"" \
+    {\^done} \
+    "change C thru. -data-write-memory-bytes"
+
+# Modify code section also triggers MI notification.
+
+# Get the instruction content of function main and its address.
+set main_addr ""
+set main_insn ""
+set test "get address of main"
+send_gdb "x/x main\n"
+gdb_expect {
+    -re ".*(${hex}) <main>:.*(${hex}).*$mi_gdb_prompt$" {
+	set main_addr $expect_out(1,string)
+	set main_insn $expect_out(2,string)
+	pass $test
+    }
+    -re ".*$mi_gdb_prompt$" {
+	fail $test
+	return
+    }
+    timeout {
+	fail "$test (timeout)"
+	return
+    }
+}
+
+mi_gdb_test "set var *(unsigned int *) ${main_addr} = ${main_insn}" \
+    ".*=memory-changed,thread-group=\"i${decimal}\".addr=\"${main_addr}\",len=\"0x4\",type=\"code\".*\\^done"
+mi_gdb_exit
+return 0
-- 
1.7.7.6

Re: [PATCH 1/2] new memory-changed MI notification.

[dje]

Yao Qi writes:
 > On 09/29/2012 01:17 AM, Tom Tromey wrote:
 > > Usually I think it would be preferable to have a flag correspond to a
 > > notification and not a command; but this would not work so well if a
 > > command needed to suppress two different messages.  (Though if that
 > > happens then maybe we should have a slightly different approach based on
 > > bitmasks.)
 > > 
 > 
 > I agree with you that one flag should correspond to a notification.  I
 > revised my patch a little bit to get rid of suppression flag
 > 'var_assign'.

Hi.

For my own education, is this suppression just an optimization, or is there a correctness issue here?
I can imagine that it's an optimization, why notify the frontend something changed when it's the frontend that requested the change.
But there is *zero* documentation in mi-main.h on *why* struct mi_suppress_notification exists, so it's hard to tell. :-(
[I realize your patch is just adding an entry, but I'd like to learn what the reason for it is.]

Re: [PATCH 1/2] new memory-changed MI notification.

[Tom Tromey]

>>>>> "Doug" == Douglas Evans <dje@google.com> writes:

Doug> For my own education, is this suppression just an optimization, or is
Doug> there a correctness issue here?
Doug> I can imagine that it's an optimization, why notify the frontend
Doug> something changed when it's the frontend that requested the change.
Doug> But there is *zero* documentation in mi-main.h on *why* struct
Doug> mi_suppress_notification exists, so it's hard to tell. :-(
Doug> [I realize your patch is just adding an entry, but I'd like to learn
Doug> what the reason for it is.]

I don't know about this case in particular, but in many cases in MI, a
command will have a result record that encodes the exact same data that
an async response would give.  On the other hand, if a CLI command is
invoked, then there is no relevant response record, and so the async
response must be generated.

I think it is an optimization.  Offhand I can't think of a correctness
issue here, but I guess I wouldn't be surprised if there is one lurking
for some command or another.

Tom

Re: [PATCH 1/2] new memory-changed MI notification.

[Yao Qi]

On 10/16/2012 01:58 AM, dje@google.com wrote:
> For my own education, is this suppression just an optimization, or is there a correctness issue here?
> I can imagine that it's an optimization, why notify the frontend something changed when it's the frontend that requested the change.

The notifications are designed to tell frontend something about the 
changes of GDB states which frontend is not aware of.  If the changes 
are requested from fronend, so we think frontend should be aware of 
these changes, and then notifications are not necessary to be sent.

> But there is*zero*  documentation in mi-main.h on*why*  struct mi_suppress_notification exists, so it's hard to tell.:-(
> [I realize your patch is just adding an entry, but I'd like to learn what the reason for it is.]

It needs some comments here.  I'll document it shortly.

-- 
Yao

Re: [PATCH 1/2] new memory-changed MI notification.

[Tom Tromey]

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

Tom> Usually I think it would be preferable to have a flag correspond to a
Tom> notification and not a command; but this would not work so well if a
Tom> command needed to suppress two different messages.  (Though if that
Tom> happens then maybe we should have a slightly different approach based on
Tom> bitmasks.)

Yao> I agree with you that one flag should correspond to a notification.  I
Yao> revised my patch a little bit to get rid of suppression flag
Yao> 'var_assign'.

Funny -- your previous message got me to agree that the bitmask approach
is overkill :)

Yao> 2012-10-09  Yao Qi  <yao@codesourcery.com>
Yao> 	* breakpoint.c (invalidate_bp_value_on_memory_change): Add one
Yao> 	more parameter 'inferior'.
Yao> 	* corefile.c (write_memory_with_notification): Caller update.
Yao> 	* mi/mi-cmd-var.c: Include "mi-main.h".
Yao> 	(mi_cmd_var_assign): Set mi_suppress_notification.data_write_memory
Yao> 	to 1 and restore it later.
Yao> 	* mi/mi-cmds.c (mi_cmd mi_cmds): Update for "data-write-memory"
Yao> 	and "data-write-memory-bytes.
Yao> 	* mi/mi-interp.c: Include objfiles.h.
Yao> 	(mi_interpreter_init): Call observer_attach_memory_changed.
Yao> 	(mi_memory_changed): New.
Yao> 	* mi/mi-main.h (struct mi_suppress_notification) <memory>:
Yao> 	New field.

Either version of the patch is ok.  Check in whichever one you prefer.

Tom

[PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

Hi,
This patch is to add a new MI notification to MI front-end when
tracepoints are downloaded to target.

gdb:

2012-09-27  Yao Qi  <yao@codesourcery.com>

	* target.c: Include "observer.h".
	(target_download_tracepoint): New.
	* target.h (target_download_tracepoint): Remoe macro.
	Declare target_download_tracepoint.
	* mi/mi-interp.c (mi_interpreter_init):
	(mi_tracepoint_downloaded): New.
	* observer.sh (struct bp_location): Forward declaration.

	* NEWS: Mention it.

gdb/doc:

2012-09-27  Yao Qi  <yao@codesourcery.com>

	* observer.texi (GDB Observers): New observer
	'tracepoint-downloaded'.
	* gdb.texinfo (GDB/MI Async Records): Document for MI notification
	"=tracepoint-downloaded".

gdb/testsuite:

2012-09-27  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/mi-traceframe-changed.exp (test_tfind_remote): Adjust.
	* gdb.trace/mi-tracepoint-downloaded.exp: New.
---
 gdb/NEWS                                           |    2 +
 gdb/doc/gdb.texinfo                                |    5 +
 gdb/doc/observer.texi                              |    4 +
 gdb/mi/mi-interp.c                                 |   18 +++
 gdb/observer.sh                                    |    1 +
 gdb/target.c                                       |    8 ++
 gdb/target.h                                       |    3 +-
 gdb/testsuite/gdb.trace/mi-traceframe-changed.exp  |    5 +-
 .../gdb.trace/mi-tracepoint-downloaded.exp         |  120 ++++++++++++++++++++
 9 files changed, 162 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 798f050..edff7b6 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -59,6 +59,8 @@ py [command]
      async record "=record-started" and "=record-stopped".
   ** Memory changes are now notified using new async record
      "=memory-changed".
+  ** Download of tracepoints are now notified using new async record
+     "=tracepoint-downloaded".
 
 *** Changes in GDB 7.5
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index fa925d7..19fcd13 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27658,6 +27658,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-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
+address where tracepoint was downloaded.
+
 @item =record-started,thread-group="@var{id}"
 @itemx =record-stopped,thread-group="@var{id}"
 Execution log recording was either started or stopped on an
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 106475b..9fd92fb 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -194,6 +194,10 @@ A tracepoint has been modified in some way.  The argument @var{tpnum}
 is the number of the modified tracepoint.
 @end deftypefun
 
+@deftypefun void tracepoint_downloaded (struct bp_location *@var{loc})
+A tracepoint location @var{loc} has been downloaded.
+@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-interp.c b/gdb/mi/mi-interp.c
index e58d9e0..6e712ea 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_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,
 			       ssize_t len, const bfd_byte *myaddr);
@@ -140,6 +141,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_downloaded (mi_tracepoint_downloaded);
       observer_attach_command_param_changed (mi_command_param_changed);
       observer_attach_memory_changed (mi_memory_changed);
 
@@ -681,6 +683,22 @@ mi_breakpoint_modified (struct breakpoint *b)
   gdb_flush (mi->event_channel);
 }
 
+/* Emit notification about downloaded tracepoint.  */
+
+static void
+mi_tracepoint_downloaded (struct bp_location *loc)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+
+  target_terminal_ours ();
+
+  fprintf_unfiltered (mi->event_channel,
+		      "tracepoint-downloaded,id=\"%d\",address=\"%s\"\n",
+		      loc->owner->number, core_addr_to_string (loc->address));
+
+  gdb_flush (mi->event_channel);
+}
+
 static int
 mi_output_running_pid (struct thread_info *info, void *arg)
 {
diff --git a/gdb/observer.sh b/gdb/observer.sh
index c98afd0..3df6578 100755
--- a/gdb/observer.sh
+++ b/gdb/observer.sh
@@ -64,6 +64,7 @@ struct so_list;
 struct objfile;
 struct thread_info;
 struct inferior;
+struct bp_location;
 EOF
         ;;
 esac
diff --git a/gdb/target.c b/gdb/target.c
index 1fc8802..0fbc60f 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -43,6 +43,7 @@
 #include "tracepoint.h"
 #include "gdb/fileio.h"
 #include "agent.h"
+#include "observer.h"
 
 static void target_info (char *, int);
 
@@ -384,6 +385,13 @@ target_has_execution_current (void)
   return target_has_execution_1 (inferior_ptid);
 }
 
+void
+target_download_tracepoint (struct bp_location *loc)
+{
+  (*current_target.to_download_tracepoint) (loc);
+  observer_notify_tracepoint_downloaded (loc);
+}
+
 /* Add a possible target architecture to the list.  */
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index 382dacb..8a2c411 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1650,8 +1650,7 @@ extern char *target_fileio_read_stralloc (const char *filename);
 #define target_trace_init() \
   (*current_target.to_trace_init) ()
 
-#define target_download_tracepoint(t) \
-  (*current_target.to_download_tracepoint) (t)
+extern void target_download_tracepoint (struct bp_location *t);
 
 #define target_can_download_tracepoint() \
   (*current_target.to_can_download_tracepoint) ()
diff --git a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
index b587d10..c1f9e2b 100644
--- a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
+++ b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
@@ -100,7 +100,7 @@ if ![gdb_target_supports_trace] {
 gdb_exit
 
 proc test_tfind_remote { } { with_test_prefix "remote" {
-    global decimal
+    global decimal hex
 
     if [mi_gdb_start] {
 	return
@@ -109,7 +109,8 @@ proc test_tfind_remote { } { with_test_prefix "remote" {
 
     mi_gdb_test "-break-insert end" "\\^done.*" "break end"
     mi_gdb_test "-break-insert -a func2" "\\^done.*" "break func2"
-    mi_gdb_test "-trace-start" "\\^done.*" "trace start"
+    mi_gdb_test "-trace-start" ".*=tracepoint-downloaded,id=\"${decimal}\",address=\"${hex}\".*\\^done.*" \
+	"trace start"
 
     mi_execute_to "exec-continue" "breakpoint-hit" end "" ".*" ".*" \
 	{ "" "disp=\"keep\"" } \
diff --git a/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp b/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp
new file mode 100644
index 0000000..97bb2bb
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp
@@ -0,0 +1,120 @@
+# Copyright 2012 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/>.
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+load_lib trace-support.exp
+load_lib mi-support.exp
+
+standard_testfile change-loc.c
+set libfile1 "change-loc-1"
+set libfile2 "change-loc-2"
+set executable $testfile
+set libsrc1 $srcdir/$subdir/$libfile1.c
+set libsrc2 $srcdir/$subdir/$libfile2.c
+set lib_sl1 [standard_output_file $libfile1.sl]
+set lib_sl2 [standard_output_file $libfile2.sl]
+
+set lib_opts debug
+
+if [get_compiler_info] {
+    return -1
+}
+
+# Some targets have leading underscores on assembly symbols.
+set additional_flags [list debug shlib=$lib_sl1 shlib_load [gdb_target_symbol_prefix_flags]]
+
+if { [gdb_compile_shlib $libsrc1 $lib_sl1 $lib_opts] != ""
+     || [gdb_compile_shlib $libsrc2 $lib_sl2 $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags] != ""} {
+    untested "Could not compile either $libsrc1 or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+clean_restart $executable
+
+gdb_load_shlibs $lib_sl1
+gdb_load_shlibs $lib_sl2
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "Current target does not support trace"
+    return -1;
+}
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+mi_load_shlibs $lib_sl1 $lib_sl2
+
+mi_gdb_test "-break-insert main" {.*\^done,bkpt=.*} \
+    "insert breakpoint on main"
+mi_gdb_test "-break-insert marker" {.*\^done,bkpt=.*} \
+    "insert breakpoint on marker"
+mi_gdb_test "-break-insert -a main" {.*\^done,bkpt=.*} \
+    "insert tracepoint on main"
+# Insert a tracepoint of two different locations.
+mi_gdb_test "-break-insert -a -f set_tracepoint" {.*\^done,bkpt=.*} \
+    "insert tracepoint on set_tracepoint"
+
+mi_run_cmd
+mi_expect_stop "breakpoint-hit" "main" ""\
+    ".*" ".*" {"" "disp=\"keep\""} \
+    "continue to main breakpoint"
+
+# Two tracepoints (three locations) are downloaded.
+mi_gdb_test "-trace-start" \
+    "=tracepoint-downloaded,id=\"3\",address=\"${hex}\".*=tracepoint-downloaded,id=\"4\".*=tracepoint-downloaded,id=\"4\".*\\^done" \
+    "trace start"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 1"
+
+mi_gdb_test "-break-insert -a -f func2" {.*\^done,bkpt=.*} \
+    "insert tracepoint on func2"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+set test "pending tracepoint downloaded"
+gdb_expect {
+    -re ".*=tracepoint-downloaded,id=\"4\",address=\"${hex}\"" {
+	pass "$test: 4"
+	exp_continue
+    }
+    -re ".*=tracepoint-downloaded,id=\"5\",address=\"${hex}\"" {
+	pass "$test: 5"
+    }
+    -re ".*${mi_gdb_prompt}$" {
+	fail $test
+    }
+    timeout {
+	fail "$test (timeout)"
+    }
+}
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 2"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 3"
-- 
1.7.7.6

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Fri, 28 Sep 2012 08:49:07 +0800
> 
> Hi,
> This patch is to add a new MI notification to MI front-end when
> tracepoints are downloaded to target.

The documentation parts are OK.  Thanks.

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

On 09/28/2012 01:49 AM, Yao Qi wrote:
> +		      loc->owner->number, core_addr_to_string (loc->address));

BTW, paddress would avoid the leading 0s on 64-bit host, 32-bit target.

-- 
Pedro Alves

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

On 09/28/2012 01:49 AM, Yao Qi wrote:
> +@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
> +address where tracepoint was downloaded.

The "address where tracepoint was downloaded" makes me think this
returns the address in gdbserver's memory that holds the tracepoint
object.  But it's not, it's the tracepoint's address, as in the
address the tracepoint is set at in the inferior.

Took me a second to recall, but the reason the address is
necessary is multi-location tracepoints -- a tracepoint on the
target is identified by the { number, address } tuple.  We don't
send over the location's sub number (like 1.1, 1.2, etc.).

Should we mention this somewhere (other than at the tracepoint
packets description), so frontend people don't wonder whether they
can ignore the address field, and why aren't the other fields of
the tracepoint (like spec string) included?

-- 
Pedro Alves

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

On 09/28/2012 06:44 PM, Pedro Alves wrote:
> On 09/28/2012 01:49 AM, Yao Qi wrote:
>> +@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
>> +address where tracepoint was downloaded.
> 
> The "address where tracepoint was downloaded" makes me think this
> returns the address in gdbserver's memory that holds the tracepoint
> object.  But it's not, it's the tracepoint's address, as in the
> address the tracepoint is set at in the inferior.
> 
> Took me a second to recall, but the reason the address is
> necessary is multi-location tracepoints -- a tracepoint on the
> target is identified by the { number, address } tuple.  We don't
> send over the location's sub number (like 1.1, 1.2, etc.).
> 
> Should we mention this somewhere (other than at the tracepoint
> packets description), so frontend people don't wonder whether they
> can ignore the address field, and why aren't the other fields of
> the tracepoint (like spec string) included?

And I guess the related question is, are frontends interested
in { number, address }, which is target side detail, or on
{ number, location number }, which is how other breakpoints are
presented to the frontend?  I would think the latter?

-- 
Pedro Alves

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

On 09/29/2012 01:46 AM, Pedro Alves wrote:
>> Took me a second to recall, but the reason the address is
>> >necessary is multi-location tracepoints -- a tracepoint on the
>> >target is identified by the { number, address } tuple.  We don't
>> >send over the location's sub number (like 1.1, 1.2, etc.).
>> >
>> >Should we mention this somewhere (other than at the tracepoint
>> >packets description), so frontend people don't wonder whether they
>> >can ignore the address field, and why aren't the other fields of
>> >the tracepoint (like spec string) included?
> And I guess the related question is, are frontends interested
> in { number, address }, which is target side detail, or on
> { number, location number }, which is how other breakpoints are
> presented to the frontend?  I would think the latter?

 From the frontend's point of view, {number, location number} is better, 
and the schema "number.location_number" has been used in 
"=breakpoint-modified" notification.  However, if we want to use 
{number, location number} here, we have to guarantee that the location 
number is an attribute of bp_location, because:

Nowadays, location number is generated by incrementing a counter during 
iterating a list of bp_location of breakpoint (in 
breakpoint.c:print_one_breakpoint), so I am wondering that the 
bp_location object may have the different number, if the list of 
bp_locations of a breakpoint is removed due to some reasons.  Looks 
bp_location list of breakpoint is *not* removed except in 
breakpoint_program_space_exit, after examine the source code.

For example,

Originally we have a tracepoint of 3 locations,

4       tracepoint     keep y   <MULTIPLE>
         collect $eip^M
4.1                         y     0x0804859c in func4 inf 1
4.2                         y     0xb7ffc480 in func4 inf 2
4.3                         y     0xb7ffc488 in func4 inf 1

due to some reason, bp_location on address 0xb7ffc480 is removed (for 
example, inferior 2 is removed), and original bp_location 4.3 becomes 4.2.

In short, if we can make location number persistent (unchanged for a 
given bp_location object), then {number, location number} is fine, 
otherwise, I'd prefer {number, address}.

-- 
Yao

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

On 09/29/2012 10:13 PM, Yao Qi wrote:
> Nowadays, location number is generated by incrementing a counter during
> iterating a list of bp_location of breakpoint (in
> breakpoint.c:print_one_breakpoint), so I am wondering that the
> bp_location object may have the different number, if the list of
> bp_locations of a breakpoint is removed due to some reasons.  Looks
> bp_location list of breakpoint is *not* removed except in
> breakpoint_program_space_exit, after examine the source code.
>
> For example,
>
> Originally we have a tracepoint of 3 locations,
>
> 4       tracepoint     keep y   <MULTIPLE>
>          collect $eip^M
> 4.1                         y     0x0804859c in func4 inf 1
> 4.2                         y     0xb7ffc480 in func4 inf 2
> 4.3                         y     0xb7ffc488 in func4 inf 1
>
> due to some reason, bp_location on address 0xb7ffc480 is removed (for
> example, inferior 2 is removed), and original bp_location 4.3 becomes 4.2.
>
> In short, if we can make location number persistent (unchanged for a
> given bp_location object), then {number, location number} is fine,
> otherwise, I'd prefer {number, address}.

On the other hand, if 'adding bp_location number of a breakpoint' can be 
justified, I'd like to do that, and use bp_location number instead of 
address here.  However, I am not sure whether we need add bp_location 
number.  It is slightly overkill to do so.  Any thoughts?

-- 
Yao

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

On 09/29/2012 03:13 PM, Yao Qi wrote:
> On 09/29/2012 01:46 AM, Pedro Alves wrote:
>>> Took me a second to recall, but the reason the address is
>>> >necessary is multi-location tracepoints -- a tracepoint on the
>>> >target is identified by the { number, address } tuple.  We don't
>>> >send over the location's sub number (like 1.1, 1.2, etc.).
>>> >
>>> >Should we mention this somewhere (other than at the tracepoint
>>> >packets description), so frontend people don't wonder whether they
>>> >can ignore the address field, and why aren't the other fields of
>>> >the tracepoint (like spec string) included?
>> And I guess the related question is, are frontends interested
>> in { number, address }, which is target side detail, or on
>> { number, location number }, which is how other breakpoints are
>> presented to the frontend?  I would think the latter?
> 
> From the frontend's point of view, {number, location number} is better, and the schema "number.location_number" has been used in "=breakpoint-modified" notification.  However, if we want to use {number, location number} here, we have to guarantee that the location number is an attribute of bp_location, because:

This is not a particular issue of tracepoint locations, so, if it was a problem, it
would be a problem for the existing notifications and MI commands as well.

IOW, this would need to be fixed for all those other cases that expose location
numbers, not just come up with an ad hoc solution.

IOW, there's no good justification for deviating this notification from
existing practice.

> 
> Nowadays, location number is generated by incrementing a counter during iterating 
> a list of bp_location of breakpoint (in breakpoint.c:print_one_breakpoint), so
> I am wondering that the bp_location object may have the different number,
> if the list of bp_locations of a breakpoint is removed due to some reasons.
> Looks bp_location list of breakpoint is *not* removed except in
> breakpoint_program_space_exit, after examine the source code.


> 
> For example,
> 
> Originally we have a tracepoint of 3 locations,
> 
> 4       tracepoint     keep y   <MULTIPLE>
>         collect $eip^M
> 4.1                         y     0x0804859c in func4 inf 1
> 4.2                         y     0xb7ffc480 in func4 inf 2
> 4.3                         y     0xb7ffc488 in func4 inf 1
> 
> due to some reason, bp_location on address 0xb7ffc480 is removed (for example, inferior 2 is removed), and original bp_location 4.3 becomes 4.2.

(BTW, I tried it and looks to be broken - the location of the removed
inferior is left behind, but with "inf 2" removed...)

Locations also come and go with shared libraries being loaded and unloaded,
along with other similar cases of loading and unloading symbols from GDB.

In the case above, the frontend gets notified of the inferior exiting.  It
feels like it should be notified that the breakpoint has been modified, but
that doesn't appear to trigger presently.

> In short, if we can make location number persistent (unchanged for a given bp_location object), then {number, location number} is fine, otherwise, I'd prefer {number, address}.

{number, address} may looks stabler, but not even that is completely stable.
Change the program's code a little, recompile, reload symbols, and you'll see the
address change.  Whatever smarts you'd want the frontend to have to track
locations, GDB could do instead, once for all frontends, and better, because
it may use properties other than the address to decided whether a new instance
of a location is logically the same as the old one (think of stap probes or static
tracepoints, where the address is really a very low target side detail; or just
consider line number instead of address).  I think all this goes hand in hand with
redesigning how GDB handles breakpoint re-setting (or rather, get rid of re-setting as is),
which will make this whole stabler locations issue better.  Keith will be going to
work on that very soon, IIUC.

-- 
Pedro Alves

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Doug Evans]

On Wed, Oct 31, 2012 at 10:59 AM, Pedro Alves <palves@redhat.com> wrote:
> [...] I think all this goes hand in hand with
> redesigning how GDB handles breakpoint re-setting (or rather, get rid of re-setting as is),
> which will make this whole stabler locations issue better.  Keith will be going to
> work on that very soon, IIUC.

I'm looking forward to that. :-)

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

On 11/01/2012 01:59 AM, Pedro Alves wrote:
>>  From the frontend's point of view, {number, location number} is better, and the schema "number.location_number" has been used in "=breakpoint-modified" notification.  However, if we want to use {number, location number} here, we have to guarantee that the location number is an attribute of bp_location, because:
> This is not a particular issue of tracepoint locations, so, if it was a problem, it
> would be a problem for the existing notifications and MI commands as well.
>
> IOW, this would need to be fixed for all those other cases that expose location
> numbers, not just come up with an ad hoc solution.
>

I agree.

> IOW, there's no good justification for deviating this notification from
> existing practice.
>

What do you mean by "existing practice"?  Is {number, location number} 
the "existing practise"?

>> >In short, if we can make location number persistent (unchanged for a given bp_location object), then {number, location number} is fine, otherwise, I'd prefer {number, address}.
> {number, address} may looks stabler, but not even that is completely stable.
> Change the program's code a little, recompile, reload symbols, and you'll see the
> address change.  Whatever smarts you'd want the frontend to have to track
> locations, GDB could do instead, once for all frontends, and better, because
   ^^^^^^^^^
IIUC, "locations" here mean location address instead of location number.

> it may use properties other than the address to decided whether a new instance
> of a location is logically the same as the old one (think of stap probes or static
> tracepoints, where the address is really a very low target side detail; or just
> consider line number instead of address).  I think all this goes hand in hand with
> redesigning how GDB handles breakpoint re-setting (or rather, get rid of re-setting as is),
> which will make this whole stabler locations issue better.  Keith will be going to
> work on that very soon, IIUC.

FAOD, we should continue to use {number, location number}, and it 
requires improvement of breakpoint re-setting, correct?

-- 
Yao

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

On 11/01/2012 12:34 AM, Yao Qi wrote:
> On 11/01/2012 01:59 AM, Pedro Alves wrote:
>>>  From the frontend's point of view, {number, location number} is better, and the schema "number.location_number" has been used in "=breakpoint-modified" notification.  However, if we want to use {number, location number} here, we have to guarantee that the location number is an attribute of bp_location, because:
>> This is not a particular issue of tracepoint locations, so, if it was a problem, it
>> would be a problem for the existing notifications and MI commands as well.
>>
>> IOW, this would need to be fixed for all those other cases that expose location
>> numbers, not just come up with an ad hoc solution.
>>
> 
> I agree.
> 
>> IOW, there's no good justification for deviating this notification from
>> existing practice.
>>
> 
> What do you mean by "existing practice"?  Is {number, location number} the "existing practise"?

Almost.  AFAICS, the existing practice is "bp_number.loc_number" in MI, just like in the CLI.

&"b main\n"
~"Breakpoint 8 at 0x457aab: main. (2 locations)\n"
=breakpoint-created,bkpt={number="8",type="breakpoint",disp="keep",enabled="y",addr="<MULTIPLE>",times="0",original-location="main"},{number="8.1",enabled="y",addr="0x0000000000457aab",func="main",file="../../src/gdb/gdb.c",fullname="/home/pedro/gdb/mygit/src/gdb/gdb.c",line="29"},{number="8.2",enabled="y",addr="0x0000000000457aab",func="main",file="../../src/gdb/gdb.c",fullname="/home/pedro/gdb/mygit/src/gdb/gdb.c",line="29"}
^done

Note 'number="8.1"'.

Or,

(gdb)
-break-disable 9.1
^done
(gdb)
info breakpoint
&"info breakpoint\n"
~"Num     Type           Disp Enb Address            What\n"
~"9       tracepoint     keep y   <MULTIPLE>         \n"
~"9.1                         n     0x0000000000457aab in main at ../../src/gdb/gdb.c:29 inf 1\n"
~"9.2                         y     0x0000000000457aab in main at ../../src/gdb/gdb.c:29 inf 2\n"

Looks like a =breakpoint-modified notification is missing for this, BTW:

disable 8.1
&"disable 8.1\n"
^done
(gdb)

While this emitted the notification:

disable 8
&"disable 8\n"
=breakpoint-modified,bkpt={number="8",type="breakpoint",disp="keep",enabled="n",addr="<MULTIPLE>",times="0",original-location="main"},{number="8.1",enabled="n",addr="0x0000000000457aab",func="main",file="../../src/gdb/gdb.c",fullname="/home/pedro/gdb/mygit/src/gdb/gdb.c",line="29"},{number="8.2",enabled="y",addr="0x0000000000457aab",func="main",file="../../src/gdb/gdb.c",fullname="/home/pedro/gdb/mygit/src/gdb/gdb.c",line="29"}
^done
(gdb)

>>> >In short, if we can make location number persistent (unchanged for a given bp_location object), then {number, location number} is fine, otherwise, I'd prefer {number, address}.
>> {number, address} may looks stabler, but not even that is completely stable.
>> Change the program's code a little, recompile, reload symbols, and you'll see the
>> address change.  Whatever smarts you'd want the frontend to have to track
>> locations, GDB could do instead, once for all frontends, and better, because
>   ^^^^^^^^^
> IIUC, "locations" here mean location address instead of location number.

I mean location as the abstract location a bp_location represents.  By track, I mean,
recognize that a given location is the same as another location (remember that
catchpoints often don't even have a concept of address!).

> 
>> it may use properties other than the address to decided whether a new instance
>> of a location is logically the same as the old one (think of stap probes or static
>> tracepoints, where the address is really a very low target side detail; or just
>> consider line number instead of address).  I think all this goes hand in hand with
>> redesigning how GDB handles breakpoint re-setting (or rather, get rid of re-setting as is),
>> which will make this whole stabler locations issue better.  Keith will be going to
>> work on that very soon, IIUC.
> 
> FAOD, we should continue to use {number, location number}, and it requires improvement of breakpoint re-setting, correct?

Yes.  Though AFAICS, current notifications and commands use 'number="N.M"', not '{number, location number}'.

-- 
Pedro Alves

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

On 11/02/2012 03:46 PM, Pedro Alves wrote:
>> > FAOD, we should continue to use {number, location number}, and it requires improvement of breakpoint re-setting, correct?
> Yes.  Though AFAICS, current notifications and commands use 'number="N.M"', not '{number, location number}'.

And FAOD, The "yes" was for the first question.  I'm not sure what you meant by
"it", but the notification should not be blocked by that.

Let me grok the responses on the usefulness of the notification though (and
continue the discussion there, if necessary).

-- 
Pedro Alves

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

On 09/29/2012 01:44 AM, Pedro Alves wrote:
> The "address where tracepoint was downloaded" makes me think this
> returns the address in gdbserver's memory that holds the tracepoint
> object.  But it's not, it's the tracepoint's address, as in the
> address the tracepoint is set at in the inferior.

Hi Pedro,
This sentence is revised to "address where tracepoint was set at in the
inferior".

> 
> Took me a second to recall, but the reason the address is
> necessary is multi-location tracepoints -- a tracepoint on the
> target is identified by the { number, address } tuple.  We don't
> send over the location's sub number (like 1.1, 1.2, etc.).

I sent two notes on this, but didn't hear back.  Not sure you still
insist on "{number, loc_number}" pair.

  http://sourceware.org/ml/gdb-patches/2012-09/msg00704.html
  http://sourceware.org/ml/gdb-patches/2012-10/msg00148.html

> 
> Should we mention this somewhere (other than at the tracepoint
> packets description), so frontend people don't wonder whether they
> can ignore the address field, and why aren't the other fields of
> the tracepoint (like spec string) included?

Well, I am not sure such explanation should go into manual.  It is
related to the design and internals, and it may be confusing to users
to explain it in manual.  IMO, manual is about "what" instead of "why".

On 10/16/2012 02:03 AM, dje@google.com wrote:> Hi.
> It would be useful if the reason why this notification exists was specified in the code.
> E.g, "This notification exists because frontends ... [fill in the blank]."

More comments are added to function mi_tracepoint_downloaded to address
this.

-- 
Yao

gdb:

2012-10-18  Yao Qi  <yao@codesourcery.com>

	* target.c: Include "observer.h".
	(target_download_tracepoint): New.
	* target.h (target_download_tracepoint): Remoe macro.
	Declare target_download_tracepoint.
	* mi/mi-interp.c (mi_interpreter_init):
	(mi_tracepoint_downloaded): New.
	* observer.sh (struct bp_location): Forward declaration.

	* NEWS: Mention it.

gdb/doc:

2012-10-18  Yao Qi  <yao@codesourcery.com>

	* observer.texi (GDB Observers): New observer
	'tracepoint-downloaded'.
	* gdb.texinfo (GDB/MI Async Records): Document for MI notification
	"=tracepoint-downloaded".

gdb/testsuite:

2012-10-18  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/mi-traceframe-changed.exp (test_tfind_remote): Adjust.
	* gdb.trace/mi-tracepoint-downloaded.exp: New.
---
 gdb/NEWS                                           |    2 +
 gdb/doc/gdb.texinfo                                |    5 +
 gdb/doc/observer.texi                              |    4 +
 gdb/mi/mi-interp.c                                 |   22 ++++
 gdb/observer.sh                                    |    1 +
 gdb/target.c                                       |    8 ++
 gdb/target.h                                       |    3 +-
 gdb/testsuite/gdb.trace/mi-traceframe-changed.exp  |    5 +-
 .../gdb.trace/mi-tracepoint-downloaded.exp         |  120 ++++++++++++++++++++
 9 files changed, 166 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 798f050..edff7b6 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -59,6 +59,8 @@ py [command]
      async record "=record-started" and "=record-stopped".
   ** Memory changes are now notified using new async record
      "=memory-changed".
+  ** Download of tracepoints are now notified using new async record
+     "=tracepoint-downloaded".
 
 *** Changes in GDB 7.5
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 19d0868..94df41f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27658,6 +27658,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-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
+address where tracepoint was set at in the inferior.
+
 @item =record-started,thread-group="@var{id}"
 @itemx =record-stopped,thread-group="@var{id}"
 Execution log recording was either started or stopped on an
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 106475b..9fd92fb 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -194,6 +194,10 @@ A tracepoint has been modified in some way.  The argument @var{tpnum}
 is the number of the modified tracepoint.
 @end deftypefun
 
+@deftypefun void tracepoint_downloaded (struct bp_location *@var{loc})
+A tracepoint location @var{loc} has been downloaded.
+@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-interp.c b/gdb/mi/mi-interp.c
index d3c3d81..e226223 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -76,6 +76,8 @@ 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,
 			       ssize_t len, const bfd_byte *myaddr);
@@ -140,6 +142,8 @@ 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);
 
@@ -681,6 +685,24 @@ mi_breakpoint_modified (struct breakpoint *b)
   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.  */
+
+static void
+mi_tracepoint_downloaded (struct bp_location *loc)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+
+  target_terminal_ours ();
+
+  fprintf_unfiltered (mi->event_channel,
+		      "tracepoint-downloaded,id=\"%d\",address=\"%s\"\n",
+		      loc->owner->number, core_addr_to_string (loc->address));
+
+  gdb_flush (mi->event_channel);
+}
+
 static int
 mi_output_running_pid (struct thread_info *info, void *arg)
 {
diff --git a/gdb/observer.sh b/gdb/observer.sh
index c98afd0..3df6578 100755
--- a/gdb/observer.sh
+++ b/gdb/observer.sh
@@ -64,6 +64,7 @@ struct so_list;
 struct objfile;
 struct thread_info;
 struct inferior;
+struct bp_location;
 EOF
         ;;
 esac
diff --git a/gdb/target.c b/gdb/target.c
index 861e6a6..3791aef 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -43,6 +43,7 @@
 #include "tracepoint.h"
 #include "gdb/fileio.h"
 #include "agent.h"
+#include "observer.h"
 
 static void target_info (char *, int);
 
@@ -384,6 +385,13 @@ target_has_execution_current (void)
   return target_has_execution_1 (inferior_ptid);
 }
 
+void
+target_download_tracepoint (struct bp_location *loc)
+{
+  (*current_target.to_download_tracepoint) (loc);
+  observer_notify_tracepoint_downloaded (loc);
+}
+
 /* Add a possible target architecture to the list.  */
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index 382dacb..8a2c411 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1650,8 +1650,7 @@ extern char *target_fileio_read_stralloc (const char *filename);
 #define target_trace_init() \
   (*current_target.to_trace_init) ()
 
-#define target_download_tracepoint(t) \
-  (*current_target.to_download_tracepoint) (t)
+extern void target_download_tracepoint (struct bp_location *t);
 
 #define target_can_download_tracepoint() \
   (*current_target.to_can_download_tracepoint) ()
diff --git a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
index b587d10..c1f9e2b 100644
--- a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
+++ b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
@@ -100,7 +100,7 @@ if ![gdb_target_supports_trace] {
 gdb_exit
 
 proc test_tfind_remote { } { with_test_prefix "remote" {
-    global decimal
+    global decimal hex
 
     if [mi_gdb_start] {
 	return
@@ -109,7 +109,8 @@ proc test_tfind_remote { } { with_test_prefix "remote" {
 
     mi_gdb_test "-break-insert end" "\\^done.*" "break end"
     mi_gdb_test "-break-insert -a func2" "\\^done.*" "break func2"
-    mi_gdb_test "-trace-start" "\\^done.*" "trace start"
+    mi_gdb_test "-trace-start" ".*=tracepoint-downloaded,id=\"${decimal}\",address=\"${hex}\".*\\^done.*" \
+	"trace start"
 
     mi_execute_to "exec-continue" "breakpoint-hit" end "" ".*" ".*" \
 	{ "" "disp=\"keep\"" } \
diff --git a/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp b/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp
new file mode 100644
index 0000000..97bb2bb
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp
@@ -0,0 +1,120 @@
+# Copyright 2012 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/>.
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+load_lib trace-support.exp
+load_lib mi-support.exp
+
+standard_testfile change-loc.c
+set libfile1 "change-loc-1"
+set libfile2 "change-loc-2"
+set executable $testfile
+set libsrc1 $srcdir/$subdir/$libfile1.c
+set libsrc2 $srcdir/$subdir/$libfile2.c
+set lib_sl1 [standard_output_file $libfile1.sl]
+set lib_sl2 [standard_output_file $libfile2.sl]
+
+set lib_opts debug
+
+if [get_compiler_info] {
+    return -1
+}
+
+# Some targets have leading underscores on assembly symbols.
+set additional_flags [list debug shlib=$lib_sl1 shlib_load [gdb_target_symbol_prefix_flags]]
+
+if { [gdb_compile_shlib $libsrc1 $lib_sl1 $lib_opts] != ""
+     || [gdb_compile_shlib $libsrc2 $lib_sl2 $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags] != ""} {
+    untested "Could not compile either $libsrc1 or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+clean_restart $executable
+
+gdb_load_shlibs $lib_sl1
+gdb_load_shlibs $lib_sl2
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "Current target does not support trace"
+    return -1;
+}
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+mi_load_shlibs $lib_sl1 $lib_sl2
+
+mi_gdb_test "-break-insert main" {.*\^done,bkpt=.*} \
+    "insert breakpoint on main"
+mi_gdb_test "-break-insert marker" {.*\^done,bkpt=.*} \
+    "insert breakpoint on marker"
+mi_gdb_test "-break-insert -a main" {.*\^done,bkpt=.*} \
+    "insert tracepoint on main"
+# Insert a tracepoint of two different locations.
+mi_gdb_test "-break-insert -a -f set_tracepoint" {.*\^done,bkpt=.*} \
+    "insert tracepoint on set_tracepoint"
+
+mi_run_cmd
+mi_expect_stop "breakpoint-hit" "main" ""\
+    ".*" ".*" {"" "disp=\"keep\""} \
+    "continue to main breakpoint"
+
+# Two tracepoints (three locations) are downloaded.
+mi_gdb_test "-trace-start" \
+    "=tracepoint-downloaded,id=\"3\",address=\"${hex}\".*=tracepoint-downloaded,id=\"4\".*=tracepoint-downloaded,id=\"4\".*\\^done" \
+    "trace start"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 1"
+
+mi_gdb_test "-break-insert -a -f func2" {.*\^done,bkpt=.*} \
+    "insert tracepoint on func2"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+set test "pending tracepoint downloaded"
+gdb_expect {
+    -re ".*=tracepoint-downloaded,id=\"4\",address=\"${hex}\"" {
+	pass "$test: 4"
+	exp_continue
+    }
+    -re ".*=tracepoint-downloaded,id=\"5\",address=\"${hex}\"" {
+	pass "$test: 5"
+    }
+    -re ".*${mi_gdb_prompt}$" {
+	fail $test
+    }
+    timeout {
+	fail "$test (timeout)"
+    }
+}
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 2"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 3"
-- 
1.7.7.6

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

On 10/18/2012 09:16 AM, Yao Qi wrote:
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -76,6 +76,8 @@ 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);

I made a mistake when splitting patches.  Here is the updated patch.
Sorry.

-- 
Yao

gdb:

2012-10-18  Yao Qi  <yao@codesourcery.com>

	* target.c: Include "observer.h".
	(target_download_tracepoint): New.
	* target.h (target_download_tracepoint): Remoe macro.
	Declare target_download_tracepoint.
	* mi/mi-interp.c (mi_interpreter_init):
	(mi_tracepoint_downloaded): New.
	* observer.sh (struct bp_location): Forward declaration.

	* NEWS: Mention it.

gdb/doc:

2012-10-18  Yao Qi  <yao@codesourcery.com>

	* observer.texi (GDB Observers): New observer
	'tracepoint-downloaded'.
	* gdb.texinfo (GDB/MI Async Records): Document for MI
	notification "=tracepoint-downloaded".

gdb/testsuite:

2012-10-18  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/mi-traceframe-changed.exp (test_tfind_remote): Adjust.
	* gdb.trace/mi-tracepoint-downloaded.exp: New.
---
 gdb/NEWS                                           |    2 +
 gdb/doc/gdb.texinfo                                |    5 +
 gdb/doc/observer.texi                              |    4 +
 gdb/mi/mi-interp.c                                 |   20 ++++
 gdb/observer.sh                                    |    1 +
 gdb/target.c                                       |    8 ++
 gdb/target.h                                       |    3 +-
 gdb/testsuite/gdb.trace/mi-traceframe-changed.exp  |    5 +-
 .../gdb.trace/mi-tracepoint-downloaded.exp         |  120 ++++++++++++++++++++
 9 files changed, 164 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 798f050..edff7b6 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -59,6 +59,8 @@ py [command]
      async record "=record-started" and "=record-stopped".
   ** Memory changes are now notified using new async record
      "=memory-changed".
+  ** Download of tracepoints are now notified using new async record
+     "=tracepoint-downloaded".
 
 *** Changes in GDB 7.5
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 19d0868..94df41f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27658,6 +27658,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-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
+address where tracepoint was set at in the inferior.
+
 @item =record-started,thread-group="@var{id}"
 @itemx =record-stopped,thread-group="@var{id}"
 Execution log recording was either started or stopped on an
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 106475b..9fd92fb 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -194,6 +194,10 @@ A tracepoint has been modified in some way.  The argument @var{tpnum}
 is the number of the modified tracepoint.
 @end deftypefun
 
+@deftypefun void tracepoint_downloaded (struct bp_location *@var{loc})
+A tracepoint location @var{loc} has been downloaded.
+@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-interp.c b/gdb/mi/mi-interp.c
index d3c3d81..d83d493 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_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,
 			       ssize_t len, const bfd_byte *myaddr);
@@ -140,6 +141,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_downloaded (mi_tracepoint_downloaded);
       observer_attach_command_param_changed (mi_command_param_changed);
       observer_attach_memory_changed (mi_memory_changed);
 
@@ -681,6 +683,24 @@ mi_breakpoint_modified (struct breakpoint *b)
   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.  */
+
+static void
+mi_tracepoint_downloaded (struct bp_location *loc)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+
+  target_terminal_ours ();
+
+  fprintf_unfiltered (mi->event_channel,
+		      "tracepoint-downloaded,id=\"%d\",address=\"%s\"\n",
+		      loc->owner->number, core_addr_to_string (loc->address));
+
+  gdb_flush (mi->event_channel);
+}
+
 static int
 mi_output_running_pid (struct thread_info *info, void *arg)
 {
diff --git a/gdb/observer.sh b/gdb/observer.sh
index c98afd0..3df6578 100755
--- a/gdb/observer.sh
+++ b/gdb/observer.sh
@@ -64,6 +64,7 @@ struct so_list;
 struct objfile;
 struct thread_info;
 struct inferior;
+struct bp_location;
 EOF
         ;;
 esac
diff --git a/gdb/target.c b/gdb/target.c
index 861e6a6..3791aef 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -43,6 +43,7 @@
 #include "tracepoint.h"
 #include "gdb/fileio.h"
 #include "agent.h"
+#include "observer.h"
 
 static void target_info (char *, int);
 
@@ -384,6 +385,13 @@ target_has_execution_current (void)
   return target_has_execution_1 (inferior_ptid);
 }
 
+void
+target_download_tracepoint (struct bp_location *loc)
+{
+  (*current_target.to_download_tracepoint) (loc);
+  observer_notify_tracepoint_downloaded (loc);
+}
+
 /* Add a possible target architecture to the list.  */
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index 382dacb..8a2c411 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1650,8 +1650,7 @@ extern char *target_fileio_read_stralloc (const char *filename);
 #define target_trace_init() \
   (*current_target.to_trace_init) ()
 
-#define target_download_tracepoint(t) \
-  (*current_target.to_download_tracepoint) (t)
+extern void target_download_tracepoint (struct bp_location *t);
 
 #define target_can_download_tracepoint() \
   (*current_target.to_can_download_tracepoint) ()
diff --git a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
index b587d10..c1f9e2b 100644
--- a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
+++ b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
@@ -100,7 +100,7 @@ if ![gdb_target_supports_trace] {
 gdb_exit
 
 proc test_tfind_remote { } { with_test_prefix "remote" {
-    global decimal
+    global decimal hex
 
     if [mi_gdb_start] {
 	return
@@ -109,7 +109,8 @@ proc test_tfind_remote { } { with_test_prefix "remote" {
 
     mi_gdb_test "-break-insert end" "\\^done.*" "break end"
     mi_gdb_test "-break-insert -a func2" "\\^done.*" "break func2"
-    mi_gdb_test "-trace-start" "\\^done.*" "trace start"
+    mi_gdb_test "-trace-start" ".*=tracepoint-downloaded,id=\"${decimal}\",address=\"${hex}\".*\\^done.*" \
+	"trace start"
 
     mi_execute_to "exec-continue" "breakpoint-hit" end "" ".*" ".*" \
 	{ "" "disp=\"keep\"" } \
diff --git a/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp b/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp
new file mode 100644
index 0000000..97bb2bb
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/mi-tracepoint-downloaded.exp
@@ -0,0 +1,120 @@
+# Copyright 2012 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/>.
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+load_lib trace-support.exp
+load_lib mi-support.exp
+
+standard_testfile change-loc.c
+set libfile1 "change-loc-1"
+set libfile2 "change-loc-2"
+set executable $testfile
+set libsrc1 $srcdir/$subdir/$libfile1.c
+set libsrc2 $srcdir/$subdir/$libfile2.c
+set lib_sl1 [standard_output_file $libfile1.sl]
+set lib_sl2 [standard_output_file $libfile2.sl]
+
+set lib_opts debug
+
+if [get_compiler_info] {
+    return -1
+}
+
+# Some targets have leading underscores on assembly symbols.
+set additional_flags [list debug shlib=$lib_sl1 shlib_load [gdb_target_symbol_prefix_flags]]
+
+if { [gdb_compile_shlib $libsrc1 $lib_sl1 $lib_opts] != ""
+     || [gdb_compile_shlib $libsrc2 $lib_sl2 $lib_opts] != ""
+     || [gdb_compile $srcdir/$subdir/$srcfile $binfile executable $additional_flags] != ""} {
+    untested "Could not compile either $libsrc1 or $srcdir/$subdir/$srcfile."
+    return -1
+}
+
+clean_restart $executable
+
+gdb_load_shlibs $lib_sl1
+gdb_load_shlibs $lib_sl2
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "Current target does not support trace"
+    return -1;
+}
+
+gdb_exit
+if [mi_gdb_start] {
+    continue
+}
+mi_gdb_reinitialize_dir $srcdir/$subdir
+mi_gdb_load ${binfile}
+mi_load_shlibs $lib_sl1 $lib_sl2
+
+mi_gdb_test "-break-insert main" {.*\^done,bkpt=.*} \
+    "insert breakpoint on main"
+mi_gdb_test "-break-insert marker" {.*\^done,bkpt=.*} \
+    "insert breakpoint on marker"
+mi_gdb_test "-break-insert -a main" {.*\^done,bkpt=.*} \
+    "insert tracepoint on main"
+# Insert a tracepoint of two different locations.
+mi_gdb_test "-break-insert -a -f set_tracepoint" {.*\^done,bkpt=.*} \
+    "insert tracepoint on set_tracepoint"
+
+mi_run_cmd
+mi_expect_stop "breakpoint-hit" "main" ""\
+    ".*" ".*" {"" "disp=\"keep\""} \
+    "continue to main breakpoint"
+
+# Two tracepoints (three locations) are downloaded.
+mi_gdb_test "-trace-start" \
+    "=tracepoint-downloaded,id=\"3\",address=\"${hex}\".*=tracepoint-downloaded,id=\"4\".*=tracepoint-downloaded,id=\"4\".*\\^done" \
+    "trace start"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 1"
+
+mi_gdb_test "-break-insert -a -f func2" {.*\^done,bkpt=.*} \
+    "insert tracepoint on func2"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+set test "pending tracepoint downloaded"
+gdb_expect {
+    -re ".*=tracepoint-downloaded,id=\"4\",address=\"${hex}\"" {
+	pass "$test: 4"
+	exp_continue
+    }
+    -re ".*=tracepoint-downloaded,id=\"5\",address=\"${hex}\"" {
+	pass "$test: 5"
+    }
+    -re ".*${mi_gdb_prompt}$" {
+	fail $test
+    }
+    timeout {
+	fail "$test (timeout)"
+    }
+}
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 2"
+
+mi_send_resuming_command "exec-continue" "continuing execution to marker"
+mi_expect_stop "breakpoint-hit" "marker" ".*" ".*" ".*" \
+    {"" "disp=\"keep\""} "continue to marker 3"
-- 
1.7.7.6

[ping]: [PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

On 10/18/2012 09:28 AM, Yao Qi wrote:
> gdb:
> 
> 2012-10-18  Yao Qi<yao@codesourcery.com>
> 
> 	* target.c: Include "observer.h".
> 	(target_download_tracepoint): New.
> 	* target.h (target_download_tracepoint): Remove macro.
> 	Declare target_download_tracepoint.
> 	* mi/mi-interp.c (mi_interpreter_init):
> 	(mi_tracepoint_downloaded): New.
> 	* observer.sh (struct bp_location): Forward declaration.
> 
> 	* NEWS: Mention it.
> 
> gdb/doc:
> 
> 2012-10-18  Yao Qi<yao@codesourcery.com>
> 
> 	* observer.texi (GDB Observers): New observer
> 	'tracepoint-downloaded'.
> 	* gdb.texinfo (GDB/MI Async Records): Document for MI
> 	notification "=tracepoint-downloaded".
> 
> gdb/testsuite:
> 
> 2012-10-18  Yao Qi<yao@codesourcery.com>
> 
> 	* gdb.trace/mi-traceframe-changed.exp (test_tfind_remote): Adjust.
> 	* gdb.trace/mi-tracepoint-downloaded.exp: New.

Ping the non-doc part of this patch.
http://sourceware.org/ml/gdb-patches/2012-10/msg00310.html

gdb:

2012-10-18  Yao Qi  <yao@codesourcery.com>

	* target.c: Include "observer.h".
	(target_download_tracepoint): New.
	* target.h (target_download_tracepoint): Remove macro.
	Declare target_download_tracepoint.
	* mi/mi-interp.c (mi_interpreter_init):
	(mi_tracepoint_downloaded): New.
	* observer.sh (struct bp_location): Forward declaration.

	* NEWS: Mention it.

gdb/testsuite:

2012-10-18  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/mi-traceframe-changed.exp (test_tfind_remote): Adjust.
	* gdb.trace/mi-tracepoint-downloaded.exp: New.

-- 
Yao

Re: [ping]: [PATCH 2/2] new tracepoint downloaded MI notification.

[Eli Zaretskii]

> Date: Tue, 30 Oct 2012 15:06:48 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 10/18/2012 09:28 AM, Yao Qi wrote:
> > gdb:
> > 
> > 2012-10-18  Yao Qi<yao@codesourcery.com>
> > 
> > 	* target.c: Include "observer.h".
> > 	(target_download_tracepoint): New.
> > 	* target.h (target_download_tracepoint): Remove macro.
> > 	Declare target_download_tracepoint.
> > 	* mi/mi-interp.c (mi_interpreter_init):
> > 	(mi_tracepoint_downloaded): New.
> > 	* observer.sh (struct bp_location): Forward declaration.
> > 
> > 	* NEWS: Mention it.
> > 
> > gdb/doc:
> > 
> > 2012-10-18  Yao Qi<yao@codesourcery.com>
> > 
> > 	* observer.texi (GDB Observers): New observer
> > 	'tracepoint-downloaded'.
> > 	* gdb.texinfo (GDB/MI Async Records): Document for MI
> > 	notification "=tracepoint-downloaded".
> > 
> > gdb/testsuite:
> > 
> > 2012-10-18  Yao Qi<yao@codesourcery.com>
> > 
> > 	* gdb.trace/mi-traceframe-changed.exp (test_tfind_remote): Adjust.
> > 	* gdb.trace/mi-tracepoint-downloaded.exp: New.
> 
> Ping the non-doc part of this patch.
> http://sourceware.org/ml/gdb-patches/2012-10/msg00310.html

OK.

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Eli Zaretskii]

> Date: Thu, 18 Oct 2012 09:16:15 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> +  ** Download of tracepoints are now notified using new async record
        ^^^^^^^^                ^^^
> +     "=tracepoint-downloaded".

Either "downloads are" or "download is".  I prefer the former.

The documentation parts are OK otherwise.

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Tom Tromey]

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

Yao> 	* target.h (target_download_tracepoint): Remoe macro.

Typo, "remove".

Tom

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Tom Tromey]

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

Yao> 2012-09-27  Yao Qi  <yao@codesourcery.com>
Yao> 	* target.c: Include "observer.h".
Yao> 	(target_download_tracepoint): New.
Yao> 	* target.h (target_download_tracepoint): Remoe macro.
Yao> 	Declare target_download_tracepoint.
Yao> 	* mi/mi-interp.c (mi_interpreter_init):
Yao> 	(mi_tracepoint_downloaded): New.
Yao> 	* observer.sh (struct bp_location): Forward declaration.

Looks good to me.
Ok.

Tom

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Tom Tromey]

Tom> Looks good to me.
Tom> Ok.

Though of course you should listen to Pedro first.
I didn't see his responses before replying.

Tom

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[dje]

Yao Qi writes:
 > Hi,
 > This patch is to add a new MI notification to MI front-end when
 > tracepoints are downloaded to target.
 > 
 > gdb:
 > 
 > 2012-09-27  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* target.c: Include "observer.h".
 > 	(target_download_tracepoint): New.
 > 	* target.h (target_download_tracepoint): Remoe macro.
 > 	Declare target_download_tracepoint.
 > 	* mi/mi-interp.c (mi_interpreter_init):
 > 	(mi_tracepoint_downloaded): New.
 > 	* observer.sh (struct bp_location): Forward declaration.
 > 
 > 	* NEWS: Mention it.
 > 
 > gdb/doc:
 > 
 > 2012-09-27  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* observer.texi (GDB Observers): New observer
 > 	'tracepoint-downloaded'.
 > 	* gdb.texinfo (GDB/MI Async Records): Document for MI notification
 > 	"=tracepoint-downloaded".
 > 
 > gdb/testsuite:
 > 
 > 2012-09-27  Yao Qi  <yao@codesourcery.com>
 > 
 > 	* gdb.trace/mi-traceframe-changed.exp (test_tfind_remote): Adjust.
 > 	* gdb.trace/mi-tracepoint-downloaded.exp: New.

Hi.
It would be useful if the reason why this notification exists was specified in the code.
E.g, "This notification exists because frontends ... [fill in the blank]."

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

On 10/15/2012 07:03 PM, dje@google.com wrote:
> Yao Qi writes:
>  > Hi,
>  > This patch is to add a new MI notification to MI front-end when
>  > tracepoints are downloaded to target.
>  > 
>  > gdb:
>  > 
>  > 2012-09-27  Yao Qi  <yao@codesourcery.com>
>  > 
>  > 	* target.c: Include "observer.h".
>  > 	(target_download_tracepoint): New.
>  > 	* target.h (target_download_tracepoint): Remoe macro.
>  > 	Declare target_download_tracepoint.
>  > 	* mi/mi-interp.c (mi_interpreter_init):
>  > 	(mi_tracepoint_downloaded): New.
>  > 	* observer.sh (struct bp_location): Forward declaration.
>  > 
>  > 	* NEWS: Mention it.
>  > 
>  > gdb/doc:
>  > 
>  > 2012-09-27  Yao Qi  <yao@codesourcery.com>
>  > 
>  > 	* observer.texi (GDB Observers): New observer
>  > 	'tracepoint-downloaded'.
>  > 	* gdb.texinfo (GDB/MI Async Records): Document for MI notification
>  > 	"=tracepoint-downloaded".
>  > 
>  > gdb/testsuite:
>  > 
>  > 2012-09-27  Yao Qi  <yao@codesourcery.com>
>  > 
>  > 	* gdb.trace/mi-traceframe-changed.exp (test_tfind_remote): Adjust.
>  > 	* gdb.trace/mi-tracepoint-downloaded.exp: New.
> 
> Hi.
> It would be useful if the reason why this notification exists was specified in the code.
> E.g, "This notification exists because frontends ... [fill in the blank]."

Yes, indeed.  I'll probably upgrade that "useful" to "required".  :-)
It is not clear for example, why would the frontend
care about a particular tracepoint having been downloaded.  I can see
it wanting to know when a trace run has started from the CLI, for instance,
which already implies that tracepoints have been downloaded.

-- 
Pedro Alves

RE: [PATCH 2/2] new tracepoint downloaded MI notification.

[Marc Khouzam]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Wednesday, October 31, 2012 2:03 PM
> To: dje@google.com
> Cc: Yao Qi; gdb-patches@sourceware.org
> Subject: Re: [PATCH 2/2] new tracepoint downloaded MI notification.
> 
> >  > 2012-09-27  Yao Qi  <yao@codesourcery.com>
> >  > 
> >  > 	* gdb.trace/mi-traceframe-changed.exp 
> (test_tfind_remote): Adjust.
> >  > 	* gdb.trace/mi-tracepoint-downloaded.exp: New.
> > 
> > Hi.
> > It would be useful if the reason why this notification 
> exists was specified in the code.
> > E.g, "This notification exists because frontends ... [fill 
> in the blank]."
> 
> Yes, indeed.  I'll probably upgrade that "useful" to "required".  :-)
> It is not clear for example, why would the frontend
> care about a particular tracepoint having been downloaded.  I can see
> it wanting to know when a trace run has started from the CLI, 
> for instance,
> which already implies that tracepoints have been downloaded.

In older GDB versions, when creating a tracepoint during a trace run,
that tracepoint would not be pushed to the target until the next
trace run.  The idea is that a frontend could indicate which tracespoints
were active on the target and which were not.

Now that GDB pushes new tracepoints to the target immediately, that
use-case may not apply, but I wonder if there are other situations
where some tracepoints will be on the target and other will not?

Marc

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

On 11/01/2012 03:09 AM, Marc Khouzam wrote:
> Now that GDB pushes new tracepoints to the target immediately, that
> use-case may not apply, but I wonder if there are other situations
> where some tracepoints will be on the target and other will not?

Yes, the pending tracepoints won't be downloaded after tracing is 
started until they are resolved.  The notification is required for this 
case.

-- 
Yao

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

Marc Khouzam wrote:

> In older GDB versions, when creating a tracepoint during a trace run,
> that tracepoint would not be pushed to the target until the next
> trace run.  The idea is that a frontend could indicate which tracespoints
> were active on the target and which were not.
>
> Now that GDB pushes new tracepoints to the target immediately, that
> use-case may not apply, but I wonder if there are other situations
> where some tracepoints will be on the target and other will not?

What about the case of connecting to a target that is tracing, after
disconnected tracing?  Do we already tell the frontend somehow which
tracepoints are active on the target?  Should tracepoints have an
"installed on target" field?

Yao Qi wrote:
> On 11/01/2012 03:09 AM, Marc Khouzam wrote:
>> Now that GDB pushes new tracepoints to the target immediately, that
>> use-case may not apply, but I wonder if there are other situations
>> where some tracepoints will be on the target and other will not?
> 
> Yes, the pending tracepoints won't be downloaded after tracing is started until they are resolved.  The notification is required for this case.

Ok.  If the answer to my question above is yes, it might be this
notification ends up unnecessary in favor of a generic
=breakpoint-modified.

-- 
Pedro Alves

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Yao Qi]

On 11/23/2012 02:32 AM, Pedro Alves wrote:
> What about the case of connecting to a target that is tracing, after
> disconnected tracing?  Do we already tell the frontend somehow which
> tracepoints are active on the target?  Should tracepoints have an
> "installed on target" field?
>
> Yao Qi wrote:
>> >On 11/01/2012 03:09 AM, Marc Khouzam wrote:
>>> >>Now that GDB pushes new tracepoints to the target immediately, that
>>> >>use-case may not apply, but I wonder if there are other situations
>>> >>where some tracepoints will be on the target and other will not?
>> >
>> >Yes, the pending tracepoints won't be downloaded after tracing is started until they are resolved.  The notification is required for this case.
> Ok.  If the answer to my question above is yes, it might be this
> notification ends up unnecessary in favor of a generic
> =breakpoint-modified.

Pedro, to make sure I don't misread your comments, I'd like to ask are 
you suggesting that we can add an 'installed on target' field for 
tracepoint in '=breakpoint-modified' notification?

-- 
Yao (齐尧)

Re: [PATCH 2/2] new tracepoint downloaded MI notification.

[Pedro Alves]

On 11/29/2012 03:46 PM, Yao Qi wrote:
> On 11/23/2012 02:32 AM, Pedro Alves wrote:
>> What about the case of connecting to a target that is tracing, after
>> disconnected tracing?  Do we already tell the frontend somehow which
>> tracepoints are active on the target?  Should tracepoints have an
>> "installed on target" field?
>>
>> Yao Qi wrote:
>>> >On 11/01/2012 03:09 AM, Marc Khouzam wrote:
>>>> >>Now that GDB pushes new tracepoints to the target immediately, that
>>>> >>use-case may not apply, but I wonder if there are other situations
>>>> >>where some tracepoints will be on the target and other will not?
>>> >
>>> >Yes, the pending tracepoints won't be downloaded after tracing is started until they are resolved.  The notification is required for this case.
>> Ok.  If the answer to my question above is yes, it might be this
>> notification ends up unnecessary in favor of a generic
>> =breakpoint-modified.
> 
> Pedro, to make sure I don't misread your comments, I'd like to ask are you suggesting that we can add an 'installed on target' field for tracepoint in '=breakpoint-modified' notification?

Well, sort of, but by side effect, since =breakpoint-modified includes all
the fields of a breakpoint/tracepoint.

Say you've set up a disconnected tracing session, and then disconnect.
Later, you start a new clean gdb session, create a new tracepoint (never
downloaded/installed on the target), and reconnect.  At this point, GDB will fetch
the target's tracepoint list, and sync it with GDB's.  The frontend gets
a =breakpoint-created for each of those uploaded tracepoints, but it has
no clue why they were created / their installed status.

You end up with some tracepoints that are installed, and some that aren't
in GDB's list.  Does the frontend know this today by some means I'm missing?
If not, does fixing this mean adding an "installed" property to
breakpoints/tracepoints?  Say we added your new notification, and then fixed the
above as I'm suggesting.  At that point, this new tracepoint downloaded
notification ends up being redundant.

So I'm trying to get us to think with a broad perspective around
the "installed on target" frontend needs.

-- 
Pedro Alves

RE: [PATCH 2/2] new tracepoint downloaded MI notification.

[Marc Khouzam]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com] 
> Sent: Thursday, November 29, 2012 11:00 AM
> To: Yao Qi
> Cc: Marc Khouzam; 'dje@google.com'; 'gdb-patches@sourceware.org'
> Subject: Re: [PATCH 2/2] new tracepoint downloaded MI notification.
> 
> On 11/29/2012 03:46 PM, Yao Qi wrote:
> > On 11/23/2012 02:32 AM, Pedro Alves wrote:
> >> What about the case of connecting to a target that is 
> tracing, after
> >> disconnected tracing?  Do we already tell the frontend 
> somehow which
> >> tracepoints are active on the target?  Should tracepoints have an
> >> "installed on target" field?
> >>
> >> Yao Qi wrote:
> >>> >On 11/01/2012 03:09 AM, Marc Khouzam wrote:
> >>>> >>Now that GDB pushes new tracepoints to the target 
> immediately, that
> >>>> >>use-case may not apply, but I wonder if there are 
> other situations
> >>>> >>where some tracepoints will be on the target and other 
> will not?
> >>> >
> >>> >Yes, the pending tracepoints won't be downloaded after 
> tracing is started until they are resolved.  The notification 
> is required for this case.
> >> Ok.  If the answer to my question above is yes, it might be this
> >> notification ends up unnecessary in favor of a generic
> >> =breakpoint-modified.
> > 
> > Pedro, to make sure I don't misread your comments, I'd like 
> to ask are you suggesting that we can add an 'installed on 
> target' field for tracepoint in '=breakpoint-modified' notification?
> 
> Well, sort of, but by side effect, since =breakpoint-modified 
> includes all
> the fields of a breakpoint/tracepoint.
> 
> Say you've set up a disconnected tracing session, and then disconnect.
> Later, you start a new clean gdb session, create a new 
> tracepoint (never
> downloaded/installed on the target), and reconnect.  At this 
> point, GDB will fetch
> the target's tracepoint list, and sync it with GDB's.  The 
> frontend gets
> a =breakpoint-created for each of those uploaded tracepoints, 
> but it has
> no clue why they were created / their installed status.
> 
> You end up with some tracepoints that are installed, and some 
> that aren't
> in GDB's list.  Does the frontend know this today by some 
> means I'm missing?

I think the frontend may be able to guess, when connecting to the
target, that any tracepoint it did not create itself must be
tracepoints already installed on target.

However, this sounds a little risky and maybe prone to race-conditions.

> If not, does fixing this mean adding an "installed" property to
> breakpoints/tracepoints?  Say we added your new notification, 
> and then fixed the
> above as I'm suggesting.  At that point, this new tracepoint 
> downloaded
> notification ends up being redundant.
> 
> So I'm trying to get us to think with a broad perspective around
> the "installed on target" frontend needs.

I think Pedro's point is very good.  It would be better to have 
GDB actually report the "installed/not installed" state when 
providing tracepoint info.  Not only will it help in the
disconnected-tracing case, but it will allow the frontend to
refresh its state.  Without it, any lost MI notification about
tracepoint download, cannot be recovered from.

IIUC, adding this entry the tracepoint info will also automatically
cause a =breakpoint-modified notif when tracepoints are downloaded
to the target.  This is something the frontend still needs, as to 
avoid forcing a refresh after any operation that could cause a
tracepoint to be installed/un-installed.

Thanks for bringing this point up!

Marc