From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web12.4128.1601585764065088082 for ; Thu, 01 Oct 2020 13:56:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=DBOdxf+z; spf=pass (domain: nuviainc.com, ip: 209.85.221.65, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f65.google.com with SMTP id o5so186395wrn.13 for ; Thu, 01 Oct 2020 13:56:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=sMDXcaLSLIMr/DMk7LIWZsFhdDKxrRd/QjCqgIik99s=; b=DBOdxf+zORMsKMJCzSYiVpCAzvn8hLCWC/v3gUfPxfOF7wsz0XoxUz8m5OCtm4J6bc aDc+j4Z22KiVMUd6E/BYWXLLS/mWWF2CHkLLH6y+jSbMHhHov9fMSxTpdiCOIqtUizuN Ew8m9BClhCyMNtcwtVC1zlFf1Su62r+ODouJE4VWQ3eEwVexMxGauO+9TIlOJ2X2TcE5 /tHD29fl7kXGHOwxSWJcL6K7pmsEOQV3acxgMVs9XAvNvLaCxC/wTaoBgttw3P3rEGbH 9K7BrE4EWEj3VzqL0nCsa4Am8qrH/Fx4biRzCn5L5lzI7A9R+NdZCohwSPRRZKmENa3a kRzw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=sMDXcaLSLIMr/DMk7LIWZsFhdDKxrRd/QjCqgIik99s=; b=KKKhhKpY9MmjBa4U9AKu7QQylwSvrbaH8471HC9hwvXP1F5Qjj4JUxUBIGdlys6Qnq yvRnYd44+UT4Bz5aV38yST7kSl0jGeLSNwMj36UddCGPCGSRFc4Ijpt/JhWqlWtZELAS Tbb+MA3Vb52ekV6iz7U/lusraHHvicd+A1GA//hBcGSfa2HkPByJU924lsrd0mqG1kQp xLVr4YleyqoMKkRU/IPKgxO92Bgcz6nei1cljXYsT7m3fge/3l4XVq/ZI38eLKYAUzFr wMGgdLoke+Mre1Ropjr3O9ajCXWnI6+iGBP5LrQEnG2ldIrB66wDN1t6Mp4yJ8Ah+n6z nlvg== X-Gm-Message-State: AOAM532bhhH+VpWF+mleqYFydAe9iYwhajphclPMqQ6Oo2iDCdTamah+ x1P94YSGr5K4o9hto41Xu38QpA== X-Google-Smtp-Source: ABdhPJxvbaZOfyK9WaYG7FJ2Ylze/GcyLV5vGQvj+x8oN580IKmsp0NBKuNWz5H4ggXriiJX7Sk8gQ== X-Received: by 2002:a5d:470f:: with SMTP id y15mr10903187wrq.420.1601585762510; Thu, 01 Oct 2020 13:56:02 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id s17sm10955032wrr.40.2020.10.01.13.56.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Oct 2020 13:56:01 -0700 (PDT) Date: Thu, 1 Oct 2020 21:55:54 +0100 From: "Leif Lindholm" To: Ard Biesheuvel Cc: devel@edk2.groups.io, Michael D Kinney , Liming Gao , Zhiguang Liu Subject: Re: [PATCH 2/5] MdePkg/BaseLib: add ASSERT in ARM* SetJump implementations Message-ID: <20201001205554.GJ5623@vanye> References: <20201001183712.1738-1-leif@nuviainc.com> <20201001183712.1738-3-leif@nuviainc.com> <4ae3978a-8d10-522a-050f-45ed865781d5@arm.com> MIME-Version: 1.0 In-Reply-To: <4ae3978a-8d10-522a-050f-45ed865781d5@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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? / 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 > > >