* [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump
@ 2017-12-22 19:14 M1cha
2017-12-22 19:14 ` [PATCH v2 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump M1cha
2018-01-02 12:31 ` [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump Gao, Liming
0 siblings, 2 replies; 3+ messages in thread
From: M1cha @ 2017-12-22 19:14 UTC (permalink / raw)
To: edk2-devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v2 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump
2017-12-22 19:14 [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump M1cha
@ 2017-12-22 19:14 ` M1cha
2018-01-02 12:31 ` [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump Gao, Liming
1 sibling, 0 replies; 3+ messages in thread
From: M1cha @ 2017-12-22 19:14 UTC (permalink / raw)
To: edk2-devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao
This fixes compiler warnings when using them in functions which
should return a value but rely on LongJump to never return instead.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
---
MdePkg/Include/Library/BaseLib.h | 1 +
MdePkg/Library/BaseLib/BaseLibInternals.h | 1 +
MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c | 1 +
MdePkg/Library/BaseLib/Ia32/LongJump.c | 1 +
MdePkg/Library/BaseLib/LongJump.c | 1 +
5 files changed, 5 insertions(+)
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 10976032adaa..e2eb46da4584 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4929,6 +4929,7 @@ SetJump (
**/
VOID
EFIAPI
+NORETURN
LongJump (
IN BASE_LIBRARY_JUMP_BUFFER *JumpBuffer,
IN UINTN Value
diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h b/MdePkg/Library/BaseLib/BaseLibInternals.h
index 9dca97a0dcc9..4486b15bd151 100644
--- a/MdePkg/Library/BaseLib/BaseLibInternals.h
+++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
@@ -443,6 +443,7 @@ InternalAssertJumpBuffer (
**/
VOID
EFIAPI
+NORETURN
InternalLongJump (
IN BASE_LIBRARY_JUMP_BUFFER *JumpBuffer,
IN UINTN Value
diff --git a/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c b/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
index e309e8b57d7a..255464240ffa 100644
--- a/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
+++ b/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
@@ -56,6 +56,7 @@ SetJump (
**/
VOID
EFIAPI
+NORETURN
InternalLongJump (
IN BASE_LIBRARY_JUMP_BUFFER *JumpBuffer,
IN UINTN Value
diff --git a/MdePkg/Library/BaseLib/Ia32/LongJump.c b/MdePkg/Library/BaseLib/Ia32/LongJump.c
index 73973a9cceae..d4b101fcb213 100644
--- a/MdePkg/Library/BaseLib/Ia32/LongJump.c
+++ b/MdePkg/Library/BaseLib/Ia32/LongJump.c
@@ -30,6 +30,7 @@
__declspec (naked)
VOID
EFIAPI
+NORETURN
InternalLongJump (
IN BASE_LIBRARY_JUMP_BUFFER *JumpBuffer,
IN UINTN Value
diff --git a/MdePkg/Library/BaseLib/LongJump.c b/MdePkg/Library/BaseLib/LongJump.c
index 062be8f2daa1..7b91e076ba60 100644
--- a/MdePkg/Library/BaseLib/LongJump.c
+++ b/MdePkg/Library/BaseLib/LongJump.c
@@ -35,6 +35,7 @@
**/
VOID
EFIAPI
+NORETURN
LongJump (
IN BASE_LIBRARY_JUMP_BUFFER *JumpBuffer,
IN UINTN Value
--
2.15.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump
2017-12-22 19:14 [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump M1cha
2017-12-22 19:14 ` [PATCH v2 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump M1cha
@ 2018-01-02 12:31 ` Gao, Liming
1 sibling, 0 replies; 3+ messages in thread
From: Gao, Liming @ 2018-01-02 12:31 UTC (permalink / raw)
To: M1cha, edk2-devel@lists.01.org; +Cc: Ard Biesheuvel, Kinney, Michael D
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-01-02 12:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-22 19:14 [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump M1cha
2017-12-22 19:14 ` [PATCH v2 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump M1cha
2018-01-02 12:31 ` [PATCH v2 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump Gao, Liming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox