This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Remove frame_observer_target_changed


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 (&current_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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]