[PATCH v5] gdb/arm: Handle lazy FPU state preservation

[PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Torbjörn SVENSSON]

Read LSPEN, ASPEN and LSPACT bits from FPCCR and use them together
with FPCAR to identify if lazy FPU state preservation is active for
the current frame.  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/arch/arm.h |  7 ++++++-
 gdb/arm-tdep.c | 56 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index 36757493406..d384b952144 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -115,7 +115,12 @@ enum system_register_address : CORE_ADDR
   /* M-profile Floating-Point Context Control Register address, defined in
      ARMv7-M (Section B3.2.2) and ARMv8-M (Section D1.2.99) reference
      manuals.  */
-  FPCCR = 0xe000ef34
+  FPCCR = 0xe000ef34,
+
+  /* M-profile Floating-Point Context Address Register address, defined in
+     ARMv7-M (Section B3.2.2) and ARMv8-M (Section D1.2.98) reference
+     manuals.  */
+  FPCAR = 0xe000ef38
 };
 
 /* Instruction condition field values.  */
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 2810232fcb8..d357066653b 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3588,27 +3588,48 @@ arm_m_exception_cache (struct frame_info *this_frame)
       if (extended_frame_used)
 	{
 	  ULONGEST fpccr;
+	  ULONGEST fpcar;
 
 	  /* Read FPCCR register.  */
 	  gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
 							 ARM_INT_REGISTER_SIZE,
 							 byte_order, &fpccr));
-	  bool fpccr_ts = bit (fpccr, 26);
 
-	  /* 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.  */
+	  /* Read FPCAR register.  */
+	  if (!safe_read_memory_unsigned_integer (FPCAR, ARM_INT_REGISTER_SIZE,
+						  byte_order, &fpcar))
+	    {
+	      warning (_("Could not fetch FPCAR content. Further unwinding of "
+			 "FP register values will be unreliable."));
+	      fpcar = 0;
+	    }
+
+	  bool fpccr_aspen = bit (fpccr, 31);
+	  bool fpccr_lspen = bit (fpccr, 30);
+	  bool fpccr_ts = bit (fpccr, 26);
+	  bool fpccr_lspact = bit (fpccr, 0);
+
+	  /* The LSPEN and ASPEN bits indicate if the lazy state preservation
+	     for FP registers is enabled or disabled.  The LSPACT bit indicate,
+	     together with FPCAR, if the lazy state preservation feature is
+	     active for the current frame or for another frame.
+	     See "Lazy context save of FP state", in B1.5.7, also ARM AN298,
+	     supported by Cortex-M4F architecture for details.  */
+	  bool fpcar_points_to_this_frame = ((unwound_sp + sp_r0_offset + 0x20)
+					     == (fpcar & ~0x7));
+	  bool read_fp_regs_from_stack = (!(fpccr_aspen && fpccr_lspen
+					    && fpccr_lspact
+					    && fpcar_points_to_this_frame));
 
 	  /* Extended stack frame type used.  */
