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
