[PATCH 0/5] Cache code access for disassemble

[PATCH 0/5] Cache code access for disassemble

[Yao Qi]

Hi,
In general, this patch series improve the speed of disassemble in
remote debugging, by means of caching code access.

When I think about code cache, I realize not only code cache but
also target_dcache can be per-address-space.  OTOH, when the patch
'[RFA] Use data cache for stack accesses' was reviewed, the
intention of getting target_dcache per-address-space was mentioned, but
it was not done because we don't have an address_space structure at
that moment (2009-07).  'struct address_space' was added by
multi-executable patch in 2009-09.  Patch #1 adds REGISTRY for
'struct address_space' and patch #2 get variable 'target_dcache'
per-address-space, and add a 'struct target_dcache' to hold all
caches, which paves a way for code-cache.

Patch #3 adds most of the stuff to cache code access, similar to
the code cache stack access.  Patch #4 is to use target_read_code
to read code when doing disassembly.  Patch #5 is the doc and news
entry (mostly copied from 'set stack-cache'.).

The performance is measured by disassembling three large functions
in GDB (such as evaluate_subexp_standard) twice, and here is the
result I got:

The number of 'm' packets:

	Original	Patched
	83054		1257
	74240		844
	74240		844

				Original	Patched
disassemble	cpu_time	3.48		0.56
disassemble	cpu_time	2.95		0.36
disassemble	cpu_time	1.78		0.47
disassemble	wall_time	6.07428908348	0.628926992416
disassemble	wall_time	5.23267793655	0.413200855255
disassemble	wall_time	3.2379360199	0.554491043091
disassemble	vmsize		61152		61204
disassemble	vmsize		61544		61596
disassemble	vmsize		61544		61596

Regression tested on x86_64-linux.  I also tried using target_read_code
for skip_prologue, but the change is quite specific to each target
(using target_read_code in *-tdep.c files for reading code).  I plan to
do it in next step.

*** BLURB HERE ***

Yao Qi (5):
  Add REGISTRY for struct address_space.
  Associate target_dcache to address_space.
  set/show code-cache
  Use target_read_code in disassemble.
  set/show code-cache NEWS and doc

 gdb/NEWS            |    6 ++
 gdb/disasm.c        |    2 +-
 gdb/doc/gdb.texinfo |   10 ++++
 gdb/progspace.c     |   14 ++---
 gdb/progspace.h     |   22 +++++++-
 gdb/target.c        |  148 +++++++++++++++++++++++++++++++++++++++++++++------
 gdb/target.h        |    5 ++
 7 files changed, 180 insertions(+), 27 deletions(-)

-- 
1.7.7.6

[PATCH 1/5] Add REGISTRY for struct address_space.

[Yao Qi]

This patch adds REGISTRY for struct address_space.

gdb:

2013-10-23  Yao Qi  <yao@codesourcery.com>

	* progspace.c: DEFINE_REGISTRY for address_space.
	(new_address_space): Call address_space_alloc_data.
	(free_address_space): Call address_space_free_data.
	(struct address_space): Move it to ...
	* progspace.h: ... here.
	(struct address_space) <REGISTRY_FIELDS>: New.
	Use DECLARE_REGISTRY.
---
 gdb/progspace.c |   14 ++++++--------
 gdb/progspace.h |   18 +++++++++++++++++-
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/gdb/progspace.c b/gdb/progspace.c
index 6e72211..b9b32dd 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -44,16 +44,12 @@ static int highest_address_space_num;
 
 DEFINE_REGISTRY (program_space, REGISTRY_ACCESS_FIELD)
 
-\f
+/* Keep a registry of per-address_space data-pointers required by other GDB
+   modules.  */
 
-/* An address space.  Currently this is not used for much other than
-   for comparing if pspaces/inferior/threads see the same address
-   space.  */
+DEFINE_REGISTRY (address_space, REGISTRY_ACCESS_FIELD)
 
-struct address_space
-{
-  int num;
-};
+\f
 
 /* Create a new address space object, and add it to the list.  */
 
@@ -64,6 +60,7 @@ new_address_space (void)
 
   aspace = XZALLOC (struct address_space);
   aspace->num = ++highest_address_space_num;
+  address_space_alloc_data (aspace);
 
   return aspace;
 }
@@ -89,6 +86,7 @@ maybe_new_address_space (void)
 static void
 free_address_space (struct address_space *aspace)
 {
+  address_space_free_data (aspace);
   xfree (aspace);
 }
 
diff --git a/gdb/progspace.h b/gdb/progspace.h
index f24a569..3735202 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -31,7 +31,6 @@ struct bfd;
 struct objfile;
 struct inferior;
 struct exec;
-struct address_space;
 struct program_space_data;
 
 typedef struct so_list *so_list_ptr;
@@ -131,6 +130,18 @@ DEF_VEC_P (so_list_ptr);
    (traditional unix/uClinux), or, in the DICOS case, the address
    space bound to the program space is mostly ignored.  */
 
+/* An address space.  It is used for comparing if pspaces/inferior/threads
+   see the same address space and for associating caches to each address
+   space.  */
+
+struct address_space
+{
+  int num;
+
+  /* Per aspace data-pointers required by other GDB modules.  */
+  REGISTRY_FIELDS;
+};
+
 /* The program space structure.  */
 
 struct program_space
@@ -304,4 +315,9 @@ extern void clear_program_space_solib_cache (struct program_space *);
 
 DECLARE_REGISTRY (program_space);
 
+/* Keep a registry of per-aspace data-pointers required by other GDB
+   modules.  */
+
+DECLARE_REGISTRY (address_space);
+
 #endif
-- 
1.7.7.6

Re: [PATCH 1/5] Add REGISTRY for struct address_space.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> This patch adds REGISTRY for struct address_space.

Yao> 2013-10-23  Yao Qi  <yao@codesourcery.com>

Yao> 	* progspace.c: DEFINE_REGISTRY for address_space.
Yao> 	(new_address_space): Call address_space_alloc_data.
Yao> 	(free_address_space): Call address_space_free_data.
Yao> 	(struct address_space): Move it to ...
Yao> 	* progspace.h: ... here.
Yao> 	(struct address_space) <REGISTRY_FIELDS>: New.
Yao> 	Use DECLARE_REGISTRY.

