Re: [PATCH 09/26] gdbserver: extract out regcache::invalidate and regcache::discard

[Simon Marchi <simark@simark.ca>]

On 2/28/23 06:28, Tankut Baris Aktemur via Gdb-patches wrote:
> Extract out a piece of code from the `regcache_invalidate_thread`
> function and turn into a new method of regcache named 'invalidate'.

For clarity, if you can think of a better name than invalidate, it would
be nice.  To me, invalidate sounds like we just mark the registers as
stale (what you discard method does, basically).  This method writes the
registers back to the target and invalidates / discard.

> We also introduce a small method named 'discard' to give the clients
> an option to discard the cache without storing the contents.  This
> method is utilized in a downstream debugger.

I would perhaps suggest keeping the discard method as a downstream
modification, since it's trivial (and therefore doesn't need much
maintenance effort to keep up to date).

> ---
>  gdbserver/regcache.cc | 24 +++++++++++++++++-------
>  gdbserver/regcache.h  |  6 ++++++
>  2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 7b6337166ad..1db78951cc2 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -85,18 +85,22 @@ regcache_invalidate_thread (struct thread_info *thread)
>  
>    regcache = thread_regcache_data (thread);
>  
> -  if (regcache == NULL)
> -    return;
> +  if (regcache != nullptr)
> +    regcache->invalidate ();
> +}
>  
> -  if (regcache->registers_valid)
> +void
> +regcache::invalidate ()

Add the typical:

/* See regcache.h.  */

> +{
> +  if (registers_valid)
>      {
>        scoped_restore_current_thread restore_thread;
> -
> -      switch_to_thread (thread);
> -      store_inferior_registers (regcache, -1);
> +      gdb_assert (this->thread != nullptr);
> +      switch_to_thread (this->thread);
> +      store_inferior_registers (this, -1);

Ok, so I note that this is another place where you use regcache::thread
(which makes sense, it's the mirror operation to fetch).  I guess that
the same idea could be use there, it could be either a method on
thread_info, or event stay a free function.

Similar comment as earlier, as a general improvement, it would be nice
if we could get rid of this switch_to_thread.

>      }
>  
> -  regcache->registers_valid = 0;
> +  discard ();

Why is it necessary to mark the register content invalid here?  In
theory, it should be valid as long as we don't resume the thread.  It's
already done like that, so it's fine to keep it as-is, I'm just
wondering.

Simon