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

[Simon Marchi <simark@simark.ca>]

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