Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations

["Leif Lindholm" <leif@nuviainc.com>]

On Thu, Oct 01, 2020 at 22:49:05 +0200, Ard Biesheuvel wrote:
> On 10/1/20 8:37 PM, Leif Lindholm wrote:
> > The SetJump comment header states that:
> >    If JumpBuffer is NULL, then ASSERT().
> > 
> > However, this was not currently done.
> > Add a call to InternalAssertJumpBuffer.
> > 
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S   | 3 +++
> >   MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm | 3 +++
> >   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S       | 3 +++
> >   MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm     | 3 +++
> >   4 files changed, 12 insertions(+)
> > 
> > diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > index 989736cee74c..34765a676430 100644
> > --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S
> > @@ -45,6 +45,9 @@ GCC_ASM_EXPORT(InternalLongJump)
> >   #  );
> >   #
> >   ASM_PFX(SetJump):
> > +        stp     x30, x0, [sp, #-16]!
> > +        bl      InternalAssertJumpBuffer
> > +        ldp     x30, x0, [sp], #16
> 
> I think we should make this more idiomatic for AArch64 function calls, i.e.
> 
>         stp     x29, x30, [sp, #-32]!
>         mov     x29, sp
>         str     x0, [sp, #16]
>         bl      InternalAssertJumpBuffer
>         ldr     x0, [sp, #16]
>         ldp     x29, x30, [sp], #32
> 
> That way, we'll have a well formed call stack with all the frame records
> linked together, allowing the debugger to show you where SetJump() was
> called from to begin with if you are stuck in the deadloop or hit the
> breakpoint (depending on how the PCD was configured to begin with)

Mmm, you have a point.

> I wouldn't mind putting the whole thing inside #ifndef MDEPKG_NDEBUG /#endif
> btw

Well...
At that point we might as well go and un-bonkers-ify the interfaces and
make the C function the wrapper that does the assert check before
calling this function renamed to InternalSetJump - making it symmetric
with the LongJump side of the equation.
Which I was hoping to avoid, since that messes with all architectures.

Urgh, that is actually the sensible thing to do here, isn't it?

/
    Leif

> >           mov     x16, sp // use IP0 so save SP
> >   #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
> >   #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
> > diff --git a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > index 8922128e8c62..f2729a8bb03e 100644
> > --- a/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > +++ b/MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.asm
> > @@ -44,6 +44,9 @@
> >   ;  );
> >   ;
> >   SetJump
> > +        stp     x30, x0, [sp, #-16]!
> > +        bl      InternalAssertJumpBuffer
> > +        ldp     x30, x0, [sp], #16
> 
> Same here
> 
> >           mov     x16, sp // use IP0 so save SP
> >   #define REG_PAIR(REG1, REG2, OFFS)      stp REG1, REG2, [x0, OFFS]
> >   #define REG_ONE(REG1, OFFS)             str REG1, [x0, OFFS]
> > diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> > index e4c1946a28ff..54b11ad2197c 100644
> > --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> > +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.S
> > @@ -31,6 +31,9 @@ GCC_ASM_EXPORT(InternalLongJump)
> >   #  );
> >   #
> >   ASM_PFX(SetJump):
> > +  push  {r0, lr}
> > +  bl    InternalAssertJumpBuffer
> > +  pop   {r0, lr}
> 
> ...  but not here (but the #ifndef would still be an improvement imo)
> 
> 
> >     mov   r3, r13
> >     stmia r0, {r3-r12,r14}
> >     eor   r0, r0, r0
> > diff --git a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > index e1eff758f7ab..6d47033975f2 100644
> > --- a/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > +++ b/MdePkg/Library/BaseLib/Arm/SetJumpLongJump.asm
> > @@ -31,6 +31,9 @@
> >   ;  )
> >   ;
> >   SetJump
> > +  PUSH {R0, LR}
> > +  BL   InternalAssertJumpBuffer
> > +  POP  {R0, LR}
> >     MOV  R3, R13
> >     STM  R0, {R3-R12,R14}
> >     EOR  R0, R0
> > 
>