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

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

[Torbjörn SVENSSON]

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 */
-- 
2.25.1

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

[Thiago Jung Bauermann]

Hello,

Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:

> 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;

I'm a bit concerned about having a global variable to indicate what is
(IIUC) a per-CPU state. Does this logic work with multi-processor
inferiors?

Instead of using a global variable, can arm_m_exception_cache use
“this_frame->level > 0” to decide whether to get the FPU registers from
the stack?

-- 
Thiago

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

[Torbjorn SVENSSON]

Hello,

On 2022-09-27 22:08, Thiago Jung Bauermann wrote:
> 
> Hello,
> 
> Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> 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;
> 
> I'm a bit concerned about having a global variable to indicate what is
> (IIUC) a per-CPU state. Does this logic work with multi-processor
> inferiors?
> 
> Instead of using a global variable, can arm_m_exception_cache use
> “this_frame->level > 0” to decide whether to get the FPU registers from
> the stack?
> 

I share your concern, but haven't found any better way to achieve the 
required condition.

The content of FPCCR shall only be considered for the first extended 
exception frame on the stack. As there are likely other kinds of frames 
on the stack, the level would not be enough to decide if stack or 
registers should be used.

Do you know a way that this state can be saved per inferior?

On the other hand, I got the impression that the cache is purged when 
the inferior is switched, but this assumption might be wrong.

I've only verified the function with a simple single inferior.

Kind regards,
Torbjörn

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

[Thiago Jung Bauermann]

Hello,

Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> writes:

> Hello,
>
> On 2022-09-27 22:08, Thiago Jung Bauermann wrote:
>> Hello,
>> Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>> 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;
>> I'm a bit concerned about having a global variable to indicate what is
>> (IIUC) a per-CPU state. Does this logic work with multi-processor
>> inferiors?
>> Instead of using a global variable, can arm_m_exception_cache use
>> “this_frame->level > 0” to decide whether to get the FPU registers from
>> the stack?
>> 
>
> I share your concern, but haven't found any better way to achieve the required condition.
>
> The content of FPCCR shall only be considered for the first extended exception frame on
> the stack. As there are likely other kinds of frames on the stack, the level would not be
> enough to decide if stack or registers should be used.
>
> Do you know a way that this state can be saved per inferior?

You could create a subclass of private_inferior containing
force_fpu_regs_from_stack and store it in inferior->priv.

An example is the remote_inferior class used by the remote target.

It would still be inadequate for multi-core inferiors — I don't know how
each CPU is modelled by GDB in that case. But if that scenario isn't
currently required then perhaps we can cross that bridge when we get
there...

> On the other hand, I got the impression that the cache is purged when the inferior is
> switched, but this assumption might be wrong.

switch_to_inferior_no_thread calls reinit_frame_cache so you're right
IIUC. Even then, I think it's conceptually more correct to store this
state in a private_inferior struct/class.

-- 
Thiago

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

[Luis Machado]

On 9/30/22 03:55, Thiago Jung Bauermann via Gdb-patches wrote:
> 
> Hello,
> 
> Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> writes:
> 
>> Hello,
>>
>> On 2022-09-27 22:08, Thiago Jung Bauermann wrote:
>>> Hello,
>>> Torbjörn SVENSSON via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>
>>>> 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;
>>> I'm a bit concerned about having a global variable to indicate what is
>>> (IIUC) a per-CPU state. Does this logic work with multi-processor
>>> inferiors?
>>> Instead of using a global variable, can arm_m_exception_cache use
>>> “this_frame->level > 0” to decide whether to get the FPU registers from
>>> the stack?
>>>
>>
>> I share your concern, but haven't found any better way to achieve the required condition.
>>
>> The content of FPCCR shall only be considered for the first extended exception frame on
>> the stack. As there are likely other kinds of frames on the stack, the level would not be
>> enough to decide if stack or registers should be used.
>>
>> Do you know a way that this state can be saved per inferior?
> 
> You could create a subclass of private_inferior containing
> force_fpu_regs_from_stack and store it in inferior->priv.
> 
> An example is the remote_inferior class used by the remote target.
> 
> It would still be inadequate for multi-core inferiors — I don't know how
> each CPU is modelled by GDB in that case. But if that scenario isn't
> currently required then perhaps we can cross that bridge when we get
> there...
> 

