Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 12 Dec 2018 at 13:31, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Dec 12, 2018 at 12:33:07PM +0100, Ard Biesheuvel wrote:
> > On Wed, 12 Dec 2018 at 12:30, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > On Wed, Dec 12, 2018 at 10:12:11AM +0100, Ard Biesheuvel wrote:
> > > > Clang does not like the legacy 32-bit assembler syntax used in the
> > > > gdbstub exception handling routines, so update them to something
> > > > more fashionable.
> > > >
> > > > So switch the order of the conditional suffix
> > >
> > > Yes, this is essentially a bugfix. The syntax change predates ARM's
> > > involvement in EDK2.
> > >
> > > > and the increment/decrement
> > > > specifier, and use decrement-after (da) and increment-before (ib) as
> > > > appropriate for an empty descending stack.
> > >
> > > But I very much prefer the FA/FD/EA/ED syntax. It gives symmetry,
> > > which helps me a lot when dealing with 7-character mnemonics.
> > > These aliases are also defined in the ARM ARM and won't be going away.
> >
> > Clang doesn't support them though.
>
> *Goes to check*
>
> *Boggles*
>
> Yeah, it supports FD/EA, but not FA/ED. That's ... special.
>
> > This code is not used anywhere (it is referenced in BeagleBoardPkg but
> > commented out), so we could also simply remove it.
>
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > ---
> > > >  EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > > index df1543a6d242..924578c7af2f 100644
> > > > --- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > > +++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > > @@ -57,6 +57,7 @@ GCC_ASM_EXPORT(AsmCommonExceptionEntry)
> > > >  GCC_ASM_EXPORT(GdbExceptionHandler)
> > > >
> > > >  .text
> > > > +.syntax unified
> > > >  .align 3
> > > >
> > > >
> > > > @@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
> > > >    and       R3, R1, #0x1f           @ Check CPSR to see if User or System Mode
> > > >    cmp       R3, #0x1f               @ if ((CPSR == 0x10) || (CPSR == 0x1df))
> > > >    cmpne     R3, #0x10               @
> > > > -  stmeqed   R2, {lr}^               @   save unbanked lr
> > > > +  stmdaeq   R2, {lr}^               @   save unbanked lr
>
> Then again, looking closer at these instructions, they're not really
> doing stack operations. Just (ab)using the instruction to get at the
> banked User mode LR from a different mode. So
> a) The symmetry thing doesn't really apply, so the ED is actively
>    misleading here.
> b) This could trivially be changed to use FD anyway, just setting R2's
>    offset from PC to #0x34 :)
>

Actually, since there is no writeback, what is the point of the
decrement-after? Can't we just drop it?

The more I look at this code the more broken it seems. I'll just remove it.


> > > >                                      @ else
> > > > -  stmneed   R2, {lr}                @   save SVC lr
> > > > +  stmdane   R2, {lr}                @   save SVC lr
> > > >
> > > >
> > > >    ldr       R5, [SP, #0x58]         @ PC is the LR pushed by srsfd
> > > > @@ -245,9 +246,9 @@ GdbExceptionHandler (
> > > >    and       R1, R1, #0x1f           @ Check to see if User or System Mode
> > > >    cmp       R1, #0x1f               @ if ((CPSR == 0x10) || (CPSR == 0x1f))
> > > >    cmpne     R1, #0x10               @
> > > > -  ldmeqed   R2, {lr}^               @   restore unbanked lr
> > > > +  ldmibeq   R2, {lr}^               @   restore unbanked lr
> > > >                                      @ else
> > > > -  ldmneed   R3, {lr}                @   restore SVC lr, via ldmfd SP!, {LR}
> > > > +  ldmibne   R3, {lr}                @   restore SVC lr, via ldmfd SP!, {LR}
> > > >
> > > >    ldmfd     SP!,{R0-R12}            @ Restore general purpose registers
> > > >                                      @ Exception handler can not change SP
> > > > --
> > > > 2.19.2
> > > >
>
> c) But given a), I would take this one if the comment was updated to
>    be explicit about how esoteric this operation really is.
>    I.e.: "(ab)use STM^ to save banked User mode LR from SVC/HYP mode"
>    and   "(ab)use LDM^ to restore banked User mode LR from SVC/HYP mode"
>
> I would also be happy to nuke it (including the commented out
> inclusion for BeagleBoardPkg).
>
> /
>     Leif