From: Mike Beaton <mjsbeaton@gmail.com>

This warning was already disabled in CLANGDWARF by commit
d3225577123767fd09c91201d27e9c91663ae132.

gcc can distinguish between optimised-away variable usage (as  can occur in
valid debug code) and genuinely unused variables, and only complains about
the latter. clang cannot, and therefore this warning ends up complaining
about valid debug code under clang.

Since EDK-II code is in general going to be compiled by gcc as well as clang
then disabling this warning in clang does not amount to entirely removing
potentially valid warnings about genuinely unused variables.

Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>
---
 BaseTools/Conf/tools_def.template | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index c34ecfd557..48cf45245f 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -1754,7 +1754,7 @@ DEFINE CLANGPDB_X64_PREFIX           = ENV(CLANG_BIN)
 DEFINE CLANGPDB_IA32_TARGET          = -target i686-unknown-windows-gnu
 DEFINE CLANGPDB_X64_TARGET           = -target x86_64-unknown-windows-gnu
 
-DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare -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-microsoft-enum-forward-reference
+DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare -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 -Wno-microsoft-enum-forward-reference
 DEFINE CLANGPDB_ALL_CC_FLAGS         = DEF(GCC48_ALL_CC_FLAGS) DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -funsigned-char -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library-redeclaration -Wno-null-dereference -mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib -nostdlibinc -fseh-exceptions
 
 ###########################
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112259): https://edk2.groups.io/g/devel/message/112259
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 12/10/23 11:18, Mike Beaton wrote:
> From: Mike Beaton <mjsbeaton@gmail.com>
> 
> This warning was already disabled in CLANGDWARF by commit
> d3225577123767fd09c91201d27e9c91663ae132.
> 
> gcc can distinguish between optimised-away variable usage (as  can occur in
> valid debug code) and genuinely unused variables, and only complains about
> the latter. clang cannot, and therefore this warning ends up complaining
> about valid debug code under clang.
> 
> Since EDK-II code is in general going to be compiled by gcc as well as clang
> then disabling this warning in clang does not amount to entirely removing
> potentially valid warnings about genuinely unused variables.
> 
> Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>
> ---
>  BaseTools/Conf/tools_def.template | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index c34ecfd557..48cf45245f 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -1754,7 +1754,7 @@ DEFINE CLANGPDB_X64_PREFIX           = ENV(CLANG_BIN)
>  DEFINE CLANGPDB_IA32_TARGET          = -target i686-unknown-windows-gnu
>  DEFINE CLANGPDB_X64_TARGET           = -target x86_64-unknown-windows-gnu
>  
> -DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare -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-microsoft-enum-forward-reference
> +DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare -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 -Wno-microsoft-enum-forward-reference
>  DEFINE CLANGPDB_ALL_CC_FLAGS         = DEF(GCC48_ALL_CC_FLAGS) DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -funsigned-char -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library-redeclaration -Wno-null-dereference -mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib -nostdlibinc -fseh-exceptions
>  
>  ###########################

AFAICT, CLANGPDB_WARNING_OVERRIDES gets included in
CLANGPDB_ALL_CC_FLAGS, which in turn gets included in all three of
DEBUG, RELEASE and NOOPT build target flags.

The original report was "RELEASE CLANGPDB OVMF currently does not compile".

Can we use "-Wno-unneeded-internal-declaration" with RELEASE builds only?

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112304): https://edk2.groups.io/g/devel/message/112304
Mute This Topic: https://groups.io/mt/103087794/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 this would be logically wrong, as the other versions still
wouldn't compile if you changed the relevant debug Pcds. (Which are
logically independent of the compile and link options - e.g. what if for
some reason you wanted to single step with the Debug Pcds set to disabled,
in a NOOPT build?)




On Mon, 11 Dec 2023, 15:00 Laszlo Ersek, <lersek@redhat.com> wrote:

