[RFA] Use data cache for stack accesses

[Jacob Potter <jdpotter@google.com>]

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

This is the second half of the pair of patches I first submitted last
week. Differences from the first pass are:

- Added a NEWS entry for the new option
- Changed the stackcache option to default to on, rather than off
- Got rid of the unnecessary new value_at_lazy_stack() function
- Flush the cache when switching inferiors.

I haven't changed the new read_stack function to take a target_ops,
since it's intended to be consistent with read_memory(); converting
read_memory() to take target_ops would be out of the scope of this
patch (it and target_read_memory have a *lot* of callers). Other than
that, I think I've addressed all the issues with the first pass; is
there anything else to fix?

Thanks!

- Jacob

2009-07-08  Jacob Potter  <jdpotter@google.com>

        * NEWS: Add description of stackcache option.
        * corefile.c (read_stack): New helper for stack reads.
        * gdbcore.h (read_stack): Declare.
        * dcache.c (dcache_enabled_p, remotecache_enabled_p): Rename
        dcache_enabled_p to remotecache_enabled_p.
        (stack_cache_enabled_p): New flag.
        (show_dcache_enabled_p): Change for use with stack cache flag.
        (show_remotecache_enabled_p): New function.
        (_initialize_dcache): Rewrite flag documentation; add new flag.
        * dwarf2loc.c (dwarf2_evaluate_loc_desc): Mark value as stack.
        * frame-unwind.c (frame_unwind_got_memory): Mark value as stack.
        * target.c (memory_xfer_partial): Add a target_object parameter
        to indicate the type of access; use dcache for stack.
        (target_read_stack): New helper for stack reads.
        * target.h (enum target_object): Add TARGET_OBJECT_STACK_MEMORY.
        (target_read_stack): Declare.
        * thread.c (switch_to_thread): Flush cache when pid changes.
        * valops.c (get_value_at): Refactor common code from value_at()
        and value_at_lazy() into new helper function.
        (value_fetch_lazy): Read from stack when necessary.
        * value.c (struct value): Add new field "stack" for values on
        the stack.
        (value_stack, set_value_stack): New accessors.
        * value.h (value_stack, set_value_stack): Declare.

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

diff --git a/gdb/NEWS b/gdb/NEWS
index 68776a5..b30f266 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -342,6 +342,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..f915074 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 */
 
@@ -506,20 +514,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/dwarf2loc.c b/gdb/dwarf2loc.c
index 7a54f43..23ae052 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/target.c b/gdb/target.c
index 0623bc3..89810ec 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1143,8 +1143,9 @@ 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;
@@ -1223,7 +1224,8 @@ memory_xfer_partial (struct target_ops *ops, void *readbuf, const void *writebuf
       return -1;
     }
 
-  if (region->attrib.cache)
+  if (region->attrib.cache
+      || (dcache_stack_enabled_p && object == TARGET_OBJECT_STACK_MEMORY))
     {
       if (readbuf != NULL)
 	res = dcache_xfer_memory (ops, target_dcache, memaddr, readbuf,
@@ -1245,6 +1247,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 (readbuf == NULL && !region->attrib.cache &&
+      dcache_stack_enabled_p && object != TARGET_OBJECT_STACK_MEMORY)
+    {
+      dcache_update (target_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 +1319,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 +1403,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)
 {
diff --git a/gdb/target.h b/gdb/target.h
index 24c803c..fc6b503 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.  */
@@ -667,6 +671,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..0ad3ec7 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -848,6 +848,9 @@ switch_to_thread (ptid_t ptid)
   if (ptid_equal (ptid, inferior_ptid))
     return;
 
+  if (ptid_get_pid (ptid) != ptid_get_pid (inferior_ptid))
+    dcache_invalidate (target_dcache);
+
   inferior_ptid = ptid;
   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