* [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
@ 2023-12-12 8:48 Mike Beaton
2023-12-12 9:26 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Mike Beaton @ 2023-12-12 8:48 UTC (permalink / raw)
To: devel; +Cc: ardb, lersek, Mike Beaton
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
2023-12-12 8:48 [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang Mike Beaton
@ 2023-12-12 9:26 ` Ard Biesheuvel
2023-12-12 10:33 ` Mike Beaton
2023-12-12 12:57 ` Mike Beaton
0 siblings, 2 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2023-12-12 9:26 UTC (permalink / raw)
To: devel, mjsbeaton; +Cc: ardb, lersek
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
2023-12-12 9:26 ` Ard Biesheuvel
@ 2023-12-12 10:33 ` Mike Beaton
2023-12-12 15:26 ` Laszlo Ersek
2023-12-12 12:57 ` Mike Beaton
1 sibling, 1 reply; 8+ messages in thread
From: Mike Beaton @ 2023-12-12 10:33 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, ardb, lersek
> 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
2023-12-12 10:33 ` Mike Beaton
@ 2023-12-12 15:26 ` Laszlo Ersek
0 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2023-12-12 15:26 UTC (permalink / raw)
To: Mike Beaton, Ard Biesheuvel; +Cc: devel, ardb
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
2023-12-12 9:26 ` Ard Biesheuvel
2023-12-12 10:33 ` Mike Beaton
@ 2023-12-12 12:57 ` Mike Beaton
2023-12-12 15:28 ` Laszlo Ersek
1 sibling, 1 reply; 8+ messages in thread
From: Mike Beaton @ 2023-12-12 12:57 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, ardb, lersek
> 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang
2023-12-12 12:57 ` Mike Beaton
@ 2023-12-12 15:28 ` Laszlo Ersek
2023-12-12 15:33 ` Mike Beaton
0 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2023-12-12 15:28 UTC (permalink / raw)
To: Mike Beaton, Ard Biesheuvel; +Cc: devel, ardb
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-12-12 16:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-12 8:48 [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang Mike Beaton
2023-12-12 9:26 ` Ard Biesheuvel
2023-12-12 10:33 ` Mike Beaton
2023-12-12 15:26 ` Laszlo Ersek
2023-12-12 12:57 ` Mike Beaton
2023-12-12 15:28 ` Laszlo Ersek
2023-12-12 15:33 ` Mike Beaton
2023-12-12 16:52 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox