Re: [PATCH] Upload TSVs in extend-remote mode

[Yao Qi <yao at codesourcery dot com>]

On 06/24/2013 11:25 PM, Pedro Alves wrote:
>> +  /* Upload TSVs regardless the target is running or not.  The remote
> ... regardless of whether the ...
> 
>> >+     stub, such as GDBserver, may have some predefined or builtin TSVs,
>> >+     even if the target is not running.  GDB has to upload them too when
>> >+     connects to the remote stub.  */
> "when it connects", "when connecting", or better "GDB has to upload them
> too in that case".
> 
> But I'd drop that last sentence entirely.

Fixed by removing the last sentence.

> 
>> >+  if (remote_get_trace_status (current_trace_status ()) != -1)
>> >+    {
>> >+      struct uploaded_tsv *uploaded_tsvs = NULL;
>> >+
>> >+      remote_upload_trace_state_variables (&uploaded_tsvs);
>> >+      merge_uploaded_trace_state_variables (&uploaded_tsvs);
>> >+    }
>> >+
>> >+# If there are predefined TSVs, test these predefined TSVs are correctly
>> >+# uploaded.
>> >+if [target_info exists gdb,predefined_tsv] {
>> >+    set tsv [target_info gdb,predefined_tsv]
>> >+
>> >+    # Restart.
>> >+    clean_restart ${binfile}
>> >+
>> >+    if ![runto_main] then {
>> >+	fail "Can't run to main"
>> >+	return
>> >+    }
>> >+
>> >+    # Test predefined TSVs are uploaded.
>> >+    gdb_test "info tvariables" ".*${tsv}.*" "check uploaded tsv"
>> >+}
> I think there could be an else branch here, that did "info tvariables"
> and made sure nothing comes out.  Whoever tests against a target that
> has builtin tsvs that is not GDBserver, would then notice the FAIL and

I agree.  I add a test in else branch to make sure no tsvs are uploaded.
It reports a fail if I hack GDBserver to remove $trace_timestamp, which
is expected.

> update the board file.  I'd suggest in the same vein, making ".*${tsv}.*"
> less lax.  Otherwise, I'm afraid this is the sort of board setting that
> will just go silently unnoticed.

I think about making the regexp less lax, but run out of ideas.  Users
can set a regexp in gdb,predefined_tsv if their targets has multiple
predfined tsvs, for example, there are two predefined tsvs "$foo" and
"$bar", and this line can be written in the board file,

  set_board_info gdb,predefined_tsv "\\\$foo.*\\\$bar"

so it is hard to make ".*${tsv}.*" more strict, afaik.

> 
> The patch is okay with or without that change.
> 

Patch below is committed, and we can work on the follow-up to refine
the pattern to match.

> In the future, we should be thinking of coming up with a new
> boards/gdbserver.exp file with these GDBserver specific
> settings that native-gdbserver.exp, native-extended-gdbserver.exp,
> etc. would source, instead of duplicating these bits across
> several boards.

Right, that is what I thought when I added a line across three board
files.  Will post a patch for it.

-- 
Yao (éå)

gdb:

2013-06-25  Yao Qi  <yao@codesourcery.com>

        * remote.c (remote_start_remote): Move code to upload tsv
        earlier.

gdb/testsuite:

2013-06-25  Yao Qi  <yao@codesourcery.com>

	* boards/native-extended-gdbserver.exp: Set board_info
	'gdb,predefined_tsv'.
	* boards/native-gdbserver.exp: Likewise.
	* boards/native-stdio-gdbserver.exp: Likewise.
	* gdb.server/ext-attach.exp: Load trace-support.exp.  Check
	uploaded TSVs if target supports tracing.
	* gdb.trace/tsv.exp: Check uploaded TSVs if target supports
	tracing and target has predefined tsv.

gdb/doc:

2013-06-25  Yao Qi  <yao@codesourcery.com>

	* gdbint.texinfo (Testsuite): Document 'gdb,predefined_tsv'.
---
 gdb/doc/gdbint.texinfo                             |    3 ++
 gdb/remote.c                                       |   19 ++++++++++-------
 gdb/testsuite/boards/native-extended-gdbserver.exp |    3 ++
 gdb/testsuite/boards/native-gdbserver.exp          |    3 ++
 gdb/testsuite/boards/native-stdio-gdbserver.exp    |    3 ++
 gdb/testsuite/gdb.server/ext-attach.exp            |   10 +++++++++
 gdb/testsuite/gdb.trace/tsv.exp                    |   22 ++++++++++++++++++++
 7 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/gdb/doc/gdbint.texinfo b/gdb/doc/gdbint.texinfo
index 7f1f49f..e7caabe 100644
--- a/gdb/doc/gdbint.texinfo
+++ b/gdb/doc/gdbint.texinfo
@@ -7983,6 +7983,9 @@ The board does not support type @code{long long}.
 @c NEED DOCUMENT.
 @item use_gdb_stub
 The tests are running with gdb stub.
+@item gdb,predefined_tsv
+The predefined trace state variables the board has.
+
 @end table
 
 @node Hints
diff --git a/gdb/remote.c b/gdb/remote.c
index be9186b..6354fff 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3452,6 +3452,17 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 	error (_("Remote refused setting all-stop mode with: %s"), rs->buf);
     }
 
