[09/26] gdbserver: extract out regcache::invalidate and regcache::discard
Commit Message
Extract out a piece of code from the `regcache_invalidate_thread`
function and turn into a new method of regcache named 'invalidate'.
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.
---
gdbserver/regcache.cc | 24 +++++++++++++++++-------
gdbserver/regcache.h | 6 ++++++
2 files changed, 23 insertions(+), 7 deletions(-)
Comments
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
@@ -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 ()
+{
+ 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);
}
- regcache->registers_valid = 0;
+ discard ();
}
/* See regcache.h. */
@@ -121,6 +125,12 @@ regcache_invalidate (void)
#endif
+void
+regcache::discard ()
+{
+ registers_valid = 0;
+}
+
void
regcache::initialize (const target_desc *tdesc,
unsigned char *regbuf)
@@ -78,6 +78,12 @@ struct regcache : public reg_buffer_common
/* Copy the contents of SRC into this regcache. */
void copy_from (regcache *src);
+
+ /* Store the cached registers to the target and then discard the cache. */
+ void invalidate ();
+
+ /* Discard the cache without storing the registers to the target. */
+ void discard ();
};
regcache *get_thread_regcache (thread_info *thread, bool fetch = true);