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

Yao Qi
2012-10-18T01:16:13Z

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