+  /* Upload TSVs regardless of whether the target is running or not.  The
+     remote stub, such as GDBserver, may have some predefined or builtin
+     TSVs, even if the target is not running.  */
+  if (remote_get_trace_status (current_trace_status ()) != -1)
+    {
+      struct uploaded_tsv *uploaded_tsvs = NULL;
+
+      remote_upload_trace_state_variables (&uploaded_tsvs);
+      merge_uploaded_trace_state_variables (&uploaded_tsvs);
+    }
+
   /* Check whether the target is running now.  */
   putpkt ("?");
   getpkt (&rs->buf, &rs->buf_size, 0);
@@ -3591,18 +3602,10 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
   if (remote_get_trace_status (current_trace_status ()) != -1)
     {
       struct uploaded_tp *uploaded_tps = NULL;
-      struct uploaded_tsv *uploaded_tsvs = NULL;
 
       if (current_trace_status ()->running)
 	printf_filtered (_("Trace is already running on the target.\n"));
 
-      /* Get trace state variables first, they may be checked when
-	 parsing uploaded commands.  */
-
-      remote_upload_trace_state_variables (&uploaded_tsvs);
-
-      merge_uploaded_trace_state_variables (&uploaded_tsvs);
-
       remote_upload_tracepoints (&uploaded_tps);
 
       merge_uploaded_tracepoints (&uploaded_tps);
diff --git a/gdb/testsuite/boards/native-extended-gdbserver.exp b/gdb/testsuite/boards/native-extended-gdbserver.exp
index bf363c7..e9b2998 100644
--- a/gdb/testsuite/boards/native-extended-gdbserver.exp
+++ b/gdb/testsuite/boards/native-extended-gdbserver.exp
@@ -148,3 +148,6 @@ proc ${board}_file { dest op args } {
     }
     return [eval [list standard_file $dest $op] $args]
 }
+
+# The predefined TSVs in GDBserver.
+set_board_info gdb,predefined_tsv "\\\$trace_timestamp"
diff --git a/gdb/testsuite/boards/native-gdbserver.exp b/gdb/testsuite/boards/native-gdbserver.exp
index 8034a48..f32a37e 100644
--- a/gdb/testsuite/boards/native-gdbserver.exp
+++ b/gdb/testsuite/boards/native-gdbserver.exp
@@ -90,3 +90,6 @@ proc ${board}_file { dest op args } {
     }
     return [eval [list standard_file $dest $op] $args]
 }
+
+# The predefined TSVs in GDBserver.
+set_board_info gdb,predefined_tsv "\\\$trace_timestamp"
diff --git a/gdb/testsuite/boards/native-stdio-gdbserver.exp b/gdb/testsuite/boards/native-stdio-gdbserver.exp
index 7e74970..d4983a6 100644
--- a/gdb/testsuite/boards/native-stdio-gdbserver.exp
+++ b/gdb/testsuite/boards/native-stdio-gdbserver.exp
@@ -152,3 +152,6 @@ proc ${board}_file { dest op args } {
     }
     return [eval [list standard_file $dest $op] $args]
 }
+
+# The predefined TSVs in GDBserver.
+set_board_info gdb,predefined_tsv "\\\$trace_timestamp"
diff --git a/gdb/testsuite/gdb.server/ext-attach.exp b/gdb/testsuite/gdb.server/ext-attach.exp
index 1a2b539..61f93c1 100644
--- a/gdb/testsuite/gdb.server/ext-attach.exp
+++ b/gdb/testsuite/gdb.server/ext-attach.exp
@@ -18,6 +18,7 @@
 # Test attaching to already-running programs using extended-remote.
 
 load_lib gdbserver-support.exp
+load_lib trace-support.exp
 
 standard_testfile
 
@@ -56,6 +57,15 @@ if { [istarget "*-*-cygwin*"] } {
 gdb_test "attach $testpid" \
     "Attaching to program: .*, process $testpid.*(in|at).*" \
     "attach to remote program 1"
+
+if { [gdb_target_supports_trace] } then {
+    # Test predefined TSVs are uploaded.
+    gdb_test_sequence "info tvariables" "check uploaded tsv" {
+	"\[\r\n\]+Name\[\t \]+Initial\[\t \]+Current"
+	"\[\r\n\]+\\\$trace_timestamp 0"
+    }
+}
+
 gdb_test "backtrace" ".*main.*" "backtrace 1"
 
 gdb_test "detach" "Detaching from program.*process.*"
diff --git a/gdb/testsuite/gdb.trace/tsv.exp b/gdb/testsuite/gdb.trace/tsv.exp
index 4177d13..cd0b36b 100644
--- a/gdb/testsuite/gdb.trace/tsv.exp
+++ b/gdb/testsuite/gdb.trace/tsv.exp
@@ -188,3 +188,25 @@ gdb_test_multiple "target ctf ${tracefile}.ctf" "" {
 	check_tsv "ctf"
     }
 }
+
+# Restart.
+clean_restart ${binfile}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return
+}
+
+# If there are predefined TSVs, test these predefined TSVs are correctly
+# uploaded.
+if [target_info exists gdb,predefined_tsv] {
+    set tsv [target_info gdb,predefined_tsv]
+
+    # Test predefined TSVs are uploaded.
+    gdb_test "info tvariables" ".*${tsv}.*" "predefined tsvs are uploaded"
+} else {
+    # Otherwise (the predefined TSVs are not defined in the board file),
+    # test there is no TSVs in GDB.
+    gdb_test "info tvariables" "No trace state variables\." \
+	"no predefined tsvs"
+}
-- 
1.7.7.6