Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.

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

Hi Mikhail,

Are you able to provide a few examples of dead code this change uncovers?

If there is additional issues then wouldn't applying this patch break all the
open source package builds that contain these issues?  Does this patch 
series pass EDK II CI?

Wouldn't it also make sense to remove the disable of these warnings from
all the toolchains if these updates are the only ones required to pass
EDK II CI for all supported toolchains?

You are also modifying the DebugLib in the paths where ASSERT() macros
are disabled.  When they are disabled, we want all code/data associated
with ASSERT() to be removed by the optimizing compiler/linker.  The source
code change appears to force a reference to a variable/expression.  Does
this have any size impact to any of the toolchains when ASSERT() is 
disabled?  Can you provide the size comparison before and after this
change?

I would like to add that I am in favor of using the compilers to help find
issues in source code and increasing warning levels is great technique.
We just need to make sure the changes do not generate false positives on
issues and that the size and performance impacts of the changes are 
measured as part of the evaluation of the change.  Especially components
like DebugLib.h that can impact almost every component in the entire 
FW stack.

Thanks,

Mike

> -----Original Message-----
> From: krichanov@ispras.ru <krichanov@ispras.ru>
> Sent: Monday, November 22, 2021 12:15 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: gaoliming@byosoft.com.cn; Liu, Zhiguang <zhiguang.liu@intel.com>; vit9696@protonmail.com
> Subject: Re: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.
> 
> Hi Mike,
> 
> This warning helps to find dead code. Disabling it leads to code quality
> degradation.
> ---
> Mikhail Krichanov
> 
> On 2021-11-19 19:51, Kinney, Michael D wrote:
> > Hi Mikhail,
> >
> > For RELEASE GCC5 toolchains in tools_def.txt, I see this warning is
> > disabled.  Likely for the same reason.
> >
> > RELEASE_GCC5_IA32_CC_FLAGS       = DEF(GCC5_IA32_CC_FLAGS) -flto -Os
> > -Wno-unused-but-set-variable -Wno-unused-const-variable
> > RELEASE_GCC5_X64_CC_FLAGS        = DEF(GCC5_X64_CC_FLAGS) -flto
> > -DUSING_LTO -Os -Wno-unused-but-set-variable
> > -Wno-unused-const-variable
> > RELEASE_GCC5_ARM_CC_FLAGS        = DEF(GCC5_ARM_CC_FLAGS) -flto
> > -Wno-unused-but-set-variable -Wno-unused-const-variable
> > RELEASE_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -flto
> > -Wno-unused-but-set-variable -Wno-unused-const-variable
> >
> > I think it would be better to ignore the same warning in RELEASE
> > CLANG38 tool chains with an update to tools_def.txt instead of
> > DebugLib.h.
> >
> > Unless you are thinking that disabling this warning is hiding some bad
> > code practices that need to be cleaned up.
> >
> > Best regards,
> >
> > Mike
> >
> >
> >> -----Original Message-----
> >> From: Mikhail Krichanov <krichanov@ispras.ru>
> >> Sent: Friday, November 19, 2021 1:05 AM
> >> To: devel@edk2.groups.io
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao
> >> <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> >> <zhiguang.liu@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>;
> >> Mikhail Krichanov <krichanov@ispras.ru>
> >> Subject: [PATCH] MdePkg: DebugLib: Compilation fix for clang-13.
> >>
> >> build -a X64 -t CLANG38 -b RELEASE -p OvmfPkg/OvmfPkgX64.dsc
> >> results in
> >> UDK/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c:1284:31:
> >> error: variable 'Status' set but not used
> >> [-Werror,-Wunused-but-set-variable]
> >>   EFI_STATUS                  Status;
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3704
> >>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> >> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> >> Signed-off-by: Mikhail Krichanov <krichanov@ispras.ru>
> >> ---
> >>  MdePkg/Include/Library/DebugLib.h | 23 ++++++++++++++++++++---
> >>  1 file changed, 20 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/MdePkg/Include/Library/DebugLib.h
> >> b/MdePkg/Include/Library/DebugLib.h
> >> index 4cacd4b8..7785e99d 100644
> >> --- a/MdePkg/Include/Library/DebugLib.h
> >> +++ b/MdePkg/Include/Library/DebugLib.h
> >> @@ -370,7 +370,10 @@ UnitTestDebugAssert (
> >>        }                             \
> >>      } while (FALSE)
> >>  #else
> >> -  #define ASSERT(Expression)
> >> +  #define ASSERT(Expression)       \
> >> +    do {                           \
> >> +      (VOID) (Expression);         \
> >> +    } while (FALSE)
> >>  #endif
> >>
> >>  /**
> >> @@ -392,6 +395,14 @@ UnitTestDebugAssert (
> >>          _DEBUG (Expression);       \
> >>        }                            \
> >>      } while (FALSE)
> >> +#elif defined(__GNUC__) || defined(__clang__)
> >> +  #define DEBUG(Expression)                                \
> >> +    do {                                                   \
> >> +      _Pragma("GCC diagnostic push")                       \
> >> +      _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
> >> +      (VOID) Expression;                                   \
> >> +      _Pragma("GCC diagnostic pop")                        \
> >> +    } while (FALSE)
> >>  #else
> >>    #define DEBUG(Expression)
> >>  #endif
> >> @@ -419,7 +430,10 @@ UnitTestDebugAssert (
> >>        }
> >>                    \
> >>      } while (FALSE)
> >>  #else
> >> -  #define ASSERT_EFI_ERROR(StatusParameter)
> >> +  #define ASSERT_EFI_ERROR(StatusParameter)
> >>                   \
> >> +    do {
> >>                   \
> >> +      (VOID) (StatusParameter);
> >>                   \
> >> +    } while (FALSE)
> >>  #endif
> >>
> >>  /**
> >> @@ -446,7 +460,10 @@ UnitTestDebugAssert (
> >>        }
> >>   \
> >>      } while (FALSE)
> >>  #else
> >> -  #define ASSERT_RETURN_ERROR(StatusParameter)
> >> +  #define ASSERT_RETURN_ERROR(StatusParameter)
> >>   \
> >> +    do {
> >>   \
> >> +      (VOID) (StatusParameter);
> >>   \
> >> +    } while (FALSE)
> >>  #endif
> >>
> >>  /**
> >> --
> >> 2.20.1