Re: [PATCH] MdePkg/DebugLib; swap if conditions in ASSERT_[EFI|RETURN]_ERROR

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Ard,

The reason for the current ordering is for size optimization.

The most common implementation of DebugAssertEnabled() uses
a FixedAtBuild PCD to determine if these are enabled.  The 
check of status can be optimized away if they are disabled.
If you reverse them, then the status check is always performed.

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, December 7, 2017 7:26 AM
> To: edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Gao, Liming
> <liming.gao@intel.com>; Evan Lloyd <evan.lloyd@arm.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Alexei
> Fedorov <Alexei.Fedorov@arm.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [PATCH] MdePkg/DebugLib; swap if conditions
> in ASSERT_[EFI|RETURN]_ERROR
> 
> On 7 December 2017 at 15:12, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > ASSERT_EFI_ERROR () is currently defined as
> >
> >   #if !defined(MDEPKG_NDEBUG)
> >     #define ASSERT_EFI_ERROR(StatusParameter)
> \
> >       do {
> \
> >         if (DebugAssertEnabled ()) {
> \
> >           if (EFI_ERROR (StatusParameter)) {
> \
> >             DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR
> (Status = %r)\n", StatusParameter));  \
> >             _ASSERT (!EFI_ERROR (StatusParameter));
> \
> >           }
> \
> >         }
> \
> >       } while (FALSE)
> >   #else
> >     #define ASSERT_EFI_ERROR(StatusParameter)
> >   #endif
> >
> > This is suboptimal, given that the DebugAssertEnabled
> () call in the
> > outer if must be executed unconditionally, since the
> compiler does not
> > know that it does not have any side effects. Instead,
> let's swap the
> > two ifs, and only call DebugAssertEnabled () if
> StatusParameter contains
> > an error value to begin with. Do the same for
> ASSERT_RETURN_ERROR
> > as well.
> >
> 
> I just noticed we could do the same for ASSERT () as
> well.
> 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> > Reported-by: Alexei Fedorov <Alexei.Fedorov@arm.com>
> > ---
> >  MdePkg/Include/Library/DebugLib.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdePkg/Include/Library/DebugLib.h
> b/MdePkg/Include/Library/DebugLib.h
> > index 3a910e6a208b..8369c378e79c 100644
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -337,8 +337,8 @@ DebugPrintLevelEnabled (
> >  #if !defined(MDEPKG_NDEBUG)
> >    #define ASSERT_EFI_ERROR(StatusParameter)
> \
> >      do {
> \
> > -      if (DebugAssertEnabled ()) {
> \
> > -        if (EFI_ERROR (StatusParameter)) {
> \
> > +      if (EFI_ERROR (StatusParameter)) {
> \
> > +        if (DebugAssertEnabled ()) {
> \
> >            DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR
> (Status = %r)\n", StatusParameter));  \
> >            _ASSERT (!EFI_ERROR (StatusParameter));
> \
> >          }
> \
> > @@ -363,8 +363,8 @@ DebugPrintLevelEnabled (
> >  #if !defined(MDEPKG_NDEBUG)
> >    #define ASSERT_RETURN_ERROR(StatusParameter)
> \
> >      do {
> \
> > -      if (DebugAssertEnabled ()) {
> \
> > -        if (RETURN_ERROR (StatusParameter)) {
> \
> > +      if (RETURN_ERROR (StatusParameter)) {
> \
> > +        if (DebugAssertEnabled ()) {
> \
> >            DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR
> (Status = %r)\n", \
> >              StatusParameter));
> \
> >            _ASSERT (!RETURN_ERROR (StatusParameter));
> \
> > --
> > 2.11.0
> >