Re: [PATCH 1/3] Move the traceframe_available_memory code from memory_xfer_partial_1 down to the targets

[Yao Qi <yao at codesourcery dot com>]

On 03/21/2014 07:56 PM, Pedro Alves wrote:
> This "&remote_ops" here will be wrong in case we're connected
> with extended-remote.  Looks like remote_read_bytes needs to be
> gain an ops parameter.

Right, add 'ops' argument to remote_read_bytes in the updated patch.
Patch below is pushed in.

-- 
Yao (éå)

Subject: [PATCH 1/3] Move the traceframe_available_memory code from memory_xfer_partial_1 down to the targets

As a follow-up to

  [PATCH 7/8] Adjust read_value_memory to use to_xfer_partial
  https://sourceware.org/ml/gdb-patches/2014-02/msg00384.html

this patch moves traceframe_available_memory down to the target side.
After this patch, the gdb core code is cleaner, and code on handling
unavailable memory is moved to remote/tfile/ctf targets.

In details, this patch moves traceframe_available_memory code from
memory_xfer_partial_1 to remote target only, so remote target still
uses traceframe_info mechanism to check unavailable memory, and use
remote_ops to read them from read-only sections.  We don't use
traceframe_info mechanism for tfile and ctf target, because it is
fast to iterate all traceframes from trace file, so the summary
information got from traceframe_info is not necessary.

This patch also moves two functions to remote.c from target.c,
because they are only used in remote.c.  I'll clean them up in another
patch.

gdb:

2014-03-22  Yao Qi  <yao@codesourcery.com>

	* ctf.c (ctf_xfer_partial): Check the return value of
	exec_read_partial_read_only, if it is not TARGET_XFER_OK,
	return TARGET_XFER_UNAVAILABLE.
	* tracefile-tfile.c (tfile_xfer_partial): Likewise.
	* target.c (target_read_live_memory): Move it to remote.c.
	(memory_xfer_live_readonly_partial): Likewise.
	(memory_xfer_partial_1): Move some code to remote_read_bytes.
	* remote.c (target_read_live_memory): Moved from target.c.
	(memory_xfer_live_readonly_partial): Likewise.
	(remote_read_bytes): Factored out from
	memory_xfer_partial_1.
---
 gdb/ctf.c             |   16 +++++-
 gdb/remote.c          |  144 +++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/target.c          |  138 ----------------------------------------------
 gdb/tracefile-tfile.c |   16 +++++-
 4 files changed, 171 insertions(+), 143 deletions(-)

diff --git a/gdb/ctf.c b/gdb/ctf.c
index 95fd31f..ebd40d6 100644
--- a/gdb/ctf.c
+++ b/gdb/ctf.c
@@ -1377,6 +1377,7 @@ ctf_xfer_partial (struct target_ops *ops, enum target_object object,
     {
       struct bt_iter_pos *pos;
       int i = 0;
+      enum target_xfer_status res;
 
       gdb_assert (ctf_iter != NULL);
       /* Save the current position.  */
@@ -1466,7 +1467,20 @@ ctf_xfer_partial (struct target_ops *ops, enum target_object object,
       /* Restore the position.  */
       bt_iter_set_pos (bt_ctf_get_iter (ctf_iter), pos);
 
-      return exec_read_partial_read_only (readbuf, offset, len, xfered_len);
+      /* Requested memory is unavailable in the context of traceframes,
+	 and this address falls within a read-only section, fallback
+	 to reading from executable.  */
+      res = exec_read_partial_read_only (readbuf, offset, len, xfered_len);
+
+      if (res == TARGET_XFER_OK)
+	return TARGET_XFER_OK;
+      else
+	{
+	  /* No use trying further, we know some memory starting
+	     at MEMADDR isn't available.  */
+	  *xfered_len = len;
+	  return TARGET_XFER_UNAVAILABLE;
+	}
     }
   else
     {
diff --git a/gdb/remote.c b/gdb/remote.c
index e03d3bf..dcee6e1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6824,6 +6824,87 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
 				 packet_format[0], 1);
 }
 
+/* Read memory from the live target, even if currently inspecting a
+   traceframe.  The return is the same as that of target_read.  */
+
+static enum target_xfer_status
+target_read_live_memory (enum target_object object,
+			 ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len,
+			 ULONGEST *xfered_len)
+{
+  enum target_xfer_status ret;
+  struct cleanup *cleanup;
+
+  /* Switch momentarily out of tfind mode so to access live memory.
+     Note that this must not clear global state, such as the frame
+     cache, which must still remain valid for the previous traceframe.
+     We may be _building_ the frame cache at this point.  */
+  cleanup = make_cleanup_restore_traceframe_number ();
+  set_traceframe_number (-1);
+
+  ret = target_xfer_partial (current_target.beneath, object, NULL,
+			     myaddr, NULL, memaddr, len, xfered_len);
+
+  do_cleanups (cleanup);
+  return ret;
+}
+
+/* Using the set of read-only target sections of OPS, read live
+   read-only memory.  Note that the actual reads start from the
+   top-most target again.
+
+   For interface/parameters/return description see target.h,
+   to_xfer_partial.  */
+
+static enum target_xfer_status
+memory_xfer_live_readonly_partial (struct target_ops *ops,
+				   enum target_object object,
+				   gdb_byte *readbuf, ULONGEST memaddr,
+				   ULONGEST len, ULONGEST *xfered_len)
+{
+  struct target_section *secp;
+  struct target_section_table *table;
+
+  secp = target_section_by_addr (ops, memaddr);
+  if (secp != NULL
+      && (bfd_get_section_flags (secp->the_bfd_section->owner,
+				 secp->the_bfd_section)
+	  & SEC_READONLY))
+    {
+      struct target_section *p;
+      ULONGEST memend = memaddr + len;
+
+      table = target_get_section_table (ops);
+
+      for (p = table->sections; p < table->sections_end; p++)
+	{
+	  if (memaddr >= p->addr)
+	    {
+	      if (memend <= p->endaddr)
+		{
+		  /* Entire transfer is within this section.  */
+		  return target_read_live_memory (object, memaddr,
+						  readbuf, len, xfered_len);
+		}
+	      else if (memaddr >= p->endaddr)
+		{
+		  /* This section ends before the transfer starts.  */
+		  continue;
+		}
+	      else
+		{
+		  /* This section overlaps the transfer.  Just do half.  */
+		  len = p->endaddr - memaddr;
+		  return target_read_live_memory (object, memaddr,
+						  readbuf, len, xfered_len);
+		}
+	    }
+	}
+    }
+
+  return TARGET_XFER_EOF;
+}
+
 /* Read memory data directly from the remote machine.
    This does not use the data cache; the data cache uses this.
    MEMADDR is the address in the remote memory space.
@@ -6835,8 +6916,8 @@ remote_write_bytes (CORE_ADDR memaddr, const gdb_byte *myaddr, ULONGEST len,
    transferred in *XFERED_LEN.  */
 
 static enum target_xfer_status
-remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
-		   ULONGEST *xfered_len)
+remote_read_bytes (struct target_ops *ops, CORE_ADDR memaddr,
+		   gdb_byte *myaddr, ULONGEST len, ULONGEST *xfered_len)
 {
   struct remote_state *rs = get_remote_state ();
   int max_buf_size;		/* Max size of packet output buffer.  */
@@ -6847,6 +6928,63 @@ remote_read_bytes (CORE_ADDR memaddr, gdb_byte *myaddr, ULONGEST len,
   if (len == 0)
     return 0;
 
+  if (get_traceframe_number () != -1)
+    {
+      VEC(mem_range_s) *available;
+
+      /* If we fail to get the set of available memory, then the
+	 target does not support querying traceframe info, and so we
+	 attempt reading from the traceframe anyway (assuming the
+	 target implements the old QTro packet then).  */
+      if (traceframe_available_memory (&available, memaddr, len))
+	{
+	  struct cleanup *old_chain;
+
+	  old_chain = make_cleanup (VEC_cleanup(mem_range_s), &available);
+
+	  if (VEC_empty (mem_range_s, available)
+	      || VEC_index (mem_range_s, available, 0)->start != memaddr)
+	    {
+	      enum target_xfer_status res;
+
+	      /* Don't read into the traceframe's available
+		 memory.  */
+	      if (!VEC_empty (mem_range_s, available))
+		{
+		  LONGEST oldlen = len;
+
+		  len = VEC_index (mem_range_s, available, 0)->start - memaddr;
+		  gdb_assert (len <= oldlen);
+		}
+
+	      do_cleanups (old_chain);
+
+	      /* This goes through the topmost target again.  */
+	      res = memory_xfer_live_readonly_partial (ops,
+						       TARGET_OBJECT_MEMORY,
+						       myaddr, memaddr,
+						       len, xfered_len);
+	      if (res == TARGET_XFER_OK)
+		return TARGET_XFER_OK;
+	      else
+		{
+		  /* No use trying further, we know some memory starting
+		     at MEMADDR isn't available.  */
+		  *xfered_len = len;
+		  return TARGET_XFER_UNAVAILABLE;
+		}
+	    }
+
+	  /* Don't try to read more than how much is available, in
+	     case the target implements the deprecated QTro packet to
+	     cater for older GDBs (the target's knowledge of read-only
+	     sections may be outdated by now).  */
+	  len = VEC_index (mem_range_s, available, 0)->length;
+
+	  do_cleanups (old_chain);
+	}
+    }
+
   max_buf_size = get_memory_read_packet_size ();
   /* The packet buffer will be large enough for the payload;
      get_memory_packet_size ensures this.  */
@@ -8698,7 +8836,7 @@ remote_xfer_partial (struct target_ops *ops, enum target_object object,
       if (writebuf != NULL)
 	return remote_write_bytes (offset, writebuf, len, xfered_len);
       else
-	return remote_read_bytes (offset, readbuf, len, xfered_len);
+	return remote_read_bytes (ops, offset, readbuf, len, xfered_len);
     }
 
   /* Handle SPU memory using qxfer packets.  */
diff --git a/gdb/target.c b/gdb/target.c
index 0d22297..1b48f79 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -940,87 +940,6 @@ target_section_by_addr (struct target_ops *target, CORE_ADDR addr)
   return NULL;
 }
 
-/* Read memory from the live target, even if currently inspecting a
-   traceframe.  The return is the same as that of target_read.  */
-
-static enum target_xfer_status
-target_read_live_memory (enum target_object object,
-			 ULONGEST memaddr, gdb_byte *myaddr, ULONGEST len,
-			 ULONGEST *xfered_len)
-{
-  enum target_xfer_status ret;
-  struct cleanup *cleanup;
-
-  /* Switch momentarily out of tfind mode so to access live memory.
-     Note that this must not clear global state, such as the frame
-     cache, which must still remain valid for the previous traceframe.
-     We may be _building_ the frame cache at this point.  */
-  cleanup = make_cleanup_restore_traceframe_number ();
-  set_traceframe_number (-1);
-
-  ret = target_xfer_partial (current_target.beneath, object, NULL,
-			     myaddr, NULL, memaddr, len, xfered_len);
-
-  do_cleanups (cleanup);
-  return ret;
-}
-
-/* Using the set of read-only target sections of OPS, read live
-   read-only memory.  Note that the actual reads start from the
-   top-most target again.
-
-   For interface/parameters/return description see target.h,
-   to_xfer_partial.  */
-
-static enum target_xfer_status
-memory_xfer_live_readonly_partial (struct target_ops *ops,
-				   enum target_object object,
-				   gdb_byte *readbuf, ULONGEST memaddr,
-				   ULONGEST len, ULONGEST *xfered_len)
-{
-  struct target_section *secp;
-  struct target_section_table *table;
-
-  secp = target_section_by_addr (ops, memaddr);
-  if (secp != NULL
-      && (bfd_get_section_flags (secp->the_bfd_section->owner,
-				 secp->the_bfd_section)
-	  & SEC_READONLY))
-    {
-      struct target_section *p;
-      ULONGEST memend = memaddr + len;
-
-      table = target_get_section_table (ops);
-
-      for (p = table->sections; p < table->sections_end; p++)
-	{
-	  if (memaddr >= p->addr)
-	    {
-	      if (memend <= p->endaddr)
-		{
-		  /* Entire transfer is within this section.  */
-		  return target_read_live_memory (object, memaddr,
-						  readbuf, len, xfered_len);
-		}
-	      else if (memaddr >= p->endaddr)
-		{
-		  /* This section ends before the transfer starts.  */
-		  continue;
-		}
-	      else
-		{
-		  /* This section overlaps the transfer.  Just do half.  */
-		  len = p->endaddr - memaddr;
-		  return target_read_live_memory (object, memaddr,
-						  readbuf, len, xfered_len);
-		}
-	    }
-	}
-    }
-
-  return TARGET_XFER_EOF;
-}
-
 /* Read memory from more than one valid target.  A core file, for
    instance, could have some of memory but delegate other bits to
    the target below it.  So, we must manually try all targets.  */
@@ -1128,63 +1047,6 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	}
     }
 
