Re: [PATCH 21/26] gdbserver: use REG_UNKNOWN for a regcache's register statuses

[Simon Marchi <simark@simark.ca>]

On 2023-02-28 06:28, Tankut Baris Aktemur via Gdb-patches 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.

That makes sense to me.

> ---
>  gdbserver/regcache.cc | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
> index 09ea58bdbd6..2befb30e337 100644
> --- a/gdbserver/regcache.cc
> +++ b/gdbserver/regcache.cc
> @@ -64,9 +64,17 @@ regcache::fetch ()
>        switch_to_thread (this->thread);
>  
>        /* Invalidate all registers, to prevent stale left-overs.  */
> -      memset (register_status, REG_UNAVAILABLE, tdesc->reg_defs.size ());
> +      discard ();
>        fetch_inferior_registers (this, -1);
>        registers_fetched = true;
> +
> +      /* Make sure that the registers that could not be fetched are
> +	 now unavailable.  */
> +      for (int i = 0; i < tdesc->reg_defs.size (); ++i)
> +	{
> +	  if (register_status[i] == REG_UNKNOWN)
> +	    set_register_status (i, REG_UNAVAILABLE);
> +	}

The braces are not needed.

But is it really needed to do this?  Unavailable is only meaningful for
tracing, in a regcache that is the result of tracepoint collection.  For
a regcache created by reading registers from the target, I don't think
that setting statuses to unavailable makes sense.

Also, if asking the target to fetch all registers, why would some
registers still be unknown?  It's the target that provides the target
description, it's supposed to only include registers that exist (like
the comment on top of REG_UNAVAILABLE says).  So it should be able to
read them all.  In other words, I think this loop should be asserting
that all statuses are REG_VALID.

But then for the tracing case, I wonder who sets the statuses to
REG_UNAVAILABLE for those statuses that should really be
REG_UNAVAILABLE.  From what I can see, all regcaches used on the tracing
code paths use the regcache constructor with 2 arguments, which means
regcaches that don't track the status of registers
(regcache::register_status is nullptr).  So... now that you changed
regcache to use REG_UNKNOWN by default, is REG_UNAVAILABLE really
used in GDBserver?

I noticed that do_action_at_tracepoint has a comment that says "Collect
all registers for now.".  I guess that if it didn't collect all
registers, this would be a spot that would explicitly set some registers
to REG_UNAVAILABLE.

I see that for fast tracepoints, for instance
supply_fast_tracepoint_registers in linux-amd64-ipa.cc, we supply a
fixed set of registers.  Clearly, these are not all the registers that
an amd64 machine could have.  But it just writes a register buffer that
ends up in the trace, there doesn't seem to be a way to communicate
which registers were collected and which weren't.  When we create a new
regcache to read these registers, when handling the 'g' packet in
server.cc, there's no way to know which individual registers were
collected and which weren't.

Simon