> On 12/10/23 11:18, Mike Beaton wrote:
> > From: Mike Beaton <mjsbeaton@gmail.com>
> >
> > This warning was already disabled in CLANGDWARF by commit
> > d3225577123767fd09c91201d27e9c91663ae132.
> >
> > gcc can distinguish between optimised-away variable usage (as  can occur
> in
> > valid debug code) and genuinely unused variables, and only complains
> about
> > the latter. clang cannot, and therefore this warning ends up complaining
> > about valid debug code under clang.
> >
> > Since EDK-II code is in general going to be compiled by gcc as well as
> clang
> > then disabling this warning in clang does not amount to entirely removing
> > potentially valid warnings about genuinely unused variables.
> >
> > Signed-off-by: Mike Beaton <mjsbeaton@gmail.com>
> > ---
> >  BaseTools/Conf/tools_def.template | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/BaseTools/Conf/tools_def.template
> b/BaseTools/Conf/tools_def.template
> > index c34ecfd557..48cf45245f 100755
> > --- a/BaseTools/Conf/tools_def.template
> > +++ b/BaseTools/Conf/tools_def.template
> > @@ -1754,7 +1754,7 @@ DEFINE CLANGPDB_X64_PREFIX           =
> ENV(CLANG_BIN)
> >  DEFINE CLANGPDB_IA32_TARGET          = -target i686-unknown-windows-gnu
> >  DEFINE CLANGPDB_X64_TARGET           = -target
> x86_64-unknown-windows-gnu
> >
> > -DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality
> -Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare
> -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-microsoft-enum-forward-reference
> > +DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality
> -Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare
> -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 -Wno-microsoft-enum-forward-reference
> >  DEFINE CLANGPDB_ALL_CC_FLAGS         = DEF(GCC48_ALL_CC_FLAGS)
> DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -funsigned-char
> -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
> -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
> -Wno-incompatible-library-redeclaration -Wno-null-dereference
> -mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib
> -nostdlibinc -fseh-exceptions
> >
> >  ###########################
>
> AFAICT, CLANGPDB_WARNING_OVERRIDES gets included in
> CLANGPDB_ALL_CC_FLAGS, which in turn gets included in all three of
> DEBUG, RELEASE and NOOPT build target flags.
>
> The original report was "RELEASE CLANGPDB OVMF currently does not compile".
>
> Can we use "-Wno-unneeded-internal-declaration" with RELEASE builds only?
>
> Thanks,
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112306): https://edk2.groups.io/g/devel/message/112306
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 12/11/23 16:18, Mike Beaton wrote:
> I believe this would be logically wrong, as the other versions still
> wouldn't compile if you changed the relevant debug Pcds. (Which are
> logically independent of the compile and link options - e.g. what if for
> some reason you wanted to single step with the Debug Pcds set to
> disabled, in a NOOPT build?)

I don't think that use case exists in practice.

Anyway, my suggestion is based on prior art: I *think* we ask gcc to
whine about unused local variables in RELEASE builds only, too. See
commits 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
only on RELEASE builds", 2016-03-25) and 8b6366f87584 ("BaseTools/GCC:
set -Wno-unused-const-variable on RELEASE builds", 2017-09-08).