-	  CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x20;
-	  for (int i = 0; i < 8; i++)
+	  if (read_fp_regs_from_stack)
 	    {
-	      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 +3638,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 (read_fp_regs_from_stack)
 		{
-		  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,
-- 
2.25.1

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Luis Machado]

Hi Torbjörn,

Thanks for the updated patch.

On 10/5/22 09:48, Torbjörn SVENSSON wrote:
> Read LSPEN, ASPEN and LSPACT bits from FPCCR and use them together
> with FPCAR to identify if lazy FPU state preservation is active for
> the current frame.  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/arch/arm.h |  7 ++++++-
>   gdb/arm-tdep.c | 56 +++++++++++++++++++++++++++++++++++---------------
>   2 files changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
> index 36757493406..d384b952144 100644
> --- a/gdb/arch/arm.h
> +++ b/gdb/arch/arm.h
> @@ -115,7 +115,12 @@ enum system_register_address : CORE_ADDR
>     /* M-profile Floating-Point Context Control Register address, defined in
>        ARMv7-M (Section B3.2.2) and ARMv8-M (Section D1.2.99) reference
>        manuals.  */
> -  FPCCR = 0xe000ef34
> +  FPCCR = 0xe000ef34,
> +
> +  /* M-profile Floating-Point Context Address Register address, defined in
> +     ARMv7-M (Section B3.2.2) and ARMv8-M (Section D1.2.98) reference
> +     manuals.  */
> +  FPCAR = 0xe000ef38
>   };
>   
>   /* Instruction condition field values.  */
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 2810232fcb8..d357066653b 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3588,27 +3588,48 @@ arm_m_exception_cache (struct frame_info *this_frame)
>         if (extended_frame_used)
>   	{
>   	  ULONGEST fpccr;
> +	  ULONGEST fpcar;
>   
>   	  /* Read FPCCR register.  */
>   	  gdb_assert (safe_read_memory_unsigned_integer (FPCCR,
>   							 ARM_INT_REGISTER_SIZE,
>   							 byte_order, &fpccr));
> -	  bool fpccr_ts = bit (fpccr, 26);
>   
> -	  /* 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.  */
> +	  /* Read FPCAR register.  */
> +	  if (!safe_read_memory_unsigned_integer (FPCAR, ARM_INT_REGISTER_SIZE,
> +						  byte_order, &fpcar))
> +	    {
> +	      warning (_("Could not fetch FPCAR content. Further unwinding of "
> +			 "FP register values will be unreliable."));
> +	      fpcar = 0;
> +	    }
> +
> +	  bool fpccr_aspen = bit (fpccr, 31);
> +	  bool fpccr_lspen = bit (fpccr, 30);
> +	  bool fpccr_ts = bit (fpccr, 26);
> +	  bool fpccr_lspact = bit (fpccr, 0);
> +
> +	  /* The LSPEN and ASPEN bits indicate if the lazy state preservation
> +	     for FP registers is enabled or disabled.  The LSPACT bit indicate,
> +	     together with FPCAR, if the lazy state preservation feature is
> +	     active for the current frame or for another frame.
> +	     See "Lazy context save of FP state", in B1.5.7, also ARM AN298,
> +	     supported by Cortex-M4F architecture for details.  */
> +	  bool fpcar_points_to_this_frame = ((unwound_sp + sp_r0_offset + 0x20)
> +					     == (fpcar & ~0x7));
> +	  bool read_fp_regs_from_stack = (!(fpccr_aspen && fpccr_lspen
> +					    && fpccr_lspact
> +					    && fpcar_points_to_this_frame));
>   
>   	  /* Extended stack frame type used.  */
> -	  CORE_ADDR addr = unwound_sp + sp_r0_offset + 0x20;
> -	  for (int i = 0; i < 8; i++)
> +	  if (read_fp_regs_from_stack)
>   	    {
> -	      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 +3638,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 (read_fp_regs_from_stack)
>   		{
> -		  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,

This LGTM.

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Joel Brobecker]

Hi Torbjörn,

Luis Machado said:
> Thanks for the updated patch.
[...]
> This LGTM.

For the avoidance of doubt, Luis is maintainer for arm and AArch64,
so when he says it's good for him, it means the patch is approved
for pushing ;-).

I don't see you in the list of contributors who have "Write After
Approval" privileges. Do all the authors of the patch have an FSF
copyright assignment on file? I checked the record, but didn't
find anything...

Thank you,
-- 
Joel

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Yvan Roux]

Hi Joel,

On Sun, Oct 09, 2022 at 04:45:09PM -0700, Joel Brobecker wrote:
> Hi Torbjörn,
> 
> Luis Machado said:
> > Thanks for the updated patch.
> [...]
> > This LGTM.
> 
> For the avoidance of doubt, Luis is maintainer for arm and AArch64,
> so when he says it's good for him, it means the patch is approved
> for pushing ;-).

Yes thanks for the heads up, I pushed the patch last Friday.
 
> I don't see you in the list of contributors who have "Write After
> Approval" privileges. Do all the authors of the patch have an FSF
> copyright assignment on file? I checked the record, but didn't
> find anything...

Torbjorn and I are covered by STMicro copyright assignement, so it
should be fine.

Thanks,
Yvan
 
> Thank you,
> -- 
> Joel

-- 
Y.

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Luis Machado]

On 10/10/22 08:20, Yvan Roux via Gdb-patches wrote:
> Hi Joel,
> 
> On Sun, Oct 09, 2022 at 04:45:09PM -0700, Joel Brobecker wrote:
>> Hi Torbjörn,
>>
>> Luis Machado said:
>>> Thanks for the updated patch.
>> [...]
>>> This LGTM.
>>
>> For the avoidance of doubt, Luis is maintainer for arm and AArch64,
>> so when he says it's good for him, it means the patch is approved
>> for pushing ;-).
> 
> Yes thanks for the heads up, I pushed the patch last Friday.
>   
>> I don't see you in the list of contributors who have "Write After
>> Approval" privileges. Do all the authors of the patch have an FSF
>> copyright assignment on file? I checked the record, but didn't
>> find anything...
> 
> Torbjorn and I are covered by STMicro copyright assignement, so it
> should be fine.
> 
> Thanks,
> Yvan
>   
>> Thank you,
>> -- 
>> Joel
> 

