[1/5] gdb/arm: Fix prologue analysis to support vpush

Message ID 20220114163552.4107885-1-christophe.lyon@foss.st.com
State Superseded
Headers show
Series
  • [1/5] gdb/arm: Fix prologue analysis to support vpush
Related show

Commit Message

Simon Marchi via Gdb-patches Jan. 14, 2022, 4:35 p.m.
While working on adding support for Non-secure/Secure modes unwinding,
I noticed that the prologue analysis lacked support for vpush, which
is used for instance in the CMSE stub routine.

This patch updates thumb_analyze_prologue accordingly.
---
 gdb/arm-tdep.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

-- 
2.25.1

Comments

Simon Marchi via Gdb-patches Jan. 17, 2022, 12:49 p.m. | #1
Hi!

On 1/14/22 1:35 PM, Christophe Lyon via Gdb-patches wrote:
> While working on adding support for Non-secure/Secure modes unwinding,

> I noticed that the prologue analysis lacked support for vpush, which

> is used for instance in the CMSE stub routine.


Thanks for the patch.

> 

> This patch updates thumb_analyze_prologue accordingly.

> ---

>   gdb/arm-tdep.c | 21 +++++++++++++++++++++

>   1 file changed, 21 insertions(+)

> 

> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c

> index 7495434484e..14ec0bc8f9e 100644

> --- a/gdb/arm-tdep.c

> +++ b/gdb/arm-tdep.c

> @@ -896,6 +896,27 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,

>   		regs[bits (insn, 0, 3)] = addr;

>   	    }

>   


I wish we'd use constants for instruction masks, but right now the code 
is full of magic numbers, so it is fine to use these. The code doesn't 
change that much anyway.

> +	  else if ((insn & 0xff20) == 0xed20    /* vstmdb Rn{!},

> +						   { registers } */

> +		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))

> +	    {

> +	      pv_t addr = regs[bits (insn, 0, 3)];

> +	      int number = bits (inst2, 0, 7) >> 1;


I'd add a comment making it clear what is being checked/extracted here...

> +

> +	      if (stack.store_would_trash (addr))

> +		break;

> +

> +	      /* Calculate offsets of saved registers.  */

> +	      for (; number > 0; number--)

> +		{

> +		  addr = pv_add_constant (addr, -8);

> +		  stack.store (addr, 8, pv_register (ARM_D0_REGNUM + number, 0));

> +		}

> +

> +	      if (insn & 0x0020)

> +		regs[bits (insn, 0, 3)] = addr;


... and here as well.

> +	    }

> +

>   	  else if ((insn & 0xff50) == 0xe940	/* strd Rt, Rt2,

>   						   [Rn, #+/-imm]{!} */

>   		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))

> 


Otherwise this look good to me.

Patch

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7495434484e..14ec0bc8f9e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -896,6 +896,27 @@  thumb_analyze_prologue (struct gdbarch *gdbarch,
 		regs[bits (insn, 0, 3)] = addr;
 	    }
 
+	  else if ((insn & 0xff20) == 0xed20    /* vstmdb Rn{!},
+						   { registers } */
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      pv_t addr = regs[bits (insn, 0, 3)];
+	      int number = bits (inst2, 0, 7) >> 1;
+
+	      if (stack.store_would_trash (addr))
+		break;
+
+	      /* Calculate offsets of saved registers.  */
+	      for (; number > 0; number--)
+		{
+		  addr = pv_add_constant (addr, -8);
+		  stack.store (addr, 8, pv_register (ARM_D0_REGNUM + number, 0));
+		}
+
+	      if (insn & 0x0020)
+		regs[bits (insn, 0, 3)] = addr;
+	    }
+
 	  else if ((insn & 0xff50) == 0xe940	/* strd Rt, Rt2,
 						   [Rn, #+/-imm]{!} */
 		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))