| Message ID | 1436361858-4345-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 8 July 2015 at 16:59, Olivier Martin <Olivier.Martin@arm.com> wrote: > For ARM architectures, I only disable this flag for RELEASE build: > OK, I hadn't noticed that. This means my patch would be only for the convenience of the Intel engineers (and my peace of mind as operator of a Jenkins job that gets broken all the time :-)) since the other use case I described is already covered by these. So please disregard my patch then. > $ grep -r unused-but-set-variable Conf/tools_def.txt > DEFINE GCC46_IA32_CC_FLAGS = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable > DEFINE GCC46_X64_CC_FLAGS = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable > RELEASE_GCC46_ARM_CC_FLAGS = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-variable > RELEASE_GCC47_ARM_CC_FLAGS = DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-variable > RELEASE_GCC47_AARCH64_CC_FLAGS = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable > RELEASE_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-variable > RELEASE_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable > RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-variable > RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable > > > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: 08 July 2015 14:24 > To: edk2-devel@lists.sourceforge.net; lersek@redhat.com; Olivier Martin; leif.lindholm@linaro.org; jordan.l.justen@intel.com > Cc: roy.franz@linaro.org; Ard Biesheuvel > Subject: [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. > > 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. > > 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.biesheuvel@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 > > > -- 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/
On 9 July 2015 at 01:42, Jordan Justen <jordan.l.justen@intel.com> wrote: > On 2015-07-08 08:58:04, Laszlo Ersek wrote: >> On 07/08/15 17:02, Ard Biesheuvel wrote: >> > On 8 July 2015 at 16:59, Olivier Martin <Olivier.Martin@arm.com> wrote: >> >> For ARM architectures, I only disable this flag for RELEASE build: >> > >> > OK, I hadn't noticed that. This means my patch would be only for the >> > convenience of the Intel engineers (and my peace of mind as operator >> > of a Jenkins job that gets broken all the time :-)) since the other >> > use case I described is already covered by these. >> > >> > So please disregard my patch then. >> >> I think your idea is good. For RELEASE builds, the valid pattern you >> described should be kept working, but for DEBUG and NOOPT builds, the >> warning should be emitted and it should also break the build. It's >> regrettable that it's always us having to report / clean up after the >> Intel developers working with MSVC, but ultimately it leads to a tidier >> code base. > > What is the use case? Someone has a regular build pool that *only* > does RELEASE builds? > > I think nearly all automated build systems will also do DEBUG builds, > and thus will still see the build error. > > What about a lower bar for committing build break fixes? What if we > said that compiler warning fixes could be committed by any package > maintainer for any package as long as it is an obvious trivial fix and > it has at least one r-b? > Yes, I think that sounds reasonable, and I would prefer it over relaxing the GCC error policies. Thanks, Ard. > I think qemu has a 'trivial' patch process. I can't remember the > details, but it may involve just Cc'ing the list with a different > name. Like: > > Cc: edk2-trivial <edk2-devel@lists.sourceforge.net> > > Anyway, I don't really support this build flag change, but I suppose > it could be acceptable for RELEASE builds. > > -Jordan > >> (Perhaps NOOPT should be handled similarly to RELEASE, and not DEBUG. In >> that case I should update the patch I attached to my other email. We >> don't really use NOOPT with gcc at all, so I included it in my patch >> only for completeness.) >> >> Thanks >> Laszlo >> >> > >> > >> >> $ grep -r unused-but-set-variable Conf/tools_def.txt >> >> DEFINE GCC46_IA32_CC_FLAGS = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable >> >> DEFINE GCC46_X64_CC_FLAGS = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable >> >> RELEASE_GCC46_ARM_CC_FLAGS = DEF(GCC46_ARM_CC_FLAGS) -Wno-unused-but-set-variable >> >> RELEASE_GCC47_ARM_CC_FLAGS = DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-variable >> >> RELEASE_GCC47_AARCH64_CC_FLAGS = DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable >> >> RELEASE_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-variable >> >> RELEASE_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable >> >> RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-variable >> >> RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable >> >> >> >> >> >> -----Original Message----- >> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> >> Sent: 08 July 2015 14:24 >> >> To: edk2-devel@lists.sourceforge.net; lersek@redhat.com; Olivier Martin; leif.lindholm@linaro.org; jordan.l.justen@intel.com >> >> Cc: roy.franz@linaro.org; Ard Biesheuvel >> >> Subject: [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. >> >> >> >> 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. >> >> >> >> 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.biesheuvel@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 >> >> >> >> >> >> -- 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/
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
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. 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. 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.biesheuvel@linaro.org> --- BaseTools/Conf/tools_def.template | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)