Re: [PATCH v2 08/11] gdbserver: use REG_UNKNOWN for a regcache's register statuses

Simon Marchi
2025-01-07T05:18:29Z

On 2024-12-30 05:49, Tankut Baris Aktemur wrote:
> When a regcache is initialized, the values of registers are not
> fetched yet. Thus, initialize the register statuses to REG_UNKNOWN
> instead of REG_UNAVAILABLE, because the latter rather means "we
> attempted to fetch but could not obtain the value".
>
> The definitions of the reg status enums (from
> gdbsupport/common-regcache.h) as a reminder:
>
> /* The register value is not in the cache, and we don't know yet
> whether it's available in the target (or traceframe). */
> REG_UNKNOWN = 0,
>
> /* The register value is valid and cached. */
> REG_VALID = 1,
>
> /* The register value is unavailable. E.g., we're inspecting a
> traceframe, and this register wasn't collected. Note that this
> "unavailable" is different from saying the register does not
> exist in the target's architecture --- in that case, the target
> should have given us a target description that does not include
> the register in the first place. */
> REG_UNAVAILABLE = -1
>
> Similarly, when the regcache is invalidated, change all the statuses
> back to REG_UNKNOWN.

The change makes sense to me. I just hope that there aren't any spots
that rely on the default value being REG_UNAVAILABLE. At least in
tracepoint.cc, all spots that fill regcache contents manually sets the
status to unavailable if needed, so that's good.

> ---
> gdbserver/regcache.cc | 4 ++--
> gdbserver/regcache.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index eab6bdcfc86124c25013c565c5432c98dfc27500..03e975dc00154d01798765273534ca9cdcdc7eb7 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -52,7 +52,7 @@ get_thread_regcache (thread_info *thread, bool fetch)
>
> switch_to_thread (thread);
> /* Invalidate all registers, to prevent stale left-overs. */
> - memset (regcache->register_status, REG_UNAVAILABLE,
> + memset (regcache->register_status, REG_UNKNOWN,
> regcache->tdesc->reg_defs.size ());
> fetch_inferior_registers (regcache, -1);
> regcache->registers_fetched = true;
> @@ -142,7 +142,7 @@ regcache::regcache (const target_desc *tdesc)
> this->registers_owned = true;
> this->register_status
> = (unsigned char *) xmalloc (tdesc->reg_defs.size ());
> - memset ((void *) this->register_status, REG_UNAVAILABLE,
> + memset ((void *) this->register_status, REG_UNKNOWN,
> tdesc->reg_defs.size ());
> }
>
> diff --git a/gdbserver/regcache.h b/gdbserver/regcache.h
> index 08c2ddd04e1307e0493b45a944bd30b762b0663b..f3e99de443b884e13b02c8d858ea9e6ee668700f 100644
> --- a/gdbserver/regcache.h
> +++ b/gdbserver/regcache.h
> @@ -43,7 +43,7 @@ struct regcache : public reg_buffer_common
> bool registers_owned = false;
> unsigned char *registers = nullptr;
> #ifndef IN_PROCESS_AGENT
> - /* One of REG_UNAVAILABLE or REG_VALID. */
> + /* See gdbsupport/common-regcache.h. */
> unsigned char *register_status = nullptr;

I don't think this comment change is super clear. I guess you want to
refer to `enum register_status`, defined in that file?

The same in GDB was changed to use `enum register_status` as the element
type, making it more self-documenting:

https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=aac0d564cea04b1c5f386e8cea924ca59057e8b4

I think it would make sense to do it in gdbserver as well.

Simon