I think the per-inferior data is a nice touch. For multi-core, those are usually exposed as
multiple threads by debugging stubs. Then again ...

>> On the other hand, I got the impression that the cache is purged when the inferior is
>> switched, but this assumption might be wrong.
> 
> switch_to_inferior_no_thread calls reinit_frame_cache so you're right
> IIUC. Even then, I think it's conceptually more correct to store this
> state in a private_inferior struct/class.
> 

... the frame cache gets purged when switching threads as well. So per-inferior
data should work nicely in this case.

Ideally we'd have some shared state between all the different unwinders, but that seems a bit
more complicated than it needs to be for the current needs.

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

[Pedro Alves]

On 2022-09-27 8:09 p.m., Torbjörn SVENSSON via Gdb-patches 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).

I suspect pasting a short GDB session here instead of prose may make this
easier to grok.

>  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;
> +}
> +

Is there really no self-contained way to tell that a frame is a "following frame
after lazy stack has been enabled" ?  E.g., can we look at the address in
FPCAR and decide based on it, compared to the current frame address, or something
like that?

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

[Torbjorn SVENSSON]

Hi,

On 2022-09-30 17:13, Pedro Alves wrote:
> On 2022-09-27 8:09 p.m., Torbjörn SVENSSON via Gdb-patches 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).
> 
> I suspect pasting a short GDB session here instead of prose may make this
> easier to grok.
> 
>>   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;
>> +}
>> +
> 
> Is there really no self-contained way to tell that a frame is a "following frame
> after lazy stack has been enabled" ?  E.g., can we look at the address in
> FPCAR and decide based on it, compared to the current frame address, or something
> like that?

As the function is Cortex-M specific, I suppose it would be fine to 
assume that the stack always grows downwards. With this assumption, I 
suppose we could compare the value in FPCAR with the value of $sp of the 
frame to be unwinded after the current frame.
- If the value is lower, then I think it's safe to assume that the 
stacked content should be used.
- If the value is greater, then I think it's safe to assume that it's up 
to LSPACT, LSPEN and ASPEN to define if it's going to be register or 
stacked content that should be used.

Does this sound reasonable?

I'll try to find some time later today or in the weekend to give it a try.

Kind regards,
Torbjörn

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

[Pedro Alves]

On 2022-09-30 4:26 p.m., Torbjorn SVENSSON wrote:
> Hi,
> 
> On 2022-09-30 17:13, Pedro Alves wrote:

>> Is there really no self-contained way to tell that a frame is a "following frame
>> after lazy stack has been enabled" ?  E.g., can we look at the address in
>> FPCAR and decide based on it, compared to the current frame address, or something
>> like that?
> 
> As the function is Cortex-M specific, I suppose it would be fine to assume that the stack always grows downwards. With this assumption, I suppose we could compare the value in FPCAR with the value of $sp of the frame to be unwinded after the current frame.
> - If the value is lower, then I think it's safe to assume that the stacked content should be used.
> - If the value is greater, then I think it's safe to assume that it's up to LSPACT, LSPEN and ASPEN to define if it's going to be register or stacked content that should be used.
> 
> Does this sound reasonable?
> 

It does to me.

I suppose the scheme could break if it's possible to have something
like sigaltstack, or coroutines, i.e., some frames after lazy FPU has been enabled are
actually running with a different stack.  Not sure, maybe you'd never see lazy FPU
stacking be enabled across the cpu/thread switching stacks?

> I'll try to find some time later today or in the weekend to give it a try.
> 

Thanks.