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.4180.1601585903464649946 for ; Thu, 01 Oct 2020 13:58:23 -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 E9B4D1042; Thu, 1 Oct 2020 13:58:20 -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 03EA63F73B; Thu, 1 Oct 2020 13:58:19 -0700 (PDT) Subject: Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations To: Leif Lindholm Cc: devel@edk2.groups.io, Michael D Kinney , Liming Gao , Zhiguang Liu References: <20201001183712.1738-1-leif@nuviainc.com> <20201001183712.1738-3-leif@nuviainc.com> <4ae3978a-8d10-522a-050f-45ed865781d5@arm.com> <20201001205554.GJ5623@vanye> From: "Ard Biesheuvel" Message-ID: <9bbe6abb-2f62-9428-82e7-483b30e428a9@arm.com> Date: Thu, 1 Oct 2020 22:58:14 +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: <20201001205554.GJ5623@vanye> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >>> --- >>> 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?