[PATCH] Rely on beneath to do partial xfer from tfile target

[PATCH] Rely on beneath to do partial xfer from tfile target

[Yao Qi]

When I examine the target API usage in both tfile target and ctf
target, I find tfile_xfer_partial does something unnecessary at
the bottom half of it.  It looks at executable for read-only
pieces.  However, isn't it better to delegate this to beneath
target (exec target or remote target) on the stack?

In target.c:memory_xfer_partial_1, we can see:

  do
    {
      res = ops->to_xfer_partial (ops, TARGET_OBJECT_MEMORY, NULL,
				  readbuf, writebuf, memaddr, reg_len);
      if (res > 0)
	break;

      /* We want to continue past core files to executables, but not
	 past a running target's memory.  */
      if (ops->to_has_all_memory (ops))
	break;

      ops = ops->beneath;
    }
  while (ops != NULL);

it goes through the target vectors on stack to try to fetch the requested
memory, so in target tfile, GDB can fall back to beneath target to
read memory isn't in trace frames.  So, we only to handle the
requested memory in trace frames.  For the rest, just return zero, and
fall back to beneath target to handle.

We also need field 'to_has_all_memory' of tfile_ops return zero, so
that GDB will fall back to the beneath target, as shown above.
Considering that trace file is a snapshot, it makes sense to return
zero in 'to_has_all_memory' in tfile target.  In target.c:add_target,
if to_has_all_memory is NULL, it will be set to 'return_zero', so we
don't have to set 'to_has_all_memory' in tfile_ops.

This patch is to revert some changes from this patch

  [PATCH] Do partial xfers from trace file
  http://sourceware.org/ml/gdb-patches/2010-04/msg00082.html

the test case added by this patch still passes.  I also add
a test in gdb.trace/tfile.exp to do 'disassemble' to test GDB is able
to read read-only part from executable.  It was mentioned in the patch
post, but not implemented as a test case.

Regression tested on x86_64-linux.  Is it OK?

gdb:

2013-03-20  Yao Qi  <yao@codesourcery.com>

	* tracepoint.c: Don't include "gdbcore.h".
	(tfile_xfer_partial): Return 0 to fall back to beneath target
	to do partial xfer.
	(tfile_has_all_memory): Remove.
	(init_tfile_ops): Don't install tfile_has_all_memory to field
	to_has_all_memory of tfile_ops.

gdb/testsuite:

2013-03-20  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/tfile.exp: Test GDB reads correctly from executable
	in tfile target.
---
 gdb/testsuite/gdb.trace/tfile.exp |    4 +++
 gdb/tracepoint.c                  |   46 ++----------------------------------
 2 files changed, 7 insertions(+), 43 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/tfile.exp b/gdb/testsuite/gdb.trace/tfile.exp
index 2bdde37..82aec20 100644
--- a/gdb/testsuite/gdb.trace/tfile.exp
+++ b/gdb/testsuite/gdb.trace/tfile.exp
@@ -66,6 +66,10 @@ gdb_test "print testglob2" " = 271828" "print testglob2 on trace file"
 
 gdb_test "print constglob" " = 10000" "print constglob on trace file"
 
+# Test GDB correctly reads read-only pieces from the executable.
+gdb_test "disassemble main" \
+    "Dump of assembler code for function main:.*   $hex <\\+0\\>:.*End of assembler dump\."
+
 gdb_test "tfind" "Target failed to find requested trace frame." \
     "tfind does not find a second frame in trace file"
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 00b0b81..a2fd66c 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -39,7 +39,6 @@
 #include "observer.h"
 #include "user-regs.h"
 #include "valprint.h"
-#include "gdbcore.h"
 #include "objfiles.h"
 #include "filenames.h"
 #include "gdbthread.h"
@@ -5024,41 +5023,9 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
 	}
     }
 