This is fine, though I think it's best to hold it until the series as a
whole has been approved.

Tom

[PATCH 3/5] set/show code-cache

[Yao Qi]

Similar to stack cache, in this patch, we add
TARGET_OBJECT_CODE_MEMORY to read code from target and add a new
option "set code-cache on|off" to control use code cache or not.

The invalidation of code cache is identical to stack cache, with this
patch, function target_dcache_invalidate invalidates both.  Command
"set {stack,code}-cache" invalidates either of them.

gdb:

2013-10-23  Yao Qi  <yao@codesourcery.com>

	* target.c (struct target_dcache) <code>: New field.
	(target_dcache_alloc): Initialize field 'code'.
	(set_stack_cache_enabled_p): Invalidate corresponding dcache.
	(code_cache_enabled_p_1): New.
	(code_cache_enabled_p): New.
	(set_code_cache_enabled_p): New function.
	(show_code_cache_enabled_p): New function.
	(target_dcache_xfree): Free code cache.
	(target_dcache_invalidate): Invalidate code cache.
	(memory_xfer_partial_1): Handle TARGET_OBJECT_CODE_MEMORY and
	code_cache_enabled_p.
	(target_xfer_partial): Likewise.
	(target_read_code): New function.
	(initialize_targets): Register command.
	* target.h (enum target_object) <TARGET_OBJECT_CODE_MEMORY>: New.
	(target_read_code): Declare.
---
 gdb/target.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 gdb/target.h |    5 +++
 2 files changed, 89 insertions(+), 13 deletions(-)

diff --git a/gdb/target.c b/gdb/target.c
index 2ae42a8..624d41a 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -218,6 +218,9 @@ struct target_dcache
   /* Cache the memory on stack and areas specified by memory
      attributes.  */
   DCACHE *general;
+
+  /* Cache the code.  */
+  DCACHE *code;
 };
 
 /* Allocate an instance of struct target_dcache.  */
@@ -228,6 +231,7 @@ target_dcache_alloc (void)
   struct target_dcache *dcache = xmalloc (sizeof (*dcache));
 
   dcache->general = dcache_init ();
+  dcache->code = dcache_init ();
 
   return dcache;
 }
@@ -240,6 +244,7 @@ target_dcache_xfree (struct target_dcache *dcache)
   if (dcache != NULL)
     {
       dcache_free (dcache->general);
+      dcache_free (dcache->code);
       xfree (dcache);
     }
 }
@@ -289,7 +294,7 @@ set_stack_cache_enabled_p (char *args, int from_tty,
 			   struct cmd_list_element *c)
 {
   if (stack_cache_enabled_p != stack_cache_enabled_p_1)
-    target_dcache_invalidate ();
+    dcache_invalidate (target_dcache_get ()->general);
 
   stack_cache_enabled_p = stack_cache_enabled_p_1;
 }
@@ -301,6 +306,35 @@ show_stack_cache_enabled_p (struct ui_file *file, int from_tty,
   fprintf_filtered (file, _("Cache use for stack accesses is %s.\n"), value);
 }
 
+/* The option sets this.  */
+static int code_cache_enabled_p_1 = 1;
+/* And set_code_cache_enabled_p updates this.
+   The reason for the separation is so that we don't flush the cache for
+   on->on transitions.  */
+static int code_cache_enabled_p = 1;
+
+/* This is called *after* the code-cache has been set.
+   Flush the cache for off->on and on->off transitions.
+   There's no real need to flush the cache for on->off transitions,
+   except cleanliness.  */
+
+static void
+set_code_cache_enabled_p (char *args, int from_tty,
+			  struct cmd_list_element *c)
+{
+  if (code_cache_enabled_p != code_cache_enabled_p_1)
+    dcache_invalidate (target_dcache_get ()->code);
+
+  code_cache_enabled_p = code_cache_enabled_p_1;
+}
+
+static void
+show_code_cache_enabled_p (struct ui_file *file, int from_tty,
+			   struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("Cache use for code accesses is %s.\n"), value);
+}
+
 /* Invalidate the target dcache.  */
 
 void
@@ -309,6 +343,7 @@ target_dcache_invalidate (void)
   struct target_dcache *td = target_dcache_get ();
 
   dcache_invalidate (td->general);
+  dcache_invalidate (td->code);
 }
 
 /* The user just typed 'target' without the name of a target.  */
