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

[Doug Evans <dje at google dot com>]

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).