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

[edk2-devel] [PATCH V5] 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.

Variable EventNames in OvmfPkg/VirtioSerialDxe/VirtioSerial.c
was causing this issue in CLANGPDB X64 RELEASE build, prior to this
commit.

We also change manual exclusion of debug vars when MDEPKG_NDEBUG is
not defined, in a similar case in
ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
to inclusion when used regardless of MDEPKG_NDEBUG (the revised DEBUG
macro automatically compiles away unused variable accesses, but there
has to be a variable, access to which to discard).

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

diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
index 432112354f..c5c53ef3e1 100644
--- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
+++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
@@ -71,7 +71,7 @@ PeCoffLoaderRelocateImageExtraAction (
   IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
   )
 {
- #if !defined (MDEPKG_NDEBUG)
+ #if defined (__CC_ARM) || defined (__GNUC__)
   CHAR8  Temp[512];
  #endif
 
@@ -106,7 +106,7 @@ PeCoffLoaderUnloadImageExtraAction (
   IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
   )
 {
- #if !defined (MDEPKG_NDEBUG)
+ #if defined (__CC_ARM) || defined (__GNUC__)
   CHAR8  Temp[512];
  #endif
 
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
 
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 (#112506): https://edk2.groups.io/g/devel/message/112506
Mute This Topic: https://groups.io/mt/103166459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

(cc Michael)

On Thu, 14 Dec 2023 at 08:58, 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.
>
> Variable EventNames in OvmfPkg/VirtioSerialDxe/VirtioSerial.c
> was causing this issue in CLANGPDB X64 RELEASE build, prior to this
> commit.
>
> We also change manual exclusion of debug vars when MDEPKG_NDEBUG is
> not defined, in a similar case in
> ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> to inclusion when used regardless of MDEPKG_NDEBUG (the revised DEBUG
> macro automatically compiles away unused variable accesses, but there
> has to be a variable, access to which to discard).
>
> Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>

Hi Mike,

I'd prefer to have some discussion with the various maintainers first
- shooting off a new version of your patch every time anyone asks a
question is only going to confuse people.

IOW, please don't send a v6 until the discussion comes to a conclusion.

> ---
>  .../DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c  | 4 ++--
>  .../AArch64/DefaultExceptionHandler.c                      | 3 ---
>  BaseTools/Conf/tools_def.template                          | 2 +-
>  MdePkg/Include/Library/DebugLib.h                          | 7 ++++++-

These are going to have to be separate patches in any case, even the
MdePkg one and BaseTools changes will need to be split.

So the question is really whether the MdePkg change is palatable to
the maintainers and to the users of other toolchains.

Then, we can talk about how to phase the remaining changes so we don't
break the ARM build.



>  4 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> index 432112354f..c5c53ef3e1 100644
> --- a/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> +++ b/ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.c
> @@ -71,7 +71,7 @@ PeCoffLoaderRelocateImageExtraAction (
>    IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
>    )
>  {
> - #if !defined (MDEPKG_NDEBUG)
> + #if defined (__CC_ARM) || defined (__GNUC__)

No, this is not going to be acceptable to me. You noted that only the
ARM code seems to suffer from this issue, so surely, we can find a way
to change this code that doesn't introduce spurious dependencies on
the exact toolchain we are using.

>    CHAR8  Temp[512];
>   #endif
>
> @@ -106,7 +106,7 @@ PeCoffLoaderUnloadImageExtraAction (
>    IN OUT PE_COFF_LOADER_IMAGE_CONTEXT  *ImageContext
>    )
>  {
> - #if !defined (MDEPKG_NDEBUG)
> + #if defined (__CC_ARM) || defined (__GNUC__)
>    CHAR8  Temp[512];
>   #endif
>
> 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
>
> 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 (#112508): https://edk2.groups.io/g/devel/message/112508
Mute This Topic: https://groups.io/mt/103166459/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Mike Beaton]

> IOW, please don't send a v6 until the discussion comes to a conclusion.

Apologies, I did _not_ see this before sending.

> > - #if !defined (MDEPKG_NDEBUG)
> > + #if defined (__CC_ARM) || defined (__GNUC__)
>
> No, this is not going to be acceptable to me. You noted that only the
> ARM code seems to suffer from this issue, so surely, we can find a way
> to change this code that doesn't introduce spurious dependencies on
> the exact toolchain we are using.

With all due respect, I believe that you are incorrect that this is
spurious. Please check the surrounding code (specifically, this
compiler dependency exactly matches - and is there because of - a
compiler dependency in the surrounding code). (Additionally, though it
doesn't affect the point either way, I thought this is what you had
spotted and were asking for, when previously saying "What about
GCC?"!)

Mike


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

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

[Ard Biesheuvel]

On Thu, 14 Dec 2023 at 10:37, Mike Beaton <mjsbeaton@gmail.com> wrote:
>
> > IOW, please don't send a v6 until the discussion comes to a conclusion.
>
> Apologies, I did _not_ see this before sending.
>
> > > - #if !defined (MDEPKG_NDEBUG)
> > > + #if defined (__CC_ARM) || defined (__GNUC__)
> >
> > No, this is not going to be acceptable to me. You noted that only the
> > ARM code seems to suffer from this issue, so surely, we can find a way
> > to change this code that doesn't introduce spurious dependencies on
> > the exact toolchain we are using.
>
> With all due respect, I believe that you are incorrect that this is
> spurious. Please check the surrounding code (specifically, this
> compiler dependency exactly matches - and is there because of - a
> compiler dependency in the surrounding code). (Additionally, though it
> doesn't affect the point either way, I thought this is what you had
> spotted and were asking for, when previously saying "What about
> GCC?"!)
>

Indeed. I hadn't spotted the context, and actually (patch sent), we
need to rip that code out entirely. So please consider this piece
solved.


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