Re: [PATCH 03/24] regcache: handle invalidated regcache

["Maciej W. Rozycki" <macro@imgtec.com>]

On Mon, 27 Jun 2016, Bhushan Attarde wrote:

>     When registers are marked as change, set a new regcache_invalidated
>     variable which is used by get_thread_regcache to decide whether to
>     recreate the gdbarch.

 Can you please be a bit more specific as to why this change is needed?

> 	gdb/ChangeLog:
> 		* regcache.c (regcache_invalidated): New variable.
> 		(set_current_thread_ptid_arch): Use regcache_invalidated to
> 		set registers_changed_p.
> 		(get_thread_regcache): Reset regcache_invalidated to 0.
> 		(registers_changed_ptid): Set regcache_invalidated to 1.

 Single-tab indentation please.

 But overall it looks like a bug fix to me, applying to 01/24 and 
addressing the assumption made there that there is only a single thread 
being debugged.  Which may have been OK for the majority of bare-metal 
cases, but certainly not for OS debugging.  So please fold this change 
into 01/24.

 But I have a concern about the implementation too.

> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index c3405b6..3cbec6e 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -489,6 +489,7 @@ struct regcache_list
>  };
>  
>  static struct regcache_list *current_regcache;
> +static int regcache_invalidated = 1;
>  
>  struct regcache *
>  get_thread_arch_aspace_regcache (ptid_t ptid, struct gdbarch *gdbarch,
> @@ -547,7 +548,7 @@ set_current_thread_ptid_arch (ptid_t ptid)
>  struct regcache *
>  get_thread_regcache (ptid_t ptid)
>  {
> -  int registers_changed_p = current_regcache == NULL;
> +  int registers_changed_p = current_regcache == NULL || regcache_invalidated;
>    struct regcache *new_regcache;
>  
>    set_current_thread_ptid_arch (ptid);
> @@ -560,6 +561,7 @@ get_thread_regcache (ptid_t ptid)
>        registers_changed ();
>        set_current_thread_ptid_arch (ptid);
>        new_regcache = get_thread_arch_regcache (ptid, current_thread_arch);
> +      regcache_invalidated = 0;
>      }
>  
>    return new_regcache;
> @@ -646,6 +648,7 @@ registers_changed_ptid (ptid_t ptid)
>  	 forget about any frames we have cached, too.  */
>        reinit_frame_cache ();
>      }
> +  regcache_invalidated = 1;
>  }

 This uses a global `regcache_invalidated' flag for what looks to me like 
a thread-specific case, i.e. you really want to set `registers_changed_p' 
above only if `ptid' thread's registers have changed.  Or in other words 
if the old regcache was gone and a new one has been allocated for `ptid' 
requested.  Yes, with the MIPS FPU topology changes in particular this may 
not matter much, but let's get this piece right for the general case, like 
multiprocess scenarios perhaps.

 So it looks to me like a better approach will be to either scan the 
regcache list from `current_regcache' to see if there's a match for `ptid' 
or to make `get_thread_arch_regcache' return additional status of the scan 
it already does.  I have no clear preference between these two choices, so 
please pick up whichever you like better or maybe a general maintainer can 
speak out.

  Maciej