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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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
>