| Message ID | 20161021212737.15974-2-lersek@redhat.com |
|---|---|
| State | Accepted |
| Commit | 08bcaf20b1320845ff4b140423dc4023695fe0fd |
| Headers | show |
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 <liming.gao@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > 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
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 <Uefi/UefiBaseTypes.h>, 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 <michael.d.kinney@intel.com> Best regards, Mike > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Monday, October 24, 2016 2:00 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > <liming.gao@intel.com> > Cc: edk2-devel-01 <edk2-devel@ml01.01.org> > 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 <liming.gao@intel.com> > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166 > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > --- > > > > 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
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 > <Uefi/UefiBaseTypes.h>, 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 '\<LIBRARY_CLASS *= *[A-Za-z0-9_]+ *\|.*\<BASE\>' -- \ '*.inf' # Locate the INF files that have MODULE_TYPE=BASE, and a LIBRARY_CLASS # define without a client type list. git grep -l -E '\<MODULE_TYPE *= *BASE\>' -- '*inf' \ | xargs -r -- grep -l -E '\<LIBRARY_CLASS\>[^|]+$' -- } \ | { # 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 <michael.d.kinney@intel.com> 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 <michael.d.kinney@intel.com>; Gao, Liming >> <liming.gao@intel.com> >> Cc: edk2-devel-01 <edk2-devel@ml01.01.org> >> 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 <liming.gao@intel.com> >>> Cc: Michael D Kinney <michael.d.kinney@intel.com> >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166 >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>> --- >>> >>> 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
In fact, EFI_* codes are a subset of RETURN_* codes, so it seems work to implement ASSERT_EFI_ERROR() using new ASSERT_RETURN_ERROR(). 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 <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Cc: edk2-devel-01 <edk2-devel@ml01.01.org> 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 <Uefi/UefiBaseTypes.h>, 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 <michael.d.kinney@intel.com> Best regards, Mike > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Monday, October 24, 2016 2:00 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > <liming.gao@intel.com> > Cc: edk2-devel-01 <edk2-devel@ml01.01.org> > 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 <liming.gao@intel.com> > > Cc: Michael D Kinney <michael.d.kinney@intel.com> > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166 > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > --- > > > > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> > Cc: edk2-devel-01 <edk2-devel@ml01.01.org> > 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 <Uefi/UefiBaseTypes.h>, 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 <michael.d.kinney@intel.com> > > Best regards, > > Mike > > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Monday, October 24, 2016 2:00 PM >> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming >> <liming.gao@intel.com> >> Cc: edk2-devel-01 <edk2-devel@ml01.01.org> >> 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 <liming.gao@intel.com> >>> Cc: Michael D Kinney <michael.d.kinney@intel.com> >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166 >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>> --- >>> >>> 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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo: Thanks for your report them. I just investigate MdePkg and MdeModulePkg ones. 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 operation. 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 <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() 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=BASE, 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_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
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 <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: RE: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() Laszlo: Thanks for your report them. I just investigate MdePkg and MdeModulePkg ones. 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 operation. 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 <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> Cc: edk2-devel-01 <edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() 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=BASE, 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_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
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.
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 <liming.gao@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- 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(+) -- 2.9.2 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel