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

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

This issue was being caused by variable EventNames in
OvmfPkg/VirtioSerialDxe/VirtioSerial.c in CLANGPDB X64 RELEASE build
prior to this commit, and was also being caused by the same variable
in CLANGDWARF X64 RELEASE build, prior to
d3225577123767fd09c91201d27e9c91663ae132 - which we
revert, restoring the desirable feature of failure to build on
genuinely (unintentionally) unused variables when building in either
clang toolchain.

Also change manual exclusion of debug vars when MDEPKG_NDEBUG is
not defined in a similar cases in ArmPkg, 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 (#112510): https://edk2.groups.io/g/devel/message/112510
Mute This Topic: https://groups.io/mt/103166935/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Mike Beaton]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

Repeats V5, but with a hopefully clearer (on motivation and history) commit message.


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



[-- Attachment #2: Type: text/html, Size: 864 bytes --]

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

[Ard Biesheuvel]

Please stop sending patches.

On Thu, 14 Dec 2023 at 10:12, Mike Beaton <mjsbeaton@gmail.com> wrote:
>
> Repeats V5, but with a hopefully clearer (on motivation and history) commit message.
> 


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

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

[Mike Beaton]

> Please stop sending patches.

I believe this version is a clear improvement, with motivation.
(Certainly, it was meant as such!)

How am I meant to send improvements or updates in this email-based workflow?

Mike


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

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

[Laszlo Ersek]

On 12/14/23 10:33, Mike Beaton wrote:
>> Please stop sending patches.
> 
> I believe this version is a clear improvement, with motivation.
> (Certainly, it was meant as such!)
> 
> How am I meant to send improvements or updates in this email-based workflow?

By pacing yourself. Posting two versions of the same patch set on the
same day is usually the highest tolerable posting frequency. Many would
say 1 version/day is the limit (unless there is a pressing security
issue or CI failure etc).

Basically the request is for the submitter to think more, let their
latest version soak for longer locally, before posting it.

BTW I don't think this is specific to email, the same issue exists on
any git forge, except perhaps to a lesser extent. Both channels are
asynchronous, so if you repost or force-push too quickly, you don't give
reviewers a chance to finish (or even *start*) the last version's
review. Well, interrupting them may actually be your intent, but that's
just not how async communication works. Once you posted it, it's out
there, and it's going to take up some consumer cycles for sure, either
way, regardless of what you do later. You can't recall it, you're not in
the same office at the same time.

github *seems* to mitigate this, because the old version more or less
just disappears. But that's actually a bug of git forges, not a feature.
Patch posting history should never be forgotten. Mailing lists get this
right, but that makes misbehavior (= too frequent posting) more
damaging, as the total traffic the receiver will have to wade through
will be higher.

In short: don't experiment, don't thrash. Make every version of your
patch set *count*. Give yourself more time to think about your latest
version *in the background*, before posting it. If you sleep over it,
the next day you might get a new idea, regardless of whether you posted
or didn't yet post that version. So, as long as it hasn't settled, don't
post it. If you realize an issue after posting the latest version,
respond -- just like a reviewer would -- to the problematic patch email,
pointing out the error; but *don't* post a new version just yet (i.e.,
don't create a new version / thread on the list). Your attempt to
"recall" the problematic version is bound to fail.

In short, s/TCP_NODELAY/TCP_CORK/.

Sorry about the sermon -- you asked. :)
Laszlo



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

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

[Mike Beaton]

I did ask. Thank you for the considered answer. Ack. :)

On Thu, 14 Dec 2023 at 13:25, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/14/23 10:33, Mike Beaton wrote:
> >> Please stop sending patches.
> >
> > I believe this version is a clear improvement, with motivation.
> > (Certainly, it was meant as such!)
> >
> > How am I meant to send improvements or updates in this email-based workflow?
>
> By pacing yourself. Posting two versions of the same patch set on the
> same day is usually the highest tolerable posting frequency. Many would
> say 1 version/day is the limit (unless there is a pressing security
> issue or CI failure etc).
>
> Basically the request is for the submitter to think more, let their
> latest version soak for longer locally, before posting it.
>
> BTW I don't think this is specific to email, the same issue exists on
> any git forge, except perhaps to a lesser extent. Both channels are
> asynchronous, so if you repost or force-push too quickly, you don't give
> reviewers a chance to finish (or even *start*) the last version's
> review. Well, interrupting them may actually be your intent, but that's
> just not how async communication works. Once you posted it, it's out
> there, and it's going to take up some consumer cycles for sure, either
> way, regardless of what you do later. You can't recall it, you're not in
> the same office at the same time.
>
> github *seems* to mitigate this, because the old version more or less
> just disappears. But that's actually a bug of git forges, not a feature.
> Patch posting history should never be forgotten. Mailing lists get this
> right, but that makes misbehavior (= too frequent posting) more
> damaging, as the total traffic the receiver will have to wade through
> will be higher.
>
> In short: don't experiment, don't thrash. Make every version of your
> patch set *count*. Give yourself more time to think about your latest
> version *in the background*, before posting it. If you sleep over it,
> the next day you might get a new idea, regardless of whether you posted
> or didn't yet post that version. So, as long as it hasn't settled, don't
> post it. If you realize an issue after posting the latest version,
> respond -- just like a reviewer would -- to the problematic patch email,
> pointing out the error; but *don't* post a new version just yet (i.e.,
> don't create a new version / thread on the list). Your attempt to
> "recall" the problematic version is bound to fail.
>
> In short, s/TCP_NODELAY/TCP_CORK/.
>
> Sorry about the sermon -- you asked. :)
> Laszlo
>


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