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

Yao Qi yao@codesourcery.com
Wed Mar 20 11:40:00 GMT 2013


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



More information about the Gdb-patches mailing list