-  /* It's unduly pedantic to refuse to look at the executable for
-     read-only pieces; so do the equivalent of readonly regions aka
-     QTro packet.  */
-  /* FIXME account for relocation at some point.  */
-  if (exec_bfd)
-    {
-      asection *s;
-      bfd_size_type size;
-      bfd_vma vma;
-
-      for (s = exec_bfd->sections; s; s = s->next)
-	{
-	  if ((s->flags & SEC_LOAD) == 0
-	      || (s->flags & SEC_READONLY) == 0)
-	    continue;
-
-	  vma = s->vma;
-	  size = bfd_get_section_size (s);
-	  if (vma <= offset && offset < (vma + size))
-	    {
-	      ULONGEST amt;
-
-	      amt = (vma + size) - offset;
-	      if (amt > len)
-		amt = len;
-
-	      amt = bfd_get_section_contents (exec_bfd, s,
-					      readbuf, offset - vma, amt);
-	      return amt;
-	    }
-	}
-    }
-
-  /* Indicate failure to find the requested memory block.  */
-  return -1;
+  /* If GDB can't find the requested memory in trace frames, fall back
+     to the next target down on the stack.  */
+  return 0;
 }
 
 /* Iterate through the blocks of a trace frame, looking for a 'V'
@@ -5098,12 +5065,6 @@ tfile_get_trace_state_variable_value (int tsvnum, LONGEST *val)
 }
 
 static int
-tfile_has_all_memory (struct target_ops *ops)
-{
-  return 1;
-}
-
-static int
 tfile_has_memory (struct target_ops *ops)
 {
   return 1;
@@ -5202,7 +5163,6 @@ init_tfile_ops (void)
   tfile_ops.to_get_trace_state_variable_value
     = tfile_get_trace_state_variable_value;
   tfile_ops.to_stratum = process_stratum;
-  tfile_ops.to_has_all_memory = tfile_has_all_memory;
   tfile_ops.to_has_memory = tfile_has_memory;
   tfile_ops.to_has_stack = tfile_has_stack;
   tfile_ops.to_has_registers = tfile_has_registers;
-- 
1.7.7.6

Re: [PATCH] Rely on beneath to do partial xfer from tfile target

[Pedro Alves]

On 03/20/2013 10:09 AM, Yao Qi wrote:

> This patch is to revert some changes from this patch
> 
>   [PATCH] Do partial xfers from trace file
>   http://sourceware.org/ml/gdb-patches/2010-04/msg00082.html

Stan mentioned:

"I also made the tfile target has_all_memory, and added an emulation of QTro behavior, which
 lets disassembly and the like work, but rejects attempts to print non-constant globals
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 that were not collected."
 ^^^^^^^^^^^^^^^^^^^^^^^^

That predated the whole target_traceframe_info/traceframe_available_memory
mechanism.  If we disable it, with:

 diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
 index 71af22b..6d7bc1e 100644
 --- a/gdb/tracepoint.c
 +++ b/gdb/tracepoint.c
 @@ -5141,6 +5141,7 @@ tfile_traceframe_info (void)
    struct traceframe_info *info = XCNEW (struct traceframe_info);
  
    traceframe_walk_blocks (build_traceframe_info, 0, info);
 +  return NULL;
    return info;
  }
 
Then, without your patch, we get:

 (gdb) p non_const_not_collected_glob 
 Cannot access memory at address 0x601324

But with your patch, we'd get:

 (gdb) p non_const_not_collected_glob 
 $1 = 14124

But since we do have the target_traceframe_info mechanism,
then we always get:

 (gdb) p non_const_not_collected_glob 
 $1 = <unavailable>

Which is better than the original error.  So far so good.

But the patch has another related change in behavior which
it didn't look like was considered.  If we load the tfile target,
but we're not yet looking at any trace frame, we currently get:

 (gdb) p testglob
 Cannot access memory at address 0x60131c

With your patch, we now get:

 (gdb) p testglob
 $1 = 31415

So we're left discussing whether that is a desirable change.
I can see arguments for error, <unavailable> or printing
the value of the variable in the executable.  The latter seems
to me the least desirable, with <unavailable> perhaps being
the best one?

Meanwhile, I've applied this testsuite patch that covers
the behavior that the patch doesn't change, WRT to non-const
globals, and your disassembly bit.

Thanks!

------------------------
Subject: tfile.exp: Test printing a non-const global that is not covered by the trace frame; test disassembling.

Make sure we don't fallback to printing the initial value of a
non-const variable in the executable.

Also make sure we can do 'disassemble', as another test that GDB is
able to read read-only parts from the executable (the existing test of
printing constglob also covers that case).

gdb/testsuite/
2013-03-20  Pedro Alves  <palves@redhat.com>
	    Yao Qi <yao@codesourcery.com>

	* gdb.trace/tfile.c: Add comments.
	(nonconstglob): New global.
	* gdb.trace/tfile.exp: Add comments.  Test printing a non-const
	global that is not covered by the trace frame.  Test
	disassembling.
---
 gdb/testsuite/gdb.trace/tfile.c   |    6 ++++++
 gdb/testsuite/gdb.trace/tfile.exp |   15 +++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/gdb/testsuite/gdb.trace/tfile.c b/gdb/testsuite/gdb.trace/tfile.c
index 11c79d9..7c0c774 100644
--- a/gdb/testsuite/gdb.trace/tfile.c
+++ b/gdb/testsuite/gdb.trace/tfile.c
@@ -13,12 +13,18 @@ char trbuf[1000];
 char *trptr;
 char *tfsizeptr;
 
+/* These globals are put in the trace buffer.  */
+
 int testglob = 31415;
 
 int testglob2 = 271828;
 
