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

[Christophe Lyon <christophe.lyon.oss@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

On Mon, Jan 17, 2022 at 1:50 PM Luis Machado via Gdb-patches <
gdb-patches@sourceware.org> wrote:

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

Yeah, I merely cut/pasted the previous code block, just updating the fields.


>
> > +       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.
>

Is the attached patch OK?

Thanks,

Christophe

[-- Attachment #2: 0001-gdb-arm-Fix-prologue-analysis-to-support-vpush.patch --]
[-- Type: text/x-patch, Size: 1756 bytes --]

From 39da58e92d00cb55c3f07d638afc4c617f9c1bb8 Mon Sep 17 00:00:00 2001
From: Christophe Lyon <christophe.lyon@linaro.org>
Date: Fri, 14 Jan 2022 15:24:02 +0100
Subject: [PATCH] gdb/arm: Fix prologue analysis to support vpush

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, adding support
for vpush of D-registers.
---
 gdb/arm-tdep.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 7495434484e..32e7b9ceb87 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -896,6 +896,31 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 		regs[bits (insn, 0, 3)] = addr;
 	    }
 
+	  else if ((insn & 0xff20) == 0xed20    /* vstmdb Rn{!}, { D-registers} */
+		   && (insn2 & 0x0f00) == 0x0b00/* (aka vpush) */
+		   && pv_is_register (regs[bits (insn, 0, 3)], ARM_SP_REGNUM))
+	    {
+	      /* Address SP points to.  */
+	      pv_t addr = regs[bits (insn, 0, 3)];
+
+	      /* Number of registers saved.  */
+	      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));
+		}
+
+	      /* Writeback SP if needed.  */
+	      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))
-- 
2.25.1