Thank you in advance for your comprehensive example, I understood it now. I just haven't considered that the macros need to ensure var-changing instructions may only be ran once. :)
Regards, Marvin. > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Monday, May 30, 2016 8:54 PM > To: Marvin Häuser <[email protected]> > Cc: [email protected] <[email protected]> > Subject: Re: [edk2] [PATCH v1 1/1] MdePkg/DebugLib: Ignore DEBUG > variables in RELEASE builds. > > 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