Torbjörn has contributed a few patches now, so I think it is fair to say
he should have Write-After-Approval permission now.

It escapes me though what the process was for that.

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Pedro Alves]

On 2022-10-10 10:26 a.m., Luis Machado wrote:

> Torbjörn has contributed a few patches now, so I think it is fair to say
> he should have Write-After-Approval permission now.
> 
> It escapes me though what the process was for that.


See:

"I need an account on sourceware for git/svn/cvs write access to project 
XYZ?"

at sourceware.org.

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Luis Machado]

On 10/10/22 14:02, Pedro Alves wrote:
> On 2022-10-10 10:26 a.m., Luis Machado wrote:
> 
>> Torbjörn has contributed a few patches now, so I think it is fair to say
>> he should have Write-After-Approval permission now.
>>
>> It escapes me though what the process was for that.
> 
> 
> See:
> 
> "I need an account on sourceware for git/svn/cvs write access to project XYZ?"
> 
> at sourceware.org.

Ah, handy. Thanks!

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Joel Brobecker]

> > For the avoidance of doubt, Luis is maintainer for arm and AArch64,
> > so when he says it's good for him, it means the patch is approved
> > for pushing ;-).
> 
> Yes thanks for the heads up, I pushed the patch last Friday.

I see; thanks for confirming.

It used to be that we asked people to send a quick reply to the approval
email to confirm that the commit was pushed.  In the absence of a review
tool that keeps track of patches, I have found this to be quite useful
to keep track.

> > I don't see you in the list of contributors who have "Write After
> > Approval" privileges. Do all the authors of the patch have an FSF
> > copyright assignment on file? I checked the record, but didn't
> > find anything...
> 
> Torbjorn and I are covered by STMicro copyright assignement, so it
> should be fine.

Nice!

-- 
Joel

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Luis Machado]

On 10/10/22 15:50, Joel Brobecker via Gdb-patches wrote:
>>> For the avoidance of doubt, Luis is maintainer for arm and AArch64,
>>> so when he says it's good for him, it means the patch is approved
>>> for pushing ;-).
>>
>> Yes thanks for the heads up, I pushed the patch last Friday.
> 
> I see; thanks for confirming.
> 
> It used to be that we asked people to send a quick reply to the approval
> email to confirm that the commit was pushed.  In the absence of a review
> tool that keeps track of patches, I have found this to be quite useful
> to keep track.

A bit off-topic for this patch, but personally I've been using the IRC bot to keep track of this, so
I don't mind as much. Besides, quite a bit of GDB chat goes on in there. Maybe patchworks can keep
track of it as well.

> 
>>> I don't see you in the list of contributors who have "Write After
>>> Approval" privileges. Do all the authors of the patch have an FSF
>>> copyright assignment on file? I checked the record, but didn't
>>> find anything...
>>
>> Torbjorn and I are covered by STMicro copyright assignement, so it
>> should be fine.
> 
> Nice!
> 

Speaking of which, I find the copyright verification to be a bit of a nuisance. Every once
in a while I need to verify a contributor's copyright assignment, but I can't do it in an
easy way. I usually ask Mark W. to help me with it, which is something I could've done
on my own.

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Joel Brobecker]

> Speaking of which, I find the copyright verification to be a bit of a
> nuisance. Every once in a while I need to verify a contributor's
> copyright assignment, but I can't do it in an easy way. I usually ask
> Mark W. to help me with it, which is something I could've done on my
> own.

Because having a copyright assignment on file is a condition for getting
Write After Approval privileges, I only do the check when I notice
that someone doesn't have an account (usually when the contributor
says he cannot push the patch when I approve it). It's sufficiently
rare that I haven't seen this as a big issue.

I don't know if we can give access to the file to more people or not...

-- 
Joel

Re: [PATCH v5] gdb/arm: Handle lazy FPU state preservation

[Luis Machado]

On 10/10/22 17:48, Joel Brobecker wrote:
>> Speaking of which, I find the copyright verification to be a bit of a
>> nuisance. Every once in a while I need to verify a contributor's
>> copyright assignment, but I can't do it in an easy way. I usually ask
>> Mark W. to help me with it, which is something I could've done on my
>> own.
> 
> Because having a copyright assignment on file is a condition for getting
> Write After Approval privileges, I only do the check when I notice
> that someone doesn't have an account (usually when the contributor
> says he cannot push the patch when I approve it). It's sufficiently
> rare that I haven't seen this as a big issue.
> 
> I don't know if we can give access to the file to more people or not...
> 

Sure, but I don't think we need access to the file itself, but it would certainly
help if we could verify someone is good to go given an e-mail or name I guess.