On 05/30/16 20:13, Marvin Häuser wrote:
> Hey Laszlo,
>
> Thanks for your comment. To be honest, I was not aware of any side
> effects when casting to VOID - that's what I have been used to in the
> past and also saw it in several sources, including Microsoft (yes,
> not a good example for good src :) ).
No, that's not what I meant. It is fine to cast an expression that you
already have in place to VOID. For example, if you have the
(statement-)expression
fprintf(stdout, "hello world\n");
and you expressly don't care about the return value (an "int", for
printf()), you can write
(void)fprintf(stdout, "hello world\n");
But that's not *all* your patch does. Instead, your patch adds brand new
instances (evaluations) of the debug expression that have been there before.
For example:
>>> @@ -288,6 +288,8 @@ DebugPrintLevelEnabled (
>>> #if !defined(MDEPKG_NDEBUG)
>>> #define ASSERT(Expression) \
>>> do { \
>>> + (VOID)(Expression); \
>>> + \
>>> if (DebugAssertEnabled ()) { \
>>> if (!(Expression)) { \
>>> _ASSERT (Expression); \
>>> @@ -295,7 +297,7 @@ DebugPrintLevelEnabled (
>>> } \
>>> } while (FALSE)
>>> #else
>>> - #define ASSERT(Expression)
>>> + #define ASSERT(Expression) (VOID)(Expression)
>>> #endif
First, some background:
* On a toolchain that supports link time optimization (here: elimination
of dead code at linking), the MDEPKG_NDEBUG macro is not supposed to be
defined. Instead, the DebugAssertEnabled() function is called. That
function is usually based on PcdDebugPropertyMask. If the latter PCD is
a fixed PCD, then the condition can be evaluated at build time, and if
it is false, then an LTO-capable toolchain can remove the entire code
block dependent on the condition. This is what the first branch of the
outermost #if is for.
* The second branch is for toolchains that don't support LTO. For them,
the only way to remove code that is dead, due to building for RELEASE
(as opposed to DEBUG), is to #define MDEPKG_NDEBUG in the RELEASE build
flags. In this case, the compiler doesn't even see the code (it doesn't
survive into linking) because the preprocessor removes it.
But, *either way*, the end result is, if you build for RELEASE, the
Expression passed to ASSERT() is not evaluated. It doesn't matter if
it's the DebugAssertEnabled() function call or the MDEPKG_NDEBUG macro
that prevents the evaluation, the point is, the evaluation doesn't occur.
And that's what your patch breaks. Consider the following example:
STATIC UINT32 CheckCounter;
BOOLEAN
CheckIfStuffIsOkay (
VOID
)
{
BOOLEAN Okay;
Print (L"Checking (%u)...\n", CheckCounter++);
//
// Okay = ...
//
return Okay;
}
...
TYPE
SomeOtherFunction (
...
)
{
...
ASSERT (CheckIfStuffIsOkay ());
...
}
Without your patch:
- in a RELEASE build, the CheckCounter variable will not be incremented,
and the string will not appear on the console
- in a DEBUG build, the CheckCounter variable will be incremented once,
and the string will appear once, on the console.
With your patch:
- In a RELEASE build with a toolchain that supports LTO (i.e., one that
doesn't define MDEPKG_NDEBUG), the variable will be incremented, and the
message will be printed.
- In a DEBUG build with the same toolchain, the increment and the
corresponding Print() will be executed twice.
In short, your patch changes behavior for when Expression, passed to
ASSERT(), has side effects, and the toolchain supports LTO (i.e.,
MDEPKG_NDEBUG is never defined).
It's a separate question whether it makes sense for any programmer to
pass any such expression (i.e., one with side effects) to ASSERT(), but
that's for the caller to decide, not the callee. As far as I can see,
the ASSERT() specification in "MdePkg/Include/Library/DebugLib.h" does
not require Expression to be free of side effects.
> But as the issue is obviously
> known already, which I didn't know either, I will just wait it out.
> Though, if only DEBUG builds are checked for unused variables, might
> be an issue for those who only build RELEASE.
The idea is, people who build only RELEASE are likely end-users (or,
perhaps more precisely, not edk2 developers). It is not helpful to annoy
them with such warnings. However, edk2 developers are definitely
expected to build DEBUG as well, which is how they can root out issues
such as set-but-unused variables, and fix them.
Thanks,
Laszlo
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:[email protected]]
>> Sent: Monday, May 30, 2016 6:03 PM
>> To: Marvin Häuser <[email protected]>; edk2-
>> [email protected] <[email protected]>
>> Cc: [email protected]; [email protected]; Ard Biesheuvel
>> <[email protected]>
>> Subject: Re: [edk2] [PATCH v1 1/1] MdePkg/DebugLib: Ignore DEBUG
>> variables in RELEASE builds.
>>
>> On 05/30/16 15:22, Marvin Häuser wrote:
>>> When the only point of use of a variable is a DEBUG() or ASSERT()
>>> expression, most compilers will issue a warning when targeting
>>> RELEASE. Cast the parameters to VOID to silence these warnings.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Marvin Haeuser <[email protected]>
>>> ---
>>> MdePkg/Include/Library/DebugLib.h | 19 ++++++++++++++-----
>>> 1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> This seems incorrect to me. The patch doesn't only cast existing expressions
>> to VOID, it evaluates expressions for one more time. If the expressions have
>> side effects (which may or may not be a smart thing, but it depends on the
>> caller of DEBUG()), this patch will cause problems.
>>
>> Instead, the compiler flags should be changed for RELEASE builds not to warn
>> about such variables. Please see the following messages:
>>
>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/9184/focus=9454
>> http://thread.gmane.org/gmane.comp.bios.edk2.devel/9702/focus=9735
>>
>> In brief, we converted the tree to be "clean" with regard to gcc's "-Wunused-
>> but-set-variable", but only for DEBUG builds.
>>
>> Thanks
>> Laszlo
>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h
>>> b/MdePkg/Include/Library/DebugLib.h
>>> index 93b6f8df34ae..25c8ebb95740 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -270,7 +270,7 @@ DebugPrintLevelEnabled (
>>> } while (FALSE)
>>> #define _DEBUG(Expression) _DEBUG_PRINT Expression
>>> #else
>>> -#define _DEBUG(Expression) DebugPrint Expression
>>> + #define _DEBUG(Expression) DebugPrint Expression
>>> #endif
>>>
>>> /**
>>> @@ -288,6 +288,8 @@ DebugPrintLevelEnabled (
>>> #if !defined(MDEPKG_NDEBUG)
>>> #define ASSERT(Expression) \
>>> do { \
>>> + (VOID)(Expression); \
>>> + \
>>> if (DebugAssertEnabled ()) { \
>>> if (!(Expression)) { \
>>> _ASSERT (Expression); \
>>> @@ -295,7 +297,7 @@ DebugPrintLevelEnabled (
>>> } \
>>> } while (FALSE)
>>> #else
>>> - #define ASSERT(Expression)
>>> + #define ASSERT(Expression) (VOID)(Expression)
>>> #endif
>>>
>>> /**
>>> @@ -312,13 +314,15 @@ DebugPrintLevelEnabled ( **/
>>> #if !defined(MDEPKG_NDEBUG)
>>> #define DEBUG(Expression) \
>>> + (VOID)(Expression); \
>>> + \
>>> do { \
>>> if (DebugPrintEnabled ()) { \
>>> _DEBUG (Expression); \
>>> } \
>>> } while (FALSE)
>>> #else
>>> - #define DEBUG(Expression)
>>> + #define DEBUG(Expression) (VOID)(Expression)
>>> #endif
>>>
>>> /**
>>> @@ -336,6 +340,8 @@ DebugPrintLevelEnabled ( #if
>>> !defined(MDEPKG_NDEBUG)
>>> #define ASSERT_EFI_ERROR(StatusParameter)
>>> \
>>> do {
>>> \
>>> + (VOID)(StatusParameter);
>>> \
>>> +
>>> + \
>>> if (DebugAssertEnabled ()) {
>>> \
>>> if (EFI_ERROR (StatusParameter)) {
>>> \
>>> DEBUG ((EFI_D_ERROR, "\nASSERT_EFI_ERROR (Status = %r)\n",
>>> StatusParameter)); \ @@ -344,7 +350,7 @@ DebugPrintLevelEnabled (
>>> }
>>> \
>>> } while (FALSE)
>>> #else
>>> - #define ASSERT_EFI_ERROR(StatusParameter)
>>> + #define ASSERT_EFI_ERROR(StatusParameter) (VOID)(StatusParameter)
>>> #endif
>>>
>>> /**
>>> @@ -372,6 +378,9 @@ DebugPrintLevelEnabled ( #if
>>> !defined(MDEPKG_NDEBUG)
>>> #define ASSERT_PROTOCOL_ALREADY_INSTALLED(Handle, Guid)
>> \
>>> do {
>>> \
>>> + (VOID)(Handle);
>>> \
>>> + (VOID)(Guid);
>>> \
>>> +
>>> + \
>>> if (DebugAssertEnabled ()) {
>>> \
>>> VOID *Instance;
>>> \
>>> ASSERT (Guid != NULL);
>>> \
>>> @@ -387,7 +396,7 @@ DebugPrintLevelEnabled (
>>> }
>>> \
>>> } while (FALSE)
>>> #else
>>> - #define ASSERT_PROTOCOL_ALREADY_INSTALLED(Handle, Guid)
>>> + #define ASSERT_PROTOCOL_ALREADY_INSTALLED(Handle, Guid)
>>> + (VOID)(Handle)
>>> #endif
>>>
>>> /**
>>>
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
