This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Remove frame_observer_target_changed
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <yao at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 12 Sep 2012 15:41:32 +0100
- Subject: Re: [PATCH] Remove frame_observer_target_changed
- References: <1347457356-30627-1-git-send-email-yao@codesourcery.com>
On 09/12/2012 02:42 PM, Yao Qi wrote:
> Hi,
> At the bottom of valops.c:value_assign, we have code to call
> reinit_frame_cache,
>
> /* Assigning to the stack pointer, frame pointer, and other
> (architecture and calling convention specific) registers may
> cause the frame cache to be out of date. Assigning to memory
> also can. We just do this on all assignments to registers or
> memory, for simplicity's sake; I doubt the slowdown matters. */
> switch (VALUE_LVAL (toval))
> {
> case lval_memory:
> case lval_register:
> case lval_computed:
>
> reinit_frame_cache ();
>
> in the switch block above, observer_notify_target_changed will be
> called.
>
> switch (VALUE_LVAL (toval))
> ....
> case lval_register:
> {
> ...
> observer_notify_target_changed (¤t_target);
>
> However, frame_observer_target_changed is attached to 'target_changed'
> observer in frame.c:_initialize_frame, which calls reinit_frame_cache.
> In this path, reinit_frame_cache is called twice. As
> observer_notify_target_changed is only used in valops.c:value_assign,
> so we can remove frame_observer_target_changed and don't attach to
> observer 'target_changed'. This is what this patch tries to do.
>
> Regression tested on x86_64-linux. OK?
Hmm. It looks okay if you only take the duplication into account.
But, is there a general direction this is following? I ask because
reinit_frame_cache will still be called twice, through
regcache_observer_target_changed, which is installed as target_changed observer
too. Extra calls to reinit_frame_cache should be extremely cheap (given
the caches are built on demand).
The observer was introduced from this discussion:
http://sourceware.org/ml/gdb-patches/2004-04/msg00520.html
And looking at:
/* Assigning to the stack pointer, frame pointer, and other
(architecture and calling convention specific) registers may
cause the frame cache to be out of date. Assigning to memory
also can. We just do this on all assignments to registers or
memory, for simplicity's sake; I doubt the slowdown matters. */
switch (VALUE_LVAL (toval))
{
case lval_memory:
case lval_register:
case lval_computed:
reinit_frame_cache ();
makes me wonder ... shouldn't we also be invalidating the register
cache when we change memory, considering writes to memory-mapped
registers? Andrew even mentioned something of the sort in that
email - it seems like it was overlooked. With that in mind, we'd go the
other way around, and replace that reinit_frame_cache call with a
observer_notify_target_changed, and remove the call from
the earlier switch.
>
> gdb:
>
> 2012-09-12 Yao Qi <yao@codesourcery.com>
>
> * frame.c: Remove inclusion of "observer.h".
> (frame_observer_target_changed): Remove.
> (_initialize_frame): Don't call
> observer_attach_target_changed.
> ---
> gdb/frame.c | 11 -----------
> 1 files changed, 0 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 9ed49f6..a4dcb59 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -37,7 +37,6 @@
> #include "frame-base.h"
> #include "command.h"
> #include "gdbcmd.h"
> -#include "observer.h"
> #include "objfiles.h"
> #include "exceptions.h"
> #include "gdbthread.h"
> @@ -1514,14 +1513,6 @@ get_next_frame (struct frame_info *this_frame)
> return NULL;
> }
>
> -/* Observer for the target_changed event. */
> -
> -static void
> -frame_observer_target_changed (struct target_ops *target)
> -{
> - reinit_frame_cache ();
> -}
> -
> /* Flush the entire frame cache. */
>
> void
> @@ -2449,8 +2440,6 @@ _initialize_frame (void)
> {
> obstack_init (&frame_cache_obstack);
>
> - observer_attach_target_changed (frame_observer_target_changed);
> -
> add_prefix_cmd ("backtrace", class_maintenance, set_backtrace_cmd, _("\
> Set backtrace specific variables.\n\
> Configure backtrace variables such as the backtrace limit"),
>
--
Pedro Alves