.../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(-)
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> 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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.