Re: [RFA] Use data cache for stack accesses

[Jacob Potter <jdpotter@google.com>]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

On Fri, Jul 10, 2009 at 2:33 AM, Pedro Alves<pedro@codesourcery.com> wrote:
> You can't do that unconditionaly.  struct inferior's are mostly a
> process_stratum and inferior control entity: it is an internal error to
> call it when there's no current inferior (but note that it is still valid
> to read memory from the executable).

OK, how's this look? I've:
- moved target_dcache to be in struct inferior
- added documentation on the flag changes to gdb.texinfo

- Jacob

[-- Attachment #2: part2-use-cache-for-stack.patch.txt --]
[-- Type: text/plain, Size: 21264 bytes --]

diff --git a/gdb/NEWS b/gdb/NEWS
index b444f74..b70bcc9 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -345,6 +345,12 @@ show schedule-multiple
   Allow GDB to resume all threads of all processes or only threads of
   the current process.
 
+set stackcache
+show stackcache
+  Use more aggressive caching for accesses to the stack. This improves
+  performance of remote debugging (particularly tracebacks) without
+  affecting correctness.
+
 * Removed commands
 
 info forks
diff --git a/gdb/corefile.c b/gdb/corefile.c
index 6de0772..e70688e 100644
--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -228,6 +228,7 @@ memory_error (int status, CORE_ADDR memaddr)
 }
 
 /* Same as target_read_memory, but report an error if can't read.  */
+
 void
 read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
 {
@@ -237,6 +238,17 @@ read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
     memory_error (status, memaddr);
 }
 
+/* Same as target_read_stack, but report an error if can't read.  */
+
+void
+read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
+{
+  int status;
+  status = target_read_stack (memaddr, myaddr, len);
+  if (status != 0)
+    memory_error (status, memaddr);
+}
+
 /* Argument / return result struct for use with
    do_captured_read_memory_integer().  MEMADDR and LEN are filled in
    by gdb_read_memory_integer().  RESULT is the contents that were
diff --git a/gdb/dcache.c b/gdb/dcache.c
index 06956cb..cf531df 100644
--- a/gdb/dcache.c
+++ b/gdb/dcache.c
@@ -116,15 +116,23 @@ static void dcache_info (char *exp, int tty);
 
 void _initialize_dcache (void);
 
-static int dcache_enabled_p = 0;
+int dcache_stack_enabled_p = 1;
+
+int remotecache_enabled_p = 0;		/* OBSOLETE */
 
 static void
 show_dcache_enabled_p (struct ui_file *file, int from_tty,
 		       struct cmd_list_element *c, const char *value)
 {
-  fprintf_filtered (file, _("Cache use for remote targets is %s.\n"), value);
+  fprintf_filtered (file, _("Cache use for stack accesses is %s.\n"), value);
 }
 
+static void
+show_remotecache_enabled_p (struct ui_file *file, int from_tty,
+		       struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("Deprecated remotecache flag is %s.\n"), value);
+}
 
 DCACHE *last_cache;		/* Used by info dcache */
 
@@ -135,6 +143,8 @@ dcache_invalidate (DCACHE *dcache)
 {
   struct dcache_block *block, *next;
 
+  if (!dcache) return;
+
   block = dcache->oldest;
 
   while (block)
@@ -159,13 +169,17 @@ dcache_invalidate (DCACHE *dcache)
 static struct dcache_block *
 dcache_hit (DCACHE *dcache, CORE_ADDR addr)
 {
+  struct dcache_block *db;
+
   splay_tree_node node = splay_tree_lookup (dcache->tree,
 					    (splay_tree_key) MASK(addr));
 
-  if (node)
-    return (struct dcache_block *) node->value;
-  else
+  if (!node)
     return NULL;
+
+  db = (struct dcache_block *) node->value;
+  db->refs++;
+  return db;
 }
 
 
@@ -506,20 +520,33 @@ dcache_info (char *exp, int tty)
 void
 _initialize_dcache (void)
 {
-  add_setshow_boolean_cmd ("remotecache", class_support,
-			   &dcache_enabled_p, _("\
-Set cache use for remote targets."), _("\
-Show cache use for remote targets."), _("\
-When on, use data caching for remote targets.  For many remote targets\n\
-this option can offer better throughput for reading target memory.\n\
-Unfortunately, gdb does not currently know anything about volatile\n\
-registers and thus data caching will produce incorrect results with\n\
-volatile registers are in use.  By default, this option is off."),
+  add_setshow_boolean_cmd ("stackcache", class_support,
+			   &dcache_stack_enabled_p, _("\
+Set cache use for stack access."), _("\
+Show cache use for stack access."), _("\
+When on, use the data cache for all stack access, regardless of any\n\
+configured memory regions.  This improves remote performance significantly.\n\
+By default, caching for stack access is on."),
 			   NULL,
 			   show_dcache_enabled_p,
 			   &setlist, &showlist);
 
+  add_setshow_boolean_cmd ("remotecache", class_support,
+			   &remotecache_enabled_p, _("\
+Set remotecache flag."), _("\
+Show remotecache flag."), _("\
+This used to enable the data cache for remote targets.  The cache\n\
+functionality is now controlled by the memory region system and the\n\
+\"stackcache\" flag; \"remotecache\" now does nothing and exists only\n\
+for compatibility reasons."),
+			   NULL,
+			   show_remotecache_enabled_p,
+			   &setlist, &showlist);
+
   add_info ("dcache", dcache_info,
 	    _("\
-Print information on the dcache performance."));
+Print information on the dcache performance.\n\
+With no arguments, this command prints the cache configuration and a\n\
+summary of each line in the cache.  Use \"info dcache <lineno> to dump\"\n\
+the contents of a given line."));
 }
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index fc5e60f..6280e97 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -8397,32 +8397,47 @@ character.
 @section Caching Data of Remote Targets
 @cindex caching data of remote targets
 
