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 <l...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
