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

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

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: M1cha [mailto:sigmaepsilon92@gmail.com]
>Sent: Monday, January 08, 2018 1:31 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 v3 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 05eb4e33a086..39573db0c8da 100644
>--- a/MdePkg/Include/Library/BaseLib.h
>+++ b/MdePkg/Include/Library/BaseLib.h
>@@ -4903,6 +4903,7 @@ MemoryFence (
>   @retval 0 Indicates a return from SetJump().
>
> **/
>+RETURNS_TWICE
> UINTN
> EFIAPI
> SetJump (
>diff --git a/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
>b/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
>index 4c0dba55d52f..683527a36fda 100644
>--- a/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
>+++ b/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
>@@ -32,6 +32,7 @@
>   @retval 0 Indicates a return from SetJump().
>
> **/
>+RETURNS_TWICE
> UINTN
> EFIAPI
> SetJump (
>diff --git a/MdePkg/Library/BaseLib/Ia32/SetJump.c
>b/MdePkg/Library/BaseLib/Ia32/SetJump.c
>index 304f3839b108..652d45d53ba7 100644
>--- a/MdePkg/Library/BaseLib/Ia32/SetJump.c
>+++ b/MdePkg/Library/BaseLib/Ia32/SetJump.c
>@@ -49,6 +49,7 @@ InternalAssertJumpBuffer (
>
> **/
> _declspec (naked)
>+RETURNS_TWICE
> UINTN
> EFIAPI
> SetJump (
>--
>2.15.1