Re: [PATCH v2 10/11] gdbserver: refactor the definition and uses of supply_regblock

Simon Marchi
2025-01-07T05:35:45Z

On 2024-12-30 05:49, Tankut Baris Aktemur wrote:
> The supply_regblock function takes a pointer to a buffer as an
> argument and implements two different behavior based on the pointer
> being null. There are three cases where we pass nullptr, all in
> tracepoint.cc, where we are essentially doing a reset on the regcache.
>
> In fast_tracepoint_ctx::get_regcache and
> static_tracepoint_ctx::get_regcache, register_status array does not
> even exist. Hence, those uses simply boil down to zeroing of register
> data. Do this at the time of creating the buffer and remove the calls
> to supply_regblock.
>
> In fetch_traceframe_registers, inline the use with a call to `reset`.
>
> Hence, there are no more cases left, where a nullptr would be passed
> to supply_regblock. Assert that the buffer argument is non-null and
> simplify the implementation.
> ---
> gdbserver/regcache.cc | 14 +++++---------
> gdbserver/tracepoint.cc | 8 +++++---
> 2 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 7e1a722b0f80b9e5641bddfbbe95cbcf2b228df8..562524306dfb36c8be88be5056b2fdddb6ca0b3c 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -369,18 +369,14 @@ supply_register_by_name_zeroed (struct regcache *regcache,
> void
> supply_regblock (struct regcache *regcache, const void *buf)
> {
> - if (buf)
> - {
> - const struct target_desc *tdesc = regcache->tdesc;
> + gdb_assert (buf != nullptr);
> + const struct target_desc *tdesc = regcache->tdesc;
>
> - memcpy (regcache->registers, buf, tdesc->registers_size);
> + memcpy (regcache->registers, buf, tdesc->registers_size);
> #ifndef IN_PROCESS_AGENT
> - for (int i = 0; i < tdesc->reg_defs.size (); i++)
> - regcache->set_register_status (i, REG_VALID);
> + for (int i = 0; i < tdesc->reg_defs.size (); i++)
> + regcache->set_register_status (i, REG_VALID);
> #endif
> - }
> - else
> - regcache->reset (REG_UNAVAILABLE);
> }
>
> #ifndef IN_PROCESS_AGENT
> diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
> index 9a00d1f272f905cadbc87265ed0be3d87f7b54df..9e48b1e5afc75c028942212c0f9e788afe025c38 100644
> --- a/gdbserver/tracepoint.cc
> +++ b/gdbserver/tracepoint.cc
> @@ -4697,7 +4697,6 @@ fast_tracepoint_ctx::get_regcache ()
> if (!this->regcache.has_value ())
> {
> this->regcache.emplace (ipa_tdesc, this->regspace);
> - supply_regblock (&this->regcache.value (), nullptr);
> supply_fast_tracepoint_registers (&this->regcache.value (),
> this->regs);
> }
> @@ -4714,7 +4713,6 @@ static_tracepoint_ctx::get_regcache ()
> if (!this->regcache.has_value ())
> {
> this->regcache.emplace (ipa_tdesc, this->regspace);
> - supply_regblock (&this->regcache.value (), nullptr);
> /* Pass down the tracepoint address, because REGS doesn't
> include the PC, but we know what it must have been. */
> supply_static_tracepoint_registers (&this->regcache.value (),

Nit: can you please leave an empty line before the comment?

> @@ -5168,7 +5166,7 @@ fetch_traceframe_registers (int tfnum, struct regcache *regcache, int regnum)
> if (dataptr == NULL)
> {
> /* Mark registers unavailable. */
> - supply_regblock (regcache, NULL);
> + regcache->reset (REG_UNAVAILABLE);
>
> /* We can generally guess at a PC, although this will be
> misleading for while-stepping frames and multi-location
> @@ -5776,6 +5774,8 @@ gdb_collect (struct tracepoint *tpoint, unsigned char *regs)
> return;
> }
>
> + memset (ctx.regspace, 0, ipa_tdesc->registers_size);

Alternatively, I guess that the regcache constructor that takes an
external buffer could take care of zeroing out the buffer, ensuring the
invariant of having the register contents to 0 when unknown/unavailable
is respected.

Either way:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon