From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.3999.1601585355512416210 for ; Thu, 01 Oct 2020 13:49:15 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A3EBA30E; Thu, 1 Oct 2020 13:49:12 -0700 (PDT) Received: from [192.168.1.81] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 802003F73B; Thu, 1 Oct 2020 13:49:11 -0700 (PDT) Subject: Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations To: Leif Lindholm , devel@edk2.groups.io Cc: Michael D Kinney , Liming Gao , Zhiguang Liu References: <20201001183712.1738-1-leif@nuviainc.com> <20201001183712.1738-3-leif@nuviainc.com> From: "Ard Biesheuvel" Message-ID: <4ae3978a-8d10-522a-050f-45ed865781d5@arm.com> Date: Thu, 1 Oct 2020 22:49:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201001183712.1738-3-leif@nuviainc.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > --- > 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 >