[RFA] dcache.c cleanup

[RFA] dcache.c cleanup

[Doug Evans]

IIRC, "set remotecache" isn't being removed to avoid breaking existing
scripts.  It's not used for anything, so this patch marks it as deprecated,
and hopefully in a future release we can delete it.
[If folks want I can instead submit a patch that makes it useful again -
e.g. as an escape hatch in case caching isn't working, it would globally
override the mem attribute settings.]

This patch also makes the output of "info dcache" more readable by printing
letters instead of magic numbers to indicate each byte's state.
I wanted to use something more common to cache state than "bad" and "ok"
in the output so I changed ENTRY_BAD/ENTRY_OK to INVALID/VALID.
[If folks want to go with something like Modified/Shared/Invalid, ok.]

Ok to check in?

2008-09-17  Doug Evans  <dje@google.com>

	* dcache.c (state_chars): New static global.
	(ENTRY_INVALID,ENTRY_VALID): Renamed from ENTRY_BAD,ENTRY_OK.
	All uses updated.
	(dcache_info): Print cache state as mnemonically useful letters instead
	of magic numbers.
	(_initialize_dcache): Mark "set remotecache" as deprecated.
	* doc/gdb.texinfo (info dcache): Update.

Index: dcache.c
===================================================================
RCS file: /cvs/src/src/gdb/dcache.c,v
retrieving revision 1.30
diff -u -p -u -p -r1.30 dcache.c
--- dcache.c	1 Jan 2008 22:53:09 -0000	1.30
+++ dcache.c	17 Sep 2008 21:35:51 -0000
@@ -51,15 +51,15 @@
    multiple of the LINE_SIZE) and a vector of bytes over the range.
    There's another vector which contains the state of the bytes.
 
-   ENTRY_BAD means that the byte is just plain wrong, and has no
+   ENTRY_INVALID means that the byte is just plain wrong, and has no
    correspondence with anything else (as it would when the cache is
-   turned on, but nothing has been done to it.
+   turned on, but nothing has been done to it).
 
    ENTRY_DIRTY means that the byte has some data in it which should be
    written out to the remote target one day, but contains correct
    data.
 
-   ENTRY_OK means that the data is the same in the cache as it is in
+   ENTRY_VALID means that the data is the same in the cache as it is in
    remote memory.
 
 
@@ -113,10 +113,16 @@
 #define MASK(x)         ((x) & ~LINE_SIZE_MASK)
 
 
