From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 742541A1E57 for ; Tue, 25 Oct 2016 01:33:14 -0700 (PDT) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ED10AC04B95C; Tue, 25 Oct 2016 08:33:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-71.phx2.redhat.com [10.3.116.71]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9P8XCTX032436; Tue, 25 Oct 2016 04:33:12 -0400 To: "Zeng, Star" , "Kinney, Michael D" , "Gao, Liming" References: <20161021212737.15974-1-lersek@redhat.com> <20161021212737.15974-2-lersek@redhat.com> <567fc440-ce97-548d-e645-6ae84dfbf488@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103959121C@shsmsx102.ccr.corp.intel.com> Cc: edk2-devel-01 From: Laszlo Ersek Message-ID: Date: Tue, 25 Oct 2016 10:33:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103959121C@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 25 Oct 2016 08:33:14 +0000 (UTC) Subject: Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2016 08:33:14 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 10/25/16 10:22, Zeng, Star wrote: > In fact, EFI_* codes are a subset of RETURN_* codes, so it seems work to implement ASSERT_EFI_ERROR() using new ASSERT_RETURN_ERROR(). Let me commit the patch as-is for now, with Mike's review, and let's continue the discussion on the above-suggested code sharing. If everyone agrees, I can submit a separate patch that reuses ASSERT_RETURN_ERROR in ASSERT_EFI_ERROR. One thing I'm not so sure about (regarding the code sharing) is that each of these macros prints its own name, as a literal string. If we simply redefine ASSERT_EFI_ERROR with ASSERT_RETURN_ERROR, the error message won't match the source code any longer for the former. We can get around this by introducing a common base macro that also takes the name to print as a parameter. But, I think that justifies a separate patch even more. Thanks! Laszlo > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Kinney, Michael D > Sent: Tuesday, October 25, 2016 7:05 AM > To: Laszlo Ersek ; Gao, Liming ; Kinney, Michael D > Cc: edk2-devel-01 > Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() > > Hi Laszlo, > > Sorry for the delay. I was traveling last week. > > I did see this and I have been thinking about it. > I think it does make sense to add this new macro for libraries of type BASE. I am surprised we did not run into an issue before that would have required the introduction of this macro earlier. Unless the workaround has been to add #include of , which makes me think we should review BASE libraries to make sure that extra include is not present. > > The EFI_* error codes are mapped to RETURN_* error codes. So the only feedback I was considering was to implement ASSERT_EFI_ERROR() using ASSERT_RETURN_ERROR(), but that might not always be the right mapping because the RETURN_* codes are a subset of EFI_* error codes. > > Reviewed-by: Michael Kinney > > Best regards, > > Mike > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Monday, October 24, 2016 2:00 PM >> To: Kinney, Michael D ; Gao, Liming >> >> Cc: edk2-devel-01 >> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add >> ASSERT_RETURN_ERROR() >> >> Mike, Liming, >> >> On 10/21/16 23:27, Laszlo Ersek wrote: >>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because >>> - the replacement text calls EFI_ERROR(), >>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h", >>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules. >>> >>> While >>> >>> ASSERT (!RETURN_ERROR (StatusParameter)) >>> >>> would be a functional statement in BASE type modules, it would be >>> less convenient and less informative: ASSERT_EFI_ERROR() prints the >>> actual StatusParameter. >>> >>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). >>> Copy the original macro definition and update it as follows: >>> - replace EFI with RETURN, >>> - wrap overlong lines in the comment block and in the code, >>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead. >>> >>> Cc: Liming Gao >>> Cc: Michael D Kinney >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166 >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek >>> --- >>> >>> Notes: >>> OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is >>> one such BASE module. >>> >>> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++ >>> 1 file changed, 27 insertions(+) >>> >>> diff --git a/MdePkg/Include/Library/DebugLib.h >>> b/MdePkg/Include/Library/DebugLib.h >>> index 81904325703f..3a910e6a208b 100644 >>> --- a/MdePkg/Include/Library/DebugLib.h >>> +++ b/MdePkg/Include/Library/DebugLib.h >>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled ( >>> #define ASSERT_EFI_ERROR(StatusParameter) #endif >>> >>> +/** >>> + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code. >>> + >>> + If MDEPKG_NDEBUG is not defined and the >>> + DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED >>> + bit of PcdDebugProperyMask is set, then this macro evaluates the >>> + RETURN_STATUS value specified by StatusParameter. If >>> + StatusParameter is an error code, then DebugAssert() is called >>> + passing in the source filename, source line number, and StatusParameter. >>> + >>> + @param StatusParameter RETURN_STATUS value to evaluate. >>> + >>> +**/ >>> +#if !defined(MDEPKG_NDEBUG) >>> + #define ASSERT_RETURN_ERROR(StatusParameter) \ >>> + do { \ >>> + if (DebugAssertEnabled ()) { \ >>> + if (RETURN_ERROR (StatusParameter)) { \ >>> + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \ >>> + StatusParameter)); \ >>> + _ASSERT (!RETURN_ERROR (StatusParameter)); \ >>> + } \ >>> + } \ >>> + } while (FALSE) >>> +#else >>> + #define ASSERT_RETURN_ERROR(StatusParameter) >>> +#endif >>> + >>> /** >>> Macro that calls DebugAssert() if a protocol is already installed in the >>> handle database. >>> >> >> can I please get a maintainer review for this patch? The rest of the >> series is ready to go, but it depends on this patch. >> >> Thanks! >> Laszlo > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >