[edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[Ard Biesheuvel]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[Mike Beaton]

> 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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[Mike Beaton]

> 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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[Mike Beaton]

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

> 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/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: 1686 bytes --]

Re: [edk2-devel] [PATCH] DebugLib: Allow -Wunneeded-internal-declaration with clang

[Laszlo Ersek]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-