Re: [PATCH 09/10] set/show code-cache

Doug Evans
2013-11-17T22:02:15Z

On Sat, Nov 2, 2013 at 10:54 PM, 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.

To help avoid future confusion, I think it would be useful to rephrase this as:

"[...] and add a new option "set code-cache on|off" to optimize code
accesses by using the target memory cache."

I'd be happy with a better name for code-cache too if that helped.
"code-cache" is good enough though.

>
> gdb:
>
> 2013-11-02 Yao Qi <yao@codesourcery.com>
>
> * NEWS: Add note on new "set code-cache" option.
> * target-dcache.c (code_cache_enabled_p): New.
> (show_code_cache_enabled_p): New function.
> (code_cache_enabled): New function.
> (_initialize_target_dcache): Register command.
> * target-dcache.h (code_cache_enabled): Declare.
> * target.c (memory_xfer_partial_1):Handle
> TARGET_OBJECT_CODE_MEMORY and code_cache_enabled.
> (target_read_code): New function.
> * target.h (enum target_object) <TARGET_OBJECT_CODE_MEMORY>:
> New.
> (target_read_code): Declare.
>
> gdb/doc:
>
> 2013-11-02 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 ++++++++++++++-
> gdb/target-dcache.c | 32 ++++++++++++++++++++++++++++++++
> gdb/target-dcache.h | 2 ++
> gdb/target.c | 22 +++++++++++++++++++---
> gdb/target.h | 5 +++++
> 6 files changed, 78 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index efeda68..dad167c 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -109,6 +109,12 @@ show startup-with-shell
> Specifies whether Unix child processes are started via a shell or
> directly.
>
> +set code-cache
> +show code-cache
> + Use more aggressive caching for accesses to the code segment. This
> + improves performance of remote debugging (particularly disassembly)
> + without affecting correctness.

I would rephrase this as:

Use the target memory cache for accesses to the code segment.
This improves the performance of remote debugging (particularly disassembly)
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 a64bb0b..405e4b5 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -10826,7 +10826,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 code segment.
> Other regions of memory can be explicitly marked as
> cacheable; see @pxref{Memory Region Attributes}.
>
> @@ -10851,6 +10852,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 code segment accesses. When @code{ON},
> +use caching. By default, this option is @code{ON}. This improves
> +performance of disassembly in remote debugging without affecting
> +correctness.
> +
> +@kindex show code-cache
> +@item show code-cache
> +Show the current state of data caching for code segment accesses.
> +
> @kindex info dcache
> @item info dcache @r{[}line@r{]}
> Print the information about the performance of data cache of the
> diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c
> index ce36bbb..e6d11b9 100644
> --- a/gdb/target-dcache.c
> +++ b/gdb/target-dcache.c
> @@ -120,6 +120,27 @@ stack_cache_enabled (void)
> return stack_cache_enabled_p;
> }
>
> +/* The option sets this. */
> +
> +static int code_cache_enabled_p = 1;
> +
> +/* Show option "code-cache". */
> +
> +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);
> +}
> +
> +/* Return true if "stack cache" is enabled, otherwise, return false. */
> +
> +int
> +code_cache_enabled (void)
> +{
> + return code_cache_enabled_p;
> +}

IWBN to name the function code_cache_enabled_p, but it could be left for later.

> +
> /* -Wmissing-prototypes */
> extern initialize_file_ftype _initialize_target_dcache;
>
> @@ -137,6 +158,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, _("\
> +Set cache use for code segment access."), _("\
> +Show cache use for code segment access."), _("\
> +When on, use the data cache for all code segment access, regardless\n\
> +of any configured memory regions. This improves remote performance\n\
> +significantly. By default, caching for code segment access is on."),
> + NULL,
> + show_code_cache_enabled_p,
> + &setlist, &showlist);

This would be clearer if "data cache" was replaced with "target memory cache".
OTOH, we use "dcache" in the various option names. Blech.
[Ok to leave the wording as is, just wanted to make this comment.]

> +
> target_dcache_aspace_key
> = register_address_space_data_with_cleanup (NULL,
> target_dcache_cleanup);
> diff --git a/gdb/target-dcache.h b/gdb/target-dcache.h
> index 06a938f..17c61a1 100644
> --- a/gdb/target-dcache.h
> +++ b/gdb/target-dcache.h
> @@ -26,3 +26,5 @@ extern DCACHE *target_dcache_get_or_init (void);
> extern int target_dcache_init_p (void);
>
> extern int stack_cache_enabled (void);
> +
> +extern int code_cache_enabled (void);
> diff --git a/gdb/target.c b/gdb/target.c
> index 55c5d78..6bdc671 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1547,7 +1547,8 @@ 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 () && object == TARGET_OBJECT_STACK_MEMORY)))
> + || (stack_cache_enabled () && object == TARGET_OBJECT_STACK_MEMORY)
> + || (code_cache_enabled () && object == TARGET_OBJECT_CODE_MEMORY)))
> {
> DCACHE *dcache = target_dcache_get_or_init ();
>
> @@ -1601,7 +1602,8 @@ memory_xfer_partial_1 (struct target_ops *ops, enum target_object object,
> && writebuf != NULL
> && target_dcache_init_p ()
> && !region->attrib.cache
> - && object != TARGET_OBJECT_STACK_MEMORY)
> + && object != TARGET_OBJECT_STACK_MEMORY
> + && object != TARGET_OBJECT_CODE_MEMORY)
> {
> DCACHE *dcache = target_dcache_get ();
>
> @@ -1690,7 +1692,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
> @@ -1792,6 +1795,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
> diff --git a/gdb/target.h b/gdb/target.h
> index 7a93646..7f43b4e 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -145,6 +145,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. */
> @@ -1050,6 +1053,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);
>

I don't see target_read_code called from anywhere but I'm guessing
that's in 10/10.

Patch is ok to me (with the suggested wording change to the NEWS entry).