Of all the gin joints in all the towns in all the world, Laszlo Ersek had to 
walk into mine at 09:17:08 on Wednesday 08 July 2015 and say:

> On 07/08/15 17:28, Scott Duplichan wrote:
> > Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] wrote:
> > 
> > ]Sent: Wednesday, July 08, 2015 08:24 AM
> > ]To: edk2-devel@lists.sourceforge.net; ler...@redhat.com;
> > olivier.mar...@arm.com; leif.lindh...@linaro.org;
> > ]jordan.l.jus...@intel.com ]Subject: [edk2] [PATCH] BaseTools/GCC: allow
> > unused but set variables ]
> > ]This fixes a recurring problem where patches that have only been
> > ]tested on MS toolchains break the build on GCC because they use
> > ]variables that are only written but never read.
> > 
> > An alternative point of view is that anyone who tests with MS tool chains
> > should also test with gcc tool chains. Windows hosted gcc tool chains of
> > all supported versions are free, small and easy to download and use:
> > http://sourceforge.net/projects/edk2developertoolsforwindows/
> 
> I agree 100%, but experience shows that MSVC users cannot be bothered.

Microsoft uses the Windows Logo program to force developers to run their 
driver code through the driver verifier (and pass) before they're allowed to 
slap the Windows Logo on their product. This was partly because MSVC users 
"couldn't be bothered" to rigorously validate their code, and Microsoft was 
the one taking the heat when a bug in a vendor-supplied driver would lead to 
Windows blue-screening. Maybe something similar culd be done here. :)
 
> > ]However, there is a valid pattern where this may happen as well.
> > ]For instance,
> > ]
> > ]  Status = SomeFunc (&OutVar);
> > ]  ASSERT_EFI_ERROR (Status);
> > ]  if (Outvar == ... ) { ... }
> > ]  // Status never referenced again
> > ]
> > ]may never access Status again at all in RELEASE builds, since the
> > ]ASSERT_EFI_ERROR () macro evaluates to nothing in that case.
> > 
> > To me this gcc warning is a good thing because it is pointing out
> > a coding problem. The coding problem is that error handling is
> > completely missing from the release build. Once the coding problem
> > is fixed, the warning goes away. After years of debate on the role
> > of ASSERT in EDK2 code, there is now a sort of official opinion in
> > 
> > the EDK2 Coding document:
> >     ASSERT macros are development and debugging aids and
> >     shall never be used for error handling. Assertions are
> >     used to catch conditions caused by programming errors
> >     that are resolved prior to product release.
> >     
> >     The EDK II PCD, PcdDebugPropertyMask, can be used to
> >     enable or disable the generation of code associated with
> >     ASSERT usage. Thus, all code must be able to operate, and
> >     recover in a reasonable maner [sic] with ASSERTs disabled.
> 
> In the above pattern, ASSERT_EFI_ERROR() is not necessarily used for
> error handling.

You are absolutely right: it's not. It's being used as a panic(). The problem 
is that panic() is usually a function, not a macro, and it isn't compiled out 
of the system. But ASSERT_EFI_ERROR() can be. If you build with the cross-
compiler toolchain (UNIXGCC) or you do a RELEASE build, then OVMF does this:

  GCC:*_UNIXGCC_*_CC_FLAGS             = -DMDEPKG_NDEBUG
  GCC:RELEASE_*_*_CC_FLAGS             = -DMDEPKG_NDEBUG

When you define MDEPKG_NDEBUG, ASSERT_EFI_ERROR(), ASSERT() and DEBUG() are 
all no-ops.

> It is perfectly possible that the programmer knows that,
> at that point, SomeFunc() *must* succeed, given the circumstances. One
> way to express that is to write a comment before the function call, and
> then ignore the return value completely. Another way is to add
> ASSERT_EFI_ERROR().

I'm sorry, but no. That is not "another way." If ASSERT_EFI_ERROR() were 
always present (i.e. even if you do -DMDEPKG_NDEBUG, it's still there), _THEN_ 
you could make this argument. But it's not.

> If an invariant turns out to be false (fully unexpectedly), there's no
> way to "recover"; the system state has been corrupted, and further
> behavior is undefined.

Yes. This is why you panic().
 
> So yes, ASSERT_EFI_ERROR() can be mis-used for error handling, but it is
> not necessarily the only use case. For a given SomeFunc() -- and
> situation --, the only reasonable "error handling" could be:
> 
>   //
>   // This shall succeed here.
>   //
>   Status = SomeFunc (&OutVar);
>   ASSERT_EFI_ERROR (Status);
> 
>   if (EFI_ERROR (Status)) {
>     //
>     // this should have never happened; system state is corrupt and we
>     // must not continue
>     //
>     CpuDeadLoop();
>   }
> 
> But I guess this just files under "debate on the role of ASSERT in EDK2
> code" :)

No. You can file it under "anti-pattern." Because that's what it is.

I also suspect that it would violate MISRA C rules, which in general preach 
that code should be unambiguous to the reader and never provoke any undefined 
behavior or side-effects. In the case where ASSERT_EFI_ERROR() is the only 
thing referencing a local variable, there is a side-effect: in the absence of 
GCC's warning, you're depending on the compiler to optimize away the variable 
and the code that references it in the case where you do a RELEASE build.

> > If you never plan on using a release build, which I think might
> > be the case with OVMF, the RELEASE option could be removed from
> > the DSC file and -Wno-error=unused-but-set-variable could be
> > added through the DSC file for that project only.
> 
> Well, *I* don't plan to use anything but DEBUG builds of OVMF (because
> ASSERT()s should never be compiled out, due to the above argument), but
> others might want to.

