Re: [PATCH V2 2/2] gdb/arm: Sync sp with other *sp registers

[Luis Machado <luis.machado@arm.com>]

Hi,

On 7/7/22 10:21, Yvan Roux wrote:
> For Arm Cortex-M33 with security extensions, there are 4 different
> stack pointers (msp_s, msp_ns, psp_s, psp_ns), without security
> extensions and for other Cortex-M targets, there are 2 different
> stack pointers (msp and psp).
> 
> With this path, sp will always be in sync with one of the real stack

path -> patch
> pointers on Arm targets that contains more than one stack pointer.

contains -> contain.

> 
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> Signed-off-by: Yvan Roux <yvan.roux@foss.st.com>
> ---
>   gdb/arm-tdep.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 89 insertions(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 89c2779bbb5..c28543229de 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3812,6 +3812,85 @@ arm_dwarf2_prev_register (struct frame_info *this_frame, void **this_cache,
>         return frame_unwind_got_constant (this_frame, regnum, cpsr);
>   
>       default:
> +      /* Handle the alternative SP on Cortex-M.  */
> +      if (arm_is_alternative_sp_register (tdep, regnum))
> +	{
> +	  bool override_with_sp_value = false;
> +	  CORE_ADDR val;
> +
> +	  if (tdep->have_sec_ext)
> +	    {
> +	      CORE_ADDR sp
> +		= get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> +	      CORE_ADDR msp_s
> +		= get_frame_register_unsigned (this_frame,
> +					       tdep->m_profile_msp_s_regnum);
> +	      CORE_ADDR msp_ns
> +		= get_frame_register_unsigned (this_frame,
> +					       tdep->m_profile_msp_ns_regnum);
> +	      CORE_ADDR psp_s
> +		= get_frame_register_unsigned (this_frame,
> +					       tdep->m_profile_psp_s_regnum);
> +	      CORE_ADDR psp_ns
> +		= get_frame_register_unsigned (this_frame,
> +					       tdep->m_profile_psp_ns_regnum);
> +
> +	      if (regnum == tdep->m_profile_msp_regnum
> +		  && (msp_s == sp || msp_ns == sp))
> +		/* Use value of SP for MSP register.  */
> +		override_with_sp_value = true;
> +	      else if (regnum == tdep->m_profile_msp_s_regnum && msp_s == sp)
> +		/* Use value of SP for MSP_S register.  */
> +		override_with_sp_value = true;
> +	      else if (regnum == tdep->m_profile_msp_ns_regnum && msp_ns == sp)
> +		/* Use value of SP for MSP_NS register.  */
> +		override_with_sp_value = true;
> +	      else if (regnum == tdep->m_profile_psp_regnum
> +		       && (psp_s == sp || psp_ns == sp))
> +		/* Use value of SP for PSP register.  */
> +		override_with_sp_value = true;
> +	      else if (regnum == tdep->m_profile_psp_s_regnum && psp_s == sp)
> +		/* Use value of SP for PSP_S register.  */
> +		override_with_sp_value = true;
> +	      else if (regnum == tdep->m_profile_psp_ns_regnum && psp_ns == sp)
> +		/* Use value of SP for PSP_NS register.  */
> +		override_with_sp_value = true;

Can the above be simplified in some way? They outcomes are the same, so maybe we
could have a single condition.

> +	    }
> +	  else if (tdep->is_m)
> +	    {
> +	      CORE_ADDR sp
> +		= get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
> +	      CORE_ADDR msp
> +		= get_frame_register_unsigned (this_frame,
> +					       tdep->m_profile_msp_regnum);
> +	      CORE_ADDR psp
> +		= get_frame_register_unsigned (this_frame,
> +					       tdep->m_profile_psp_regnum);
> +
> +	      if (regnum == tdep->m_profile_msp_regnum && sp == msp)
> +		/* Use value of SP for MSP register.  */
> +		override_with_sp_value = true;
> +	      else if (regnum == tdep->m_profile_psp_regnum && sp == psp)
> +		/* Use value of SP for PSP register.  */
> +		override_with_sp_value = true;
> +	    }

Same here.

> +
> +	  if (override_with_sp_value)
> +	    {
> +	      /* Use value of SP from previous frame.  */
> +	      struct frame_info *prev_frame = get_prev_frame(this_frame);

Formatting: Space before (

> +	      if (prev_frame)
> +		val = get_frame_register_unsigned (prev_frame, ARM_SP_REGNUM);
> +	      else
> +		val = get_frame_base(this_frame);

Formatting: Space before (

> +	    }
> +	  else
> +	    /* Use value for the register from previous frame.  */
> +	    val = get_frame_register_unsigned (this_frame, regnum);
> +
> +	  return frame_unwind_got_constant(this_frame, regnum, val);

Formatting: Space before (

Please check for other occurrences.

> +	}
> +
>         internal_error (__FILE__, __LINE__,
>   		      _("Unexpected register %d"), regnum);
>       }
> @@ -4931,6 +5010,8 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
>   			   struct dwarf2_frame_state_reg *reg,
>   			   struct frame_info *this_frame)
>   {
> +  arm_gdbarch_tdep *tdep = (arm_gdbarch_tdep *) gdbarch_tdep (gdbarch);
> +
>     if (is_pacbti_pseudo (gdbarch, regnum))
>       {
>         /* Initialize RA_AUTH_CODE to zero.  */
> @@ -4950,6 +5031,14 @@ arm_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
>       case ARM_SP_REGNUM:
>         reg->how = DWARF2_FRAME_REG_CFA;
>         break;
> +    default:
> +      /* Handle the alternative SP on Cortex-M.  */

Maybe "Handle the alternative SP registers on Cortex-M."?

> +      if (arm_is_alternative_sp_register (tdep, regnum))
> +	{
> +	  reg->how = DWARF2_FRAME_REG_FN;
> +	  reg->loc.fn = arm_dwarf2_prev_register;
> +	}
> +      break;
>       }
>   }
>   

Otherwise this LGTM.