@@ -1651,17 +1686,24 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	 the collected memory range fails.  */
       && get_traceframe_number () == -1
       && (region->attrib.cache
-	  || (stack_cache_enabled_p && object == TARGET_OBJECT_STACK_MEMORY)))
+	  || (stack_cache_enabled_p && object == TARGET_OBJECT_STACK_MEMORY)
+	  || (code_cache_enabled_p && object == TARGET_OBJECT_CODE_MEMORY)))
     {
+      struct target_dcache *td = target_dcache_get ();
+      DCACHE *dcache = NULL;
+
+      if (code_cache_enabled_p && object == TARGET_OBJECT_CODE_MEMORY)
+	dcache = td->code;
+      else
+	dcache = td->general;
+
       if (readbuf != NULL)
-	res = dcache_xfer_memory (ops, target_dcache_get ()->general,
-				  memaddr, readbuf, reg_len, 0);
+	res = dcache_xfer_memory (ops, 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_get ()->general,
-				  memaddr, (void *) writebuf,
+	res = dcache_xfer_memory (ops, dcache, memaddr, (void *) writebuf,
 				  reg_len, 1);
       if (res <= 0)
 	return -1;
@@ -1701,13 +1743,17 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 
   if (res > 0
       && inf != NULL
-      && writebuf != NULL
-      && !region->attrib.cache
-      && stack_cache_enabled_p
-      && object != TARGET_OBJECT_STACK_MEMORY)
+      && writebuf != NULL)
     {
-      dcache_update (target_dcache_get ()->general, memaddr,
-		     (void *) writebuf, res);
+      struct target_dcache *td = target_dcache_get ();
+
+      if (!region->attrib.cache
+	  && stack_cache_enabled_p
+	  && object != TARGET_OBJECT_STACK_MEMORY)
+	dcache_update (td->general, memaddr, (void *) writebuf, res);
+
+      if (code_cache_enabled_p && object != TARGET_OBJECT_CODE_MEMORY)
+	dcache_update (td->code, memaddr, (void *) writebuf, res);
     }
 
   /* If we still haven't got anything, return the last error.  We
@@ -1792,7 +1838,8 @@ 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 || object == TARGET_OBJECT_STACK_MEMORY)
+  if (object == TARGET_OBJECT_MEMORY || object == TARGET_OBJECT_STACK_MEMORY
+      || object == TARGET_OBJECT_CODE_MEMORY)
     retval = memory_xfer_partial (ops, object, readbuf,
 				  writebuf, offset, len);
   else
@@ -1894,6 +1941,19 @@ target_read_stack (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 code.  This may trigger different cache behavior.  */
+
+int
+target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
+{
+  if (target_read (current_target.beneath, TARGET_OBJECT_CODE_MEMORY, NULL,
+		   myaddr, memaddr, len) == len)
+    return 0;
+  else
+    return TARGET_XFER_E_IO;
+}
+
 /* Write LEN bytes from MYADDR to target memory at address MEMADDR.
    Returns either 0 for success or a target_xfer_error value if any
    error occurs.  If an error occurs, no guarantee is made about how
@@ -5199,6 +5259,17 @@ By default, caching for stack access is on."),
 			   show_stack_cache_enabled_p,
 			   &setlist, &showlist);
 
+  add_setshow_boolean_cmd ("code-cache", class_support,
+			   &code_cache_enabled_p_1, _("\
+Set cache use for code access."), _("\
+Show cache use for code access."), _("\
+When on, use the data cache for all code access, regardless of any\n\
+configured memory regions.  This improves remote performance significantly.\n\
+By default, caching for code access is on."),
+			   set_code_cache_enabled_p,
+			   show_code_cache_enabled_p,
+			   &setlist, &showlist);
+
   add_setshow_boolean_cmd ("may-write-registers", class_support,
 			   &may_write_registers_1, _("\
 Set permission to write into registers."), _("\
diff --git a/gdb/target.h b/gdb/target.h
index 56ca40c..eb00b83 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -144,6 +144,9 @@ enum target_object
      if it is not in a region marked as such, since it is known to be
      "normal" RAM.  */
   TARGET_OBJECT_STACK_MEMORY,
+  /* Memory known to be part of the target code.   This is cached even
+     if it is not in a region marked as such.  */
+  TARGET_OBJECT_CODE_MEMORY,
   /* Kernel Unwind Table.  See "ia64-tdep.c".  */
   TARGET_OBJECT_UNWIND_TABLE,
   /* Transfer auxilliary vector.  */
@@ -1052,6 +1055,8 @@ extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
 
 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);
+
 extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
 				ssize_t len);
 
-- 
1.7.7.6

Re: [PATCH 3/5] set/show code-cache

[Doug Evans]

On Wed, Oct 23, 2013 at 1:27 AM, Yao Qi <yao@codesourcery.com> wrote:
> Similar to stack cache, in this patch, we add
> TARGET_OBJECT_CODE_MEMORY to read code from target and add a new
> option "set code-cache on|off" to control use code cache or not.
>
> The invalidation of code cache is identical to stack cache, with this
> patch, function target_dcache_invalidate invalidates both.  Command
> "set {stack,code}-cache" invalidates either of them.
>
> gdb:
>
> 2013-10-23  Yao Qi  <yao@codesourcery.com>
>
>         * target.c (struct target_dcache) <code>: New field.
>         (target_dcache_alloc): Initialize field 'code'.
>         (set_stack_cache_enabled_p): Invalidate corresponding dcache.
>         (code_cache_enabled_p_1): New.
>         (code_cache_enabled_p): New.
>         (set_code_cache_enabled_p): New function.
>         (show_code_cache_enabled_p): New function.
>         (target_dcache_xfree): Free code cache.
>         (target_dcache_invalidate): Invalidate code cache.
>         (memory_xfer_partial_1): Handle TARGET_OBJECT_CODE_MEMORY and
>         code_cache_enabled_p.
>         (target_xfer_partial): Likewise.
>         (target_read_code): New function.
>         (initialize_targets): Register command.
>         * target.h (enum target_object) <TARGET_OBJECT_CODE_MEMORY>: New.
>         (target_read_code): Declare.
> ---
>  gdb/target.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  gdb/target.h |    5 +++
>  2 files changed, 89 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/target.c b/gdb/target.c
> index 2ae42a8..624d41a 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -218,6 +218,9 @@ struct target_dcache
>    /* Cache the memory on stack and areas specified by memory
>       attributes.  */
>    DCACHE *general;
> +
> +  /* Cache the code.  */
> +  DCACHE *code;
>  };

While perhaps it usually won't be a problem (*1), it's odd to have two caches.
If I do x/10x $addr and then x/10i $addr will both caches get populated?
[Will there be latent bugs due to "storing the same thing twice"?
(which has been a source of bugs in gdb)  Plus there's the extra
complexity of keeping both in sync.]


---
(*1): Because it's rarer to request reading areas of memory containing
code vs disassembling it or reading prologues.

Re: [PATCH 3/5] set/show code-cache

[Yao Qi]

On 10/25/2013 03:47 PM, Doug Evans wrote:
> While perhaps it usually won't be a problem (*1), it's odd to have two caches.

At the very beginning, I use single dcache for both stack access and 
code access.  However, it is odd that command "set stack-cache off" 
invalidates code caches, so I decide to add a new cache dedicated to 
code access.

> If I do x/10x $addr and then x/10i $addr will both caches get populated?

No, "x/10i $addr" gets code cache populated, while "x/10x $addr" doesn't 
get "general" or "stack" cache populated, unless I set memory attribute 
cache for this area.

> [Will there be latent bugs due to "storing the same thing twice"?
> (which has been a source of bugs in gdb)  Plus there's the extra
> complexity of keeping both in sync.]

