Re: [PATCH 2/5] gdb/arm: Define MSP and PSP registers for M-Profile

[Luis Machado <luis.machado@linaro.org>]

On 1/14/22 1:35 PM, Christophe Lyon via Gdb-patches wrote:
> This patch removes the hardcoded access to PSP in
> arm_m_exception_cache() and relies on the definition with the XML
> descriptions.
> ---
>   gdb/arch/arm.h                              |  6 ++-
>   gdb/arm-tdep.c                              | 45 ++++++++-------------
>   gdb/features/arm/arm-m-profile-with-fpa.c   |  2 +
>   gdb/features/arm/arm-m-profile-with-fpa.xml |  2 +
>   gdb/features/arm/arm-m-profile.c            |  2 +
>   gdb/features/arm/arm-m-profile.xml          |  2 +
>   6 files changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
> index eabcb434f1f..638780011fc 100644
> --- a/gdb/arch/arm.h
> +++ b/gdb/arch/arm.h
> @@ -49,6 +49,8 @@ enum gdb_regnum {
>     ARM_D0_REGNUM,		/* VFP double-precision registers.  */
>     ARM_D31_REGNUM = ARM_D0_REGNUM + 31,
>     ARM_FPSCR_REGNUM,
> +  ARM_MSP_REGNUM,		/* Cortex-M Main Stack Pointer.  */
> +  ARM_PSP_REGNUM,		/* Cortex-M Process Stack Pointer.  */

Commit ae66a8f19ef6bf2dc7369cf26073f34ddf7c175b started moving things 
away from these hardcoded register numbers. So I'd rather do a runtime 
discovery of register numbers as opposed to having these.

The above commit records some register set data in struct gdbarch_tdep:

   bool have_mve;        /* Do we have a MVE extension?  */
   int mve_vpr_regnum;   /* MVE VPR register number.  */
   int mve_pseudo_base;  /* Number of the first MVE pseudo register.  */
   int mve_pseudo_count; /* Total number of MVE pseudo registers.  */

So, for example, we'd record the numbers for both MSP and PSP.

>   
>     /* Other useful registers.  */
>     ARM_FP_REGNUM = 11,		/* Frame register in ARM code, if used.  */
> @@ -65,8 +67,8 @@ enum arm_register_counts {
>     ARM_NUM_ARG_REGS = 4,
>     /* Number of floating point argument registers.  */
>     ARM_NUM_FP_ARG_REGS = 4,
> -  /* Number of registers (old, defined as ARM_FPSCR_REGNUM + 1.  */
> -  ARM_NUM_REGS = ARM_FPSCR_REGNUM + 1
> +  /* Number of registers (old, defined as ARM_PSP_REGNUM + 1.  */
> +  ARM_NUM_REGS = ARM_PSP_REGNUM + 1
>   };

The above constant (ARM_NUM_REGS) is no longer representative of the 
full set of available registers. It is now used to initialize the number 
of registers before discovery and xml parsing. I'd like to see this 
numbering scheme replaced by a more dynamic mechanism, similar to how 
the MVE register set does.

>   
>   /* Enum describing the different kinds of breakpoints.  */
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 14ec0bc8f9e..b6a1deafad5 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3012,7 +3012,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
>     enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>     struct arm_prologue_cache *cache;
>     CORE_ADDR lr;
> -  CORE_ADDR sp;
>     CORE_ADDR unwound_sp;
>     LONGEST xpsr;
>     uint32_t exc_return;
> @@ -3028,7 +3027,6 @@ arm_m_exception_cache (struct frame_info *this_frame)
>        to the exception and if FPU is used (causing extended stack frame).  */
>   
>     lr = get_frame_register_unsigned (this_frame, ARM_LR_REGNUM);
> -  sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>   
>     /* Check EXC_RETURN indicator bits.  */
>     exc_return = (((lr >> 28) & 0xf) == 0xf);
> @@ -3037,37 +3035,13 @@ arm_m_exception_cache (struct frame_info *this_frame)
>     process_stack_used = ((lr & (1 << 2)) != 0);
>     if (exc_return && process_stack_used)
>       {
> -      /* Thread (process) stack used.
> -	 Potentially this could be other register defined by target, but PSP
> -	 can be considered a standard name for the "Process Stack Pointer".
> -	 To be fully aware of system registers like MSP and PSP, these could
> -	 be added to a separate XML arm-m-system-profile that is valid for
> -	 ARMv6-M and ARMv7-M architectures. Also to be able to debug eg a
> -	 corefile off-line, then these registers must be defined by GDB,
> -	 and also be included in the corefile regsets.  */
> -
> -      int psp_regnum = user_reg_map_name_to_regnum (gdbarch, "psp", -1);
> -      if (psp_regnum == -1)
> -	{
> -	  /* Thread (process) stack could not be fetched,
> -	     give warning and exit.  */
> -
> -	  warning (_("no PSP thread stack unwinding supported."));
> -
> -	  /* Terminate any further stack unwinding by refer to self.  */
> -	  cache->prev_sp = sp;
> -	  return cache;
> -	}
> -      else
> -	{
> -	  /* Thread (process) stack used, use PSP as SP.  */
> -	  unwound_sp = get_frame_register_unsigned (this_frame, psp_regnum);
> -	}
> +      /* Thread (process) stack used, use PSP as SP.  */
> +      unwound_sp = get_frame_register_unsigned (this_frame, ARM_PSP_REGNUM);
>       }
>     else
>       {
>         /* Main stack used, use MSP as SP.  */
> -      unwound_sp = sp;
> +      unwound_sp = get_frame_register_unsigned (this_frame, ARM_MSP_REGNUM);
>       } >
>     /* The hardware saves eight 32-bit words, comprising xPSR,
> @@ -9296,6 +9270,19 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>         if (!valid_p)
>   	return NULL;
>   
> +      if (is_m)
> +	{
> +	  feature = tdesc_find_feature (tdesc,
> +					"org.gnu.gdb.arm.m-system");

The above xml feature is not present in the xml files that were modified 
below. Is this feature being generated by some debugging stub/probe maybe?

> +	  if (feature != NULL)
> +	    {
> +	      valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
> +						  ARM_MSP_REGNUM, "msp");
> +	      valid_p &= tdesc_numbered_register (feature, tdesc_data.get (),
> +						  ARM_PSP_REGNUM, "psp");
> +	    }
> +	}
> +
>         feature = tdesc_find_feature (tdesc,
>   				    "org.gnu.gdb.arm.fpa");
>         if (feature != NULL)
> diff --git a/gdb/features/arm/arm-m-profile-with-fpa.c b/gdb/features/arm/arm-m-profile-with-fpa.c
> index 2b7c78597bb..4afba856875 100644
> --- a/gdb/features/arm/arm-m-profile-with-fpa.c
> +++ b/gdb/features/arm/arm-m-profile-with-fpa.c
> @@ -35,5 +35,7 @@ create_feature_arm_arm_m_profile_with_fpa (struct target_desc *result, long regn
>     tdesc_create_reg (feature, "", regnum++, 1, NULL, 96, "arm_fpa_ext");
>     tdesc_create_reg (feature, "", regnum++, 1, NULL, 32, "int");
>     tdesc_create_reg (feature, "xpsr", regnum++, 1, NULL, 32, "int");
> +  tdesc_create_reg (feature, "msp", regnum++, 1, NULL, 32, "data_ptr");
> +  tdesc_create_reg (feature, "psp", regnum++, 1, NULL, 32, "data_ptr");
>     return regnum;
>   }
> diff --git a/gdb/features/arm/arm-m-profile-with-fpa.xml b/gdb/features/arm/arm-m-profile-with-fpa.xml
> index fceeaea7177..ee796b549a2 100644
> --- a/gdb/features/arm/arm-m-profile-with-fpa.xml
> +++ b/gdb/features/arm/arm-m-profile-with-fpa.xml
> @@ -36,4 +36,6 @@
>     <reg name="" bitsize="32"/>
>   
>     <reg name="xpsr" bitsize="32" regnum="25"/>
> +  <reg name="msp" bitsize="32" type="data_ptr"/>
> +  <reg name="psp" bitsize="32" type="data_ptr"/>

In this xml...

>   </feature>
> diff --git a/gdb/features/arm/arm-m-profile.c b/gdb/features/arm/arm-m-profile.c
> index 027f3c15c56..db6c0b10d15 100644
> --- a/gdb/features/arm/arm-m-profile.c
> +++ b/gdb/features/arm/arm-m-profile.c
> @@ -27,5 +27,7 @@ create_feature_arm_arm_m_profile (struct target_desc *result, long regnum)
>     tdesc_create_reg (feature, "pc", regnum++, 1, NULL, 32, "code_ptr");
>     regnum = 25;
>     tdesc_create_reg (feature, "xpsr", regnum++, 1, NULL, 32, "int");
> +  tdesc_create_reg (feature, "msp", regnum++, 1, NULL, 32, "data_ptr");
> +  tdesc_create_reg (feature, "psp", regnum++, 1, NULL, 32, "data_ptr");
>     return regnum;
>   }
> diff --git a/gdb/features/arm/arm-m-profile.xml b/gdb/features/arm/arm-m-profile.xml
> index d56fb246342..3253d1c5339 100644
> --- a/gdb/features/arm/arm-m-profile.xml
> +++ b/gdb/features/arm/arm-m-profile.xml
> @@ -24,4 +24,6 @@
>     <reg name="lr" bitsize="32"/>
>     <reg name="pc" bitsize="32" type="code_ptr"/>
>     <reg name="xpsr" bitsize="32" regnum="25"/>
> +  <reg name="msp" bitsize="32" type="data_ptr"/>
> +  <reg name="psp" bitsize="32" type="data_ptr"/>

... and in this xml the psp and msp registers are present in the 
"org.gnu.gdb.arm.m-profile" feature, not "org.gnu.gdb.arm.m-system".

So the code above that checks for the "org.gnu.gdb.arm.m-system" feature 
won't work as expected.

I like the idea of having these two registers in a separate feature like 
"org.gnu.gdb.arm.m-system". I think it is cleaner that way, and easier 
to manage.

>   </feature>
>