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