From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22106 invoked by alias); 17 Nov 2013 22:02:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 22095 invoked by uid 89); 17 Nov 2013 22:02:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mail-vb0-f54.google.com Received: from Unknown (HELO mail-vb0-f54.google.com) (209.85.212.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 17 Nov 2013 22:02:13 +0000 Received: by mail-vb0-f54.google.com with SMTP id p6so827804vbe.13 for ; Sun, 17 Nov 2013 14:02:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=q/Xlpmu05C5tsWL9QhiCusWIYxLopAC3J1HsQe3E9vY=; b=Mal3reSeRa+5rYi4BGh2hyeExF0QgciQrF1dBcA/Va/9OO03UT4elc63ZQJZecviU3 G/jThzXAStaYyD3HZFYS+ueuyUoHuX9yjmJIiobBjl263PbLyiAlcxVEI7nCKuQ/kvBP Ht8mkLEgQnr6HtqQNxO7RkJsKSyKx4uhLMRPlty9drZGrj6xqJ1GP80wXO9GAxNYCoNX D8Wal1448OAvMmsCvX1i37u/YP4S99GK+KX+AzYa8+hSPm8cakBo1y+PsrOIzR6g8L95 zBiPuze7dCaVTHgZImRIX5JstxnVpPOjfNUG80+VertlpLekEBwQvPjZNI3V+Rvvmqz1 9JiQ== X-Gm-Message-State: ALoCoQmSV0ZSKSB7dI/JWNryB1jy1VpzotImCaYIu0OIifFXBaopgN635HAO+Ep425tsNeFY0Ccdz41/Y8SxF/QqQLhx+EBGax18comk0mL+diTzmT5AKdVi5uoWm/Gtq8D5QuX+gCvzvne8s6qRxgpxlXYiNP92312+hQpyxHON6NoKLegeuBduFwkshkOyxAFyibD3GbPk27UUoykDlqjxgud2164PWg== MIME-Version: 1.0 X-Received: by 10.220.253.66 with SMTP id mz2mr12401269vcb.10.1384725724785; Sun, 17 Nov 2013 14:02:04 -0800 (PST) Received: by 10.52.163.52 with HTTP; Sun, 17 Nov 2013 14:02:04 -0800 (PST) In-Reply-To: <1383458049-20893-10-git-send-email-yao@codesourcery.com> References: <1383458049-20893-1-git-send-email-yao@codesourcery.com> <1383458049-20893-10-git-send-email-yao@codesourcery.com> Date: Mon, 18 Nov 2013 08:23:00 -0000 Message-ID: Subject: Re: [PATCH 09/10] set/show code-cache From: Doug Evans To: Yao Qi Cc: gdb-patches Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-11/txt/msg00454.txt.bz2 On Sat, Nov 2, 2013 at 10:54 PM, Yao Qi 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 > > * 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) : > New. > (target_read_code): Declare. > > gdb/doc: > > 2013-11-02 Yao Qi > > * 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).