+/* But these below are not.  */
+
 const int constglob = 10000;
 
+int nonconstglob = 14124;
+
 int
 start_trace_file (char *filename)
 {
diff --git a/gdb/testsuite/gdb.trace/tfile.exp b/gdb/testsuite/gdb.trace/tfile.exp
index 2bdde37..56e2a79 100644
--- a/gdb/testsuite/gdb.trace/tfile.exp
+++ b/gdb/testsuite/gdb.trace/tfile.exp
@@ -64,8 +64,23 @@ gdb_test "print testglob" " = 31415" "print testglob on trace file"
 
 gdb_test "print testglob2" " = 271828" "print testglob2 on trace file"
 
+# This global is not covered by the trace frame, but since it's const,
+# we should be able to read it from the executable.
+
 gdb_test "print constglob" " = 10000" "print constglob on trace file"
 
+# Similarly, disassembly should find the read-only pieces in the executable.
+gdb_test "disassemble main" \
+    "Dump of assembler code for function main:.*   $hex <\\+0\\>:.*End of assembler dump\."
+
+# This global is also not covered by the trace frame, and since it's
+# non-const, we should _not_ read it from the executable, as that
+# would show the variable's initial value, not the current at time the
+# trace frame was created.
+
+gdb_test "print nonconstglob" \
+    " = <unavailable>" "print nonconstglob on trace file"
+
 gdb_test "tfind" "Target failed to find requested trace frame." \
     "tfind does not find a second frame in trace file"
 

Re: [PATCH] Rely on beneath to do partial xfer from tfile target

[Yao Qi]

On 03/20/2013 07:40 PM, Pedro Alves wrote:
> But the patch has another related change in behavior which
> it didn't look like was considered.  If we load the tfile target,
> but we're not yet looking at any trace frame, we currently get:
> 
>   (gdb) p testglob
>   Cannot access memory at address 0x60131c
> 
> With your patch, we now get:
> 
>   (gdb) p testglob
>   $1 = 31415
> 
> So we're left discussing whether that is a desirable change.
> I can see arguments for error, <unavailable> or printing
> the value of the variable in the executable.  The latter seems
> to me the least desirable, with <unavailable> perhaps being
> the best one?

I agree that <unavailable> is the best choice.  The "non-const-
not-collected data" should be <unavailable> also, right?  If so, I'll
post a patch to change "Cannot access memory at address" to
"<unavailable>".

> 
> Meanwhile, I've applied this testsuite patch that covers
> the behavior that the patch doesn't change, WRT to non-const
> globals, and your disassembly bit.

Thanks.  Inspired by your patch, I also add a new test to cover more,
which checks printing four variables {collected, not collected} x
{const, non-const} in different situations.  It shows the change you
gave above, and it is also easy to be extended to test ctf.

-- 
Yao (齐尧)

gdb/testsuite:

	* gdb.trace/read-memory.c: New.
	* gdb.trace/read-memory.exp: New.
---
 gdb/testsuite/gdb.trace/read-memory.c   |   46 +++++++++
 gdb/testsuite/gdb.trace/read-memory.exp |  163 +++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/read-memory.c
 create mode 100644 gdb/testsuite/gdb.trace/read-memory.exp

diff --git a/gdb/testsuite/gdb.trace/read-memory.c b/gdb/testsuite/gdb.trace/read-memory.c
new file mode 100644
index 0000000..bc423b9
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/read-memory.c
@@ -0,0 +1,46 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+int testglob = 0;
+
+int testglob_not_collected = 10;
+
+const int constglob = 10000;
+
+const int constglob_not_collected = 100;
+
+static void
+start (void)
+{}
+
+static void
+end (void)
+{}
+
+int
+main (void)
+{
+  testglob++;
+  testglob_not_collected++;
+
+  start ();
+
+  testglob++;
+  testglob_not_collected++;
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/read-memory.exp b/gdb/testsuite/gdb.trace/read-memory.exp
new file mode 100644
index 0000000..e7c0af3
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/read-memory.exp
@@ -0,0 +1,163 @@
+#   Copyright 2013 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/>.
+
+load_lib "trace-support.exp"
+
+standard_testfile
+
+if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
+	  executable {debug nowarnings}] != "" } {
+    untested read-memory.exp
+    return -1
+}
+clean_restart $testfile
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+# Set tracepoints, start tracing and collect data.
+
+proc set_tracepoint_and_collect { } {
+    global testfile srcfile
+
+    # Start with a fresh gdb.
+    clean_restart ${testfile}
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+    gdb_test "break end" "Breakpoint \[0-9\] at .*"
+    gdb_test "trace start" "Tracepoint \[0-9\] at .*"
+    gdb_trace_setactions "set action for tracepoint"  "" \
+	"collect testglob" "^$" \
+	"collect constglob" "^$"
+
+    gdb_test_no_output "tstart"
+    gdb_test "continue" ".*Breakpoint.* end .*at.*$srcfile.*" \
+	"continue to end"
+    gdb_test_no_output "tstop"
+}
+
+with_test_prefix "live" {
+    set_tracepoint_and_collect
+
+    gdb_test "print testglob" " = 2"
+    gdb_test "print testglob_not_collected" " = 12"
+    gdb_test "print constglob" " = 10000"
+    gdb_test "print constglob_not_collected" " = 100"
+}
+
+with_test_prefix "live target tfind" {
+    gdb_test "tfind 0" "Found trace frame 0, tracepoint \[0-9\]+.*" \
+	"tfind 0"
+    gdb_test "print testglob" " = 1"
+    gdb_test "print testglob_not_collected" " = <unavailable>"
+    gdb_test "print constglob" " = 10000"
+    gdb_test "print constglob_not_collected" " = 100"
+    gdb_test "tfind" "Target failed to find requested trace frame." \
+	"tfind does not find a second frame"
+}
+
+# Save trace frames to trace file.
+set tracefile [standard_output_file ${testfile}]
+gdb_test "tsave ${tracefile}.tfile" \
+    "Trace data saved to file '${tracefile}.tfile'.*" \
+    "save tfile trace"
+
+# Test read memory when changing target from remote to ${target}.
+
+proc test_from_remote { target } {
+    global gdb_prompt testfile
+    global tracefile
+
+    with_test_prefix "remote to ${target}" {
+	set_tracepoint_and_collect
+
+	# Change target to ${target}.
+	set test "change target"
+	gdb_test_multiple "target ${target} ${tracefile}.${target}" "$test" {
+	    -re "A program is being debugged already.  Kill it. .y or n. " {
+		send_gdb "y\n"
+		exp_continue
+	    }
+	    -re "$gdb_prompt $" {
+		pass "$test"
+	    }
+	}
+
+	with_test_prefix "/wo setting traceframe" {
+	    gdb_test "print testglob" "Cannot access memory at address.*"
+	    gdb_test "print testglob_not_collected" \
+		"Cannot access memory at address.*"
+	    gdb_test "print constglob" " = 10000"
+	    gdb_test "print constglob_not_collected" " = 100"
+	}
+	with_test_prefix "/w setting traceframe" {
+	    gdb_test "tfind 0" "Found trace frame 0, tracepoint \[0-9\]+.*" \
+		"tfind 0"
+	    gdb_test "print testglob" " = 1"
+	    gdb_test "print testglob_not_collected" " = <unavailable>"
+	    gdb_test "print constglob" " = 10000"
+	    gdb_test "print constglob_not_collected" " = 100"
+	    gdb_test "tfind" "Target failed to find requested trace frame." \
+		"tfind does not find a second frame"
+	}
+    }
+}
+
+test_from_remote "tfile"
+
+# Test read memory when changing target from exec to ${target}
+
+proc teset_from_exec { target } {
+    global srcdir subdir binfile testfile
+    global tracefile
+
+    # Restart GDB and read the trace data in ${target} target.
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_file_cmd $binfile
+
+    gdb_test "target ${target} ${tracefile}.${target}" ".*" \
+	"change to ${target} target"
+
+    with_test_prefix "exec to ${target} /wo setting traceframe" {
+	gdb_test "print testglob" "Cannot access memory at address.*"
+	gdb_test "print testglob_not_collected" \
+	    "Cannot access memory at address.*"
+	gdb_test "print constglob" " = 10000"
+	gdb_test "print constglob_not_collected" " = 100"
+    }
+
+    with_test_prefix "exec to ${target} /w setting traceframe" {
+	gdb_test "tfind 0" "Found trace frame 0, tracepoint \[0-9\]+.*" \
+	    "tfind 0"
+	gdb_test "print testglob" " = 1"
+	gdb_test "print testglob_not_collected" " = <unavailable>"
+	gdb_test "print constglob" " = 10000"
+	gdb_test "print constglob_not_collected" " = 100"
+	gdb_test "tfind" "Target failed to find requested trace frame." \
+	    "tfind does not find a second frame"
+    }
+}
+
+teset_from_exec "tfile"
-- 
1.7.7.6

