Re: [PATCH v3] gdb: xtensa: fix registers supply if they not present

[John Baldwin <jhb@FreeBSD.org>]

On 5/16/23 12:17 AM, Alexey Lapshin via Gdb-patches wrote:
> When parsing a core file on hardware configurations without the
> zero-overhead loop option (e.g. ESP32-S2 chip), GDB used to assert
> while trying to call 'raw_supply' for lbeg, lend, lcount registers,
> even though they were not set.
> 
> This was because:
> regnum == -1 was used to indicate "supply all registers"
> lbeg_regnum == -1 was used to indicate "lbeg register not present"
> regnum == lbeg_regnum check was considered successful
> ---
>   gdb/xtensa-tdep.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/xtensa-tdep.c b/gdb/xtensa-tdep.c
> index a47a2987499..af0f5c86bab 100644
> --- a/gdb/xtensa-tdep.c
> +++ b/gdb/xtensa-tdep.c
> @@ -803,6 +803,18 @@ xtensa_register_reggroup_p (struct gdbarch *gdbarch,
>   }
>   
>   
> +/* Check if register raw supplied
> +   Note:
> +   - check_regnum == -1 means register is not present.
> +   - regnum == -1 means all present registers are supplied.  */
> +
> +static inline bool
> +is_reg_raw_supplied (int check_regnum, int regnum)
> +{
> +  return check_regnum > 0 && (regnum == check_regnum || regnum == -1);
> +}
> +
> +
>   /* Supply register REGNUM from the buffer specified by GREGS and LEN
>      in the general-purpose register set REGSET to register cache
>      REGCACHE.  If REGNUM is -1 do this for all registers in REGSET.  */
> @@ -825,22 +837,22 @@ xtensa_supply_gregset (const struct regset *regset,
>       rc->raw_supply (gdbarch_pc_regnum (gdbarch), (char *) &regs->pc);
>     if (regnum == gdbarch_ps_regnum (gdbarch) || regnum == -1)
>       rc->raw_supply (gdbarch_ps_regnum (gdbarch), (char *) &regs->ps);
> -  if (regnum == tdep->wb_regnum || regnum == -1)
> +  if (is_reg_raw_supplied (tdep->wb_regnum, regnum))
>       rc->raw_supply (tdep->wb_regnum,
>   		    (char *) &regs->windowbase);
> -  if (regnum == tdep->ws_regnum || regnum == -1)
> +  if (is_reg_raw_supplied (tdep->ws_regnum, regnum))
>       rc->raw_supply (tdep->ws_regnum,
>   		    (char *) &regs->windowstart);
> -  if (regnum == tdep->lbeg_regnum || regnum == -1)
> +  if (is_reg_raw_supplied (tdep->lbeg_regnum, regnum))
>       rc->raw_supply (tdep->lbeg_regnum,
>   		    (char *) &regs->lbeg);
> -  if (regnum == tdep->lend_regnum || regnum == -1)
> +  if (is_reg_raw_supplied (tdep->lend_regnum, regnum))
>       rc->raw_supply (tdep->lend_regnum,
>   		    (char *) &regs->lend);
> -  if (regnum == tdep->lcount_regnum || regnum == -1)
> +  if (is_reg_raw_supplied (tdep->lcount_regnum, regnum))
>       rc->raw_supply (tdep->lcount_regnum,
>   		    (char *) &regs->lcount);
> -  if (regnum == tdep->sar_regnum || regnum == -1)
> +  if (is_reg_raw_supplied (tdep->sar_regnum, regnum))
>       rc->raw_supply (tdep->sar_regnum,
>   		    (char *) &regs->sar);
>     if (regnum >=tdep->ar_base

This looks ok to me, but is a bit ununusal.  For other GDB arches we tend to add
helper methods to the tdep class to indicate if an optional feature is present
and then use those helpers in functions like this.  For example, aarch64 has an
optional TLS register set with the following class members:

/* Target-dependent structure in gdbarch.  */
struct aarch64_gdbarch_tdep : gdbarch_tdep_base
{
...
   /* TLS registers.  This is -1 if the TLS registers are not available.  */
   int tls_regnum_base = 0;
   int tls_register_count = 0;

   bool has_tls() const
   {
     return tls_regnum_base != -1;
   }
...
};

Code that supports the TLS register set uses the has_tls function to instead
of comparing tls_regnum_base against -1 explicitly, e.g. in aarch64-fbsd-nat.c:

void
aarch64_fbsd_nat_target::fetch_registers (struct regcache *regcache,
					  int regnum)
{
...
   gdbarch *gdbarch = regcache->arch ();
   aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
   if (tdep->has_tls ())
     fetch_regset<uint64_t> (regcache, regnum, NT_ARM_TLS,
			    &aarch64_fbsd_tls_regset, tdep->tls_regnum_base);
}

Looking at the xtensa-tdep bits more though, I don't think there is a clear
analog to this for the way xtensa_rmap[] works (it is not clear to me how
a xtensa gdbarch can ever have a subset of registers given that the same
static xtensa_rmap[] is always passed to the tdep object constructed in
xtensa_gdbarch_init and xtensa_derive_tdep is called immediately after the
constructor, so it seems like lbeg_regnum should always be set?)

-- 
John Baldwin