Re: [PATCH] Delegate to target_ops->beneath to read cache lines

[Pedro Alves <palves at redhat dot com>]

On 12/02/2013 06:05 AM, Yao Qi wrote:
> On 11/30/2013 03:42 AM, Doug Evans wrote:
>> I think a comment is required here explaining why things are the way they are.
>> i.e., why we use current_target.beneath instead of &current_target.
> 
> I find the comments in target_read_memory can do some explanations,
> so I copy that paragraph here.  Is it clear to you?
> 

I'd much prefer following my suggestion:

  https://sourceware.org/ml/gdb-patches/2013-11/msg00941.html

Note we already have a target_write_raw_memory function.

Do you see a problem with this?

---------
Add new target_read_raw_memory function, and consolidate comments.

Tested on x86_64 Fedora 17.

gdb/
2013-12-02  Pedro Alves  <palves@redhat.com>

	* dcache.c (dcache_read_line): Use target_read_raw_memory.
	* target.c (target_read_raw_memory): New function.
	(target_read_stack, target_write_memory, target_write_raw_memory):
	Update comment.
	(target_read_core): Add comment.
	* target.h (target_read_raw_memory): Declare.
---

 gdb/dcache.c |   11 +++++------
 gdb/target.c |   34 ++++++++++++++++++++++++----------
 gdb/target.h |    3 +++
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/gdb/dcache.c b/gdb/dcache.c
index 12d1a4b..804d567 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -337,14 +337,13 @@ dcache_read_line (DCACHE *dcache, struct dcache_block *db)
 	  continue;
 	}
 
-      res = target_read (current_target.beneath, TARGET_OBJECT_RAW_MEMORY,
-			 NULL, myaddr, memaddr, reg_len);
-      if (res < reg_len)
+      res = target_read_raw_memory (memaddr, myaddr, reg_len);
+      if (res != 0)
 	return 0;
 
-      memaddr += res;
-      myaddr += res;
-      len -= res;
+      memaddr += reg_len;
+      myaddr += reg_len;
+      len -= reg_len;
     }
 
   return 1;
diff --git a/gdb/target.c b/gdb/target.c
index 6c72e70..85b5037 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1791,16 +1791,30 @@ target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
     return TARGET_XFER_E_IO;
 }
 
+/* Like target_read_memory, but specify explicitly that this is a read
+   from the target's raw memory.  That is, this read bypasses the
+   dcache, breakpoint shadowing, etc.  */
+
+int
+target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
+{
+  /* See comment in target_read_memory about why the request starts at
+     current_target.beneath.  */
+  if (target_read (current_target.beneath, TARGET_OBJECT_RAW_MEMORY, NULL,
+		   myaddr, memaddr, len) == len)
+    return 0;
+  else
+    return TARGET_XFER_E_IO;
+}
+
 /* Like target_read_memory, but specify explicitly that this is a read from
    the target's stack.  This may trigger different cache behavior.  */
 
 int
 target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 {
-  /* Dispatch to the topmost target, not the flattened current_target.
-     Memory accesses check target->to_has_(all_)memory, and the
-     flattened target doesn't inherit those.  */
-
+  /* See comment in target_read_memory about why the request starts at
+     current_target.beneath.  */
   if (target_read (current_target.beneath, TARGET_OBJECT_STACK_MEMORY, NULL,
 		   myaddr, memaddr, len) == len)
     return 0;
@@ -1814,6 +1828,8 @@ target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 int
 target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 {
+  /* See comment in target_read_memory about why the request starts at
+     current_target.beneath.  */
   if (target_read (current_target.beneath, TARGET_OBJECT_CODE_MEMORY, NULL,
 		   myaddr, memaddr, len) == len)
     return 0;
@@ -1830,9 +1846,8 @@ target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
 int
 target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
 {
-  /* Dispatch to the topmost target, not the flattened current_target.
-     Memory accesses check target->to_has_(all_)memory, and the
-     flattened target doesn't inherit those.  */
+  /* See comment in target_read_memory about why the request starts at
+     current_target.beneath.  */
   if (target_write (current_target.beneath, TARGET_OBJECT_MEMORY, NULL,
 		    myaddr, memaddr, len) == len)
     return 0;
@@ -1849,9 +1864,8 @@ target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
 int
 target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
 {
-  /* Dispatch to the topmost target, not the flattened current_target.
-     Memory accesses check target->to_has_(all_)memory, and the
-     flattened target doesn't inherit those.  */
+  /* See comment in target_read_memory about why the request starts at
+     current_target.beneath.  */
   if (target_write (current_target.beneath, TARGET_OBJECT_RAW_MEMORY, NULL,
 		    myaddr, memaddr, len) == len)
     return 0;
diff --git a/gdb/target.h b/gdb/target.h
index 890171d..f22e5c6 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1051,6 +1051,9 @@ extern int target_read_string (CORE_ADDR, char **, int, int *);
 extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
 			       ssize_t len);
 
+extern int target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
+				   ssize_t len);
+
 extern int target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
 
 extern int target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);