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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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)

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

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