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

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

[Mike Beaton]

From: Mikhail Krichanov <mikhailkrichanov@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 as part of
valid debug patterns (such as a STATIC variable which is only used in
DEBUG statements) - rather than just seeing such variables as
completely unused - therefore we can keep
-Wunneeded-internal-declaration (as part of -Wall) to warn about
mistakenly genuinely unused variables elsewhere.

Note that the _Pragma in the clang/gcc variant is to temporarily
suppress the warning about `(VOID) Expression;`, whilst the presence
of `(VOID) Expression;` (once allowed) is what prevents the unwanted
warning about unneeded variables.

Signed-off-by: Mikhail Krichanov <mikhailkrichanov@gmail.com>
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 40772f2e0f..7368004f46 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -425,6 +425,16 @@ UnitTestDebugAssert (
         _DEBUG (Expression);       \
       }                            \
     } while (FALSE)
+#elif defined (__GNUC__) || 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 (#112457): https://edk2.groups.io/g/devel/message/112457
Mute This Topic: https://groups.io/mt/103138778/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Mike Beaton]

On Tue, 12 Dec 2023 at 21:45, <mjsbeaton@gmail.com> wrote:
>
> From: Mikhail Krichanov <mikhailkrichanov@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 as part of
> valid debug patterns (such as a STATIC variable which is only used in
> DEBUG statements) - rather than just seeing such variables as
> completely unused - therefore we can keep
> -Wunneeded-internal-declaration (as part of -Wall) to warn about
> mistakenly genuinely unused variables elsewhere.
>
> Note that the _Pragma in the clang/gcc variant is to temporarily
> suppress the warning about `(VOID) Expression;`, whilst the presence
> of `(VOID) Expression;` (once allowed) is what prevents the unwanted
> warning about unneeded variables.
>
> Signed-off-by: Mikhail Krichanov <mikhailkrichanov@gmail.com>
> 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 40772f2e0f..7368004f46 100644
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -425,6 +425,16 @@ UnitTestDebugAssert (
>          _DEBUG (Expression);       \
>        }                            \
>      } while (FALSE)
> +#elif defined (__GNUC__) || 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
>

Hi Mikhail,

Are you okay to reply confirming that you're happy with your
Signed-off-by in the above?

Thanks,
Mike


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

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

[Ard Biesheuvel]

On Tue, 12 Dec 2023 at 22:46, Mike Beaton <mjsbeaton@gmail.com> wrote:
>
> From: Mikhail Krichanov <mikhailkrichanov@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 as part of
> valid debug patterns (such as a STATIC variable which is only used in
> DEBUG statements) - rather than just seeing such variables as
> completely unused - therefore we can keep
> -Wunneeded-internal-declaration (as part of -Wall) to warn about
> mistakenly genuinely unused variables elsewhere.
>
> Note that the _Pragma in the clang/gcc variant is to temporarily
> suppress the warning about `(VOID) Expression;`, whilst the presence
> of `(VOID) Expression;` (once allowed) is what prevents the unwanted
> warning about unneeded variables.
>
> Signed-off-by: Mikhail Krichanov <mikhailkrichanov@gmail.com>
> Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>

This patch breaks the build on ARM (undeclared functions and
variables). Nothing we couldn't fix, but that needs to be done before
we can consider this change.


> ---
>  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 40772f2e0f..7368004f46 100644
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -425,6 +425,16 @@ UnitTestDebugAssert (
>          _DEBUG (Expression);       \
>        }                            \
>      } while (FALSE)
> +#elif defined (__GNUC__) || defined (__clang__)

This is now being enabled on GCC too - why?

> +#define DEBUG(Expression)                                \
> +    do {                                                   \
> +      _Pragma("GCC diagnostic push")                       \
> +      _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
> +      if ((FALSE)) {                                       \
> +        (VOID) Expression;                                 \

So what is the point of the VOID cast here? Usually, these are added
to avoid unused value warnings, but these are disabled explicitly via
the pragma.

Why can't we just use

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

 /**

here?

> +      }                                                    \
> +      _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 (#112459): https://edk2.groups.io/g/devel/message/112459
Mute This Topic: https://groups.io/mt/103138778/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Mike Beaton]

> > +#define DEBUG(Expression)                                \
> > +    do {                                                   \
> > +      _Pragma("GCC diagnostic push")                       \
> > +      _Pragma("GCC diagnostic ignored \"-Wunused-value\"") \
> > +      if ((FALSE)) {                                       \
> > +        (VOID) Expression;                                 \
>
> So what is the point of the VOID cast here? Usually, these are added
> to avoid unused value warnings, but these are disabled explicitly via
> the pragma.

```
$ cat test.c
static int a = 0;

void foo()
{
  if ((0)) {
    (void)((void *)(long)0, a);
  }
}
$ clang -c -Wall test.c -o test.o
test.c:6:12: warning: expression result unused; should this cast be to
'void'? [-Wunused-value]
    (void)((void *)(long)0, a);
           ^     ~
1 warning generated.
```

Without the pragmas, various DEBUG statements sent into the macro
generate unused *value* warnings exactly such as the above. Once we
suppress these unused value warnings, the existence of the
comma-operator separated list of values, cast to void, avoids unused
*variable* warnings, such as the unused variable warning that would
otherwise occur about `a` above - allowing `a` to instead just be
optimised away when it becomes unused in this way, as we want.

For the other questions, let me get back to you! Thank you.


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