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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