Re: [PATCH] Rely on beneath to do partial xfer from tfile target

[Yao Qi]

On 03/22/2013 05:49 PM, Yao Qi wrote:
> I agree that <unavailable> is the best choice.  The "non-const-
> not-collected data" should be <unavailable> also, right?  If so, I'll
> post a patch to change "Cannot access memory at address" to
> "<unavailable>".

In valops.c:read_value_memory, we have

      if (get_traceframe_number () < 0
	  || !traceframe_available_memory (&available_memory, memaddr, length))
	{
	}
      else
	{
	  /* Fallback to reading from read-only sections.  */
	}

The code is correct to remote target, however it is not perfect to
tfile target when the current traceframe is not selected
(get_traceframe_number returns -1).  In this case, GDB should fall back
to the 'else' branch to read from read-only sections, so the fix could
be 'don't check the return value of get_traceframe_number and let the
target decide where to read data from (live inferior or read-only
sections)'.

We don't check get_traceframe_number here, means target_ops method
'to_traceframe_info' will be called on many targets, so we change its
default to 'return_zero' instead of 'tcomplain'.

Fortunately, we make use of an inconsistency of 'to_traceframe_info'
of remote target and tfile target.  That is, when current traceframe
number is -1, remote_traceframe_info returns NULL and
tfile_traceframe_info return a empty traceframe_info pointer.  In this
way, the usage of 'to_traceframe_info' is extended, so I add some
comments to it.

