From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 816F51A1E43 for ; Wed, 26 Oct 2016 19:54:01 -0700 (PDT) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP; 26 Oct 2016 19:54:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,404,1473145200"; d="scan'208,217";a="24226439" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by orsmga005.jf.intel.com with ESMTP; 26 Oct 2016 19:54:00 -0700 Received: from orsmsx161.amr.corp.intel.com (10.22.240.84) by ORSMSX109.amr.corp.intel.com (10.22.240.7) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 26 Oct 2016 19:54:00 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.50]) by ORSMSX161.amr.corp.intel.com ([10.22.240.84]) with mapi id 14.03.0248.002; Wed, 26 Oct 2016 19:54:00 -0700 From: "Kinney, Michael D" To: "Gao, Liming" , Laszlo Ersek , "Kinney, Michael D" CC: edk2-devel-01 , "Justen, Jordan L" , Ard Biesheuvel Thread-Topic: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() Thread-Index: AQHSK+H8HrgS1BpVOk2rwDlzLm52ZqC4kKAA//+qaGCAAQyOgIAALCsAgAIt7kA= Date: Thu, 27 Oct 2016 02:53:59 +0000 Message-ID: References: <20161021212737.15974-1-lersek@redhat.com> <20161021212737.15974-2-lersek@redhat.com> <567fc440-ce97-548d-e645-6ae84dfbf488@redhat.com> <5f28fb02-1b03-e3e8-7a8d-443579bfe30a@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14B4992EF@shsmsx102.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14B4992EF@shsmsx102.ccr.corp.intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGE2OTdhZDYtZTNmNC00ZGRmLTgwNjQtMzVhOWM4NjM5YTMyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IjRVVDFzN2xDSTZPSFdiaGE4NGxXZUdtMHJScEJWWW1ndVV5UHJBcDhicnc9In0= x-originating-ip: [10.22.254.140] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 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: Thu, 27 Oct 2016 02:54:01 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, I investigated the QuarkSocPkg ones. The extra #include of BaseType.h should be removed from: QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c However, it should not be removed from the other one: QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c The ResetSystemLib uses EFI_TIME that is defined in BaseType.h. I will send patch for QNCSmmLib. Mike From: Gao, Liming Sent: Tuesday, October 25, 2016 3:33 AM To: Laszlo Ersek ; Kinney, Michael D Cc: edk2-devel-01 ; Justen, Jordan L ; Ard Biesheuvel Subject: RE: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERRO= R() Laszlo: Thanks for your report them. I just investigate MdePkg and MdeModulePkg o= nes. MdePkg BaseLib should be BASE type. It doesn't depend on UEFI. I will = clean up it. MdeModulePkg FrameBufferBltLib is designed for UEFI GOP BLT op= eration. This library instance type should be UEFI_DRIVER. I will provide the patch to clean up them. Thanks Liming From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Tuesday, October 25, 2016 3:54 PM To: Kinney, Michael D >; Gao, Liming > Cc: edk2-devel-01 >; = Justen, Jordan L >; Ard Biesheuvel > Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERRO= R() On 10/25/16 01:05, Kinney, Michael D wrote: > 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. I spent a few minutes on the following shell script, to identify such libraries: { # Locate the INF files that have a LIBRARY_CLASS define with a client # module type list that explicitly includes BASE git grep -l -E '\' -- \ '*.inf' # Locate the INF files that have MODULE_TYPE=3DBASE, and a LIBRARY_CLASS # define without a client type list. git grep -l -E '\' -- '*inf' \ | xargs -r -- grep -l -E '\[^|]+$' -- } \ | { # Cut off the last pathname component, in order to get the pathname of # the directory containing the INF file rev | cut -f 2- -d / | rev } \ | { # If a directory has several matching INF files, list the directory # only once. sort -u } \ | { # Check if any file in these directories includes # "Uefi/UefiBaseType.h". xargs -r -- grep -r -l Uefi/UefiBaseType.h -- } It prints the following files: CorebootModulePkg/Library/CbParseLib/CbParseLib.c CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.c MdePkg/Library/BaseLib/FilePaths.c OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c We should likely investigate them. I'll handle the OvmfPkg one. > > 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 Thank you! Laszlo > 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_E= RROR() >> >> 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 modul= es. >>> >>> 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 t= he >>> 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=3D166 >>> 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 err= or code. >>> + >>> + If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_E= NABLED >>> + 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 filena= me, >>> + 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 =3D %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 th= e >>> 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 >