On 07/08/15 20:15, Bill Paul wrote: > 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:
>> 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(). Good point! I guess I've always used ASSERT() in the sense of PANIC_UNLESS(). > [...] 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? Honestly, I don't know. I only concern myself with the case when even DEBUG_VERBOSE is enabled (PcdDebugPrintErrorLevel == 0x8040004F). The case when I have to look at debug messages and analyze behavior is the *norm*, not the exception (even if OVMF is not at fault -- the user may have passed wrong QEMU options, for example). It's an incredible chore when a user reports behavior he doesn't understand or expect, and I first have to ask him to reproduce on a DEBUG_VERBOSE build, and that he send me *that* log. Usually he has no access to such a build, cannot build one himself, so I get to build it for him. Blech. (I think Gerd's packages do enable DEBUG_VERBOSE, but I'm not sure.) > 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.) SeaBIOS employs the following trick to avoid this: all CONFIG_XXXX flags are checked in regular if() statements, not in #if macros. That is, any elmination happens during compilation / optimization / linking, not in preprocessing. This allows the compiler to catch such errors even when a CONFIG_XXXX option is predominantly false (or predominantly true). >> 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. My lack of faith is based on "experience" :) We've been complaining about this for ages on the list. I think it is safe to assume that the primary "participant" that has legal access to all supported Microsoft toolchains is Intel. As described (and provided!) by Scott, gcc-for-Windows is almost trivially available (all supported gcc versions). It seems to follow that Intel should operate such a build farm. Based on how long we've been whining about this, I don't think it's going to happen any time soon. > 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. Optimally, we shouldn't conflate the different (deeper) reasons this warning gets emitted for. Unfortunately, it's not practical even for me to build OVMF in all possible configurations: - DEBUG and RELEASE (2) - gcc 4.4 to 4.9 (6) - Ia32, X64, Ia32X64 for OVMF (3) That's 36 configs (and yes, the data flow analysis is specific to both target architecture and gcc version, so some warnings would only be found in configs that I can't test). Add ARM and AARCH64 for ArmVirtPkg (QEMU and Xen separately)... Secure Boot enabled vs. disabled... ARM BDS vs. Intel BDS... For OVMF, CSM enabled vs. disabled; IPv6 enabled vs. disabled... It's really unmanageable without a build farm. Yes, someone could say "hey Laszlo why don't you create a number of virtual machines, with different gcc versions installed, and implement at least the 'free software' build farm?" -- Because I don't have time for it. (And that's the reason I can't really blame Intel people either, for not setting up the MSVC+gcc build farm. Someone would have to take a *personal* hit.) For now we can say that we'll fix up such errors when we find out about them (and thus we don't patch the warning flags now). Annoying, but it does lead to a better code base. Thanks Laszlo > > -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 > ------------------------------------------------------------------------------ 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