-#define ENTRY_BAD   0		/* data at this byte is wrong */
-#define ENTRY_DIRTY 1		/* data at this byte needs to be written back */
-#define ENTRY_OK    2		/* data at this byte is same as in memory */
-
+#define ENTRY_INVALID 0 /* data at this byte is wrong */
+#define ENTRY_DIRTY   1 /* data at this byte needs to be written back */
+#define ENTRY_VALID   2 /* data at this byte is same as in memory */
+
+/* For cache state display by "info dcache".
+   The letters I,D,V map to
+   I = ENTRY_INVALID
+   D = ENTRY_DIRTY
+   V = ENTRY_VALID  */
+static const char state_chars[3] = { 'I', 'D', 'V' };
 
 struct dcache_block
   {
@@ -175,6 +181,7 @@ static void dcache_info (char *exp, int 
 void _initialize_dcache (void);
 
 static int dcache_enabled_p = 0;
+
 static void
 show_dcache_enabled_p (struct ui_file *file, int from_tty,
 		       struct cmd_list_element *c, const char *value)
@@ -305,7 +312,7 @@ dcache_write_line (DCACHE *dcache, struc
 	  if (res < dirty_len)
 	    return 0;
 
-	  memset (&db->state[XFORM(memaddr)], ENTRY_OK, res);
+	  memset (&db->state[XFORM(memaddr)], ENTRY_VALID, res);
 	  memaddr += res;
 	  myaddr += res;
 	  len -= res;
@@ -365,7 +372,7 @@ dcache_read_line (DCACHE *dcache, struct
       len -= res;
     }
 
-  memset (db->state, ENTRY_OK, sizeof (db->data));
+  memset (db->state, ENTRY_VALID, sizeof (db->data));
   db->anydirty = 0;
   
   return 1;
@@ -399,7 +406,7 @@ dcache_alloc (DCACHE *dcache, CORE_ADDR 
   db->addr = MASK(addr);
   db->refs = 0;
   db->anydirty = 0;
-  memset (db->state, ENTRY_BAD, sizeof (db->data));
+  memset (db->state, ENTRY_INVALID, sizeof (db->data));
 
   /* append this line to end of valid list */
   if (!dcache->valid_head)
@@ -447,7 +454,7 @@ dcache_peek_byte (DCACHE *dcache, CORE_A
 	return 0;
     }
   
-  if (db->state[XFORM (addr)] == ENTRY_BAD)
+  if (db->state[XFORM (addr)] == ENTRY_INVALID)
     {
       if (!dcache_read_line(dcache, db))
          return 0;
@@ -569,7 +576,7 @@ dcache_info (char *exp, int tty)
 	  printf_filtered (("\n"));
 
 	  for (j = 0; j < LINE_SIZE; j++)
-	    printf_filtered ("%2x", p->state[j]);
+	    printf_filtered (" %c", state_chars[p->state[j]]);
 	  printf_filtered ("\n");
 	}
     }
@@ -578,20 +585,31 @@ dcache_info (char *exp, int tty)
 void
 _initialize_dcache (void)
 {
+  int rc;
+  struct cmd_list_element *cmd = NULL;
+  char *cmd_text = "remotecache";
+
   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."),
+This option is no longer useful and is ignored.\n\
+To control cacheability of memory regions, use the `mem' command."),
 			   NULL,
 			   show_dcache_enabled_p,
 			   &setlist, &showlist);
+  /* Mark the option as deprecated.
+     We don't get the cmd_list_element back from add_setshow_boolean_cmd,
+     so we have to fetch it the hard way.  */
+  cmd = lookup_cmd (&cmd_text, setlist, "set", 0, 1);
+  gdb_assert (cmd != NULL);
+  deprecate_cmd (cmd, "mem");
 
   add_info ("dcache", dcache_info,
-	    _("Print information on the dcache performance."));
-
+	    _("\
+Print information on the dcache performance.\n\
+The state of each cached byte is represented by a letter:\n\
+  I = invalid\n\
+  D = dirty\n\
+  V = valid"));
 }
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.521
diff -u -p -u -p -r1.521 gdb.texinfo
--- doc/gdb.texinfo	3 Sep 2008 23:54:19 -0000	1.521
+++ doc/gdb.texinfo	17 Sep 2008 21:35:54 -0000
@@ -7961,7 +7961,7 @@ Show the current state of data caching f
 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 (dirty, bad, ok, etc.).  This command is useful for debugging
+state (invalid, dirty, valid).  This command is useful for debugging
 the data cache operation.
 @end table
 

Re: [RFA] dcache.c cleanup

[Daniel Jacobowitz]

On Wed, Sep 17, 2008 at 02:44:44PM -0700, Doug Evans wrote:
> IIRC, "set remotecache" isn't being removed to avoid breaking existing
> scripts.  It's not used for anything, so this patch marks it as deprecated,
> and hopefully in a future release we can delete it.
> [If folks want I can instead submit a patch that makes it useful again -
> e.g. as an escape hatch in case caching isn't working, it would globally
> override the mem attribute settings.]

Just my two cents, but I think that the command is useful (even if
currently broken) - before deprecating it we should have a good idea
of what to replace it with.  Maybe a non-target-specific command
and make this a deprecated alias to it.

Should we come up with a coherent picture of the different kinds of
memory accesses GDB needs?  This is something I've been thinking about
on and off.  Particular ideas:

- When we are doing prologue analysis, we use symbols from the
executable (either minimal or full symbols) to find function
boundaries.  Then we analyze forwards from there.  In every case I can
think of, this means that we should use the code as present in the
executable.  It's like trust-readonly-sections, but for only
prologue analysis reads; the manual disassemble command would still
show if any code had been corrupted.

- When reading data from the stack for unwinding, we can reliably
assume that the data is thread-specific and not volatile.  So we
should be able to cache it automatically, even without user
instruction, until that thread resumes.  This is like memory
attributes, but only for stack unwinding.

- If we have memory region descriptions, it's probably safe to assume
that we have the stack described as RAM.  This is like "set mem
inaccessible-by-default", but only for stack unwinding.

WDYT - do those make sense?

> 2008-09-17  Doug Evans  <dje@google.com>
> 
> 	* dcache.c (state_chars): New static global.
> 	(ENTRY_INVALID,ENTRY_VALID): Renamed from ENTRY_BAD,ENTRY_OK.
> 	All uses updated.
> 	(dcache_info): Print cache state as mnemonically useful letters instead
> 	of magic numbers.
> 	(_initialize_dcache): Mark "set remotecache" as deprecated.
> 	* doc/gdb.texinfo (info dcache): Update.

The changes other than to "set remotecache" look OK to me (remember
that doc has its own ChangeLog).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] dcache.c cleanup

[Doug Evans]

On Thu, Sep 18, 2008 at 3:04 PM, Daniel Jacobowitz <drow@false.org> wrote:
> On Wed, Sep 17, 2008 at 02:44:44PM -0700, Doug Evans wrote:
>> IIRC, "set remotecache" isn't being removed to avoid breaking existing
>> scripts.  It's not used for anything, so this patch marks it as deprecated,
>> and hopefully in a future release we can delete it.
>> [If folks want I can instead submit a patch that makes it useful again -
>> e.g. as an escape hatch in case caching isn't working, it would globally
>> override the mem attribute settings.]
>
> Just my two cents, but I think that the command is useful (even if
> currently broken) - before deprecating it we should have a good idea
> of what to replace it with.  Maybe a non-target-specific command
> and make this a deprecated alias to it.
>
> Should we come up with a coherent picture of the different kinds of
> memory accesses GDB needs?  This is something I've been thinking about
> on and off.  Particular ideas:
>
> - When we are doing prologue analysis, we use symbols from the
> executable (either minimal or full symbols) to find function
> boundaries.  Then we analyze forwards from there.  In every case I can
> think of, this means that we should use the code as present in the
> executable.  It's like trust-readonly-sections, but for only
> prologue analysis reads; the manual disassemble command would still
> show if any code had been corrupted.
>
> - When reading data from the stack for unwinding, we can reliably
> assume that the data is thread-specific and not volatile.  So we
> should be able to cache it automatically, even without user
> instruction, until that thread resumes.  This is like memory
> attributes, but only for stack unwinding.
>
> - If we have memory region descriptions, it's probably safe to assume
> that we have the stack described as RAM.  This is like "set mem
> inaccessible-by-default", but only for stack unwinding.
>
> WDYT - do those make sense?

Sure.

>
>> 2008-09-17  Doug Evans  <dje@google.com>
>>
>>       * dcache.c (state_chars): New static global.
>>       (ENTRY_INVALID,ENTRY_VALID): Renamed from ENTRY_BAD,ENTRY_OK.
>>       All uses updated.
>>       (dcache_info): Print cache state as mnemonically useful letters instead
>>       of magic numbers.
>>       (_initialize_dcache): Mark "set remotecache" as deprecated.
>>       * doc/gdb.texinfo (info dcache): Update.
>
> The changes other than to "set remotecache" look OK to me (remember
> that doc has its own ChangeLog).

Checked in (w/o remotecache).  Thanks.

IWBN to still at least add a comment or some such to the code so the
next person won't look at it and waste time trying errant fixes.