BaseTools/Conf/tools_def.template | 2 +- MdePkg/Include/Library/DebugLib.h | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-)
From: Mike Beaton <mjsbeaton@gmail.com>
Provides a variant of the DEBUG macro for clang when MDEPKG_NDEBUG is defined,
which uses but discards the contained expression, this means clang can tell
that it has optimised away variable usage, therefore we can keep
-Wunneeded-internal-declaration (as part of -Wall) to warn about any
mistakenly genuinely unused variables.
Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>
---
BaseTools/Conf/tools_def.template | 2 +-
MdePkg/Include/Library/DebugLib.h | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
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 f0c9f64487..e2158b1a3d 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -425,6 +425,16 @@ UnitTestDebugAssert (
_DEBUG (Expression); \
} \
} while (FALSE)
+#elif defined (__clang__)
+#define DEBUG(Expression) \
+ do { \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
+ if ((FALSE)) { \
+ (VOID) Expression; \
+ } \
+ _Pragma("GCC diagnostic pop") \
+ } while (FALSE)
#else
#define DEBUG(Expression)
#endif
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112363): https://edk2.groups.io/g/devel/message/112363
Mute This Topic: https://groups.io/mt/103126777/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-On Tue, 12 Dec 2023 at 09:49, Mike Beaton <mjsbeaton@gmail.com> wrote:
>
> From: Mike Beaton <mjsbeaton@gmail.com>
>
> Provides a variant of the DEBUG macro for clang when MDEPKG_NDEBUG is defined,
> which uses but discards the contained expression, this means clang can tell
> that it has optimised away variable usage, therefore we can keep
> -Wunneeded-internal-declaration (as part of -Wall) to warn about any
> mistakenly genuinely unused variables.
>
> Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>
You failed to mention where you found this patch.
> ---
> BaseTools/Conf/tools_def.template | 2 +-
> MdePkg/Include/Library/DebugLib.h | 10 ++++++++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> 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 f0c9f64487..e2158b1a3d 100644
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -425,6 +425,16 @@ UnitTestDebugAssert (
> _DEBUG (Expression); \
> } \
> } while (FALSE)
> +#elif defined (__clang__)
> +#define DEBUG(Expression) \
> + do { \
> + _Pragma("GCC diagnostic push") \
> + _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
This seems redundant to me. Either we set the pragma and the compiler
does not care, or we don't, and rely on the fact that the compiler can
infer that 'Expression' will never be evaluated at runtime, but won't
complain about symbols that are only referenced via 'Expression' and
nowhere else.
> + if ((FALSE)) { \
> + (VOID) Expression; \
> + } \
> + _Pragma("GCC diagnostic pop") \
> + } while (FALSE)
> #else
> #define DEBUG(Expression)
> #endif
> --
> 2.39.2
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#112363): https://edk2.groups.io/g/devel/message/112363
> Mute This Topic: https://groups.io/mt/103126777/5717338
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianocore@kernel.org]
> ------------
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112366): https://edk2.groups.io/g/devel/message/112366
Mute This Topic: https://groups.io/mt/103126777/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> You failed to mention where you found this patch. It's from https://github.com/acidanthera/audk/commit/dcd0a768b0f (which actually contains the code described in its commit message, but I believe some other code, including this specific part, which got squashed in at some point, and I believe is from the committer, not the alleged author actually!). Would it work to just add that origin commit to the commit message (given the licensing, which is the same as EDK-II, ofc), or would you prefer/need an additional Signed-off-by:? (Instead?) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112390): https://edk2.groups.io/g/devel/message/112390 Mute This Topic: https://groups.io/mt/103126777/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 12/12/23 13:57, Mike Beaton wrote: >> You failed to mention where you found this patch. > > It's from https://github.com/acidanthera/audk/commit/dcd0a768b0f > (which actually contains the code described in its commit message, but > I believe some other code, including this specific part, which got > squashed in at some point, and I believe is from the committer, not > the alleged author actually!). > > Would it work to just add that origin commit to the commit message > (given the licensing, which is the same as EDK-II, ofc), or would you > prefer/need an additional Signed-off-by:? (Instead?) > I believe one way to approach it would be to (a) amend the patch with --author="Marvin Häuser <mhaeuser@posteo.de>" and (b) have S-o-b's from both Marvin and you at the end of the commit message. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112449): https://edk2.groups.io/g/devel/message/112449 Mute This Topic: https://groups.io/mt/103126777/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> I believe one way to approach it would be to (a) amend the patch with > > --author="Marvin Häuser <mhaeuser@posteo.de>" > > and (b) have S-o-b's from both Marvin and you at the end of the commit > message. > As I say, I think the code in question is actual by Mikhail, despite appearances, but I should be able to get one of them to sign it off. Cheers. > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112450): https://edk2.groups.io/g/devel/message/112450 Mute This Topic: https://groups.io/mt/103126777/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 12/12/23 16:33, Mike Beaton wrote: > > I believe one way to approach it would be to (a) amend the patch with > > --author="Marvin Häuser <mhaeuser@posteo.de > <mailto:mhaeuser@posteo.de>>" > > and (b) have S-o-b's from both Marvin and you at the end of the commit > message. > > > As I say, I think the code in question is actual by Mikhail, despite > appearances, but I should be able to get one of them to sign it off. Cheers. > apologies, I missed that. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112451): https://edk2.groups.io/g/devel/message/112451 Mute This Topic: https://groups.io/mt/103126777/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> This seems redundant to me. Either we set the pragma and the compiler
> does not care, or we don't, and rely on the fact that the compiler can
> infer that 'Expression' will never be evaluated at runtime, but won't
> complain about symbols that are only referenced via 'Expression' and
> nowhere else.
I thought so too on initially reading the code, so I tried removing
the pragmas, and in fact the pragma is to tell the compiler not to
warn about the contained `(VOID) Expression`, not to fix the actual
problem warning - which `(VOID) Expression` itself does, once allowed.
So without the pragmas we get instead errors such as:
```
/home/mjsbeaton/OpenSource/edk2/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:444:86:
error: expression result unused; should this cast be to 'void'?
[-Werror,-Wunused-value]
DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Loading DXE CORE at 0x%11p
EntryPoint=0x%11p\n", (VOID *)(UINTN)DxeCoreAddress,
FUNCTION_ENTRY_POINT (DxeCoreEntryPoint)));
^ ~
/home/mjsbeaton/OpenSource/edk2/MdePkg/Include/Library/DebugLib.h:432:16:
note: expanded from macro 'DEBUG'
(VOID) Expression; \
^
1 error generated.
```
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112387): https://edk2.groups.io/g/devel/message/112387
Mute This Topic: https://groups.io/mt/103126777/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 12/12/23 11:33, Mike Beaton wrote: >> This seems redundant to me. Either we set the pragma and the compiler >> does not care, or we don't, and rely on the fact that the compiler can >> infer that 'Expression' will never be evaluated at runtime, but won't >> complain about symbols that are only referenced via 'Expression' and >> nowhere else. > > I thought so too on initially reading the code, so I tried removing > the pragmas, and in fact the pragma is to tell the compiler not to > warn about the contained `(VOID) Expression`, not to fix the actual > problem warning - which `(VOID) Expression` itself does, once allowed. > > So without the pragmas we get instead errors such as: > > ``` > /home/mjsbeaton/OpenSource/edk2/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c:444:86: > error: expression result unused; should this cast be to 'void'? > [-Werror,-Wunused-value] > DEBUG ((DEBUG_INFO | DEBUG_LOAD, "Loading DXE CORE at 0x%11p > EntryPoint=0x%11p\n", (VOID *)(UINTN)DxeCoreAddress, > FUNCTION_ENTRY_POINT (DxeCoreEntryPoint))); > > ^ ~ > /home/mjsbeaton/OpenSource/edk2/MdePkg/Include/Library/DebugLib.h:432:16: > note: expanded from macro 'DEBUG' > (VOID) Expression; \ > ^ > 1 error generated. > ``` > FWIW: Acked-by: Laszlo Ersek <lersek@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112448): https://edk2.groups.io/g/devel/message/112448 Mute This Topic: https://groups.io/mt/103126777/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.