-@value{GDBN} can cache data exchanged between the debugger and a
+@value{GDBN} caches data exchanged between the debugger and a
 remote target (@pxref{Remote Debugging}).  Such caching generally improves
 performance, because it reduces the overhead of the remote protocol by
-bundling memory reads and writes into large chunks.  Unfortunately,
-@value{GDBN} does not currently know anything about volatile
-registers, and thus data caching will produce incorrect results when
-volatile registers are in use.
+bundling memory reads and writes into large chunks.  Unfortunately, simply
+caching everything would lead to incorrect results, since @value{GDBN} 
+does not necessarily know anything about volatile values, memory-mapped I/O
+addresses, etc.  Therefore, by default, @value{GDBN} only caches data
+known to be on the stack.  Other regions of memory can be explicitly marked
+cacheable; see @pxref{Memory Region Attributes}.
 
 @table @code
 @kindex set remotecache
 @item set remotecache on
 @itemx set remotecache off
-Set caching state for remote targets.  When @code{ON}, use data
-caching.  By default, this option is @code{OFF}.
+This option no longer does anything; it exists for compatibility
+with old scripts.
 
 @kindex show remotecache
 @item show remotecache
-Show the current state of data caching for remote targets.
+Show the current state of the obsolete remotecache flag.
+
+@kindex set stackcache
+@item set stackcache on
+@itemx set stackcache off
+Enable or diable caching of stack accesses.  When @code{ON}, use
+caching.  By default, this option is @code{ON}.
+
+@kindex show remotecache
+@item show remotecache
+Show the current state of data caching for stack accesses.
 
 @kindex info dcache
-@item info dcache
+@item info dcache @r{[}line@r{]}
 Print the information about the data cache performance.  The
-information displayed includes: the dcache width and depth; and for
-each cache line, how many times it was referenced, and its data and
-state (invalid, dirty, valid).  This command is useful for debugging
-the data cache operation.
+information displayed includes the dcache width and depth, and for
+each cache line, its number, address, and how many times it was
+referenced.  This command is useful for debugging the data cache
+operation.
+
+If a line number is specified, the contents of that line will be
+printed in hex.
 @end table
 
 @node Searching Memory
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index b163231..da8f53c 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -271,6 +271,7 @@ dwarf2_evaluate_loc_desc (struct symbol *var, struct frame_info *frame,
       retval = allocate_value (SYMBOL_TYPE (var));
       VALUE_LVAL (retval) = lval_memory;
       set_value_lazy (retval, 1);
+      set_value_stack (retval, 1);
       set_value_address (retval, address);
     }
 
diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 238c6a1..f0c2a64 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -153,8 +153,10 @@ struct value *
 frame_unwind_got_memory (struct frame_info *frame, int regnum, CORE_ADDR addr)
 {
   struct gdbarch *gdbarch = frame_unwind_arch (frame);
+  struct value *v = value_at_lazy (register_type (gdbarch, regnum), addr);
 
-  return value_at_lazy (register_type (gdbarch, regnum), addr);
+  set_value_stack (v, 1);
+  return v;
 }
 
 /* Return a value which indicates that FRAME's saved version of
diff --git a/gdb/gdbcore.h b/gdb/gdbcore.h
index e339c0b..7a7dcb2 100644
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -47,6 +47,10 @@ extern void memory_error (int status, CORE_ADDR memaddr);
 
 extern void read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
 
+/* Like target_read_stack, but report an error if can't read.  */
+
+extern void read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
+
 /* Read an integer from debugged memory, given address and number of
    bytes.  */
 
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 43eacda..8b0964c 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -49,6 +49,7 @@ free_inferior (struct inferior *inf)
 {
   discard_all_inferior_continuations (inf);
   xfree (inf->private);
+  dcache_free (inf->dcache);
   xfree (inf);
 }
 
@@ -85,6 +86,8 @@ add_inferior_silent (int pid)
   inf->next = inferior_list;
   inferior_list = inf;
 
+  inf->dcache = dcache_init ();
+
   observer_notify_new_inferior (pid);
 
   return inf;
@@ -285,6 +288,14 @@ have_inferiors (void)
   return inferior_list != NULL;
 }
 
+void
+invalidate_inferior_dcache (void)
+{
+  struct inferior *inf = find_inferior_pid (ptid_get_pid (inferior_ptid));
+  if (!inf) return;
+  dcache_invalidate (inf->dcache);
+}
+
 int
 have_live_inferiors (void)
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 7312e51..f3690b6 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -423,6 +423,9 @@ struct inferior
   /* Terminal info and state managed by inflow.c.  */
   struct terminal_info *terminal_info;
 
+  /* Data cache used by dcache.c.  */
+  DCACHE *dcache;
+
   /* Private data used by the target vector implementation.  */
   struct private_inferior *private;
 };
@@ -494,6 +497,10 @@ extern void print_inferior (struct ui_out *uiout, int requested_inferior);
 /* Returns true if the inferior list is not empty.  */
 extern int have_inferiors (void);
 
+/* If there is an active inferior, invalidate its dcache.  If not,
+   do nothing.  */
+extern void invalidate_inferior_dcache (void);
+
 /* Returns true if there are any live inferiors in the inferior list
    (not cores, not executables, real live processes).  */
 extern int have_live_inferiors (void);
diff --git a/gdb/memattr.c b/gdb/memattr.c
index 356b4d6..5030e06 100644
--- a/gdb/memattr.c
+++ b/gdb/memattr.c
@@ -27,6 +27,7 @@
 #include "language.h"
 #include "vec.h"
 #include "gdb_string.h"
