1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH 0/5] MdePkg: various fixes to ARM/AArch64 SetJump/LongJump
  4. View patch

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

Leif Lindholm posted 5 patches 5 years, 4 months ago
There is a newer version of this series
[edk2-devel] [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
Posted by Leif Lindholm 5 years, 4 months ago 
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
         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
         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}
   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
-- 
2.20.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65813): https://edk2.groups.io/g/devel/message/65813
Mute This Topic: https://groups.io/mt/77247140/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
Posted by Ard Biesheuvel 5 years, 4 months ago 
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
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65818): https://edk2.groups.io/g/devel/message/65818
Mute This Topic: https://groups.io/mt/77247140/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
Posted by Leif Lindholm 5 years, 4 months ago 
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
> > 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65819): https://edk2.groups.io/g/devel/message/65819
Mute This Topic: https://groups.io/mt/77247140/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations
Posted by Ard Biesheuvel 5 years, 4 months ago 
On 10/1/20 10:55 PM, Leif Lindholm wrote:
> 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?
> 

Do rhetorical questions expect an answer?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65820): https://edk2.groups.io/g/devel/message/65820
Mute This Topic: https://groups.io/mt/77247140/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.