"Storing the same thing twice" may introduce bugs, but "store two things 
together" is not good either :).

The code keeping caches in sync is localized, and probably we can move 
them into a new file target-cache.c.  I don't see too much complexity 
here and the disassembly is sped up, so it is worth doing it, IMO.

-- 
Yao (齐尧)

Re: [PATCH 3/5] set/show code-cache

[Doug Evans]

On Fri, Oct 25, 2013 at 7:34 AM, Yao Qi <yao@codesourcery.com> wrote:
> On 10/25/2013 03:47 PM, Doug Evans wrote:
>>
>> While perhaps it usually won't be a problem (*1), it's odd to have two
>> caches.
>
>
> At the very beginning, I use single dcache for both stack access and code
> access.  However, it is odd that command "set stack-cache off" invalidates
> code caches, so I decide to add a new cache dedicated to code access.

If it's just that "set stack-cache off" flushed the cache (as opposed
to disabling/breaking all future use) I think that's ok.  How often
will it get turned on and off?

>> If I do x/10x $addr and then x/10i $addr will both caches get populated?
>
>
> No, "x/10i $addr" gets code cache populated, while "x/10x $addr" doesn't get
> "general" or "stack" cache populated, unless I set memory attribute cache
> for this area.

Sorry, I left the assumption that caching is turned on via memory
attributes as implicit.
To be explicit: "If I turn on caching by memory attributes, and then
do x/10x $addr and then x/10i $addr, will both caches get populated?"
:)

[Of course, if caching of the addresses in question is turned on by
memory attributes then one doesn't need special code caching, but I'm
not arguing against special support for code caching.]

>> [Will there be latent bugs due to "storing the same thing twice"?
>> (which has been a source of bugs in gdb)  Plus there's the extra
>> complexity of keeping both in sync.]
>
>
> "Storing the same thing twice" may introduce bugs, but "store two things
> together" is not good either :).

It's still the same data.

> The code keeping caches in sync is localized, and probably we can move them
> into a new file target-cache.c.  I don't see too much complexity here and
> the disassembly is sped up, so it is worth doing it, IMO.

It still seems to me to be an unnecessary complexity.

Re: [PATCH 3/5] set/show code-cache

[Yao Qi]

On 10/25/2013 11:57 PM, Doug Evans wrote:
>> At the very beginning, I use single dcache for both stack access and code
>> >access.  However, it is odd that command "set stack-cache off" invalidates
>> >code caches, so I decide to add a new cache dedicated to code access.
> If it's just that "set stack-cache off" flushed the cache (as opposed
> to disabling/breaking all future use) I think that's ok.  How often
> will it get turned on and off?
>

It is confusing if command "set stack-cache off" flushes both stack 
cache and code cache.  I'd like to have separate stack cache and code 
cache, so that GDB can control them independently.  GDB invalidates 
stack cache when GDB resumes inferior, but GDB doesn't have to 
invalidate code cache, unless the program is self-modified.

If we obsolete command "set stack-cache" and replace it with command 
"set target-cache", I agree that we can use single cache for stack and 
code.  WDYT?

>>> >>If I do x/10x $addr and then x/10i $addr will both caches get populated?
>> >
>> >
>> >No, "x/10i $addr" gets code cache populated, while "x/10x $addr" doesn't get
>> >"general" or "stack" cache populated, unless I set memory attribute cache
>> >for this area.
> Sorry, I left the assumption that caching is turned on via memory
> attributes as implicit.
> To be explicit: "If I turn on caching by memory attributes, and then
> do x/10x $addr and then x/10i $addr, will both caches get populated?"
> :)

Yes, both caches get populated.

-- 
Yao (齐尧)

Re: [PATCH 3/5] set/show code-cache

[Doug Evans]

[sorry for the resend, I didn't catch that gmail had switched to rich text mode]

On Sat, Oct 26, 2013 at 6:22 AM, Yao Qi <yao@codesourcery.com> wrote:
> On 10/25/2013 11:57 PM, Doug Evans wrote:
>>>
>>> At the very beginning, I use single dcache for both stack access and code
>>> >access.  However, it is odd that command "set stack-cache off"
>>> > invalidates
>>> >code caches, so I decide to add a new cache dedicated to code access.
>>
>> If it's just that "set stack-cache off" flushed the cache (as opposed
>> to disabling/breaking all future use) I think that's ok.  How often
>> will it get turned on and off?
>>
>
> It is confusing if command "set stack-cache off" flushes both stack cache
> and code cache.  I'd like to have separate stack cache and code cache, so
> that GDB can control them independently.  GDB invalidates stack cache when
> GDB resumes inferior, but GDB doesn't have to invalidate code cache, unless
> the program is self-modified.

I guess the confusing part is having named the option "set stack-cache
on,off".  Sorry about that.

The dcache as it is today is a global cache, used by everything.  It's
not intended to be called "the stack cache". As you know, it's also
used when a range of memory is marked cacheable by the memory
attributes.

Note that GDB can't tell if the program has self-modifying code, and
GDB still has to handle the case of munmap (perhaps followed by an
mmap that happens to put different code at the same location).

Besides, won't better trust-readonly support handle the majority of
desired cases anyway?


> If we obsolete command "set stack-cache" and replace it with command "set
> target-cache", I agree that we can use single cache for stack and code.
> WDYT?

"set stack-cache on|off" is an escape hatch in case the optimization
we do to speed up backtraces on remote targets doesn't work in some
situation - the user has a knob to turn it off.  Maybe we could rename
it to "set stack-cache-optimization on|off" or some such.

[PATCH 5/5] set/show code-cache NEWS and doc

[Yao Qi]

gdb:

2013-10-23  Yao Qi  <yao@codesourcery.com>

	* NEWS: Add note on new "set code-cache" option.

gdb/doc:

2013-10-23  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (Caching Remote Data): Document new
	`set/show stack-cache' option.
---
 gdb/NEWS            |    6 ++++++
 gdb/doc/gdb.texinfo |   10 ++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 10834df..2533b0c 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -98,6 +98,12 @@ set range-stepping
 show range-stepping
   Control whether target-assisted range stepping is enabled.
 
+set code-cache
+show code-cache
+  Use more aggressive caching for accesses to the code.  This improves
+  performance of remote debugging (particularly disassemble) without
+  affecting correctness.
+
 * You can now use a literal value 'unlimited' for options that
   interpret 0 or -1 as meaning "unlimited".  E.g., "set
   trace-buffer-size unlimited" is now an alias for "set
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 44fb174..c80f17d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10821,6 +10821,16 @@ caching.  By default, this option is @code{ON}.
 @item show stack-cache
 Show the current state of data caching for memory accesses.
 
+@kindex set code-cache
+@item set code-cache on
+@itemx set code-cache off
+Enable or disable caching of code accesses.  When @code{ON}, use
+caching.  By default, this option is @code{ON}.
+
+@kindex show code-cache
+@item show code-cache
+Show the current state of data caching for code accesses.
+
 @kindex info dcache
 @item info dcache @r{[}line@r{]}
 Print the information about the data cache performance.  The
-- 
1.7.7.6

Re: [PATCH 5/5] set/show code-cache NEWS and doc

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Wed, 23 Oct 2013 16:27:35 +0800
> 
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -98,6 +98,12 @@ set range-stepping
>  show range-stepping
>    Control whether target-assisted range stepping is enabled.
>  
> +set code-cache
> +show code-cache
> +  Use more aggressive caching for accesses to the code.  This improves
> +  performance of remote debugging (particularly disassemble) without
> +  affecting correctness.

Thanks.  However, this (and even more importantly, the manual) should
explain what is "code" in this context.  "Code" is too general a word
to hope that the reader will immediately understand what you mean.

Also, the last sentence above should be in the manual; it's not good
that the manual has less details than NEWS.

Re: [PATCH 5/5] set/show code-cache NEWS and doc

[Yao Qi]

On 10/23/2013 11:25 PM, Eli Zaretskii wrote:
> Thanks.  However, this (and even more importantly, the manual) should
> explain what is "code" in this context.  "Code" is too general a word
> to hope that the reader will immediately understand what you mean.
> 

I go through gdb.texinfo, and find "program code" is used in some
places.  I choose it.

> Also, the last sentence above should be in the manual; it's not good
> that the manual has less details than NEWS.

OK, add one line in the manual to mention that this improves
disassemble.  How about this?

-- 
Yao (齐尧)

gdb:

2013-10-23  Yao Qi  <yao@codesourcery.com>

	* NEWS: Add note on new "set code-cache" option.

gdb/doc:

2013-10-23  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (Caching Remote Data): Document new
	`set/show stack-cache' option.
---
 gdb/NEWS            |    6 ++++++
 gdb/doc/gdb.texinfo |   15 ++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 10834df..10d5804 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -98,6 +98,12 @@ set range-stepping
 show range-stepping
   Control whether target-assisted range stepping is enabled.
 
+set code-cache
+show code-cache
+  Use more aggressive caching for accesses to the program code.  This
+  improves performance of remote debugging (particularly disassemble)
+  without affecting correctness.
+
 * You can now use a literal value 'unlimited' for options that
   interpret 0 or -1 as meaning "unlimited".  E.g., "set
   trace-buffer-size unlimited" is now an alias for "set
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 44fb174..5770382 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -10796,7 +10796,8 @@ Therefore, by default, @value{GDBN} only caches data
 known to be on the stack@footnote{In non-stop mode, it is moderately
 rare for a running thread to modify the stack of a stopped thread
 in a way that would interfere with a backtrace, and caching of
-stack reads provides a significant speed up of remote backtraces.}.
+stack reads provides a significant speed up of remote backtraces.} or
+in the program code.
 Other regions of memory can be explicitly marked as
 cacheable; see @pxref{Memory Region Attributes}.
 
@@ -10821,6 +10822,18 @@ caching.  By default, this option is @code{ON}.
 @item show stack-cache
 Show the current state of data caching for memory accesses.
 
+@kindex set code-cache
+@item set code-cache on
+@itemx set code-cache off
+Enable or disable caching of program code accesses.  When @code{ON},
+use caching.  By default, this option is @code{ON}.  This improves
+performance of disassemble in remote debugging without affecting
+correctness.
+
+@kindex show code-cache
+@item show code-cache
+Show the current state of data caching for program code accesses.
+
 @kindex info dcache
 @item info dcache @r{[}line@r{]}
 Print the information about the data cache performance.  The
-- 
1.7.7.6

Re: [PATCH 5/5] set/show code-cache NEWS and doc

[Eli Zaretskii]

> Date: Thu, 24 Oct 2013 16:25:11 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 10/23/2013 11:25 PM, Eli Zaretskii wrote:
> > Thanks.  However, this (and even more importantly, the manual) should
> > explain what is "code" in this context.  "Code" is too general a word
> > to hope that the reader will immediately understand what you mean.
> > 
> 
> I go through gdb.texinfo, and find "program code" is used in some
> places.  I choose it.

The problem in this case is that it's unclear whether you are talking
about accessing the source code or the code (a.k.a. ".text") section
of the executable.  "Program code" could mean either of these two.

> > Also, the last sentence above should be in the manual; it's not good
> > that the manual has less details than NEWS.
> 
> OK, add one line in the manual to mention that this improves
> disassemble.  How about this?

Replace "disassemble" with "disassembly", and I'm happy.

Thanks.

Re: [PATCH 5/5] set/show code-cache NEWS and doc

[Yao Qi]

On 10/24/2013 11:20 PM, Eli Zaretskii wrote:
> The problem in this case is that it's unclear whether you are talking
> about accessing the source code or the code (a.k.a. ".text") section
> of the executable.  "Program code" could mean either of these two.

How about "executable code"?

-- 
Yao (齐尧)

Re: [PATCH 5/5] set/show code-cache NEWS and doc

[Eli Zaretskii]

> Date: Fri, 25 Oct 2013 17:33:59 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 10/24/2013 11:20 PM, Eli Zaretskii wrote:
> > The problem in this case is that it's unclear whether you are talking
> > about accessing the source code or the code (a.k.a. ".text") section
> > of the executable.  "Program code" could mean either of these two.
> 
> How about "executable code"?

I would suggest "code section", unless you think that is incorrect for
some reason.

Re: [PATCH 5/5] set/show code-cache NEWS and doc

[Yao Qi]

On 10/25/2013 06:00 PM, Eli Zaretskii wrote:
>> How about "executable code"?
> I would suggest "code section", unless you think that is incorrect for
> some reason.

GDB can also access code generated by JIT, but they are not from "code 
section".  On the other hand, "read code section" usually means "read 
code section in executable file" rather than "read code section in live 
inferior".

-- 
Yao (齐尧)

Re: [PATCH 5/5] set/show code-cache NEWS and doc

[Eli Zaretskii]

> Date: Sat, 2 Nov 2013 08:23:28 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: <gdb-patches@sourceware.org>
> 
> On 10/25/2013 06:00 PM, Eli Zaretskii wrote:
> >> How about "executable code"?
> > I would suggest "code section", unless you think that is incorrect for
> > some reason.
> 
> GDB can also access code generated by JIT, but they are not from "code 
> section".  On the other hand, "read code section" usually means "read 
> code section in executable file" rather than "read code section in live 
> inferior".

I'm also OK with "code segment", if you think it's more accurate.

"Executable code" is too vague to be helpful, IMO.

[PATCH 4/5] Use target_read_code in disassemble.

[Yao Qi]

gdb:

2013-10-23  Yao Qi  <yao@codesourcery.com>

	* disasm.c (dis_asm_read_memory): Call target_read_code
	instead of target_read_memory.
---
 gdb/disasm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index e643c2d..522e25f 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -47,7 +47,7 @@ static int
 dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
 		     struct disassemble_info *info)
 {
-  return target_read_memory (memaddr, myaddr, len);
+  return target_read_code (memaddr, myaddr, len);
 }
 
 /* Like memory_error with slightly different parameters.  */
-- 
1.7.7.6

[PATCH 2/5] Associate target_dcache to address_space.

[Yao Qi]

Hi,
Nowadays, 'target_dcache' is a global variable in GDB, which is not
necessary.  It can be a per-address-space variable.  In this patch, we
add a new structure 'target_dcache', which includes all dcaches of
target and all of them are per-address-space.

There is one field 'general' which is equivalent to current
variable 'target_dcache'.  I thought to name it 'stack', but it also
cache memory regions marked by attributes, I choose 'general'.

gdb:

2013-10-23  Yao Qi  <yao@codesourcery.com>

	* progspace.h (struct address_space): Declare.
	(current_address_space): New macro.
	* target.c (target_dcache_aspace_key): New.
	(target_dcache): Delete.
	(struct target_dcache): New.
	(target_dcache_alloc): New function.
	(target_dcache_xfree): New function.
	(target_dcache_get): New function.
	(target_dcache_cleanup)): New function.
	(target_dcache_invalidate): Update.
	(memory_xfer_partial_1): Update.
	(initialize_targets): Initialize target_dcache_aspace_key.
---
 gdb/progspace.h |    4 ++
 gdb/target.c    |   89 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 82 insertions(+), 11 deletions(-)

diff --git a/gdb/progspace.h b/gdb/progspace.h
index 3735202..c5098e5 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -32,6 +32,7 @@ struct objfile;
 struct inferior;
 struct exec;
 struct program_space_data;
+struct address_space_data;
 
 typedef struct so_list *so_list_ptr;
 DEF_VEC_P (so_list_ptr);
@@ -239,6 +240,9 @@ extern struct program_space *program_spaces;
 /* The current program space.  This is always non-null.  */
 extern struct program_space *current_program_space;
 
+/* The current added space.  */
+#define current_address_space current_program_space->aspace
+
 #define ALL_PSPACES(pspace) \
   for ((pspace) = program_spaces; (pspace) != NULL; (pspace) = (pspace)->next)
 
