[edk2-devel] [PATCH V3] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined

[edk2-devel] [PATCH V3] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined

[Mike Beaton]

From: Mike Beaton <mjsbeaton@gmail.com>

The variant provided when MDEPKG_NDEBUG is defined will be optimised
away in RELEASE builds, but by referencing the argument list, avoids
unused variable errors from valid debug code, for example when STATIC
variables are used only in DEBUG statements.

Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>
---
 BaseTools/Conf/tools_def.template | 2 +-
 MdePkg/Include/Library/DebugLib.h | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index c34ecfd557..eaccf0b698 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -1859,7 +1859,7 @@ DEFINE CLANGDWARF_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x22
 DEFINE CLANGDWARF_IA32_TARGET             = -target i686-pc-linux-gnu
 DEFINE CLANGDWARF_X64_TARGET              = -target x86_64-pc-linux-gnu
 
-DEFINE CLANGDWARF_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-empty-body -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option -Wno-unused-but-set-variable -Wno-unused-const-variable -Wno-unaligned-access -Wno-unneeded-internal-declaration
+DEFINE CLANGDWARF_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-empty-body -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option -Wno-unused-but-set-variable -Wno-unused-const-variable -Wno-unaligned-access
 DEFINE CLANGDWARF_ALL_CC_FLAGS         = DEF(GCC48_ALL_CC_FLAGS) DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -msoft-float -mno-implicit-float  -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -funsigned-char -fno-ms-extensions -Wno-null-dereference
 
 ###########################
diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
index 40772f2e0f..bc7789f01c 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -426,7 +426,12 @@ UnitTestDebugAssert (
       }                            \
     } while (FALSE)
 #else
-#define DEBUG(Expression)
+#define DEBUG(Expression)        \
+    do {                           \
+      if (FALSE) {                 \
+        _DEBUG (Expression);       \
+      }                            \
+    } while (FALSE)
 #endif
 
 /**
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112492): https://edk2.groups.io/g/devel/message/112492
Mute This Topic: https://groups.io/mt/103160238/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined

[Mike Beaton]

I have currently gone with `if (FALSE)` in the above, even though
Michael Brown kindly offered tests which showed that `if ((FALSE))`
might help in a closely related context - i.e. when discussing
Mikhail's variant of this and related code - two years ago now:
https://edk2.groups.io/g/devel/message/83938 (and see
https://bugzilla.tianocore.org/show_bug.cgi?id=3704).

I cannot replicate that finding, or so far find any info about it, but
possibly it relates to an older version of clang? (Which if so,
possibly means it doesn't matter now anyway?)

From my own tests and fairly extensive Acidanthera CI suite, `if
(FALSE)` works on all current toolchains.

@Ard - Does this variant still fail on Arm? It would be interesting to
know which specific build of what is/was failing.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112493): https://edk2.groups.io/g/devel/message/112493
Mute This Topic: https://groups.io/mt/103160238/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined

[Mike Beaton]

Got it. These are the fixes for ArmVirtQemu.dsc. If people have
already manually worked round this issue it interferes with this
global fix (we had this in one file in Acidanthera code). There don't
seem to be many instances of that in the entire EDK-II codebase,
however, fortunately.

diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 432112354f..dc49e8eba7 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -71,9 +71,7 @@ PeCoffLoaderRelocateImageExtraAction (
   IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
   )
 {
- #if !defined (MDEPKG_NDEBUG)
   CHAR8  Temp[512];
- #endif

   if (ImageContext->PdbPointer) {
  #ifdef __CC_ARM
@@ -106,9 +104,7 @@ PeCoffLoaderUnloadImageExtraAction (
   IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
   )
 {
- #if !defined (MDEPKG_NDEBUG)
   CHAR8  Temp[512];
- #endif

   if (ImageContext->PdbPointer) {
  #ifdef __CC_ARM
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
index a39896d576..1d3ea61311 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
@@ -157,7 +157,6 @@ DescribeExceptionSyndrome (
   DEBUG ((DEBUG_ERROR, "\n %a \n", Message));
 }

-#ifndef MDEPKG_NDEBUG
 STATIC
 CONST CHAR8 *
 BaseName (
@@ -177,8 +176,6 @@ BaseName (
   return Str;
 }

-#endif
-
 /**
   This is the default action to take on an unexpected exception


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112494): https://edk2.groups.io/g/devel/message/112494
Mute This Topic: https://groups.io/mt/103160238/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH V3] DebugLib: Update DEBUG macro used when MDEPKG_NDEBUG is defined

[Ard Biesheuvel]

(cc MdePkg and BaseTools maintainers)

On Thu, 14 Dec 2023 at 00:08, Mike Beaton <mjsbeaton@gmail.com> wrote:
>
> From: Mike Beaton <mjsbeaton@gmail.com>
>
> The variant provided when MDEPKG_NDEBUG is defined will be optimised
> away in RELEASE builds, but by referencing the argument list, avoids
> unused variable errors from valid debug code, for example when STATIC
> variables are used only in DEBUG statements.
>
> Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>

I think this is an improvement, but others should confirm that other
toolchains are happy with this too (the change is no longer specific
to clang)

> ---
>  BaseTools/Conf/tools_def.template | 2 +-
>  MdePkg/Include/Library/DebugLib.h | 7 ++++++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index c34ecfd557..eaccf0b698 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -1859,7 +1859,7 @@ DEFINE CLANGDWARF_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x22
>  DEFINE CLANGDWARF_IA32_TARGET             = -target i686-pc-linux-gnu
>  DEFINE CLANGDWARF_X64_TARGET              = -target x86_64-pc-linux-gnu
>
> -DEFINE CLANGDWARF_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-empty-body -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option -Wno-unused-but-set-variable -Wno-unused-const-variable -Wno-unaligned-access -Wno-unneeded-internal-declaration
> +DEFINE CLANGDWARF_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-empty-body -Wno-unused-const-variable -Wno-varargs -Wno-unknown-warning-option -Wno-unused-but-set-variable -Wno-unused-const-variable -Wno-unaligned-access
>  DEFINE CLANGDWARF_ALL_CC_FLAGS         = DEF(GCC48_ALL_CC_FLAGS) DEF(CLANGDWARF_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -msoft-float -mno-implicit-float  -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -funsigned-char -fno-ms-extensions -Wno-null-dereference
>
>  ###########################
> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
> index 40772f2e0f..bc7789f01c 100644
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -426,7 +426,12 @@ UnitTestDebugAssert (
>        }                            \
>      } while (FALSE)
>  #else
> -#define DEBUG(Expression)
> +#define DEBUG(Expression)        \
> +    do {                           \
> +      if (FALSE) {                 \
> +        _DEBUG (Expression);       \
> +      }                            \
> +    } while (FALSE)
>  #endif
>
>  /**
> --
> 2.39.2
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112505): https://edk2.groups.io/g/devel/message/112505
Mute This Topic: https://groups.io/mt/103160238/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-