diff mbox series

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

Message ID 9f7a82d385ccbc4c249ff7ecebff24fa42ebb149.camel@espressif.com
State New
Headers
Series [v3] gdb: xtensa: fix registers supply if they not present |

Commit Message

Alexey Lapshin May 16, 2023, 4:17 a.m. UTC 
  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(-)

-- 
2.34.1

Comments

Alexey Lapshin April 3, 2024, 2:49 p.m. UTC | #1 
Ping
John Baldwin April 9, 2024, 5:59 p.m. UTC | #2 
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?)
diff mbox series

Patch

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