Re: [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump

["Gao, Liming" <liming.gao@intel.com>]

To align to longjump() declaration, could you update SetJump()?

UINTN
EFIAPI
RETURNS_TWICE
SetJump (
);

==>

RETURNS_TWICE
UINTN
EFIAPI
SetJump (
);

Thanks
Liming
> -----Original Message-----
> From: M1cha [mailto:sigmaepsilon92@gmail.com]
> Sent: Saturday, December 23, 2017 3:14 AM
> To: edk2-devel@lists.01.org
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [edk2] [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump
> 
> When compiling with any ARM toolchain and Os, registers can get
> trashed when returning for the second time from SetJump because GCC
> only handles this correctly when using standard names like 'setjmp' or
> 'getcontext'. When different names are used you have to use the
> attribute 'returns_twice' to tell gcc to be extra careful.
> 
> example:
> extern int  FN_NAME(void*);
> 
> void jmp_buf_set(void *jmpb, void (*f)(void))
> {
>   if (!FN_NAME(jmpb))
>     f();
> }
> 
> this code produces this wrong code with Os:
> 00000000 <jmp_buf_set>:
>    0: e92d4010 push {r4, lr}
>    4: e1a04001 mov r4, r1
>    8: ebfffffe bl 0 <nonstandard_setjmp>
>    c: e3500000 cmp r0, #0
>   10: 01a03004 moveq r3, r4
>   14: 08bd4010 popeq {r4, lr}
>   18: 012fff13 bxeq r3
>   1c: e8bd4010 pop {r4, lr}
>   20: e12fff1e bx lr
> 
> The generated code pushes backups of r4 and lr to the stack and then
> saves all registers using nonstandard_setjmp.
> Then it pops the stack and jumps to the function in r3 which is the
> main problem because now the function can overwrite our register
> backups on the stack.
> When we return a second time from the call to nonstandard_setjmp, the
> stack pointer has it's original(pushed) position and when the code
> pops r4 and lr from the stack the values are not guaranteed to be the
> same.
> 
> When using a standard name like setjmp or getcontext or adding
> '__attribute__((returns_twice))' to nonstandard_setjmp's declaration
> the code looks different:
> 
> 00000000 <jmp_buf_set>:
>    0: e92d4007 push {r0, r1, r2, lr}
>    4: e58d1004 str r1, [sp, #4]
>    8: ebfffffe bl 0 <setjmp>
>    c: e3500000 cmp r0, #0
>   10: 059d3004 ldreq r3, [sp, #4]
>   14: 01a0e00f moveq lr, pc
>   18: 012fff13 bxeq r3
>   1c: e28dd00c add sp, sp, #12
>   20: e49de004 pop {lr} ; (ldr lr, [sp], #4)
>   24: e12fff1e bx lr
> 
> Here the problem is being solved by restoring r3 from the stack
> without popping it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> ---
>  MdePkg/Include/Library/BaseLib.h             | 1 +
>  MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c | 1 +
>  MdePkg/Library/BaseLib/Ia32/SetJump.c        | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> index 2b98af4cd17e..10976032adaa 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4905,6 +4905,7 @@ MemoryFence (
>  **/
>  UINTN
>  EFIAPI
> +RETURNS_TWICE
>  SetJump (
>    OUT     BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer
>    );
> diff --git a/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c b/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
> index 4c0dba55d52f..e309e8b57d7a 100644
> --- a/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
> +++ b/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
> @@ -34,6 +34,7 @@
>  **/
>  UINTN
>  EFIAPI
> +RETURNS_TWICE
>  SetJump (
>    OUT      BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer
>    )
> diff --git a/MdePkg/Library/BaseLib/Ia32/SetJump.c b/MdePkg/Library/BaseLib/Ia32/SetJump.c
> index 304f3839b108..40fd16bae8fd 100644
> --- a/MdePkg/Library/BaseLib/Ia32/SetJump.c
> +++ b/MdePkg/Library/BaseLib/Ia32/SetJump.c
> @@ -51,6 +51,7 @@ InternalAssertJumpBuffer (
>  _declspec (naked)
>  UINTN
>  EFIAPI
> +RETURNS_TWICE
>  SetJump (
>    OUT     BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer
>    )
> --
> 2.15.1