If it was never intended that ASSERT() et. al. to be compiled out, what was 
the point of making them conditional (and having a RELEASE build target where 
they're turned off) in the first place?

> Anyway, if, according to the refreshed coding style, ASSERT() must not
> be used any longer for stating (and enforcing) invariants, then I guess
> I'll just stop using ASSERT() altogether, and begin using _ASSERT()
> instead, with an explicit check around it.

Even if the coding standard originally required you to use ASSERT() to test 
invariants, I don't think it mandated that it be done in such a way that you'd 
end up with the code pattern we're talking about here.
 
> Namely, _ASSERT() is never compiled out. It is also never disabled by
> PcdDebugPropertyMask. Yet, it does adhere to PcdDebugPropertyMask with
> the *action* taken. So, henceforth I should be writing
> 
>   //
>   // This shall succeed here.
>   //
>   Status = SomeFunc (&OutVar);
> 
>   if (EFI_ERROR (Status)) {
>     //
>     // this should have never happened; system state is corrupt and we
>     // must not continue
>     //
>     _ASSERT (FALSE);
>   }
> 
> Thanks for helping me make up my mind :)
>

That's great, but take heed: ASSERT() and ASSERT_EFI_ERROR() are not the only 
macros being abused here. There is also DEBUG(). You will find several 
instances of the following construction:

    int SomeInterestingValue;

    /* Populate SomeInterestingValue */

    SomeInterestingValue = GetSomeInterestingStatus (handle);

    /* SomeInterestingValue is never referenced again */

    DEBUG(EFI_D_INFO, "INTERESTING VALUE IS: %d\n", SomeInterestingValue);

DEBUG() is also disabled when MDEPKG_NDEBUG is enabled, which creates the same 
problem. What would be the correct way to handle this case?

It should be noted that once in a while I run into this sort of thing in 
VxWorks as well, only it's often the other way around. We typically have 
DEBUG()-style messages turned off by default, and sometimes there are debug 
messages that refer to variables which no longer exist in the code. (The 
variables were removed but the debug messages were never updated to match.)

> > Just my opinion anyway. We all know from the old Microsoft tool chains
> > how annoying unwanted warnings are.
> 
> Right. I do agree with you that the optimal solution would be a build
> farm where all developers could test-build against all supported
> compilers. I doubt it's ever going to happen.

Given that UEFI firmware is supposed to be the standard for a large number of 
commercially available computer platforms, I find your lack of faith... 
disturbing.

Lastly, yesterday I actually spent some time updating the mingw-gcc-build.py 
to support binutils 2.25 and GCC 4.9.3, and I ran into this exact issue. I had 
to add -Wno-unused-but-set-variable to the UNIXGCC instances of CC_FLAGS in 
tools_def.template in order to get the OVMF build to work. But along the way, 
I noticed about a dozen instances of this anti-pattern. There are at least as 
many cases that stem from DEBUG() rather than ASSERT() or ASSERT_EFI_ERROR().

However I also found at least one legitimate error case of a useless "set but 
never used" variable too (i.e. a case that didn't involve conditional 
compilation effects). This means that now that GCC has been muzzled to 
disguise intentional abuses of this pattern, some unintentional abuses are now 
being hidden too.

-Bill

> Thanks
> Laszlo
> 
> > Thanks,
> > Scott
> > 
> > 
> > ]So let's allow this pattern, by passing the appropriate GCC command
> > ]line option.
> > ]
> > ]Contributed-under: TianoCore Contribution Agreement 1.0
> > ]Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > ]---
> > ] 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 7edd7590956b..15d8db04232f
> > 100644
> > ]--- a/BaseTools/Conf/tools_def.template
> > ]+++ b/BaseTools/Conf/tools_def.template
> > ]@@ -3813,7 +3813,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS    = /NOLOGO
> > /NODEFAULTLIB /LTCG /DLL /OPT:REF ] DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG     =
> > --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug ]
> > RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG   =
> > ]
> > ]-DEFINE GCC_ALL_CC_FLAGS            = -g -Os -fshort-wchar
> > -fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include
> > AutoGen.h ]+DEFINE GCC_ALL_CC_FLAGS            = -g -Os -fshort-wchar
> > -fno-strict-aliasing -Wall -Werror -Wno-array-]bounds -c -include
> > AutoGen.h -Wno-error=unused-but-set-variable ] DEFINE GCC_IA32_CC_FLAGS 
> >          = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks
> > -freorder-]blocks-and-partition -O2 -mno-stack-arg-probe ] DEFINE
> > GCC_X64_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone
> > -Wno-address -mno-stack-arg-probe ] DEFINE GCC_IPF_CC_FLAGS            =
> > DEF(GCC_ALL_CC_FLAGS) -minline-int-divide-min-latency ]--
> > ]1.9.1
> 
> ---------------------------------------------------------------------------
> --- Don't Limit Your Business. Reach for the Cloud.
> GigeNET's Cloud Solutions provide you with the tools and support that
> you need to offload your IT needs and focus on growing your business.
> Configured For All Businesses. Start Your Cloud Today.
> https://www.gigenetcloud.com/
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Member of Technical Staff,
                 wp...@windriver.com | Master of Unix-Fu - Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Reply via email to