About the build infrastructure.
An option would be to investigate solution like travis-ci 
(https://travis-ci.com/). The little I know about travis-ci is it is free for 
open-source project, it can follow your github project and report if it builds 
or not (as soon as your build does not take more than 10-15min). When you send 
github 'push-request' it can also say if you break the build.
The build script is part of your repository (*.travis.yml), so everyone can 
contribute to it.


-----Original Message-----
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: 08 July 2015 20:02
To: Bill Paul
Cc: edk2-devel@lists.sourceforge.net; Scott Duplichan; Olivier Martin; 
leif.lindh...@linaro.org; jordan.l.jus...@intel.com
Subject: Re: [edk2] [PATCH] BaseTools/GCC: allow unused but set variables

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
>


-- IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium.  Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered 
in England & Wales, Company No:  2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, 
Registered in England & Wales, Company No:  2548782


------------------------------------------------------------------------------
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