+#include "inferior.h"
 
 const struct mem_attrib default_mem_attrib =
 {
@@ -571,7 +572,7 @@ mem_enable_command (char *args, int from_tty)
 
   require_user_regions (from_tty);
 
-  dcache_invalidate (target_dcache);
+  invalidate_inferior_dcache ();
 
   if (p == 0)
     {
@@ -625,7 +626,7 @@ mem_disable_command (char *args, int from_tty)
 
   require_user_regions (from_tty);
 
-  dcache_invalidate (target_dcache);
+  invalidate_inferior_dcache ();
 
   if (p == 0)
     {
@@ -686,7 +687,7 @@ mem_delete_command (char *args, int from_tty)
 
   require_user_regions (from_tty);
 
-  dcache_invalidate (target_dcache);
+  invalidate_inferior_dcache ();
 
   if (p == 0)
     {
diff --git a/gdb/target.c b/gdb/target.c
index 0623bc3..ab73936 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -210,8 +210,6 @@ show_targetdebug (struct ui_file *file, int from_tty,
 
 static void setup_target_debug (void);
 
-DCACHE *target_dcache;
-
 /* The user just typed 'target' without the name of a target.  */
 
 static void
@@ -413,7 +411,7 @@ target_kill (void)
 void
 target_load (char *arg, int from_tty)
 {
-  dcache_invalidate (target_dcache);
+  invalidate_inferior_dcache ();
   (*current_target.to_load) (arg, from_tty);
 }
 
@@ -1143,12 +1141,14 @@ target_section_by_addr (struct target_ops *target, CORE_ADDR addr)
    value are just as for target_xfer_partial.  */
 
 static LONGEST
-memory_xfer_partial (struct target_ops *ops, void *readbuf, const void *writebuf,
-		     ULONGEST memaddr, LONGEST len)
+memory_xfer_partial (struct target_ops *ops, enum target_object object,
+		     void *readbuf, const void *writebuf, ULONGEST memaddr,
+		     LONGEST len)
 {
   LONGEST res;
   int reg_len;
   struct mem_region *region;
+  struct inferior *inf;
 
   /* Zero length requests are ok and require no work.  */
   if (len == 0)
@@ -1223,16 +1223,20 @@ memory_xfer_partial (struct target_ops *ops, void *readbuf, const void *writebuf
       return -1;
     }
 
-  if (region->attrib.cache)
+  inf = find_inferior_pid (ptid_get_pid (inferior_ptid));
+
+  if ((region->attrib.cache
+       || (dcache_stack_enabled_p && object == TARGET_OBJECT_STACK_MEMORY))
+      && inf)
     {
       if (readbuf != NULL)
-	res = dcache_xfer_memory (ops, target_dcache, memaddr, readbuf,
+	res = dcache_xfer_memory (ops, inf->dcache, memaddr, readbuf,
 				  reg_len, 0);
       else
 	/* FIXME drow/2006-08-09: If we're going to preserve const
 	   correctness dcache_xfer_memory should take readbuf and
 	   writebuf.  */
-	res = dcache_xfer_memory (ops, target_dcache, memaddr,
+	res = dcache_xfer_memory (ops, inf->dcache, memaddr,
 				  (void *) writebuf,
 				  reg_len, 1);
       if (res <= 0)
@@ -1245,6 +1249,15 @@ memory_xfer_partial (struct target_ops *ops, void *readbuf, const void *writebuf
 	}
     }
 
+  /* Make sure the cache gets updated no matter what - if we are writing
+     to the stack, even if this write is not tagged as such, we still need
+     to update the cache. */
+  if (inf && readbuf == NULL && !region->attrib.cache &&
+      dcache_stack_enabled_p && object != TARGET_OBJECT_STACK_MEMORY)
+    {
+      dcache_update (inf->dcache, memaddr, (void *) writebuf, reg_len);
+    }
+
   /* If none of those methods found the memory we wanted, fall back
      to a target partial transfer.  Normally a single call to
      to_xfer_partial is enough; if it doesn't recognize an object
@@ -1308,8 +1321,9 @@ target_xfer_partial (struct target_ops *ops,
   /* If this is a memory transfer, let the memory-specific code
      have a look at it instead.  Memory transfers are more
      complicated.  */
-  if (object == TARGET_OBJECT_MEMORY)
-    retval = memory_xfer_partial (ops, readbuf, writebuf, offset, len);
+  if (object == TARGET_OBJECT_MEMORY || object == TARGET_OBJECT_STACK_MEMORY)
+    retval = memory_xfer_partial (ops, object, readbuf,
+				  writebuf, offset, len);
   else
     {
       enum target_object raw_object = object;
@@ -1391,6 +1405,23 @@ target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
     return EIO;
 }
 
+/* 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, int 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.  */
+
+  if (target_read (current_target.beneath, TARGET_OBJECT_STACK_MEMORY, NULL,
+		   myaddr, memaddr, len) == len)
+    return 0;
+  else
+    return EIO;
+}
+
 int
 target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, int len)
 {
@@ -2031,7 +2062,7 @@ target_resume (ptid_t ptid, int step, enum target_signal signal)
 {
   struct target_ops *t;
 
-  dcache_invalidate (target_dcache);
+  invalidate_inferior_dcache ();
 
   for (t = current_target.beneath; t != NULL; t = t->beneath)
     {
@@ -3453,6 +3484,4 @@ Tells gdb whether to control the inferior in asynchronous mode."),
 			   show_maintenance_target_async_permitted,
 			   &setlist,
 			   &showlist);
-
-  target_dcache = dcache_init ();
 }
diff --git a/gdb/target.h b/gdb/target.h
index c425a5b..351cc32 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -202,6 +202,10 @@ enum target_object
      Target implementations of to_xfer_partial never need to handle
      this object, and most callers should not use it.  */
   TARGET_OBJECT_RAW_MEMORY,
+  /* Memory known to be part of the target's stack. This is cached even
+     if it is not in a region marked as such, since it is known to be
+     "normal" RAM. */
+  TARGET_OBJECT_STACK_MEMORY,
   /* Kernel Unwind Table.  See "ia64-tdep.c".  */
   TARGET_OBJECT_UNWIND_TABLE,
   /* Transfer auxilliary vector.  */
@@ -676,6 +680,8 @@ extern int target_read_string (CORE_ADDR, char **, int, int *);
 
 extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
 
+extern int target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, int len);
+
 extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				int len);
 
diff --git a/gdb/thread.c b/gdb/thread.c
index a7ac3c8..ca36349 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -849,6 +849,7 @@ switch_to_thread (ptid_t ptid)
     return;
 
   inferior_ptid = ptid;
+  invalidate_inferior_dcache ();
   reinit_frame_cache ();
   registers_changed ();
 
diff --git a/gdb/valops.c b/gdb/valops.c
index 2d20b41..c64598c 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -565,6 +565,32 @@ value_one (struct type *type, enum lval_type lv)
   return val;
 }
 
+/* Helper function for value_at, value_at_lazy, and value_at_lazy_stack.  */
+
+static struct value *
+get_value_at (struct type *type, CORE_ADDR addr, int lazy)
+{
+  struct value *val;
+
+  if (TYPE_CODE (check_typedef (type)) == TYPE_CODE_VOID)
+    error (_("Attempt to dereference a generic pointer."));
+
+  if (lazy)
+    {
+      val = allocate_value_lazy (type);
+    }
+  else
+    {
+      val = allocate_value (type);
+      read_memory (addr, value_contents_all_raw (val), TYPE_LENGTH (type));
+    }
+
+  VALUE_LVAL (val) = lval_memory;
+  set_value_address (val, addr);
+
+  return val;
+}
+
 /* Return a value with type TYPE located at ADDR.
 
    Call value_at only if the data needs to be fetched immediately;
@@ -580,19 +606,7 @@ value_one (struct type *type, enum lval_type lv)
 struct value *
 value_at (struct type *type, CORE_ADDR addr)
 {
-  struct value *val;
-
-  if (TYPE_CODE (check_typedef (type)) == TYPE_CODE_VOID)
-    error (_("Attempt to dereference a generic pointer."));
-
-  val = allocate_value (type);
-
-  read_memory (addr, value_contents_all_raw (val), TYPE_LENGTH (type));
-
-  VALUE_LVAL (val) = lval_memory;
-  set_value_address (val, addr);
-
-  return val;
+  return get_value_at (type, addr, 0);
 }
 
 /* Return a lazy value with type TYPE located at ADDR (cf. value_at).  */
@@ -600,17 +614,7 @@ value_at (struct type *type, CORE_ADDR addr)
 struct value *
 value_at_lazy (struct type *type, CORE_ADDR addr)
 {
-  struct value *val;
-
-  if (TYPE_CODE (check_typedef (type)) == TYPE_CODE_VOID)
-    error (_("Attempt to dereference a generic pointer."));
-
-  val = allocate_value_lazy (type);
-
-  VALUE_LVAL (val) = lval_memory;
-  set_value_address (val, addr);
-
-  return val;
+  return get_value_at (type, addr, 1);
 }
 
 /* Called only from the value_contents and value_contents_all()
@@ -637,8 +641,12 @@ value_fetch_lazy (struct value *val)
       CORE_ADDR addr = value_address (val);
       int length = TYPE_LENGTH (check_typedef (value_enclosing_type (val)));
 
-      if (length)
-	read_memory (addr, value_contents_all_raw (val), length);
+      if (length) {
+	if (value_stack (val))
+	  read_stack (addr, value_contents_all_raw (val), length);
+        else
+	  read_memory (addr, value_contents_all_raw (val), length);
+      }
     }
   else if (VALUE_LVAL (val) == lval_register)
     {
diff --git a/gdb/value.c b/gdb/value.c
index fffc183..fd0df02 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -191,6 +191,10 @@ struct value
   /* If value is a variable, is it initialized or not.  */
   int initialized;
 
+  /* If value is from the stack. If this is set, read_stack will be
+     used instead of read_memory to enable extra caching. */
+  int stack;
+
   /* Actual contents of the value.  Target byte-order.  NULL or not
      valid if lazy is nonzero.  */
   gdb_byte *contents;
@@ -427,6 +431,18 @@ set_value_lazy (struct value *value, int val)
   value->lazy = val;
 }
 
+int
+value_stack (struct value *value)
+{
+  return value->stack;
+}
+
+void
+set_value_stack (struct value *value, int val)
+{
+  value->stack = val;
+}
+
 const gdb_byte *
 value_contents (struct value *value)
 {
diff --git a/gdb/value.h b/gdb/value.h
index d816156..9259768 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -214,6 +214,9 @@ extern void *value_computed_closure (struct value *value);
 extern int value_lazy (struct value *);
 extern void set_value_lazy (struct value *value, int val);
 
+extern int value_stack (struct value *);
+extern void set_value_stack (struct value *value, int val);
+
 /* value_contents() and value_contents_raw() both return the address
    of the gdb buffer used to hold a copy of the contents of the lval.
    value_contents() is used when the contents of the buffer are needed