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

[Simon Marchi <simark@simark.ca>]

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