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.
>
>
> ]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. 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().
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.
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" :)
> 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.
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.
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 :)
>
> 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.
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