diff --git a/gdb/target.c b/gdb/target.c
index 22d7fb6..2ae42a8 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -206,6 +206,72 @@ show_targetdebug (struct ui_file *file, int from_tty,
 
 static void setup_target_debug (void);
 
+/* The target dcache is kept per-address-space.  This key lets us
+   associate the cache with the address space.  */
+
+static const struct address_space_data *target_dcache_aspace_key;
+
+/* dcache for target memory.  */
+
+struct target_dcache
+{
+  /* Cache the memory on stack and areas specified by memory
+     attributes.  */
+  DCACHE *general;
+};
+
+/* Allocate an instance of struct target_dcache.  */
+
+static struct target_dcache *
+target_dcache_alloc (void)
+{
+  struct target_dcache *dcache = xmalloc (sizeof (*dcache));
+
+  dcache->general = dcache_init ();
+
+  return dcache;
+}
+
+/* Free DCACHE and its fields.  */
+
+static void
+target_dcache_xfree (struct target_dcache *dcache)
+{
+  if (dcache != NULL)
+    {
+      dcache_free (dcache->general);
+      xfree (dcache);
+    }
+}
+
+/* Get an instance of struct target_dcache.  */
+
+static struct target_dcache *
+target_dcache_get (void)
+{
+  struct target_dcache *dcache
+    = address_space_data (current_address_space, target_dcache_aspace_key);
+
+  if (dcache == NULL)
+    {
+      dcache = target_dcache_alloc ();
+
+      set_address_space_data (current_address_space,
+			      target_dcache_aspace_key, dcache);
+    }
+
+  return dcache;
+}
+
+static void
+target_dcache_cleanup (struct address_space *aspace, void *arg)
+{
+  struct target_dcache *dcache
+    = address_space_data (aspace, target_dcache_aspace_key);
+
+  target_dcache_xfree (dcache);
+}
+
 /* The option sets this.  */
 static int stack_cache_enabled_p_1 = 1;
 /* And set_stack_cache_enabled_p updates this.
@@ -235,15 +301,14 @@ show_stack_cache_enabled_p (struct ui_file *file, int from_tty,
   fprintf_filtered (file, _("Cache use for stack accesses is %s.\n"), value);
 }
 
-/* Cache of memory operations, to speed up remote access.  */
-static DCACHE *target_dcache;
-
 /* Invalidate the target dcache.  */
 
 void
 target_dcache_invalidate (void)
 {
-  dcache_invalidate (target_dcache);
+  struct target_dcache *td = target_dcache_get ();
+
+  dcache_invalidate (td->general);
 }
 
 /* The user just typed 'target' without the name of a target.  */
@@ -1589,14 +1654,14 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	  || (stack_cache_enabled_p && object == TARGET_OBJECT_STACK_MEMORY)))
     {
       if (readbuf != NULL)
-	res = dcache_xfer_memory (ops, target_dcache, memaddr, readbuf,
-				  reg_len, 0);
+	res = dcache_xfer_memory (ops, target_dcache_get ()->general,
+				  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,
-				  (void *) writebuf,
+	res = dcache_xfer_memory (ops, target_dcache_get ()->general,
+				  memaddr, (void *) writebuf,
 				  reg_len, 1);
       if (res <= 0)
 	return -1;
@@ -1641,7 +1706,8 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
       && stack_cache_enabled_p
       && object != TARGET_OBJECT_STACK_MEMORY)
     {
-      dcache_update (target_dcache, memaddr, (void *) writebuf, res);
+      dcache_update (target_dcache_get ()->general, memaddr,
+		     (void *) writebuf, res);
     }
 
   /* If we still haven't got anything, return the last error.  We
@@ -5187,6 +5253,7 @@ Otherwise, any attempt to interrupt or stop will be ignored."),
 			   set_target_permissions, NULL,
 			   &setlist, &showlist);
 
-
-  target_dcache = dcache_init ();
+  target_dcache_aspace_key
+    = register_address_space_data_with_cleanup (NULL,
+						target_dcache_cleanup);
 }
-- 
1.7.7.6

Re: [PATCH 2/5] Associate target_dcache to address_space.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> Nowadays, 'target_dcache' is a global variable in GDB, which is not
Yao> necessary.  It can be a per-address-space variable.  In this patch, we
Yao> add a new structure 'target_dcache', which includes all dcaches of
Yao> target and all of them are per-address-space.

I like this patch quite a bit.  For one thing, it fixes the target
dcache for the coming multi-target work :-)

Yao> +/* The current added space.  */
Yao> +#define current_address_space current_program_space->aspace

On the whole I would prefer we not add new object-style macros like
this.  (Actually I'd really like it if we got rid of the existing ones
too...)  Writing out the expansion in the few places it is used seems
better to me.

Yao> +static void
Yao> +target_dcache_cleanup (struct address_space *aspace, void *arg)
Yao> +{
Yao> +  struct target_dcache *dcache
Yao> +    = address_space_data (aspace, target_dcache_aspace_key);
Yao> +
Yao> +  target_dcache_xfree (dcache);

Here you don't need to call address_space_data, since ARG is the dcache
that was passed in.

Tom

Re: [PATCH 2/5] Associate target_dcache to address_space.

[Yao Qi]

On 10/24/2013 12:37 AM, Tom Tromey wrote:
> I like this patch quite a bit.  For one thing, it fixes the target
> dcache for the coming multi-target work:-)
> 
> Yao> +/* The current added space.  */
> Yao> +#define current_address_space current_program_space->aspace
> 
> On the whole I would prefer we not add new object-style macros like
> this.  (Actually I'd really like it if we got rid of the existing ones
> too...)  Writing out the expansion in the few places it is used seems
> better to me.
> 

This macro is removed in the updated patch.

> Yao> +static void
> Yao> +target_dcache_cleanup (struct address_space *aspace, void *arg)
> Yao> +{
> Yao> +  struct target_dcache *dcache
> Yao> +    = address_space_data (aspace, target_dcache_aspace_key);
> Yao> +
> Yao> +  target_dcache_xfree (dcache);
> 
> Here you don't need to call address_space_data, since ARG is the dcache
> that was passed in.

You are right.  Fixed.  b.t.w, this sort of problems exist somewhere
else in the tree, I'll post a patch to fix them separately.

-- 
Yao (齐尧)

gdb:

2013-10-24  Yao Qi  <yao@codesourcery.com>

	* progspace.h (struct address_space): Declare.
	* target.c (target_dcache_aspace_key): New.
	(target_dcache): Delete.
	(struct target_dcache): New.
	(target_dcache_alloc): New function.
	(target_dcache_xfree): New function.
	(target_dcache_get): New function.
	(target_dcache_cleanup)): New function.
	(target_dcache_invalidate): Update.
	(memory_xfer_partial_1): Update.
	(initialize_targets): Initialize target_dcache_aspace_key.
---
 gdb/progspace.h |    1 +
 gdb/target.c    |   87 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/gdb/progspace.h b/gdb/progspace.h
index 3735202..bbe81a4 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -32,6 +32,7 @@ struct objfile;
 struct inferior;
 struct exec;
 struct program_space_data;
+struct address_space_data;
 
 typedef struct so_list *so_list_ptr;
 DEF_VEC_P (so_list_ptr);
diff --git a/gdb/target.c b/gdb/target.c
index 22d7fb6..073a602 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -206,6 +206,70 @@ show_targetdebug (struct ui_file *file, int from_tty,
 
 static void setup_target_debug (void);
 
+/* The target dcache is kept per-address-space.  This key lets us
+   associate the cache with the address space.  */
+
+static const struct address_space_data *target_dcache_aspace_key;
+
+/* dcache for target memory.  */
+
+struct target_dcache
+{
+  /* Cache the memory on stack and areas specified by memory
+     attributes.  */
+  DCACHE *general;
+};
+
+/* Allocate an instance of struct target_dcache.  */
+
+static struct target_dcache *
+target_dcache_alloc (void)
+{
+  struct target_dcache *dcache = xmalloc (sizeof (*dcache));
+
+  dcache->general = dcache_init ();
+
+  return dcache;
+}
+
+/* Free DCACHE and its fields.  */
+
+static void
+target_dcache_xfree (struct target_dcache *dcache)
+{
+  if (dcache != NULL)
+    {
+      dcache_free (dcache->general);
+      xfree (dcache);
+    }
+}
+
+/* Get an instance of struct target_dcache.  */
+
+static struct target_dcache *
+target_dcache_get (void)
+{
+  struct target_dcache *dcache
+    = address_space_data (current_program_space->aspace,
+			  target_dcache_aspace_key);
+
+  if (dcache == NULL)
+    {
+      dcache = target_dcache_alloc ();
+
+      set_address_space_data (current_program_space->aspace,
+			      target_dcache_aspace_key, dcache);
+    }
+
+  return dcache;
+}
+
+static void
+target_dcache_cleanup (struct address_space *aspace, void *arg)
+{
+  target_dcache_xfree (arg);
+}
+
 /* The option sets this.  */
 static int stack_cache_enabled_p_1 = 1;
 /* And set_stack_cache_enabled_p updates this.
@@ -235,15 +299,14 @@ show_stack_cache_enabled_p (struct ui_file *file, int from_tty,
   fprintf_filtered (file, _("Cache use for stack accesses is %s.\n"), value);
 }
 
-/* Cache of memory operations, to speed up remote access.  */
-static DCACHE *target_dcache;
-
 /* Invalidate the target dcache.  */
 
 void
 target_dcache_invalidate (void)
 {
-  dcache_invalidate (target_dcache);
+  struct target_dcache *td = target_dcache_get ();
+
+  dcache_invalidate (td->general);
 }
 
 /* The user just typed 'target' without the name of a target.  */
@@ -1589,14 +1652,14 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
 	  || (stack_cache_enabled_p && object == TARGET_OBJECT_STACK_MEMORY)))
     {
       if (readbuf != NULL)
-	res = dcache_xfer_memory (ops, target_dcache, memaddr, readbuf,
-				  reg_len, 0);
+	res = dcache_xfer_memory (ops, target_dcache_get ()->general,
+				  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,
-				  (void *) writebuf,
+	res = dcache_xfer_memory (ops, target_dcache_get ()->general,
+				  memaddr, (void *) writebuf,
 				  reg_len, 1);
       if (res <= 0)
 	return -1;
@@ -1641,7 +1704,8 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
       && stack_cache_enabled_p
       && object != TARGET_OBJECT_STACK_MEMORY)
     {
-      dcache_update (target_dcache, memaddr, (void *) writebuf, res);
+      dcache_update (target_dcache_get ()->general, memaddr,
+		     (void *) writebuf, res);
     }
 
   /* If we still haven't got anything, return the last error.  We
@@ -5187,6 +5251,7 @@ Otherwise, any attempt to interrupt or stop will be ignored."),
 			   set_target_permissions, NULL,
 			   &setlist, &showlist);
 
-
-  target_dcache = dcache_init ();
+  target_dcache_aspace_key
+    = register_address_space_data_with_cleanup (NULL,
+						target_dcache_cleanup);
 }
-- 
1.7.7.6

Re: [PATCH 2/5] Associate target_dcache to address_space.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2013-10-24  Yao Qi  <yao@codesourcery.com>
Yao> 	* progspace.h (struct address_space): Declare.
Yao> 	* target.c (target_dcache_aspace_key): New.
Yao> 	(target_dcache): Delete.
Yao> 	(struct target_dcache): New.
Yao> 	(target_dcache_alloc): New function.
Yao> 	(target_dcache_xfree): New function.
Yao> 	(target_dcache_get): New function.
Yao> 	(target_dcache_cleanup)): New function.
Yao> 	(target_dcache_invalidate): Update.
Yao> 	(memory_xfer_partial_1): Update.
Yao> 	(initialize_targets): Initialize target_dcache_aspace_key.

This is ok.

And, immediately contradicting myself, I think this is an improvement
regardless of whether the rest of the series goes in as-is or not.
That's because it removes a state-holding global in favor of a
parameterized approach.  So, it would be fine by me if you wanted to
push in #1 and #2.

If you'd rather wait for the whole series to be done, that's also ok, as
it isn't urgent.

Yao> +
Yao> +static void
Yao> +target_dcache_cleanup (struct address_space *aspace, void *arg)
Yao> +{

This one needs a (short) intro comment.

Tom

Re: [PATCH 2/5] Associate target_dcache to address_space.

[Doug Evans]

On Wed, Oct 23, 2013 at 1:27 AM, Yao Qi <yao@codesourcery.com> wrote:
> Hi,
> Nowadays, 'target_dcache' is a global variable in GDB, which is not
> necessary.  It can be a per-address-space variable.  In this patch, we
> add a new structure 'target_dcache', which includes all dcaches of
> target and all of them are per-address-space.
>
> There is one field 'general' which is equivalent to current
> variable 'target_dcache'.  I thought to name it 'stack', but it also
> cache memory regions marked by attributes, I choose 'general'.
>
> gdb:
>
> 2013-10-23  Yao Qi  <yao@codesourcery.com>
>
>         * progspace.h (struct address_space): Declare.
>         (current_address_space): New macro.
>         * target.c (target_dcache_aspace_key): New.
>         (target_dcache): Delete.
>         (struct target_dcache): New.
>         (target_dcache_alloc): New function.
>         (target_dcache_xfree): New function.
>         (target_dcache_get): New function.
>         (target_dcache_cleanup)): New function.
>         (target_dcache_invalidate): Update.
>         (memory_xfer_partial_1): Update.
>         (initialize_targets): Initialize target_dcache_aspace_key.
> [...]
> +/* Allocate an instance of struct target_dcache.  */
> +
> +static struct target_dcache *
> +target_dcache_alloc (void)
> +{
> +  struct target_dcache *dcache = xmalloc (sizeof (*dcache));
> +
> +  dcache->general = dcache_init ();
> +
> +  return dcache;
> +}

I think more work is needed here.
I looked for it in the patch series, apologies if I missed it!

Consider "info dcache" and "set dcache size,line-size".

They use a nasty global, last_cache, which needs to go away.

Plus, we need to specify their semantics in a cache-per-address-space world.