[v2,10/11] gdbserver: refactor the definition and uses of supply_regblock
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
Commit Message
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(-)
Comments
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
@@ -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
@@ -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 (),
@@ -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);
+
for (ctx.tpoint = tpoint;
ctx.tpoint != NULL && ctx.tpoint->address == tpoint->address;
ctx.tpoint = ctx.tpoint->next)
@@ -6664,6 +6664,8 @@ gdb_probe (const struct marker *mdata, void *probe_private,
return;
}
+ memset (ctx.regspace, 0, ipa_tdesc->registers_size);
+
tpoint = ust_marker_to_static_tracepoint (mdata);
if (tpoint == NULL)
{