[v2,08/11] gdbserver: use REG_UNKNOWN for a regcache's register statuses
Commit Message
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.
---
gdbserver/regcache.cc | 4 ++--
gdbserver/regcache.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
Comments
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
@@ -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 ());
}
@@ -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;
/* Constructors. */