... TBH I don't understand the current state of
"-Wno-unused-but-set-variables" and "-Wno-unused-const-variable" between
X64 and AARCH64, considering the DEBUG target. Today,
DEBUG_GCC5_AARCH64_CC_FLAGS disables these warnings, but
DEBUG_GCC5_X64_CC_FLAGS doesn't, even though *both* macros specify
-flto. Compare commit 06c8a34cc4bc ("BaseTool/tools_def GCC5: enable
optimization for ARM/AARCH64 DEBUG builds", 2017-12-08) -- I don't
understand why "-flto" had to be accompanied by
"-Wno-unused-but-set-variable -Wno-unused-const-variable" in that commit.

In brief, IA32 and X64 prior art supports my suggestion to shut up the
warning only for RELEASE (for CLANGPDB too), but ARM/AARCH64 prior art
contradicts that proposal. IOW, prior art is inconsistent per se... I
don't understand.

Laszlo

> On Mon, 11 Dec 2023, 15:00 Laszlo Ersek, <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> 
>     On 12/10/23 11:18, Mike Beaton wrote:
>     > From: Mike Beaton <mjsbeaton@gmail.com <mailto:mjsbeaton@gmail.com>>
>     >
>     > This warning was already disabled in CLANGDWARF by commit
>     > d3225577123767fd09c91201d27e9c91663ae132.
>     >
>     > gcc can distinguish between optimised-away variable usage (as  can
>     occur in
>     > valid debug code) and genuinely unused variables, and only
>     complains about
>     > the latter. clang cannot, and therefore this warning ends up
>     complaining
>     > about valid debug code under clang.
>     >
>     > Since EDK-II code is in general going to be compiled by gcc as
>     well as clang
>     > then disabling this warning in clang does not amount to entirely
>     removing
>     > potentially valid warnings about genuinely unused variables.
>     >
>     > Signed-off-by: Mike Beaton <mjsbeaton@gmail.com
>     <mailto:mjsbeaton@gmail.com>>
>     > ---
>     >  BaseTools/Conf/tools_def.template | 2 +-
>     >  1 file changed, 1 insertion(+), 1 deletion(-)
>     >
>     > diff --git a/BaseTools/Conf/tools_def.template
>     b/BaseTools/Conf/tools_def.template
>     > index c34ecfd557..48cf45245f 100755
>     > --- a/BaseTools/Conf/tools_def.template
>     > +++ b/BaseTools/Conf/tools_def.template
>     > @@ -1754,7 +1754,7 @@ DEFINE CLANGPDB_X64_PREFIX           =
>     ENV(CLANG_BIN)
>     >  DEFINE CLANGPDB_IA32_TARGET          = -target
>     i686-unknown-windows-gnu
>     >  DEFINE CLANGPDB_X64_TARGET           = -target
>     x86_64-unknown-windows-gnu
>     > 
>     > -DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality
>     -Wno-tautological-compare
>     -Wno-tautological-constant-out-of-range-compare -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-microsoft-enum-forward-reference
>     > +DEFINE CLANGPDB_WARNING_OVERRIDES    = -Wno-parentheses-equality
>     -Wno-tautological-compare
>     -Wno-tautological-constant-out-of-range-compare -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
>     -Wno-microsoft-enum-forward-reference
>     >  DEFINE CLANGPDB_ALL_CC_FLAGS         = DEF(GCC48_ALL_CC_FLAGS)
>     DEF(CLANGPDB_WARNING_OVERRIDES) -fno-stack-protector -funsigned-char
>     -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
>     -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas
>     -Wno-incompatible-library-redeclaration -Wno-null-dereference
>     -mno-implicit-float -mms-bitfields -mno-stack-arg-probe -nostdlib
>     -nostdlibinc -fseh-exceptions
>     > 
>     >  ###########################
> 
>     AFAICT, CLANGPDB_WARNING_OVERRIDES gets included in
>     CLANGPDB_ALL_CC_FLAGS, which in turn gets included in all three of
>     DEBUG, RELEASE and NOOPT build target flags.
> 
>     The original report was "RELEASE CLANGPDB OVMF currently does not
>     compile".
> 
>     Can we use "-Wno-unneeded-internal-declaration" with RELEASE builds
>     only?
> 
>     Thanks,
>     Laszlo
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112316): https://edk2.groups.io/g/devel/message/112316
Mute This Topic: https://groups.io/mt/103087794/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 this would be logically wrong, as the other versions still
> > wouldn't compile if you changed the relevant debug Pcds. (Which are
> > logically independent of the compile and link options - e.g. what if for
> > some reason you wanted to single step with the Debug Pcds set to
> > disabled, in a NOOPT build?)
>
> I don't think that use case exists in practice.
>
> Anyway, my suggestion is based on prior art: I *think* we ask gcc to
> whine about unused local variables in RELEASE builds only, too. See
> commits 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
> only on RELEASE builds", 2016-03-25) and 8b6366f87584 ("BaseTools/GCC:
> set -Wno-unused-const-variable on RELEASE builds", 2017-09-08).
>
> ... TBH I don't understand the current state of
> "-Wno-unused-but-set-variables" and "-Wno-unused-const-variable" between
> X64 and AARCH64, considering the DEBUG target. Today,
> DEBUG_GCC5_AARCH64_CC_FLAGS disables these warnings, but
> DEBUG_GCC5_X64_CC_FLAGS doesn't, even though *both* macros specify
> -flto. Compare commit 06c8a34cc4bc ("BaseTool/tools_def GCC5: enable
> optimization for ARM/AARCH64 DEBUG builds", 2017-12-08) -- I don't
> understand why "-flto" had to be accompanied by
> "-Wno-unused-but-set-variable -Wno-unused-const-variable" in that commit.
>
> In brief, IA32 and X64 prior art supports my suggestion to shut up the
> warning only for RELEASE (for CLANGPDB too), but ARM/AARCH64 prior art
> contradicts that proposal. IOW, prior art is inconsistent per se... I
> don't understand.
>
> Laszlo

Hunting around further, it is not the Pcds which are causing this to
be optimised away, but the definition of MDEPKG_NDEBUG.

A completely different approach, which allows clang to spot that the
usage has been 'optimised away' and so to not complain (and therefore
allows us to re-enable the warning in CLANGDWARF as well), is the
following:

--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -426,7 +426,10 @@ UnitTestDebugAssert (
       }                            \
     } while (FALSE)
 #else