The rationale of this patch is to teach 'to_traceframe_info' returns
something different so that GDB is able to tell the differences of
targets when current traceframe is not selected.  The other caller to
traceframe_available_memory is guarded by 'get_traceframe_number () !=
-1', so changes here don't affect it.  Regression tested on
x86_64-linux.  Is it OK?

-- 
Yao (齐尧)

gdb:

2013-04-01  Yao Qi  <yao@codesourcery.com>

	* target.c (update_current_target): Change the default action
	of 'to_traceframe_info' from tcomplain to return_zero.
	* target.h (struct target_ops) <to_traceframe_info>: Add more
	comments.
	* valops.c (read_value_memory): Call
	traceframe_available_memory unconditionally.

gdb/testsuite:

2013-04-01  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/read-memory.exp (test_from_remote): Update test.
	(teset_from_exec): Likewise.
---
 gdb/target.c                            |    2 +-
 gdb/target.h                            |   15 ++++++++++++---
 gdb/testsuite/gdb.trace/read-memory.exp |   10 ++++------
 gdb/valops.c                            |    3 +--
 4 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 9193c97..97da6b1 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -948,7 +948,7 @@ update_current_target (void)
 	    tcomplain);
   de_fault (to_traceframe_info,
 	    (struct traceframe_info * (*) (void))
-	    tcomplain);
+	    return_zero);
   de_fault (to_supports_evaluation_of_breakpoint_conditions,
 	    (int (*) (void))
 	    return_zero);