-  /* If reading unavailable memory in the context of traceframes, and
-     this address falls within a read-only section, fallback to
-     reading from live memory.  */
-  if (readbuf != NULL && get_traceframe_number () != -1)
-    {
-      VEC(mem_range_s) *available;
-
-      /* If we fail to get the set of available memory, then the
-	 target does not support querying traceframe info, and so we
-	 attempt reading from the traceframe anyway (assuming the
-	 target implements the old QTro packet then).  */
-      if (traceframe_available_memory (&available, memaddr, len))
-	{
-	  struct cleanup *old_chain;
-
-	  old_chain = make_cleanup (VEC_cleanup(mem_range_s), &available);
-
-	  if (VEC_empty (mem_range_s, available)
-	      || VEC_index (mem_range_s, available, 0)->start != memaddr)
-	    {
-	      /* Don't read into the traceframe's available
-		 memory.  */
-	      if (!VEC_empty (mem_range_s, available))
-		{
-		  LONGEST oldlen = len;
-
-		  len = VEC_index (mem_range_s, available, 0)->start - memaddr;
-		  gdb_assert (len <= oldlen);
-		}
-
-	      do_cleanups (old_chain);
-
-	      /* This goes through the topmost target again.  */
-	      res = memory_xfer_live_readonly_partial (ops, object,
-						       readbuf, memaddr,
-						       len, xfered_len);
-	      if (res == TARGET_XFER_OK)
-		return TARGET_XFER_OK;
-	      else
-		{
-		  /* No use trying further, we know some memory starting
-		     at MEMADDR isn't available.  */
-		  *xfered_len = len;
-		  return TARGET_XFER_UNAVAILABLE;
-		}
-	    }
-
-	  /* Don't try to read more than how much is available, in
-	     case the target implements the deprecated QTro packet to
-	     cater for older GDBs (the target's knowledge of read-only
-	     sections may be outdated by now).  */
-	  len = VEC_index (mem_range_s, available, 0)->length;
-
-	  do_cleanups (old_chain);
-	}
-    }
-
   /* Try GDB's internal data cache.  */
   region = lookup_mem_region (memaddr);
   /* region->hi == 0 means there's no upper bound.  */
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index cbf746d..a36596f 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -900,6 +900,7 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
   if (get_traceframe_number () != -1)
     {
       int pos = 0;
+      enum target_xfer_status res;
 
       /* Iterate through the traceframe's blocks, looking for
 	 memory.  */
@@ -937,7 +938,20 @@ tfile_xfer_partial (struct target_ops *ops, enum target_object object,
 	  pos += (8 + 2 + mlen);
 	}
 
-      return exec_read_partial_read_only (readbuf, offset, len, xfered_len);
+      /* Requested memory is unavailable in the context of traceframes,
+	 and this address falls within a read-only section, fallback
+	 to reading from executable.  */
+      res = exec_read_partial_read_only (readbuf, offset, len, xfered_len);
+
+      if (res == TARGET_XFER_OK)
+	return TARGET_XFER_OK;
+      else
+	{
+	  /* No use trying further, we know some memory starting
+	     at MEMADDR isn't available.  */
+	  *xfered_len = len;
+	  return TARGET_XFER_UNAVAILABLE;
+	}
     }
   else
     {
-- 
1.7.7.6