Re: [PATCH] gdb/arm: Handle lazy FPU register stacking

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

I think this would be better sent to gdb-patches@sourceware.com.

R.

On 27/09/2022 15:20, Torbjörn SVENSSON via Binutils wrote:
> Read LSPEN, ASPEN and LSPACT bits from FPCCR and use them for the
> first exception frame.  All frames after will always have the FPU
> registers on the stack, regadless if lazy stacking is active or
> inactive.  See "Lazy context save of FP state", in B1.5.7, also ARM
> AN298, supported by Cortex-M4F architecture for details on lazy FPU
> register stacking.  The same conditions are valid for other Cortex-M
> cores with FPU.
> 
> This patch has been verified on a STM32F4-Discovery board by:
> a) writing a non-zero value (lets use 0x1122334455667788 as an
>     example) to all the D-registers in the main function
> b) configured the SysTick to fire
> c) in the SysTick_Handler, write some other value (lets use
>     0x0022446688aaccee as an example) to one of the D-registers (D0 as
>     an example) and then do "SVC #0"
> d) in the SVC_Handler, write some other value (lets use
>     0x0099aabbccddeeff) to one of the D-registers (D0 as an example)
> 
> In GDB, suspend the execution in the SVC_Handler function and compare
> the value of the D-registers for the SVC_handler frame and the
> SysTick_Handler frame.  With the patch, the value of the modified
> D-register (D0) should be the new value (0x009..eff) on the
> SVC_Handler frame, and the intermediate value (0x002..cee) for the
> SysTick_Handler frame.  Now compare the D-register value for the
> SysTick_Handler frame and the main frame.  The main frame should
> have the initial value (0x112..788).
> 
> 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   | 62 +++++++++++++++++++++++++++++++++++-------------
>   gdb/frame.c      |  2 ++
>   gdb/observable.c |  1 +
>   gdb/observable.h |  3 +++
>   4 files changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 2810232fcb8..43ce1a45782 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -68,6 +68,7 @@
>   #endif
>   
>   static bool arm_debug;
> +static bool force_fpu_regs_from_stack = false;
>   
>   /* Print an "arm" debug statement.  */
>   
> @@ -3337,6 +3338,17 @@ struct frame_unwind arm_stub_unwind = {
>     arm_stub_unwind_sniffer
>   };
>   
> +
> +/* The first time an exception frame is seen, the lazy stacking of the FPU
> +   registers should be considered.  Any following exception frames should not
> +   consider the lazy stacking as the values will be put on the stack before
> +   branching to the nested exception handler.  */
> +static void
> +reset_force_fpu_regs_from_stack ()
> +{
> +  force_fpu_regs_from_stack = false;
> +}
> +
>   /* Put here the code to store, into CACHE->saved_regs, the addresses
>      of the saved registers of frame described by THIS_FRAME.  CACHE is
>      returned.  */
> @@ -3593,22 +3605,28 @@ arm_m_exception_cache (struct frame_info *this_frame)
>   	  gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
>   							 ARM_INT_REGISTER_SIZE,
>   							 byte_order, &fpccr));
> +	  bool fpccr_aspen = bit (fpccr, 31);
> +	  bool fpccr_lspen = bit (fpccr, 30);
>   	  bool fpccr_ts = bit (fpccr, 26);
> +	  bool fpccr_lspact = bit (fpccr, 0);
>   
> -	  /* This code does not take into account the lazy stacking, see "Lazy
> -	     context save of FP state", in B1.5.7, also ARM AN298, supported
> -	     by Cortex-M4F architecture.
> -	     To fully handle this the FPCCR register (Floating-point Context
> -	     Control Register) needs to be read out and the bits ASPEN and
> -	     LSPEN could be checked to setup correct lazy stacked FP registers.
> -	     This register is located at address 0xE000EF34.  */
> +	  /* The rule is that the lazy bit is only valid for the first frame,
> +	     all other frames will have the fpu registers stacked.  Just
> +	     consider the LSPEN, ASPEN and LSPACT bits for the first frame.
> +	     See "Lazy context save of FP state", in B1.5.7, also ARM AN298,
> +	     supported by Cortex-M4F architecture for details.  */
> +	  bool lazy_fpu_stacking_active = (fpccr_aspen && fpccr_lspen
> +					   && fpccr_lspact);
>   
>   	  /* Extended stack frame type used.  */
> -	  CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x20;
> -	  for (int i = 0; i < 8; i++)
> +	  if (force_fpu_regs_from_stack || !lazy_fpu_stacking_active)
>   	    {
> -	      cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
> -	      addr += 8;
> +	      CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x20;
> +	      for (int i = 0; i < 8; i++)
> +		{
> +		  cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
> +		  addr += 8;
> +		}
>   	    }
>   	  cache->saved_regs[ARM_FPSCR_REGNUM].set_addr (unwound_sp
>   							+ sp_r0_offset + 0x60);
> @@ -3617,11 +3635,14 @@ arm_m_exception_cache (struct frame_info *this_frame)
>   	      && fpccr_ts)
>   	    {
>   	      /* Handle floating-point callee saved registers.  */
> -	      addr = unwound_sp + sp_r0_offset + 0x68;
> -	      for (int i = 8; i < 16; i++)
> +	      if (force_fpu_regs_from_stack || !lazy_fpu_stacking_active)
>   		{
> -		  cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
> -		  addr += 8;
> +		  CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x68;
> +		  for (int i = 8; i < 16; i++)
> +		    {
> +		      cache->saved_regs[ARM_D0_REGNUM + i].set_addr (addr);
> +		      addr += 8;
> +		    }
>   		}
>   
>   	      arm_cache_set_active_sp_value (cache, tdep,
> @@ -3633,6 +3654,10 @@ arm_m_exception_cache (struct frame_info *this_frame)
>   	      arm_cache_set_active_sp_value (cache, tdep,
>   					     unwound_sp + sp_r0_offset + 0x68);
>   	    }
> +
> +	  /* Lazy stacking handled, all other frames will have the FPU
> +	     registers on the stack.  */
> +	  force_fpu_regs_from_stack = true;
>   	}
>         else
>   	{
> @@ -3746,6 +3771,7 @@ arm_m_exception_prev_register (struct frame_info *this_frame,
>         return frame_unwind_got_constant (this_frame, ARM_PS_REGNUM, xpsr);
>       }
>   
> +
>     return trad_frame_get_prev_register (this_frame, cache->saved_regs,
>   				       prev_regnum);
>   }
> @@ -10547,7 +10573,11 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>   
>     /* Add some default predicates.  */
>     if (is_m)
> -    frame_unwind_append_unwinder (gdbarch, &arm_m_exception_unwind);
> +    {
> +      gdb::observers::frame_cache_purged.attach
> +	(reset_force_fpu_regs_from_stack, "arm_m_force_fpu_regs_from_stack");
> +      frame_unwind_append_unwinder (gdbarch, &arm_m_exception_unwind);
> +    }
>     frame_unwind_append_unwinder (gdbarch, &arm_stub_unwind);
>     dwarf2_append_unwinders (gdbarch);
>     frame_unwind_append_unwinder (gdbarch, &arm_exidx_unwind);
> diff --git a/gdb/frame.c b/gdb/frame.c
> index fc883b3239e..fdbaa946a34 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -2007,6 +2007,8 @@ reinit_frame_cache (void)
>     select_frame (NULL);
>     frame_stash_invalidate ();
>   
> +  gdb::observers::frame_cache_purged.notify ();
> +
>     frame_debug_printf ("generation=%d", frame_cache_generation);
>   }
>   
> diff --git a/gdb/observable.c b/gdb/observable.c
> index 0bc8697f137..feee2b3f0a4 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -81,6 +81,7 @@ DEFINE_OBSERVABLE (gdb_exiting);
>   DEFINE_OBSERVABLE (connection_removed);
>   DEFINE_OBSERVABLE (target_pre_wait);
>   DEFINE_OBSERVABLE (target_post_wait);
> +DEFINE_OBSERVABLE (frame_cache_purged);
>   
>   } /* namespace observers */
>   } /* namespace gdb */
> diff --git a/gdb/observable.h b/gdb/observable.h
> index 796bf2a43c6..d0c9fc90a66 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -263,6 +263,9 @@ extern observable <ptid_t /* ptid */> target_pre_wait;
>   /* About to leave target_wait (). */
>   extern observable <ptid_t /* event_ptid */> target_post_wait;
>   
> +/* Observer called when the frame cache gets purged.  */
> +extern observable<> frame_cache_purged;
> +
>   } /* namespace observers */
>   
>   } /* namespace gdb */