diff --git a/gdb/target.h b/gdb/target.h
index ee3fbff..072117e 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -850,9 +850,18 @@ struct target_ops
       (const char *id);
 
     /* Return a traceframe info object describing the current
-       traceframe's contents.  This method should not cache data;
-       higher layers take care of caching, invalidating, and
-       re-fetching when necessary.  */
+       traceframe's contents.  If the target doesn't support
+       traceframe info, return NULL.  If the current traceframe is not
+       selected (the current traceframe number is -1), the target can
+       choose to return either NULL or an empty traceframe info.  If
+       NULL is returned, for example in remote target, GDB will read
+       from the live inferior.  If an empty traceframe info is
+       returned, for example in tfile target, which means the
+       traceframe info is available, but the requested memory is not
+       available in it.  GDB will try to see if the requested memory
+       is available in the read-only sections.  This method should not
+       cache data; higher layers take care of caching, invalidating,
+       and re-fetching when necessary.  */
     struct traceframe_info *(*to_traceframe_info) (void);
 
     /* Ask the target to use or not to use agent according to USE.  Return 1
diff --git a/gdb/testsuite/gdb.trace/read-memory.exp b/gdb/testsuite/gdb.trace/read-memory.exp
index e7c0af3..21b0e7e 100644
--- a/gdb/testsuite/gdb.trace/read-memory.exp
+++ b/gdb/testsuite/gdb.trace/read-memory.exp
@@ -104,9 +104,8 @@ proc test_from_remote { target } {
 	}
 
 	with_test_prefix "/wo setting traceframe" {
-	    gdb_test "print testglob" "Cannot access memory at address.*"
-	    gdb_test "print testglob_not_collected" \
-		"Cannot access memory at address.*"
+	    gdb_test "print testglob" " = <unavailable>"
+	    gdb_test "print testglob_not_collected" " = <unavailable>"
 	    gdb_test "print constglob" " = 10000"
 	    gdb_test "print constglob_not_collected" " = 100"
 	}
@@ -141,9 +140,8 @@ proc teset_from_exec { target } {
 	"change to ${target} target"
 
     with_test_prefix "exec to ${target} /wo setting traceframe" {
-	gdb_test "print testglob" "Cannot access memory at address.*"
-	gdb_test "print testglob_not_collected" \
-	    "Cannot access memory at address.*"
+	gdb_test "print testglob" " = <unavailable>"
+	gdb_test "print testglob_not_collected" " = <unavailable>"
 	gdb_test "print constglob" " = 10000"
 	gdb_test "print constglob_not_collected" " = 100"
     }
diff --git a/gdb/valops.c b/gdb/valops.c
index 93c09d8..80b9995 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1117,8 +1117,7 @@ read_value_memory (struct value *val, int embedded_offset,
     {
       VEC(mem_range_s) *available_memory;
 
-      if (get_traceframe_number () < 0
-	  || !traceframe_available_memory (&available_memory, memaddr, length))
+      if (!traceframe_available_memory (&available_memory, memaddr, length))
 	{
 	  if (stack)
 	    read_stack (memaddr, buffer, length);
-- 
1.7.7.6

Re: [PATCH] Rely on beneath to do partial xfer from tfile target

[Yao Qi]

On 03/22/2013 05:49 PM, Yao Qi wrote:
> gdb/testsuite:
>
> 	* gdb.trace/read-memory.c: New.
> 	* gdb.trace/read-memory.exp: New.

Ping.

-- 
Yao (齐尧)

Re: [PATCH] Rely on beneath to do partial xfer from tfile target

[Tom Tromey]

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

Yao> +if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
Yao> +	  executable {debug nowarnings}] != "" } {
Yao> +    untested read-memory.exp
Yao> +    return -1
Yao> +}
Yao> +clean_restart $testfile

I think this can use prepare_for_testing.

Yao> +	with_test_prefix "/wo setting traceframe" {

More normal is "w/o".

Yao> +	with_test_prefix "/w setting traceframe" {

.. and "w/".

This occurs in a couple places.

The patch is ok with those fixes.  Thanks.

Tom

Re: [PATCH] Rely on beneath to do partial xfer from tfile target

[Tom Tromey]

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

Yao> 2013-04-01  Yao Qi  <yao@codesourcery.com>
Yao> 	* target.c (update_current_target): Change the default action
Yao> 	of 'to_traceframe_info' from tcomplain to return_zero.
Yao> 	* target.h (struct target_ops) <to_traceframe_info>: Add more
Yao> 	comments.
Yao> 	* valops.c (read_value_memory): Call
Yao> 	traceframe_available_memory unconditionally.

Yao> 2013-04-01  Yao Qi  <yao@codesourcery.com>

Yao> 	* gdb.trace/read-memory.exp (test_from_remote): Update test.
Yao> 	(teset_from_exec): Likewise.

Ok.

Tom

Re: [PATCH] Rely on beneath to do partial xfer from tfile target

[Yao Qi]

On 07/18/2013 03:57 AM, Tom Tromey wrote:
> I think this can use prepare_for_testing.
> 

Fixed.

> Yao> +	with_test_prefix "/wo setting traceframe" {
> 
> More normal is "w/o".
> 
> Yao> +	with_test_prefix "/w setting traceframe" {
> 
> .. and "w/".
> 
> This occurs in a couple places.

Oh, sorry about that.  Fixed.

> 
> The patch is ok with those fixes.  Thanks.

That patch is regression tested again on current GDB trunk with board
file unix and native-gdbserver respectively.  Committed.

-- 
Yao (齐尧)

gdb/testsuite:

2013-07-19  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/read-memory.c: New.
	* gdb.trace/read-memory.exp: New.
---
 gdb/testsuite/gdb.trace/read-memory.c   |   46 +++++++++
 gdb/testsuite/gdb.trace/read-memory.exp |  162 +++++++++++++++++++++++++++++++
 2 files changed, 208 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/read-memory.c
 create mode 100644 gdb/testsuite/gdb.trace/read-memory.exp

diff --git a/gdb/testsuite/gdb.trace/read-memory.c b/gdb/testsuite/gdb.trace/read-memory.c
new file mode 100644
index 0000000..bc423b9
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/read-memory.c
@@ -0,0 +1,46 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.  */
+
+int testglob = 0;
+
+int testglob_not_collected = 10;
+
+const int constglob = 10000;
+
+const int constglob_not_collected = 100;
+
+static void
+start (void)
+{}
+
+static void
+end (void)
+{}
+
+int
+main (void)
+{
+  testglob++;
+  testglob_not_collected++;
+
+  start ();
+
+  testglob++;
+  testglob_not_collected++;
+  end ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/read-memory.exp b/gdb/testsuite/gdb.trace/read-memory.exp
new file mode 100644
index 0000000..bb59853
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/read-memory.exp
@@ -0,0 +1,162 @@
+#   Copyright 2013 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/>.
+
+load_lib "trace-support.exp"
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+    untested $testfile.exp
+    return -1
+}
+
+if ![runto_main] {
+    fail "Can't run to main to check for trace support"
+    return -1
+}
+
+if ![gdb_target_supports_trace] {
+    unsupported "target does not support trace"
+    return -1
+}
+
+# Set tracepoints, start tracing and collect data.
+
+proc set_tracepoint_and_collect { } {
+    global testfile srcfile
+
+    # Start with a fresh gdb.
+    clean_restart ${testfile}
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+    gdb_test "break end" "Breakpoint \[0-9\] at .*"
+    gdb_test "trace start" "Tracepoint \[0-9\] at .*"
+    gdb_trace_setactions "set action for tracepoint"  "" \
+	"collect testglob" "^$" \
+	"collect constglob" "^$"
+
+    gdb_test_no_output "tstart"
+    gdb_test "continue" ".*Breakpoint.* end .*at.*$srcfile.*" \
+	"continue to end"
+    gdb_test_no_output "tstop"
+}
+
+with_test_prefix "live" {
+    set_tracepoint_and_collect
+
+    gdb_test "print testglob" " = 2"
+    gdb_test "print testglob_not_collected" " = 12"
+    gdb_test "print constglob" " = 10000"
+    gdb_test "print constglob_not_collected" " = 100"
+}
+
+with_test_prefix "live target tfind" {
+    gdb_test "tfind 0" "Found trace frame 0, tracepoint \[0-9\]+.*" \
+	"tfind 0"
+    gdb_test "print testglob" " = 1"
+    gdb_test "print testglob_not_collected" " = <unavailable>"
+    gdb_test "print constglob" " = 10000"
+    gdb_test "print constglob_not_collected" " = 100"
+    gdb_test "tfind" "Target failed to find requested trace frame." \
+	"tfind does not find a second frame"
+}
+
+# Save trace frames to trace file.
+set tracefile [standard_output_file ${testfile}]
+gdb_test "tsave ${tracefile}.tfile" \
+    "Trace data saved to file '${tracefile}.tfile'.*" \
+    "save tfile trace"
+
+# Test read memory when changing target from remote to ${target}.
+
+proc test_from_remote { target } {
+    global gdb_prompt testfile
+    global tracefile
+
+    with_test_prefix "remote to ${target}" {
+	set_tracepoint_and_collect
+
+	# Change target to ${target}.
+	set test "change target"
+	gdb_test_multiple "target ${target} ${tracefile}.${target}" "$test" {
+	    -re "A program is being debugged already.  Kill it. .y or n. " {
+		send_gdb "y\n"
+		exp_continue
+	    }
+	    -re "$gdb_prompt $" {
+		pass "$test"
+	    }
+	}
+
+	with_test_prefix "w/o setting traceframe" {
+	    gdb_test "print testglob" "Cannot access memory at address.*"
+	    gdb_test "print testglob_not_collected" \
+		"Cannot access memory at address.*"
+	    gdb_test "print constglob" " = 10000"
+	    gdb_test "print constglob_not_collected" " = 100"
+	}
+	with_test_prefix "w/ setting traceframe" {
+	    gdb_test "tfind 0" "Found trace frame 0, tracepoint \[0-9\]+.*" \
+		"tfind 0"
+	    gdb_test "print testglob" " = 1"
+	    gdb_test "print testglob_not_collected" " = <unavailable>"
+	    gdb_test "print constglob" " = 10000"
+	    gdb_test "print constglob_not_collected" " = 100"
+	    gdb_test "tfind" "Target failed to find requested trace frame." \
+		"tfind does not find a second frame"
+	}
+    }
+}
+
+test_from_remote "tfile"
+
+# Test read memory when changing target from exec to ${target}
+
+proc teset_from_exec { target } {
+    global srcdir subdir binfile testfile
+    global tracefile
+
+    # Restart GDB and read the trace data in ${target} target.
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_file_cmd $binfile
+
+    gdb_test "target ${target} ${tracefile}.${target}" ".*" \
+	"change to ${target} target"
+
+    with_test_prefix "exec to ${target} w/o setting traceframe" {
+	gdb_test "print testglob" "Cannot access memory at address.*"
+	gdb_test "print testglob_not_collected" \
+	    "Cannot access memory at address.*"
+	gdb_test "print constglob" " = 10000"
+	gdb_test "print constglob_not_collected" " = 100"
+    }
+
+    with_test_prefix "exec to ${target} w/ setting traceframe" {
+	gdb_test "tfind 0" "Found trace frame 0, tracepoint \[0-9\]+.*" \
+	    "tfind 0"
+	gdb_test "print testglob" " = 1"
+	gdb_test "print testglob_not_collected" " = <unavailable>"
+	gdb_test "print constglob" " = 10000"
+	gdb_test "print constglob_not_collected" " = 100"
+	gdb_test "tfind" "Target failed to find requested trace frame." \
+	    "tfind does not find a second frame"
+    }
+}
+
+teset_from_exec "tfile"
-- 
1.7.7.6

Re: [PATCH] Rely on beneath to do partial xfer from tfile target

[Yao Qi]

On 07/18/2013 04:16 AM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
>
> Yao> 2013-04-01  Yao Qi  <yao@codesourcery.com>
> Yao> 	* target.c (update_current_target): Change the default action
> Yao> 	of 'to_traceframe_info' from tcomplain to return_zero.
> Yao> 	* target.h (struct target_ops) <to_traceframe_info>: Add more
> Yao> 	comments.
> Yao> 	* valops.c (read_value_memory): Call
> Yao> 	traceframe_available_memory unconditionally.
>
> Yao> 2013-04-01  Yao Qi  <yao@codesourcery.com>
>
> Yao> 	* gdb.trace/read-memory.exp (test_from_remote): Update test.
> Yao> 	(teset_from_exec): Likewise.
>
> Ok.
>

The patch is regression tested on GDB trunk again with board file unix 
and native-gdbserver respectively.  Committed.

Thanks for the review.

-- 
Yao (齐尧)