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

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

[Michael D Kinney]

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

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

[Michael D Kinney]

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

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

[Michael Brown]

On 22/11/2021 16:42, Michael D Kinney wrote:
> 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 very strongly recommend having the non-debug version of the 
macro use something like:

#define ASSERT(Expression) do {   \
     if (FALSE) {                  \
       (VOID) (Expression);        \
     }                             \
   } while (FALSE)

Without the "if (FALSE)", you will find that an expression that may have 
side-effects (e.g. by calling an external function) will result in 
executable code being generated.

Michael

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

[Marvin Häuser]

23.11.2021 00:17:30 Michael Brown <mcb30@ipxe.org>:

> On 22/11/2021 16:42, Michael D Kinney wrote:
>> 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 very strongly recommend having the non-debug version of the macro use something like:
>
> #define ASSERT(Expression) do {   \
>      if (FALSE) {                  \
>        (VOID) (Expression);        \
>      }                             \
>    } while (FALSE)
>
> Without the "if (FALSE)", you will find that an expression that may have side-effects (e.g. by calling an external function) will result in executable code being generated.

In theory +1, but don't some compilers generate "unreachable code" warnings for this? I unfortunately cannot check them all right now. Maybe guards push/pop'ing the warning for that need to be defined, that are only allowed to be used with explicit documentation why they are necessary?

Best regards,
Marvin

>
> Michael
>
>
> 

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

[Michael Brown]

On 23/11/2021 06:41, Marvin Häuser wrote:
> 23.11.2021 00:17:30 Michael Brown <mcb30@ipxe.org>:
>> I would very strongly recommend having the non-debug version of the macro use something like:
>>
>> #define ASSERT(Expression) do {   \
>>       if (FALSE) {                  \
>>         (VOID) (Expression);        \
>>       }                             \
>>     } while (FALSE)
>>
>> Without the "if (FALSE)", you will find that an expression that may have side-effects (e.g. by calling an external function) will result in executable code being generated.
> 
> In theory +1, but don't some compilers generate "unreachable code" warnings for this? I unfortunately cannot check them all right now. Maybe guards push/pop'ing the warning for that need to be defined, that are only allowed to be used with explicit documentation why they are necessary?

A quick experiment shows that gcc won't generate a warning but clang 
will.  Can be fixed by adding extra parentheses around FALSE:

#define ASSERT(Expression) do {   \
     if ((FALSE)) {                \
       (VOID) (Expression);        \
     }                             \
   } while (FALSE)

which is still clean and relatively elegant.

Thanks,

Michael