-#define DEBUG(Expression)
+#define DEBUG(Expression)        \
+    if (FALSE) {                   \
+      _DEBUG (Expression);         \
+    }
 #endif

 /**


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112323): https://edk2.groups.io/g/devel/message/112323
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 12/11/23 18:26, Mike Beaton wrote:
>>> I believe this would be logically wrong, as the other versions still
>>> wouldn't compile if you changed the relevant debug Pcds. (Which are
>>> logically independent of the compile and link options - e.g. what if for
>>> some reason you wanted to single step with the Debug Pcds set to
>>> disabled, in a NOOPT build?)
>>
>> I don't think that use case exists in practice.
>>
>> Anyway, my suggestion is based on prior art: I *think* we ask gcc to
>> whine about unused local variables in RELEASE builds only, too. See
>> commits 20d00edf21d2 ("BaseTools/GCC: set -Wno-unused-but-set-variables
>> only on RELEASE builds", 2016-03-25) and 8b6366f87584 ("BaseTools/GCC:
>> set -Wno-unused-const-variable on RELEASE builds", 2017-09-08).
>>
>> ... TBH I don't understand the current state of
>> "-Wno-unused-but-set-variables" and "-Wno-unused-const-variable" between
>> X64 and AARCH64, considering the DEBUG target. Today,
>> DEBUG_GCC5_AARCH64_CC_FLAGS disables these warnings, but
>> DEBUG_GCC5_X64_CC_FLAGS doesn't, even though *both* macros specify
>> -flto. Compare commit 06c8a34cc4bc ("BaseTool/tools_def GCC5: enable
>> optimization for ARM/AARCH64 DEBUG builds", 2017-12-08) -- I don't
>> understand why "-flto" had to be accompanied by
>> "-Wno-unused-but-set-variable -Wno-unused-const-variable" in that commit.
>>
>> In brief, IA32 and X64 prior art supports my suggestion to shut up the
>> warning only for RELEASE (for CLANGPDB too), but ARM/AARCH64 prior art
>> contradicts that proposal. IOW, prior art is inconsistent per se... I
>> don't understand.
>>
>> Laszlo
> 
> Hunting around further, it is not the Pcds which are causing this to
> be optimised away, but the definition of MDEPKG_NDEBUG.
> 
> A completely different approach, which allows clang to spot that the
> usage has been 'optimised away' and so to not complain (and therefore
> allows us to re-enable the warning in CLANGDWARF as well), is the
> following:
> 
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -426,7 +426,10 @@ UnitTestDebugAssert (
>        }                            \
>      } while (FALSE)
>  #else
> -#define DEBUG(Expression)
> +#define DEBUG(Expression)        \
> +    if (FALSE) {                   \
> +      _DEBUG (Expression);         \
> +    }
>  #endif
> 
>  /**
> 

But will this not litter the object files with a bunch of format strings
etc?

It feels like, for CLANGPDB (and maybe CLANGDWARF), the RELEASE target
should not define MDEPKG_NDEBUG, but make sure (instead) that
DebugPrintEnabled() evals to FALSE. If PcdDebugPropertyMask is
fixed-at-build, then DebugPrintEnabled() should be possible to evaluate
at compile time; is that right? (At least for clang?)

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112349): https://edk2.groups.io/g/devel/message/112349
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> > A completely different approach, which allows clang to spot that the
> > usage has been 'optimised away' and so to not complain (and therefore
> > allows us to re-enable the warning in CLANGDWARF as well), is the
> > following:
> >
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -426,7 +426,10 @@ UnitTestDebugAssert (
> >        }                            \
> >      } while (FALSE)
> >  #else
> > -#define DEBUG(Expression)
> > +#define DEBUG(Expression)        \
> > +    if (FALSE) {                   \
> > +      _DEBUG (Expression);         \
> > +    }
> >  #endif
> >
> >  /**
> >
>
> But will this not litter the object files with a bunch of format strings
> etc?

Yes. Which would be optimized away at link time. (Or rather, I believe
so, would need further tests to confirm exactly what is optimized away
when.)

> It feels like, for CLANGPDB (and maybe CLANGDWARF), the RELEASE target
> should not define MDEPKG_NDEBUG, but make sure (instead) that
> DebugPrintEnabled() evals to FALSE. If PcdDebugPropertyMask is
> fixed-at-build, then DebugPrintEnabled() should be possible to evaluate
> at compile time; is that right? (At least for clang?)

Yes, that is my understanding of how compile-time Pcds work too - but
wouldn't the net result be the same as what I suggested?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112358): https://edk2.groups.io/g/devel/message/112358
Mute This Topic: https://groups.io/mt/103087794/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 08:17, Mike Beaton <mjsbeaton@gmail.com> wrote:
>
> > > A completely different approach, which allows clang to spot that the
> > > usage has been 'optimised away' and so to not complain (and therefore
> > > allows us to re-enable the warning in CLANGDWARF as well), is the
> > > following:
> > >
> > > --- a/MdePkg/Include/Library/DebugLib.h
> > > +++ b/MdePkg/Include/Library/DebugLib.h
> > > @@ -426,7 +426,10 @@ UnitTestDebugAssert (
> > >        }                            \
> > >      } while (FALSE)
> > >  #else
> > > -#define DEBUG(Expression)
> > > +#define DEBUG(Expression)        \
> > > +    if (FALSE) {                   \
> > > +      _DEBUG (Expression);         \
> > > +    }
> > >  #endif
> > >
> > >  /**
> > >
> >
> > But will this not litter the object files with a bunch of format strings
> > etc?
>
> Yes. Which would be optimized away at link time. (Or rather, I believe
> so, would need further tests to confirm exactly what is optimized away
> when.)
>

Link time optimization does not usually optimize away assets at this
granularity. Even if --gc-sections is set, the only thing it can
optimize away is an entire input section.

However, the compiler should be smart enough not to emit any
references to format strings etc in the first place, as it knows the
condition can never become true (but NOOPT builds should retain them).

> > It feels like, for CLANGPDB (and maybe CLANGDWARF), the RELEASE target
> > should not define MDEPKG_NDEBUG, but make sure (instead) that
> > DebugPrintEnabled() evals to FALSE. If PcdDebugPropertyMask is
> > fixed-at-build, then DebugPrintEnabled() should be possible to evaluate
> > at compile time; is that right? (At least for clang?)
>
> Yes, that is my understanding of how compile-time Pcds work too - but
> wouldn't the net result be the same as what I suggested?

It depends on the kind of access. For PCDs that are fixed-at-build
only, the FixedPcdGet###() accessors will evaluate to constant
preprocessor expressions, allowing the compiler to do optimizations.
The ordinary PcdGet###() routines will not produce compile time
constant expressions in the same way, so there, all the logic is
retained (again, modulo LTO)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112359): https://edk2.groups.io/g/devel/message/112359
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

I have realised that this was already fixed (i.e. allowing keeping the
warning) in Acidanthera fork of EDK-II. Discussed here
https://bugzilla.tianocore.org/show_bug.cgi?id=3704 - includes the fix
in question and other fixes for newer gcc as well. I'll post a new
patch to the list proposing just the relevant fix for clang.

On Tue, 12 Dec 2023 at 07:49, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 12 Dec 2023 at 08:17, Mike Beaton <mjsbeaton@gmail.com> wrote:
> >
> > > > A completely different approach, which allows clang to spot that the
> > > > usage has been 'optimised away' and so to not complain (and therefore
> > > > allows us to re-enable the warning in CLANGDWARF as well), is the
> > > > following:
> > > >
> > > > --- a/MdePkg/Include/Library/DebugLib.h
> > > > +++ b/MdePkg/Include/Library/DebugLib.h
> > > > @@ -426,7 +426,10 @@ UnitTestDebugAssert (
> > > >        }                            \
> > > >      } while (FALSE)
> > > >  #else
> > > > -#define DEBUG(Expression)
> > > > +#define DEBUG(Expression)        \
> > > > +    if (FALSE) {                   \
> > > > +      _DEBUG (Expression);         \
> > > > +    }
> > > >  #endif
> > > >
> > > >  /**
> > > >
> > >
> > > But will this not litter the object files with a bunch of format strings
> > > etc?
> >
> > Yes. Which would be optimized away at link time. (Or rather, I believe
> > so, would need further tests to confirm exactly what is optimized away
> > when.)
> >
>
> Link time optimization does not usually optimize away assets at this
> granularity. Even if --gc-sections is set, the only thing it can
> optimize away is an entire input section.
>
> However, the compiler should be smart enough not to emit any
> references to format strings etc in the first place, as it knows the
> condition can never become true (but NOOPT builds should retain them).
>
> > > It feels like, for CLANGPDB (and maybe CLANGDWARF), the RELEASE target
> > > should not define MDEPKG_NDEBUG, but make sure (instead) that
> > > DebugPrintEnabled() evals to FALSE. If PcdDebugPropertyMask is
> > > fixed-at-build, then DebugPrintEnabled() should be possible to evaluate
> > > at compile time; is that right? (At least for clang?)
> >
> > Yes, that is my understanding of how compile-time Pcds work too - but
> > wouldn't the net result be the same as what I suggested?
>
> It depends on the kind of access. For PCDs that are fixed-at-build
> only, the FixedPcdGet###() accessors will evaluate to constant
> preprocessor expressions, allowing the compiler to do optimizations.
> The ordinary PcdGet###() routines will not produce compile time
> constant expressions in the same way, so there, all the logic is
> retained (again, modulo LTO)


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112362): https://edk2.groups.io/g/devel/message/112362
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> I have realised that this was already fixed (i.e. allowing keeping the
> warning) in Acidanthera fork of EDK-II. Discussed here
> https://bugzilla.tianocore.org/show_bug.cgi?id=3704 - includes the fix
> in question and other fixes for newer gcc as well. I'll post a new
> patch to the list proposing just the relevant fix for clang.

I'm discussing this with the Acidanthera authors, and coming round to
prefer the version proposed above, with no pragmas, actually.

I think Ard is saying that, while I misdescribed somewhat when things
would be optimised away, that they will be optimised away, and that
probably the object files wouldn't be polluted, with this version?

I'm running some further tests to confirm that the simple version
proposed above builds in all the Acidanthera test environments, which
is relevant in that it should rule out at least any obvious problems
with it building in all edk-2 environments, too. (I had already run
quick tests for CLANGDWARF, CLANGPDB and GCC RELEASE in edk2, with no
problems.) If there aren't any issues there, I'd prefer to submit a
patch for this simple version instead (combined with removing the
-Wno-unneeded-internal-declaration for clangdwarf).


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112385): https://edk2.groups.io/g/devel/message/112385
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> I'm running some further tests to confirm that the simple version
> proposed above builds in all the Acidanthera test environments, which
> is relevant in that it should rule out at least any obvious problems
> with it building in all edk-2 environments, too. (I had already run
> quick tests for CLANGDWARF, CLANGPDB and GCC RELEASE in edk2, with no
> problems.) If there aren't any issues there, I'd prefer to submit a
> patch for this simple version instead (combined with removing the
> -Wno-unneeded-internal-declaration for clangdwarf).

Apologies for the running commentary, from these tests it appears that
the VS2019 toolchain does not like the 'simple' version of the fix. So
I'm back to proposing the version in
https://edk2.groups.io/g/devel/topic/103126777 .


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112386): https://edk2.groups.io/g/devel/message/112386
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Repeats the commit already acked by Ard in
https://edk2.groups.io/g/devel/topic/103083030, though with an attempt
to provide an additional (not yet acked) useful commit message.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112261): https://edk2.groups.io/g/devel/message/112261
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Resolves https://bugzilla.tianocore.org/show_bug.cgi?id=4620


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112262): https://edk2.groups.io/g/devel/message/112262
Mute This Topic: https://groups.io/mt/103087794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-