* [PATCH V2 0/2] Add gcc flag for void* pointer arithmetics @ 2020-07-07 8:35 PierreGondois 2020-07-07 8:35 ` [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic PierreGondois 2020-07-07 8:35 ` [PATCH V2 2/2] BaseTools: Factorize GCC flags PierreGondois 0 siblings, 2 replies; 31+ messages in thread From: PierreGondois @ 2020-07-07 8:35 UTC (permalink / raw) To: devel; +Cc: Pierre Gondois, bob.c.feng, liming.gao, tomas.pilar, nd Add a flag preventing void* pointer arithmetics for ARM and AARCH64 with the gcc toolchain. The patch set also makes GCC48_ALL_CC_FLAGS dependent on GCC_ALL_CC_FLAGS. The changes can be seen a: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 Pierre Gondois (2): BaseTools: Add gcc flag to warn on void* pointer arithmetic BaseTools: Factorize GCC flags BaseTools/Conf/tools_def.template | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-07 8:35 [PATCH V2 0/2] Add gcc flag for void* pointer arithmetics PierreGondois @ 2020-07-07 8:35 ` PierreGondois 2020-07-16 9:07 ` [edk2-devel] " Yuwei Chen 2020-07-20 4:10 ` Bob Feng 2020-07-07 8:35 ` [PATCH V2 2/2] BaseTools: Factorize GCC flags PierreGondois 1 sibling, 2 replies; 31+ messages in thread From: PierreGondois @ 2020-07-07 8:35 UTC (permalink / raw) To: devel; +Cc: Pierre Gondois, bob.c.feng, liming.gao, tomas.pilar, nd From: Pierre Gondois <pierre.gondois@arm.com> By default, gcc allows void* pointer arithmetic. This is a GCC extension. However: - the C reference manual states that void* pointer "cannot be operands of addition or subtraction operators". Cf s5.3.1 "Generic Pointers"; - Visual studio compiler treat such operation as an error. To prevent such pointer arithmetic, the "-Wpointer-arith" flag should be set for all GCC versions. The "-Wpointer-arith" allows to: "Warn about anything that depends on the "size of" a function type or of void. GNU C assigns these types a size of 1, for convenience in calculations with void * pointers and pointers to functions." This flag is available since GCC2.95.3 which came out in 2001. Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> --- The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 Notes: v1: - Add "-Wpointer-arith" gcc flag. [Pierre] v2: - Only add the flag for ARM and AARCH64. [Tomas] BaseTools/Conf/tools_def.template | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template index 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f8641ac8bb95d5a2197 100755 --- a/BaseTools/Conf/tools_def.template +++ b/BaseTools/Conf/tools_def.template @@ -1,7 +1,7 @@ # # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-07 8:35 ` [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic PierreGondois @ 2020-07-16 9:07 ` Yuwei Chen 2020-07-20 4:10 ` Bob Feng 1 sibling, 0 replies; 31+ messages in thread From: Yuwei Chen @ 2020-07-16 9:07 UTC (permalink / raw) To: devel@edk2.groups.io, pierre.gondois@arm.com Cc: Feng, Bob C, Gao, Liming, tomas.pilar@arm.com, nd@arm.com Reviewed-by: Yuwei Chen<yuwei.chen@intel.com> > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > PierreGondois > Sent: Tuesday, July 7, 2020 4:35 PM > To: devel@edk2.groups.io > Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C > <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; > tomas.pilar@arm.com; nd@arm.com > Subject: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on > void* pointer arithmetic > > From: Pierre Gondois <pierre.gondois@arm.com> > > By default, gcc allows void* pointer arithmetic. > This is a GCC extension. > However: > - the C reference manual states that void* > pointer "cannot be operands of addition > or subtraction operators". Cf s5.3.1 > "Generic Pointers"; > - Visual studio compiler treat such operation as > an error. > > To prevent such pointer arithmetic, the "-Wpointer-arith" > flag should be set for all GCC versions. > > The "-Wpointer-arith" allows to: > "Warn about anything that depends on the "size of" > a function type or of void. GNU C assigns these > types a size of 1, for convenience in calculations > with void * pointers and pointers to functions." > > This flag is available since GCC2.95.3 which came out in 2001. > > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > --- > > The changes can be seen at: > https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v > 2 > > Notes: > v1: > - Add "-Wpointer-arith" gcc flag. [Pierre] > v2: > - Only add the flag for ARM and AARCH64. [Tomas] > > BaseTools/Conf/tools_def.template | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/BaseTools/Conf/tools_def.template > b/BaseTools/Conf/tools_def.template > index > 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f864 > 1ac8bb95d5a2197 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -1,7 +1,7 @@ > # > # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> # > Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> -# > Portions copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR> > +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights > +reserved.<BR> > # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # > (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # > Copyright (c) Microsoft Corporation > @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add- > gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N > DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno- > strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno- > common > 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle- > endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections - > fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft > -fno-pic -fno-pie > +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer- > arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char - > ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address - > mthumb -mfloat-abi=soft -fno-pic -fno-pie > DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access > -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle- > endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections > -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno- > unwind-tables -fno-pic -fno-pie -ffixed-x18 > +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) - > Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned- > char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous- > unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only > DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie > DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,-- > script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > \x01«¢êlŠ‚âžK\x18¢êÞqè¯y©e™ë,j > ¬±éí¶‹aŠÈ+¢êU‰ì?EêeÈéåŠwºÛ]½†Ûi³ÿÞvM ®‹©²*?ƒ÷^½é\x7f™ë,j > ¿ëmvôËy8b±:)‰Èm¶›?þ > 躛"£ùÿ¾wç^wçþ9ã»Øj躓°êÝz÷¥úŒ'z·“h+¢êlŠ…'²æìr¸›z^[m¦ÏÿyÙ6‚º.¦È¨þ\x0f > Ýz÷¥þéì¹¼®Á蜅éâž×¥r‰ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-07 8:35 ` [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic PierreGondois 2020-07-16 9:07 ` [edk2-devel] " Yuwei Chen @ 2020-07-20 4:10 ` Bob Feng 2020-07-22 18:05 ` [edk2-devel] " Leif Lindholm 1 sibling, 1 reply; 31+ messages in thread From: Bob Feng @ 2020-07-20 4:10 UTC (permalink / raw) To: PierreGondois, devel@edk2.groups.io Cc: Gao, Liming, tomas.pilar@arm.com, nd@arm.com Reviewed-by: Bob Feng<bob.c.feng@intel.com> -----Original Message----- From: PierreGondois <pierre.gondois@arm.com> Sent: Tuesday, July 7, 2020 4:35 PM To: devel@edk2.groups.io Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; tomas.pilar@arm.com; nd@arm.com Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic From: Pierre Gondois <pierre.gondois@arm.com> By default, gcc allows void* pointer arithmetic. This is a GCC extension. However: - the C reference manual states that void* pointer "cannot be operands of addition or subtraction operators". Cf s5.3.1 "Generic Pointers"; - Visual studio compiler treat such operation as an error. To prevent such pointer arithmetic, the "-Wpointer-arith" flag should be set for all GCC versions. The "-Wpointer-arith" allows to: "Warn about anything that depends on the "size of" a function type or of void. GNU C assigns these types a size of 1, for convenience in calculations with void * pointers and pointers to functions." This flag is available since GCC2.95.3 which came out in 2001. Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> --- The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 Notes: v1: - Add "-Wpointer-arith" gcc flag. [Pierre] v2: - Only add the flag for ARM and AARCH64. [Tomas] BaseTools/Conf/tools_def.template | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template index 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f8641ac8bb95d5a2197 100755 --- a/BaseTools/Conf/tools_def.template +++ b/BaseTools/Conf/tools_def.template @@ -1,7 +1,7 @@ # # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights +reserved.<BR> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-20 4:10 ` Bob Feng @ 2020-07-22 18:05 ` Leif Lindholm 2020-07-22 21:13 ` Andrew Fish 2020-07-23 1:56 ` Bob Feng 0 siblings, 2 replies; 31+ messages in thread From: Leif Lindholm @ 2020-07-22 18:05 UTC (permalink / raw) To: devel, bob.c.feng; +Cc: PierreGondois, Gao, Liming, tomas Hi Bob, This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it? Regards, Leif On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: > Reviewed-by: Bob Feng<bob.c.feng@intel.com> > > > -----Original Message----- > From: PierreGondois <pierre.gondois@arm.com> > Sent: Tuesday, July 7, 2020 4:35 PM > To: devel@edk2.groups.io > Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; tomas.pilar@arm.com; nd@arm.com > Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic > > From: Pierre Gondois <pierre.gondois@arm.com> > > By default, gcc allows void* pointer arithmetic. > This is a GCC extension. > However: > - the C reference manual states that void* > pointer "cannot be operands of addition > or subtraction operators". Cf s5.3.1 > "Generic Pointers"; > - Visual studio compiler treat such operation as > an error. > > To prevent such pointer arithmetic, the "-Wpointer-arith" > flag should be set for all GCC versions. > > The "-Wpointer-arith" allows to: > "Warn about anything that depends on the "size of" > a function type or of void. GNU C assigns these > types a size of 1, for convenience in calculations > with void * pointers and pointers to functions." > > This flag is available since GCC2.95.3 which came out in 2001. > > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > --- > > The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 > > Notes: > v1: > - Add "-Wpointer-arith" gcc flag. [Pierre] > v2: > - Only add the flag for ARM and AARCH64. [Tomas] > > BaseTools/Conf/tools_def.template | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template > index 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f8641ac8bb95d5a2197 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -1,7 +1,7 @@ > # > # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR> > +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights > +reserved.<BR> > # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation > @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N > DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common > 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access > -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only > DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie > DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-22 18:05 ` [edk2-devel] " Leif Lindholm @ 2020-07-22 21:13 ` Andrew Fish 2020-07-23 1:56 ` Bob Feng 1 sibling, 0 replies; 31+ messages in thread From: Andrew Fish @ 2020-07-22 21:13 UTC (permalink / raw) To: edk2-devel-groups-io, leif; +Cc: bob.c.feng, PierreGondois, Gao, Liming, tomas [-- Attachment #1: Type: text/plain, Size: 5216 bytes --] Leif, I also noticed we would need this flag for clang/Xcode too. ~/work/Compiler>clang -S void.c ~/work/Compiler>clang -S void.c -Wpointer-arith void.c:4:14: warning: arithmetic on a pointer to void is a GNU extension [-Wpointer-arith] return Arg + 1; ~~~ ^ 1 warning generated. ~/work/Compiler>cat void.c void * Test (void *Arg) { return Arg + 1; } Thanks, Andrew Fish > On Jul 22, 2020, at 11:05 AM, Leif Lindholm <leif@nuviainc.com> wrote: > > Hi Bob, > > This patch also breaks about half of the ARM/AARCH64 platforms in > edk2-platforms. I agree it should go in at a later stage, but for now, > can we please revert it? > > Regards, > > Leif > > On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: >> Reviewed-by: Bob Feng<bob.c.feng@intel.com> >> >> >> -----Original Message----- >> From: PierreGondois <pierre.gondois@arm.com> >> Sent: Tuesday, July 7, 2020 4:35 PM >> To: devel@edk2.groups.io >> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; tomas.pilar@arm.com; nd@arm.com >> Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic >> >> From: Pierre Gondois <pierre.gondois@arm.com> >> >> By default, gcc allows void* pointer arithmetic. >> This is a GCC extension. >> However: >> - the C reference manual states that void* >> pointer "cannot be operands of addition >> or subtraction operators". Cf s5.3.1 >> "Generic Pointers"; >> - Visual studio compiler treat such operation as >> an error. >> >> To prevent such pointer arithmetic, the "-Wpointer-arith" >> flag should be set for all GCC versions. >> >> The "-Wpointer-arith" allows to: >> "Warn about anything that depends on the "size of" >> a function type or of void. GNU C assigns these >> types a size of 1, for convenience in calculations >> with void * pointers and pointers to functions." >> >> This flag is available since GCC2.95.3 which came out in 2001. >> >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >> --- >> >> The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 >> >> Notes: >> v1: >> - Add "-Wpointer-arith" gcc flag. [Pierre] >> v2: >> - Only add the flag for ARM and AARCH64. [Tomas] >> >> BaseTools/Conf/tools_def.template | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template >> index 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f8641ac8bb95d5a2197 100755 >> --- a/BaseTools/Conf/tools_def.template >> +++ b/BaseTools/Conf/tools_def.template >> @@ -1,7 +1,7 @@ >> # >> # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. All rights reserved.<BR> >> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights >> +reserved.<BR> >> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation >> @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N >> DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common >> 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie >> +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie >> DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access >> -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 >> +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 >> DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only >> DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie >> DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds >> -- >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >> >> >> >> > > [-- Attachment #2: Type: text/html, Size: 18751 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-22 18:05 ` [edk2-devel] " Leif Lindholm 2020-07-22 21:13 ` Andrew Fish @ 2020-07-23 1:56 ` Bob Feng 2020-07-23 2:49 ` Andrew Fish 1 sibling, 1 reply; 31+ messages in thread From: Bob Feng @ 2020-07-23 1:56 UTC (permalink / raw) To: devel@edk2.groups.io, leif@nuviainc.com Cc: PierreGondois, Gao, Liming, tomas@nuviainc.com Hi Leif I agree to revert that patch for now and I sent a revert patch for review. After resolving the build break issue for ARM/AARCH64 platforms in edk2-platforms, and make sure there is no platform build break with this patch, we will push it again. Thanks, Bob -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif Lindholm Sent: Thursday, July 23, 2020 2:06 AM To: devel@edk2.groups.io; Feng, Bob C <bob.c.feng@intel.com> Cc: PierreGondois <pierre.gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic Hi Bob, This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it? Regards, Leif On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: > Reviewed-by: Bob Feng<bob.c.feng@intel.com> > > > -----Original Message----- > From: PierreGondois <pierre.gondois@arm.com> > Sent: Tuesday, July 7, 2020 4:35 PM > To: devel@edk2.groups.io > Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C > <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; > tomas.pilar@arm.com; nd@arm.com > Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* > pointer arithmetic > > From: Pierre Gondois <pierre.gondois@arm.com> > > By default, gcc allows void* pointer arithmetic. > This is a GCC extension. > However: > - the C reference manual states that void* > pointer "cannot be operands of addition > or subtraction operators". Cf s5.3.1 > "Generic Pointers"; > - Visual studio compiler treat such operation as > an error. > > To prevent such pointer arithmetic, the "-Wpointer-arith" > flag should be set for all GCC versions. > > The "-Wpointer-arith" allows to: > "Warn about anything that depends on the "size of" > a function type or of void. GNU C assigns these > types a size of 1, for convenience in calculations > with void * pointers and pointers to functions." > > This flag is available since GCC2.95.3 which came out in 2001. > > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > --- > > The changes can be seen at: > https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 > > Notes: > v1: > - Add "-Wpointer-arith" gcc flag. [Pierre] > v2: > - Only add the flag for ARM and AARCH64. [Tomas] > > BaseTools/Conf/tools_def.template | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/BaseTools/Conf/tools_def.template > b/BaseTools/Conf/tools_def.template > index > 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f8641ac > 8bb95d5a2197 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -1,7 +1,7 @@ > # > # Copyright (c) 2006 - 2018, Intel Corporation. All rights > reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. All > rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. > All rights reserved.<BR> > +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights > +reserved.<BR> > # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation > @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N > DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common > 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access > -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only > DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie > DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-23 1:56 ` Bob Feng @ 2020-07-23 2:49 ` Andrew Fish 2020-07-23 9:33 ` Leif Lindholm 0 siblings, 1 reply; 31+ messages in thread From: Andrew Fish @ 2020-07-23 2:49 UTC (permalink / raw) To: edk2-devel-groups-io, bob.c.feng Cc: leif@nuviainc.com, PierreGondois, Gao, Liming, tomas@nuviainc.com [-- Attachment #1: Type: text/plain, Size: 5912 bytes --] Bob, It also looks like clang could use this flag as the default seems to be to follow the GCC behavior. Thanks, Andrew Fish > On Jul 22, 2020, at 6:56 PM, Bob Feng <bob.c.feng@intel.com> wrote: > > Hi Leif > > I agree to revert that patch for now and I sent a revert patch for review. After resolving the build break issue for ARM/AARCH64 platforms in edk2-platforms, and make sure there is no platform build break with this patch, we will push it again. > > Thanks, > Bob > > -----Original Message----- > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Leif Lindholm > Sent: Thursday, July 23, 2020 2:06 AM > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Feng, Bob C <bob.c.feng@intel.com <mailto:bob.c.feng@intel.com>> > Cc: PierreGondois <pierre.gondois@arm.com <mailto:pierre.gondois@arm.com>>; Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; tomas@nuviainc.com <mailto:tomas@nuviainc.com> > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic > > Hi Bob, > > This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it? > > Regards, > > Leif > > On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: >> Reviewed-by: Bob Feng<bob.c.feng@intel.com> >> >> >> -----Original Message----- >> From: PierreGondois <pierre.gondois@arm.com> >> Sent: Tuesday, July 7, 2020 4:35 PM >> To: devel@edk2.groups.io >> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C >> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; >> tomas.pilar@arm.com; nd@arm.com >> Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* >> pointer arithmetic >> >> From: Pierre Gondois <pierre.gondois@arm.com> >> >> By default, gcc allows void* pointer arithmetic. >> This is a GCC extension. >> However: >> - the C reference manual states that void* >> pointer "cannot be operands of addition >> or subtraction operators". Cf s5.3.1 >> "Generic Pointers"; >> - Visual studio compiler treat such operation as >> an error. >> >> To prevent such pointer arithmetic, the "-Wpointer-arith" >> flag should be set for all GCC versions. >> >> The "-Wpointer-arith" allows to: >> "Warn about anything that depends on the "size of" >> a function type or of void. GNU C assigns these >> types a size of 1, for convenience in calculations >> with void * pointers and pointers to functions." >> >> This flag is available since GCC2.95.3 which came out in 2001. >> >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >> --- >> >> The changes can be seen at: >> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 >> >> Notes: >> v1: >> - Add "-Wpointer-arith" gcc flag. [Pierre] >> v2: >> - Only add the flag for ARM and AARCH64. [Tomas] >> >> BaseTools/Conf/tools_def.template | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/BaseTools/Conf/tools_def.template >> b/BaseTools/Conf/tools_def.template >> index >> 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f8641ac >> 8bb95d5a2197 100755 >> --- a/BaseTools/Conf/tools_def.template >> +++ b/BaseTools/Conf/tools_def.template >> @@ -1,7 +1,7 @@ >> # >> # Copyright (c) 2006 - 2018, Intel Corporation. All rights >> reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. All >> rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. >> All rights reserved.<BR> >> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights >> +reserved.<BR> >> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation >> @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N >> DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common >> 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie >> +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie >> DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access >> -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 >> +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 >> DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only >> DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie >> DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds >> -- >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' >> >> >> >> > > > > > [-- Attachment #2: Type: text/html, Size: 29524 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-23 2:49 ` Andrew Fish @ 2020-07-23 9:33 ` Leif Lindholm 2020-07-24 3:56 ` Bob Feng 0 siblings, 1 reply; 31+ messages in thread From: Leif Lindholm @ 2020-07-23 9:33 UTC (permalink / raw) To: devel, afish; +Cc: bob.c.feng, PierreGondois, Gao, Liming, tomas@nuviainc.com Hi Andrew, Agreed. I also think this should be applied across all architectures, not just ARM/AARCH64. Since Visual Studio has never been been able to compile the affected code, I expect impact to Ia32/X64 to be minimal. Regards, Leif On Wed, Jul 22, 2020 at 19:49:01 -0700, Andrew Fish via groups.io wrote: > Bob, > > It also looks like clang could use this flag as the default seems to > be to follow the GCC behavior. > > Thanks, > > Andrew Fish > > > On Jul 22, 2020, at 6:56 PM, Bob Feng <bob.c.feng@intel.com> wrote: > > > > Hi Leif > > > > I agree to revert that patch for now and I sent a revert patch for review. After resolving the build break issue for ARM/AARCH64 platforms in edk2-platforms, and make sure there is no platform build break with this patch, we will push it again. > > > > Thanks, > > Bob > > > > -----Original Message----- > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Leif Lindholm > > Sent: Thursday, July 23, 2020 2:06 AM > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Feng, Bob C <bob.c.feng@intel.com <mailto:bob.c.feng@intel.com>> > > Cc: PierreGondois <pierre.gondois@arm.com <mailto:pierre.gondois@arm.com>>; Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; tomas@nuviainc.com <mailto:tomas@nuviainc.com> > > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic > > > > Hi Bob, > > > > This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it? > > > > Regards, > > > > Leif > > > > On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: > >> Reviewed-by: Bob Feng<bob.c.feng@intel.com> > >> > >> > >> -----Original Message----- > >> From: PierreGondois <pierre.gondois@arm.com> > >> Sent: Tuesday, July 7, 2020 4:35 PM > >> To: devel@edk2.groups.io > >> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C > >> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; > >> tomas.pilar@arm.com; nd@arm.com > >> Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* > >> pointer arithmetic > >> > >> From: Pierre Gondois <pierre.gondois@arm.com> > >> > >> By default, gcc allows void* pointer arithmetic. > >> This is a GCC extension. > >> However: > >> - the C reference manual states that void* > >> pointer "cannot be operands of addition > >> or subtraction operators". Cf s5.3.1 > >> "Generic Pointers"; > >> - Visual studio compiler treat such operation as > >> an error. > >> > >> To prevent such pointer arithmetic, the "-Wpointer-arith" > >> flag should be set for all GCC versions. > >> > >> The "-Wpointer-arith" allows to: > >> "Warn about anything that depends on the "size of" > >> a function type or of void. GNU C assigns these > >> types a size of 1, for convenience in calculations > >> with void * pointers and pointers to functions." > >> > >> This flag is available since GCC2.95.3 which came out in 2001. > >> > >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > >> --- > >> > >> The changes can be seen at: > >> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 > >> > >> Notes: > >> v1: > >> - Add "-Wpointer-arith" gcc flag. [Pierre] > >> v2: > >> - Only add the flag for ARM and AARCH64. [Tomas] > >> > >> BaseTools/Conf/tools_def.template | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/BaseTools/Conf/tools_def.template > >> b/BaseTools/Conf/tools_def.template > >> index > >> 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f8641ac > >> 8bb95d5a2197 100755 > >> --- a/BaseTools/Conf/tools_def.template > >> +++ b/BaseTools/Conf/tools_def.template > >> @@ -1,7 +1,7 @@ > >> # > >> # Copyright (c) 2006 - 2018, Intel Corporation. All rights > >> reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. All > >> rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. > >> All rights reserved.<BR> > >> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights > >> +reserved.<BR> > >> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation > >> @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N > >> DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common > >> 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > >> +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > >> DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access > >> -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > >> +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > >> DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only > >> DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie > >> DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds > >> -- > >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > >> > >> > >> > >> > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-23 9:33 ` Leif Lindholm @ 2020-07-24 3:56 ` Bob Feng 2020-07-24 9:01 ` PierreGondois 2020-07-24 11:03 ` Leif Lindholm 0 siblings, 2 replies; 31+ messages in thread From: Bob Feng @ 2020-07-24 3:56 UTC (permalink / raw) To: Leif Lindholm, devel@edk2.groups.io, afish@apple.com Cc: PierreGondois, Gao, Liming, tomas@nuviainc.com What would be your suggestion on the approach to apply this build option to all architectures? Change the tool_def.txt firstly and push platform owner to fix the corresponding firmware code, or changing the tool_def.txt later, until this change will not break any platform build? -----Original Message----- From: Leif Lindholm <leif@nuviainc.com> Sent: Thursday, July 23, 2020 5:33 PM To: devel@edk2.groups.io; afish@apple.com Cc: Feng, Bob C <bob.c.feng@intel.com>; PierreGondois <pierre.gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic Hi Andrew, Agreed. I also think this should be applied across all architectures, not just ARM/AARCH64. Since Visual Studio has never been been able to compile the affected code, I expect impact to Ia32/X64 to be minimal. Regards, Leif On Wed, Jul 22, 2020 at 19:49:01 -0700, Andrew Fish via groups.io wrote: > Bob, > > It also looks like clang could use this flag as the default seems to > be to follow the GCC behavior. > > Thanks, > > Andrew Fish > > > On Jul 22, 2020, at 6:56 PM, Bob Feng <bob.c.feng@intel.com> wrote: > > > > Hi Leif > > > > I agree to revert that patch for now and I sent a revert patch for review. After resolving the build break issue for ARM/AARCH64 platforms in edk2-platforms, and make sure there is no platform build break with this patch, we will push it again. > > > > Thanks, > > Bob > > > > -----Original Message----- > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of > > Leif Lindholm > > Sent: Thursday, July 23, 2020 2:06 AM > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Feng, Bob C > > <bob.c.feng@intel.com <mailto:bob.c.feng@intel.com>> > > Cc: PierreGondois <pierre.gondois@arm.com > > <mailto:pierre.gondois@arm.com>>; Gao, Liming <liming.gao@intel.com > > <mailto:liming.gao@intel.com>>; tomas@nuviainc.com > > <mailto:tomas@nuviainc.com> > > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to > > warn on void* pointer arithmetic > > > > Hi Bob, > > > > This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it? > > > > Regards, > > > > Leif > > > > On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: > >> Reviewed-by: Bob Feng<bob.c.feng@intel.com> > >> > >> > >> -----Original Message----- > >> From: PierreGondois <pierre.gondois@arm.com> > >> Sent: Tuesday, July 7, 2020 4:35 PM > >> To: devel@edk2.groups.io > >> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C > >> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; > >> tomas.pilar@arm.com; nd@arm.com > >> Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* > >> pointer arithmetic > >> > >> From: Pierre Gondois <pierre.gondois@arm.com> > >> > >> By default, gcc allows void* pointer arithmetic. > >> This is a GCC extension. > >> However: > >> - the C reference manual states that void* > >> pointer "cannot be operands of addition > >> or subtraction operators". Cf s5.3.1 > >> "Generic Pointers"; > >> - Visual studio compiler treat such operation as > >> an error. > >> > >> To prevent such pointer arithmetic, the "-Wpointer-arith" > >> flag should be set for all GCC versions. > >> > >> The "-Wpointer-arith" allows to: > >> "Warn about anything that depends on the "size of" > >> a function type or of void. GNU C assigns these types a size of > >> 1, for convenience in calculations with void * pointers and > >> pointers to functions." > >> > >> This flag is available since GCC2.95.3 which came out in 2001. > >> > >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > >> --- > >> > >> The changes can be seen at: > >> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_ > >> v2 > >> > >> Notes: > >> v1: > >> - Add "-Wpointer-arith" gcc flag. [Pierre] > >> v2: > >> - Only add the flag for ARM and AARCH64. [Tomas] > >> > >> BaseTools/Conf/tools_def.template | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/BaseTools/Conf/tools_def.template > >> b/BaseTools/Conf/tools_def.template > >> index > >> 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f864 > >> 1ac > >> 8bb95d5a2197 100755 > >> --- a/BaseTools/Conf/tools_def.template > >> +++ b/BaseTools/Conf/tools_def.template > >> @@ -1,7 +1,7 @@ > >> # > >> # Copyright (c) 2006 - 2018, Intel Corporation. All rights > >> reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. > >> All rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. > >> All rights reserved.<BR> > >> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights > >> +reserved.<BR> > >> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation > >> @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N > >> DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common > >> 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > >> +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > >> DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access > >> -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > >> +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > >> DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only > >> DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie > >> DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds > >> -- > >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > >> > >> > >> > >> > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-24 3:56 ` Bob Feng @ 2020-07-24 9:01 ` PierreGondois 2020-07-24 11:05 ` Leif Lindholm 2020-07-24 11:03 ` Leif Lindholm 1 sibling, 1 reply; 31+ messages in thread From: PierreGondois @ 2020-07-24 9:01 UTC (permalink / raw) To: devel@edk2.groups.io, bob.c.feng@intel.com, Leif Lindholm, afish@apple.com Cc: Gao, Liming, tomas@nuviainc.com Hello everyone, Thank you for reverting the patch. I should have tested building on more platforms. I can fix the pieces of code highlighted by the -Wpointer-arith flag on ARM platforms. For now I saw 10-15 places that would need to be modified. If this is not a huge task I will try to do it for other platforms. Regards, Pierre -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bob Feng via groups.io Sent: 24 July 2020 04:56 To: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io; afish@apple.com Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic What would be your suggestion on the approach to apply this build option to all architectures? Change the tool_def.txt firstly and push platform owner to fix the corresponding firmware code, or changing the tool_def.txt later, until this change will not break any platform build? -----Original Message----- From: Leif Lindholm <leif@nuviainc.com> Sent: Thursday, July 23, 2020 5:33 PM To: devel@edk2.groups.io; afish@apple.com Cc: Feng, Bob C <bob.c.feng@intel.com>; PierreGondois <pierre.gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic Hi Andrew, Agreed. I also think this should be applied across all architectures, not just ARM/AARCH64. Since Visual Studio has never been been able to compile the affected code, I expect impact to Ia32/X64 to be minimal. Regards, Leif On Wed, Jul 22, 2020 at 19:49:01 -0700, Andrew Fish via groups.io wrote: > Bob, > > It also looks like clang could use this flag as the default seems to > be to follow the GCC behavior. > > Thanks, > > Andrew Fish > > > On Jul 22, 2020, at 6:56 PM, Bob Feng <bob.c.feng@intel.com> wrote: > > > > Hi Leif > > > > I agree to revert that patch for now and I sent a revert patch for review. After resolving the build break issue for ARM/AARCH64 platforms in edk2-platforms, and make sure there is no platform build break with this patch, we will push it again. > > > > Thanks, > > Bob > > > > -----Original Message----- > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of > > Leif Lindholm > > Sent: Thursday, July 23, 2020 2:06 AM > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Feng, Bob C > > <bob.c.feng@intel.com <mailto:bob.c.feng@intel.com>> > > Cc: PierreGondois <pierre.gondois@arm.com > > <mailto:pierre.gondois@arm.com>>; Gao, Liming <liming.gao@intel.com > > <mailto:liming.gao@intel.com>>; tomas@nuviainc.com > > <mailto:tomas@nuviainc.com> > > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to > > warn on void* pointer arithmetic > > > > Hi Bob, > > > > This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it? > > > > Regards, > > > > Leif > > > > On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: > >> Reviewed-by: Bob Feng<bob.c.feng@intel.com> > >> > >> > >> -----Original Message----- > >> From: PierreGondois <pierre.gondois@arm.com> > >> Sent: Tuesday, July 7, 2020 4:35 PM > >> To: devel@edk2.groups.io > >> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C > >> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; > >> tomas.pilar@arm.com; nd@arm.com > >> Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* > >> pointer arithmetic > >> > >> From: Pierre Gondois <pierre.gondois@arm.com> > >> > >> By default, gcc allows void* pointer arithmetic. > >> This is a GCC extension. > >> However: > >> - the C reference manual states that void* > >> pointer "cannot be operands of addition > >> or subtraction operators". Cf s5.3.1 > >> "Generic Pointers"; > >> - Visual studio compiler treat such operation as > >> an error. > >> > >> To prevent such pointer arithmetic, the "-Wpointer-arith" > >> flag should be set for all GCC versions. > >> > >> The "-Wpointer-arith" allows to: > >> "Warn about anything that depends on the "size of" > >> a function type or of void. GNU C assigns these types a size of > >> 1, for convenience in calculations with void * pointers and > >> pointers to functions." > >> > >> This flag is available since GCC2.95.3 which came out in 2001. > >> > >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > >> --- > >> > >> The changes can be seen at: > >> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_ > >> v2 > >> > >> Notes: > >> v1: > >> - Add "-Wpointer-arith" gcc flag. [Pierre] > >> v2: > >> - Only add the flag for ARM and AARCH64. [Tomas] > >> > >> BaseTools/Conf/tools_def.template | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/BaseTools/Conf/tools_def.template > >> b/BaseTools/Conf/tools_def.template > >> index > >> 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f864 > >> 1ac > >> 8bb95d5a2197 100755 > >> --- a/BaseTools/Conf/tools_def.template > >> +++ b/BaseTools/Conf/tools_def.template > >> @@ -1,7 +1,7 @@ > >> # > >> # Copyright (c) 2006 - 2018, Intel Corporation. All rights > >> reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. > >> All rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. > >> All rights reserved.<BR> > >> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights > >> +reserved.<BR> > >> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation > >> @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N > >> DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common > >> 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > >> +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > >> DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access > >> -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > >> +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > >> DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only > >> DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie > >> DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds > >> -- > >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > >> > >> > >> > >> > > > > > > > > > > > > > > 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-24 9:01 ` PierreGondois @ 2020-07-24 11:05 ` Leif Lindholm 0 siblings, 0 replies; 31+ messages in thread From: Leif Lindholm @ 2020-07-24 11:05 UTC (permalink / raw) To: Pierre Gondois Cc: devel@edk2.groups.io, bob.c.feng@intel.com, afish@apple.com, Gao, Liming, tomas@nuviainc.com On Fri, Jul 24, 2020 at 09:01:16 +0000, Pierre Gondois wrote: > Hello everyone, > > Thank you for reverting the patch. I should have tested building on more platforms. > I can fix the pieces of code highlighted by the -Wpointer-arith flag > on ARM platforms. For now I saw 10-15 places that would need to be > modified. > If this is not a huge task I will try to do it for other platforms. That would certainly be most appreciated. If not, I will get to it eventually, but it might drag out a few weeks. Regards, Leif > Regards, > Pierre > > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bob Feng via groups.io > Sent: 24 July 2020 04:56 > To: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io; afish@apple.com > Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic > > What would be your suggestion on the approach to apply this build option to all architectures? Change the tool_def.txt firstly and push platform owner to fix the corresponding firmware code, or changing the tool_def.txt later, until this change will not break any platform build? > > -----Original Message----- > From: Leif Lindholm <leif@nuviainc.com> > Sent: Thursday, July 23, 2020 5:33 PM > To: devel@edk2.groups.io; afish@apple.com > Cc: Feng, Bob C <bob.c.feng@intel.com>; PierreGondois <pierre.gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic > > Hi Andrew, > > Agreed. > > I also think this should be applied across all architectures, not just ARM/AARCH64. Since Visual Studio has never been been able to compile the affected code, I expect impact to Ia32/X64 to be minimal. > > Regards, > > Leif > > On Wed, Jul 22, 2020 at 19:49:01 -0700, Andrew Fish via groups.io wrote: > > Bob, > > > > It also looks like clang could use this flag as the default seems to > > be to follow the GCC behavior. > > > > Thanks, > > > > Andrew Fish > > > > > On Jul 22, 2020, at 6:56 PM, Bob Feng <bob.c.feng@intel.com> wrote: > > > > > > Hi Leif > > > > > > I agree to revert that patch for now and I sent a revert patch for review. After resolving the build break issue for ARM/AARCH64 platforms in edk2-platforms, and make sure there is no platform build break with this patch, we will push it again. > > > > > > Thanks, > > > Bob > > > > > > -----Original Message----- > > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> > > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of > > > Leif Lindholm > > > Sent: Thursday, July 23, 2020 2:06 AM > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Feng, Bob C > > > <bob.c.feng@intel.com <mailto:bob.c.feng@intel.com>> > > > Cc: PierreGondois <pierre.gondois@arm.com > > > <mailto:pierre.gondois@arm.com>>; Gao, Liming <liming.gao@intel.com > > > <mailto:liming.gao@intel.com>>; tomas@nuviainc.com > > > <mailto:tomas@nuviainc.com> > > > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to > > > warn on void* pointer arithmetic > > > > > > Hi Bob, > > > > > > This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it? > > > > > > Regards, > > > > > > Leif > > > > > > On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: > > >> Reviewed-by: Bob Feng<bob.c.feng@intel.com> > > >> > > >> > > >> -----Original Message----- > > >> From: PierreGondois <pierre.gondois@arm.com> > > >> Sent: Tuesday, July 7, 2020 4:35 PM > > >> To: devel@edk2.groups.io > > >> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C > > >> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; > > >> tomas.pilar@arm.com; nd@arm.com > > >> Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* > > >> pointer arithmetic > > >> > > >> From: Pierre Gondois <pierre.gondois@arm.com> > > >> > > >> By default, gcc allows void* pointer arithmetic. > > >> This is a GCC extension. > > >> However: > > >> - the C reference manual states that void* > > >> pointer "cannot be operands of addition > > >> or subtraction operators". Cf s5.3.1 > > >> "Generic Pointers"; > > >> - Visual studio compiler treat such operation as > > >> an error. > > >> > > >> To prevent such pointer arithmetic, the "-Wpointer-arith" > > >> flag should be set for all GCC versions. > > >> > > >> The "-Wpointer-arith" allows to: > > >> "Warn about anything that depends on the "size of" > > >> a function type or of void. GNU C assigns these types a size of > > >> 1, for convenience in calculations with void * pointers and > > >> pointers to functions." > > >> > > >> This flag is available since GCC2.95.3 which came out in 2001. > > >> > > >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > > >> --- > > >> > > >> The changes can be seen at: > > >> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_ > > >> v2 > > >> > > >> Notes: > > >> v1: > > >> - Add "-Wpointer-arith" gcc flag. [Pierre] > > >> v2: > > >> - Only add the flag for ARM and AARCH64. [Tomas] > > >> > > >> BaseTools/Conf/tools_def.template | 6 +++--- > > >> 1 file changed, 3 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/BaseTools/Conf/tools_def.template > > >> b/BaseTools/Conf/tools_def.template > > >> index > > >> 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f864 > > >> 1ac > > >> 8bb95d5a2197 100755 > > >> --- a/BaseTools/Conf/tools_def.template > > >> +++ b/BaseTools/Conf/tools_def.template > > >> @@ -1,7 +1,7 @@ > > >> # > > >> # Copyright (c) 2006 - 2018, Intel Corporation. All rights > > >> reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. > > >> All rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. > > >> All rights reserved.<BR> > > >> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights > > >> +reserved.<BR> > > >> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation > > >> @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N > > >> DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common > > >> 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > > >> +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > > >> DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access > > >> -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > > >> +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > > >> DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only > > >> DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie > > >> DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds > > >> -- > > >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > >> > > >> > > >> > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic 2020-07-24 3:56 ` Bob Feng 2020-07-24 9:01 ` PierreGondois @ 2020-07-24 11:03 ` Leif Lindholm 1 sibling, 0 replies; 31+ messages in thread From: Leif Lindholm @ 2020-07-24 11:03 UTC (permalink / raw) To: Feng, Bob C Cc: devel@edk2.groups.io, afish@apple.com, PierreGondois, Gao, Liming, tomas@nuviainc.com Hi Bob, I would say we should start by sending out an RFC set updating tools_def.template for the various toolchains, and then make sure we fix things in other trees (I would also like to, for example, verify this doesn't break edk2-libc). Once other trees are cleaned up, we can send out a PATCH and merge to master. This is not critical for edk2-stable202008, but it is *useful*, so it would be good if we could hit that. / Leif On Fri, Jul 24, 2020 at 03:56:25 +0000, Feng, Bob C wrote: > What would be your suggestion on the approach to apply this build > option to all architectures? Change the tool_def.txt firstly and > push platform owner to fix the corresponding firmware code, or > changing the tool_def.txt later, until this change will not break > any platform build? > > -----Original Message----- > From: Leif Lindholm <leif@nuviainc.com> > Sent: Thursday, July 23, 2020 5:33 PM > To: devel@edk2.groups.io; afish@apple.com > Cc: Feng, Bob C <bob.c.feng@intel.com>; PierreGondois <pierre.gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic > > Hi Andrew, > > Agreed. > > I also think this should be applied across all architectures, not just ARM/AARCH64. Since Visual Studio has never been been able to compile the affected code, I expect impact to Ia32/X64 to be minimal. > > Regards, > > Leif > > On Wed, Jul 22, 2020 at 19:49:01 -0700, Andrew Fish via groups.io wrote: > > Bob, > > > > It also looks like clang could use this flag as the default seems to > > be to follow the GCC behavior. > > > > Thanks, > > > > Andrew Fish > > > > > On Jul 22, 2020, at 6:56 PM, Bob Feng <bob.c.feng@intel.com> wrote: > > > > > > Hi Leif > > > > > > I agree to revert that patch for now and I sent a revert patch for review. After resolving the build break issue for ARM/AARCH64 platforms in edk2-platforms, and make sure there is no platform build break with this patch, we will push it again. > > > > > > Thanks, > > > Bob > > > > > > -----Original Message----- > > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> > > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of > > > Leif Lindholm > > > Sent: Thursday, July 23, 2020 2:06 AM > > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Feng, Bob C > > > <bob.c.feng@intel.com <mailto:bob.c.feng@intel.com>> > > > Cc: PierreGondois <pierre.gondois@arm.com > > > <mailto:pierre.gondois@arm.com>>; Gao, Liming <liming.gao@intel.com > > > <mailto:liming.gao@intel.com>>; tomas@nuviainc.com > > > <mailto:tomas@nuviainc.com> > > > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to > > > warn on void* pointer arithmetic > > > > > > Hi Bob, > > > > > > This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it? > > > > > > Regards, > > > > > > Leif > > > > > > On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote: > > >> Reviewed-by: Bob Feng<bob.c.feng@intel.com> > > >> > > >> > > >> -----Original Message----- > > >> From: PierreGondois <pierre.gondois@arm.com> > > >> Sent: Tuesday, July 7, 2020 4:35 PM > > >> To: devel@edk2.groups.io > > >> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C > > >> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; > > >> tomas.pilar@arm.com; nd@arm.com > > >> Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* > > >> pointer arithmetic > > >> > > >> From: Pierre Gondois <pierre.gondois@arm.com> > > >> > > >> By default, gcc allows void* pointer arithmetic. > > >> This is a GCC extension. > > >> However: > > >> - the C reference manual states that void* > > >> pointer "cannot be operands of addition > > >> or subtraction operators". Cf s5.3.1 > > >> "Generic Pointers"; > > >> - Visual studio compiler treat such operation as > > >> an error. > > >> > > >> To prevent such pointer arithmetic, the "-Wpointer-arith" > > >> flag should be set for all GCC versions. > > >> > > >> The "-Wpointer-arith" allows to: > > >> "Warn about anything that depends on the "size of" > > >> a function type or of void. GNU C assigns these types a size of > > >> 1, for convenience in calculations with void * pointers and > > >> pointers to functions." > > >> > > >> This flag is available since GCC2.95.3 which came out in 2001. > > >> > > >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > > >> --- > > >> > > >> The changes can be seen at: > > >> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_ > > >> v2 > > >> > > >> Notes: > > >> v1: > > >> - Add "-Wpointer-arith" gcc flag. [Pierre] > > >> v2: > > >> - Only add the flag for ARM and AARCH64. [Tomas] > > >> > > >> BaseTools/Conf/tools_def.template | 6 +++--- > > >> 1 file changed, 3 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/BaseTools/Conf/tools_def.template > > >> b/BaseTools/Conf/tools_def.template > > >> index > > >> 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f864 > > >> 1ac > > >> 8bb95d5a2197 100755 > > >> --- a/BaseTools/Conf/tools_def.template > > >> +++ b/BaseTools/Conf/tools_def.template > > >> @@ -1,7 +1,7 @@ > > >> # > > >> # Copyright (c) 2006 - 2018, Intel Corporation. All rights > > >> reserved.<BR> # Portions copyright (c) 2008 - 2009, Apple Inc. > > >> All rights reserved.<BR> -# Portions copyright (c) 2011 - 2019, ARM Ltd. > > >> All rights reserved.<BR> > > >> +# Portions copyright (c) 2011 - 2020, ARM Ltd. All rights > > >> +reserved.<BR> > > >> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR> # (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR> # Copyright (c) Microsoft Corporation > > >> @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N > > >> DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common > > >> 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_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > > >> +DEFINE GCC_ARM_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie > > >> DEFINE GCC_ARM_CC_XIPFLAGS = -mno-unaligned-access > > >> -DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > > >> +DEFINE GCC_AARCH64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 > > >> DEFINE GCC_AARCH64_CC_XIPFLAGS = -mstrict-align -mgeneral-regs-only > > >> DEFINE GCC_DLINK_FLAGS_COMMON = -nostdlib --pie > > >> DEFINE GCC_DLINK2_FLAGS_COMMON = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds > > >> -- > > >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > >> > > >> > > >> > > >> > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-07-07 8:35 [PATCH V2 0/2] Add gcc flag for void* pointer arithmetics PierreGondois 2020-07-07 8:35 ` [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic PierreGondois @ 2020-07-07 8:35 ` PierreGondois 2020-07-20 4:11 ` Bob Feng 2020-07-22 11:03 ` Laszlo Ersek 1 sibling, 2 replies; 31+ messages in thread From: PierreGondois @ 2020-07-07 8:35 UTC (permalink / raw) To: devel; +Cc: Pierre Gondois, bob.c.feng, liming.gao, tomas.pilar, nd From: Pierre Gondois <pierre.gondois@arm.com> GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. By definition, there should be such dependency. The outcomes of this patch is that GCC48_ALL_CC_FLAGS and other dependent configurations will inherit from the additional "-Os" flag. The "-Os" flag optimizes a build in size, not breaking any build. In a gcc command line, the last optimization flag has precedence. This means that this "-Os" flag will be overriden by a more specific optimization configuration, provided that this more specific flag is appended at the end of the CC_FLAGS. Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> --- The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 Notes: v2: - Make GCC48_ALL_CC_FLAGS dependent on GCC_ALL_CC_FLAGS. [Tomas] 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 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7fbed49fe24267 100755 --- a/BaseTools/Conf/tools_def.template +++ b/BaseTools/Conf/tools_def.template @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv # GCC Build Flag for included header file list generation DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-07-07 8:35 ` [PATCH V2 2/2] BaseTools: Factorize GCC flags PierreGondois @ 2020-07-20 4:11 ` Bob Feng 2020-07-30 12:08 ` [edk2-devel] " Leif Lindholm 2020-07-22 11:03 ` Laszlo Ersek 1 sibling, 1 reply; 31+ messages in thread From: Bob Feng @ 2020-07-20 4:11 UTC (permalink / raw) To: PierreGondois, devel@edk2.groups.io Cc: Gao, Liming, tomas.pilar@arm.com, nd@arm.com Reviewed-by: Bob Feng<bob.c.feng@intel.com> -----Original Message----- From: PierreGondois <pierre.gondois@arm.com> Sent: Tuesday, July 7, 2020 4:35 PM To: devel@edk2.groups.io Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; tomas.pilar@arm.com; nd@arm.com Subject: [PATCH V2 2/2] BaseTools: Factorize GCC flags From: Pierre Gondois <pierre.gondois@arm.com> GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. By definition, there should be such dependency. The outcomes of this patch is that GCC48_ALL_CC_FLAGS and other dependent configurations will inherit from the additional "-Os" flag. The "-Os" flag optimizes a build in size, not breaking any build. In a gcc command line, the last optimization flag has precedence. This means that this "-Os" flag will be overriden by a more specific optimization configuration, provided that this more specific flag is appended at the end of the CC_FLAGS. Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> --- The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 Notes: v2: - Make GCC48_ALL_CC_FLAGS dependent on GCC_ALL_CC_FLAGS. [Tomas] 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 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7fbed49fe24267 100755 --- a/BaseTools/Conf/tools_def.template +++ b/BaseTools/Conf/tools_def.template @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv # GCC Build Flag for included header file list generation DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-07-20 4:11 ` Bob Feng @ 2020-07-30 12:08 ` Leif Lindholm 0 siblings, 0 replies; 31+ messages in thread From: Leif Lindholm @ 2020-07-30 12:08 UTC (permalink / raw) To: devel, bob.c.feng; +Cc: PierreGondois, Gao, Liming, tomas, Ard Biesheuvel And now I spotted this one. Why are we going back and changing build flags for toolchain profiles that are kept around for legacy use only? GCC 4.8 was released in 2013. While I agree it *semantically* makes sense for GCC*_CC_FLAGS to inherit GCC_ALL_CC_FLAGS, I am pretty sure the discrepancy was put there for a reason - to keep the legacy profile working as it had done even if we decided to enable new features for all modern profiles. (That we then stopped having to create new profiles for each new GCC release and hence haven't gone beyond GCC5 is beside the point - GCC5 encompasses all gcc toolchains since.) I'll let Ard comment (when he's back), but I'm thinking this one ought to be reverted as well. / Leif On Mon, Jul 20, 2020 at 04:11:53 +0000, Bob Feng wrote: > Reviewed-by: Bob Feng<bob.c.feng@intel.com> > > -----Original Message----- > From: PierreGondois <pierre.gondois@arm.com> > Sent: Tuesday, July 7, 2020 4:35 PM > To: devel@edk2.groups.io > Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>; tomas.pilar@arm.com; nd@arm.com > Subject: [PATCH V2 2/2] BaseTools: Factorize GCC flags > > From: Pierre Gondois <pierre.gondois@arm.com> > > GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. > By definition, there should be such dependency. > > The outcomes of this patch is that GCC48_ALL_CC_FLAGS and other dependent configurations will inherit from the additional "-Os" flag. > The "-Os" flag optimizes a build in size, not breaking any build. In a gcc command line, the last optimization flag has precedence. This means that this "-Os" flag will be overriden by a more specific optimization configuration, provided that this more specific flag is appended at the end of the CC_FLAGS. > > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> > --- > > The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 > > Notes: > v2: > - Make GCC48_ALL_CC_FLAGS dependent on > GCC_ALL_CC_FLAGS. [Tomas] > > 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 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7fbed49fe24267 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv > # GCC Build Flag for included header file list generation > DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps > > -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings > +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings > DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 > DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address > DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-07-07 8:35 ` [PATCH V2 2/2] BaseTools: Factorize GCC flags PierreGondois 2020-07-20 4:11 ` Bob Feng @ 2020-07-22 11:03 ` Laszlo Ersek 2020-07-22 11:24 ` Laszlo Ersek 2020-08-26 16:42 ` Laszlo Ersek 1 sibling, 2 replies; 31+ messages in thread From: Laszlo Ersek @ 2020-07-22 11:03 UTC (permalink / raw) To: pierre.gondois Cc: devel, bob.c.feng, liming.gao, tomas.pilar, nd, Leif Lindholm (Nuvia address), Ard Biesheuvel (ARM address) Hi Pierre, On 07/07/20 10:35, PierreGondois wrote: > From: Pierre Gondois <pierre.gondois@arm.com> > > GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. > By definition, there should be such dependency. > > The outcomes of this patch is that GCC48_ALL_CC_FLAGS and > other dependent configurations will inherit from the > additional "-Os" flag. > The "-Os" flag optimizes a build in size, not breaking any > build. In a gcc command line, the last optimization flag > has precedence. This means that this "-Os" flag will be > overriden by a more specific optimization configuration, > provided that this more specific flag is appended at the > end of the CC_FLAGS. > > Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> > --- > > The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 > > Notes: > v2: > - Make GCC48_ALL_CC_FLAGS dependent on > GCC_ALL_CC_FLAGS. [Tomas] > > 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 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7fbed49fe24267 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv > # GCC Build Flag for included header file list generation > DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps > > -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings > +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings > DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 > DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address > DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address > As the commit message states, this change makes GCC48_ALL_CC_FLAGS inherit "-Os". It is true that all the NOOPT_GCC flags override "-Os" with "-O0": NOOPT_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -O0 NOOPT_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -O0 NOOPT_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -O0 NOOPT_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -O0 NOOPT_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -O0 NOOPT_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -O0 NOOPT_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -O0 NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 NOOPT_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -O0 NOOPT_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -O0 NOOPT_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -O0 NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 However, *some* of the DEBUG and RELEASE flags now have two "-Os" flags: DEBUG_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os RELEASE_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable DEBUG_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os RELEASE_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable DEBUG_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os RELEASE_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable DEBUG_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os RELEASE_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable DEBUG_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable DEBUG_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable (The ARM and AARCH64 DEBUG/RELEASE GCC options don't seem to be affected, as they have relied on inherited -- not open-coded -- "-Os" options from much earlier. So now they do not suffer from this duplication.) The point of this patch was a kind of "normalization", so I think the work isn't complete until the duplication is undone, i.e., the explicit "-Os" flag is removed from the last twelve defines. Can you submit a follow-up patch please? Thanks Laszlo ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-07-22 11:03 ` Laszlo Ersek @ 2020-07-22 11:24 ` Laszlo Ersek 2020-08-26 16:42 ` Laszlo Ersek 1 sibling, 0 replies; 31+ messages in thread From: Laszlo Ersek @ 2020-07-22 11:24 UTC (permalink / raw) To: pierre.gondois Cc: devel, bob.c.feng, liming.gao, tomas.pilar, nd, Leif Lindholm (Nuvia address), Ard Biesheuvel (ARM address) On 07/22/20 13:03, Laszlo Ersek wrote: > Hi Pierre, > > On 07/07/20 10:35, PierreGondois wrote: >> From: Pierre Gondois <pierre.gondois@arm.com> >> >> GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. >> By definition, there should be such dependency. >> >> The outcomes of this patch is that GCC48_ALL_CC_FLAGS and >> other dependent configurations will inherit from the >> additional "-Os" flag. >> The "-Os" flag optimizes a build in size, not breaking any >> build. In a gcc command line, the last optimization flag >> has precedence. This means that this "-Os" flag will be >> overriden by a more specific optimization configuration, >> provided that this more specific flag is appended at the >> end of the CC_FLAGS. >> >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >> Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> >> --- >> >> The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 >> >> Notes: >> v2: >> - Make GCC48_ALL_CC_FLAGS dependent on >> GCC_ALL_CC_FLAGS. [Tomas] >> >> 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 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7fbed49fe24267 100755 >> --- a/BaseTools/Conf/tools_def.template >> +++ b/BaseTools/Conf/tools_def.template >> @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv >> # GCC Build Flag for included header file list generation >> DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps >> >> -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >> +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >> DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 >> DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address >> DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address >> > > As the commit message states, this change makes GCC48_ALL_CC_FLAGS inherit "-Os". > > It is true that all the NOOPT_GCC flags override "-Os" with "-O0": > > NOOPT_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -O0 > NOOPT_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -O0 > NOOPT_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -O0 > NOOPT_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -O0 > NOOPT_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -O0 > NOOPT_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -O0 > NOOPT_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -O0 > NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 > NOOPT_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -O0 > NOOPT_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -O0 > NOOPT_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -O0 > NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 > > However, *some* of the DEBUG and RELEASE flags now have two "-Os" flags: > > DEBUG_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os > RELEASE_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable > DEBUG_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os > RELEASE_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable > DEBUG_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os > RELEASE_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os > RELEASE_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os > RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os > RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > > (The ARM and AARCH64 DEBUG/RELEASE GCC options don't seem to be affected, as they have relied on inherited -- not open-coded -- "-Os" options from much earlier. So now they do not suffer from this duplication.) We do have some AARCH64 duplication here: DEFINE GCC49_AARCH64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small because GCC48_ALL_CC_FLAGS is now based on GCC_ALL_CC_FLAGS, and GCC_AARCH64_CC_FLAGS is *also* based on GCC_ALL_CC_FLAGS. However, this duplication is not introduced by this patch -- it goes back to commit 63e1c23b2255 ("BaseTools/AARCH64: use large code model for GCC <= 4.8", 2015-10-02). So it's a separate topic. Thanks Laszlo > > The point of this patch was a kind of "normalization", so I think the work isn't complete until the duplication is undone, i.e., the explicit "-Os" flag is removed from the last twelve defines. > > Can you submit a follow-up patch please? > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-07-22 11:03 ` Laszlo Ersek 2020-07-22 11:24 ` Laszlo Ersek @ 2020-08-26 16:42 ` Laszlo Ersek 2020-08-27 8:32 ` PierreGondois 1 sibling, 1 reply; 31+ messages in thread From: Laszlo Ersek @ 2020-08-26 16:42 UTC (permalink / raw) To: pierre.gondois Cc: devel, bob.c.feng, liming.gao, tomas.pilar, nd, Leif Lindholm (Nuvia address), Ard Biesheuvel (ARM address) On 07/22/20 13:03, Laszlo Ersek wrote: > Hi Pierre, > > On 07/07/20 10:35, PierreGondois wrote: >> From: Pierre Gondois <pierre.gondois@arm.com> >> >> GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. >> By definition, there should be such dependency. >> >> The outcomes of this patch is that GCC48_ALL_CC_FLAGS and >> other dependent configurations will inherit from the >> additional "-Os" flag. >> The "-Os" flag optimizes a build in size, not breaking any >> build. In a gcc command line, the last optimization flag >> has precedence. This means that this "-Os" flag will be >> overriden by a more specific optimization configuration, >> provided that this more specific flag is appended at the >> end of the CC_FLAGS. >> >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >> Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> >> --- >> >> The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 >> >> Notes: >> v2: >> - Make GCC48_ALL_CC_FLAGS dependent on >> GCC_ALL_CC_FLAGS. [Tomas] >> >> 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 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7fbed49fe24267 100755 >> --- a/BaseTools/Conf/tools_def.template >> +++ b/BaseTools/Conf/tools_def.template >> @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv >> # GCC Build Flag for included header file list generation >> DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps >> >> -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >> +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >> DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 >> DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address >> DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address >> > > As the commit message states, this change makes GCC48_ALL_CC_FLAGS inherit "-Os". > > It is true that all the NOOPT_GCC flags override "-Os" with "-O0": > > NOOPT_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -O0 > NOOPT_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -O0 > NOOPT_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -O0 > NOOPT_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -O0 > NOOPT_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -O0 > NOOPT_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -O0 > NOOPT_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -O0 > NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 > NOOPT_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -O0 > NOOPT_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -O0 > NOOPT_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -O0 > NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 > > However, *some* of the DEBUG and RELEASE flags now have two "-Os" flags: > > DEBUG_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os > RELEASE_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable > DEBUG_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os > RELEASE_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable > DEBUG_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os > RELEASE_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os > RELEASE_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os > RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os > RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > > (The ARM and AARCH64 DEBUG/RELEASE GCC options don't seem to be affected, as they have relied on inherited -- not open-coded -- "-Os" options from much earlier. So now they do not suffer from this duplication.) > > The point of this patch was a kind of "normalization", so I think the work isn't complete until the duplication is undone, i.e., the explicit "-Os" flag is removed from the last twelve defines. > > Can you submit a follow-up patch please? I have not received an answer, and I'm not aware of a follow-up patch being on the list; so now I've filed: https://bugzilla.tianocore.org/show_bug.cgi?id=2928 Thanks Laszlo ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-26 16:42 ` Laszlo Ersek @ 2020-08-27 8:32 ` PierreGondois 2020-08-27 14:55 ` Laszlo Ersek 0 siblings, 1 reply; 31+ messages in thread From: PierreGondois @ 2020-08-27 8:32 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd, Leif Lindholm (Nuvia address), Ard Biesheuvel Hello Laszlo, I thought Leif wanted to revert this modification. Should I apply your requested changes, or should this patch be reverted? Regards, Pierre -----Original Message----- From: Laszlo Ersek <lersek@redhat.com> Sent: Wednesday, August 26, 2020 5:43 PM To: Pierre Gondois <Pierre.Gondois@arm.com> Cc: devel@edk2.groups.io; bob.c.feng@intel.com; liming.gao@intel.com; Tomas Pilar <Tomas.Pilar@arm.com>; nd <nd@arm.com>; Leif Lindholm (Nuvia address) <leif@nuviainc.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com> Subject: Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags On 07/22/20 13:03, Laszlo Ersek wrote: > Hi Pierre, > > On 07/07/20 10:35, PierreGondois wrote: >> From: Pierre Gondois <pierre.gondois@arm.com> >> >> GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. >> By definition, there should be such dependency. >> >> The outcomes of this patch is that GCC48_ALL_CC_FLAGS and other >> dependent configurations will inherit from the additional "-Os" flag. >> The "-Os" flag optimizes a build in size, not breaking any build. In >> a gcc command line, the last optimization flag has precedence. This >> means that this "-Os" flag will be overriden by a more specific >> optimization configuration, provided that this more specific flag is >> appended at the end of the CC_FLAGS. >> >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >> Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> >> --- >> >> The changes can be seen at: >> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 >> >> Notes: >> v2: >> - Make GCC48_ALL_CC_FLAGS dependent on >> GCC_ALL_CC_FLAGS. [Tomas] >> >> 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 >> 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7 >> fbed49fe24267 100755 >> --- a/BaseTools/Conf/tools_def.template >> +++ b/BaseTools/Conf/tools_def.template >> @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv >> # GCC Build Flag for included header file list generation >> DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps >> >> -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >> +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >> DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 >> DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address >> DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address >> > > As the commit message states, this change makes GCC48_ALL_CC_FLAGS inherit "-Os". > > It is true that all the NOOPT_GCC flags override "-Os" with "-O0": > > NOOPT_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -O0 > NOOPT_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -O0 > NOOPT_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -O0 > NOOPT_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -O0 > NOOPT_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -O0 > NOOPT_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -O0 > NOOPT_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -O0 > NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 > NOOPT_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -O0 > NOOPT_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -O0 > NOOPT_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -O0 > NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 > > However, *some* of the DEBUG and RELEASE flags now have two "-Os" flags: > > DEBUG_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os > RELEASE_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable > DEBUG_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os > RELEASE_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable > DEBUG_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os > RELEASE_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os > RELEASE_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os > RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > DEBUG_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os > RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > > (The ARM and AARCH64 DEBUG/RELEASE GCC options don't seem to be > affected, as they have relied on inherited -- not open-coded -- "-Os" > options from much earlier. So now they do not suffer from this > duplication.) > > The point of this patch was a kind of "normalization", so I think the work isn't complete until the duplication is undone, i.e., the explicit "-Os" flag is removed from the last twelve defines. > > Can you submit a follow-up patch please? I have not received an answer, and I'm not aware of a follow-up patch being on the list; so now I've filed: https://bugzilla.tianocore.org/show_bug.cgi?id=2928 Thanks Laszlo ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-27 8:32 ` PierreGondois @ 2020-08-27 14:55 ` Laszlo Ersek 2020-08-27 15:25 ` Leif Lindholm 0 siblings, 1 reply; 31+ messages in thread From: Laszlo Ersek @ 2020-08-27 14:55 UTC (permalink / raw) To: Pierre Gondois, devel@edk2.groups.io Cc: bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd, Leif Lindholm (Nuvia address), Ard Biesheuvel On 08/27/20 10:32, Pierre Gondois wrote: > Hello Laszlo, > I thought Leif wanted to revert this modification. Should I apply your requested changes, or should this patch be reverted? The *other* patch in this series has indeed been reverted: - original commit: dbd546a32d5a ("BaseTools: Add gcc flag to warn on void* pointer arithmetic", 2020-07-21) - revert: 91e4bcb313f0 ("Revert "BaseTools: Add gcc flag to warn on void* pointer arithmetic"", 2020-07-24) I'm not sure what the intent was ultimately with this patch though. (I.e., keep it or revert it.) Personally I'm not calling for a revert; I'd just like the "-Os" duplication to be eliminated. Also it doesn't need to occur for this stable tag, just eventually. Leif, please comment! Thanks! Laszlo > > Regards, > Pierre > > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Wednesday, August 26, 2020 5:43 PM > To: Pierre Gondois <Pierre.Gondois@arm.com> > Cc: devel@edk2.groups.io; bob.c.feng@intel.com; liming.gao@intel.com; Tomas Pilar <Tomas.Pilar@arm.com>; nd <nd@arm.com>; Leif Lindholm (Nuvia address) <leif@nuviainc.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com> > Subject: Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags > > On 07/22/20 13:03, Laszlo Ersek wrote: >> Hi Pierre, >> >> On 07/07/20 10:35, PierreGondois wrote: >>> From: Pierre Gondois <pierre.gondois@arm.com> >>> >>> GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. >>> By definition, there should be such dependency. >>> >>> The outcomes of this patch is that GCC48_ALL_CC_FLAGS and other >>> dependent configurations will inherit from the additional "-Os" flag. >>> The "-Os" flag optimizes a build in size, not breaking any build. In >>> a gcc command line, the last optimization flag has precedence. This >>> means that this "-Os" flag will be overriden by a more specific >>> optimization configuration, provided that this more specific flag is >>> appended at the end of the CC_FLAGS. >>> >>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> >>> Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> >>> --- >>> >>> The changes can be seen at: >>> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 >>> >>> Notes: >>> v2: >>> - Make GCC48_ALL_CC_FLAGS dependent on >>> GCC_ALL_CC_FLAGS. [Tomas] >>> >>> 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 >>> 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7 >>> fbed49fe24267 100755 >>> --- a/BaseTools/Conf/tools_def.template >>> +++ b/BaseTools/Conf/tools_def.template >>> @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv >>> # GCC Build Flag for included header file list generation >>> DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps >>> >>> -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >>> +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings >>> DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 >>> DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address >>> DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address >>> >> >> As the commit message states, this change makes GCC48_ALL_CC_FLAGS inherit "-Os". >> >> It is true that all the NOOPT_GCC flags override "-Os" with "-O0": >> >> NOOPT_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -O0 >> NOOPT_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -O0 >> NOOPT_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -O0 >> NOOPT_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -O0 >> NOOPT_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -O0 >> NOOPT_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -O0 >> NOOPT_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -O0 >> NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 >> NOOPT_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -O0 >> NOOPT_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -O0 >> NOOPT_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -O0 >> NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 >> >> However, *some* of the DEBUG and RELEASE flags now have two "-Os" flags: >> >> DEBUG_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os >> RELEASE_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable >> DEBUG_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os >> RELEASE_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable >> DEBUG_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os >> RELEASE_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable >> DEBUG_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os >> RELEASE_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable >> DEBUG_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os >> RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable >> DEBUG_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os >> RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable >> >> (The ARM and AARCH64 DEBUG/RELEASE GCC options don't seem to be >> affected, as they have relied on inherited -- not open-coded -- "-Os" >> options from much earlier. So now they do not suffer from this >> duplication.) >> >> The point of this patch was a kind of "normalization", so I think the work isn't complete until the duplication is undone, i.e., the explicit "-Os" flag is removed from the last twelve defines. >> >> Can you submit a follow-up patch please? > > I have not received an answer, and I'm not aware of a follow-up patch being on the list; so now I've filed: > > https://bugzilla.tianocore.org/show_bug.cgi?id=2928 > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-27 14:55 ` Laszlo Ersek @ 2020-08-27 15:25 ` Leif Lindholm 2020-08-28 16:56 ` Laszlo Ersek 0 siblings, 1 reply; 31+ messages in thread From: Leif Lindholm @ 2020-08-27 15:25 UTC (permalink / raw) To: Laszlo Ersek Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd, Ard Biesheuvel On Thu, Aug 27, 2020 at 16:55:11 +0200, Laszlo Ersek wrote: > On 08/27/20 10:32, Pierre Gondois wrote: > > Hello Laszlo, > > I thought Leif wanted to revert this modification. Should I apply > your requested changes, or should this patch be reverted? > > The *other* patch in this series has indeed been reverted: > > - original commit: dbd546a32d5a ("BaseTools: Add gcc flag to warn on > void* pointer arithmetic", 2020-07-21) > > - revert: 91e4bcb313f0 ("Revert "BaseTools: Add gcc flag to warn on > void* pointer arithmetic"", 2020-07-24) > > I'm not sure what the intent was ultimately with this patch though. > (I.e., keep it or revert it.) Personally I'm not calling for a revert; > I'd just like the "-Os" duplication to be eliminated. Also it doesn't > need to occur for this stable tag, just eventually. > > Leif, please comment! I did propose reverting it. But I asked for Ard's feedback on the reason for why we had the break in the flags-chain, in case he remembered (and he was on holiday at the time). Basically, I'm wondering whether we're better off changing this behaviour or simply nuking GCC48. Regards, Leif > Thanks! > Laszlo > > > > > Regards, > > Pierre > > > > -----Original Message----- > > From: Laszlo Ersek <lersek@redhat.com> > > Sent: Wednesday, August 26, 2020 5:43 PM > > To: Pierre Gondois <Pierre.Gondois@arm.com> > > Cc: devel@edk2.groups.io; bob.c.feng@intel.com; liming.gao@intel.com; Tomas Pilar <Tomas.Pilar@arm.com>; nd <nd@arm.com>; Leif Lindholm (Nuvia address) <leif@nuviainc.com>; Ard Biesheuvel <Ard.Biesheuvel@arm.com> > > Subject: Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags > > > > On 07/22/20 13:03, Laszlo Ersek wrote: > >> Hi Pierre, > >> > >> On 07/07/20 10:35, PierreGondois wrote: > >>> From: Pierre Gondois <pierre.gondois@arm.com> > >>> > >>> GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS. > >>> By definition, there should be such dependency. > >>> > >>> The outcomes of this patch is that GCC48_ALL_CC_FLAGS and other > >>> dependent configurations will inherit from the additional "-Os" flag. > >>> The "-Os" flag optimizes a build in size, not breaking any build. In > >>> a gcc command line, the last optimization flag has precedence. This > >>> means that this "-Os" flag will be overriden by a more specific > >>> optimization configuration, provided that this more specific flag is > >>> appended at the end of the CC_FLAGS. > >>> > >>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com> > >>> Suggested-by: Tomas Pilar <Tomas.Pilar@arm.com> > >>> --- > >>> > >>> The changes can be seen at: > >>> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2 > >>> > >>> Notes: > >>> v2: > >>> - Make GCC48_ALL_CC_FLAGS dependent on > >>> GCC_ALL_CC_FLAGS. [Tomas] > >>> > >>> 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 > >>> 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7 > >>> fbed49fe24267 100755 > >>> --- a/BaseTools/Conf/tools_def.template > >>> +++ b/BaseTools/Conf/tools_def.template > >>> @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O elf64-littleriscv -B riscv > >>> # GCC Build Flag for included header file list generation > >>> DEFINE GCC_DEPS_FLAGS = -MMD -MF $@.deps > >>> > >>> -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings > >>> +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings > >>> DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20 > >>> DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address > >>> DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address > >>> > >> > >> As the commit message states, this change makes GCC48_ALL_CC_FLAGS inherit "-Os". > >> > >> It is true that all the NOOPT_GCC flags override "-Os" with "-O0": > >> > >> NOOPT_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -O0 > >> NOOPT_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -O0 > >> NOOPT_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -O0 > >> NOOPT_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -O0 > >> NOOPT_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -O0 > >> NOOPT_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -O0 > >> NOOPT_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -O0 > >> NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 > >> NOOPT_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -O0 > >> NOOPT_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -O0 > >> NOOPT_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -O0 > >> NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 > >> > >> However, *some* of the DEBUG and RELEASE flags now have two "-Os" flags: > >> > >> DEBUG_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os > >> RELEASE_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable > >> DEBUG_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os > >> RELEASE_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable > >> DEBUG_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os > >> RELEASE_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > >> DEBUG_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os > >> RELEASE_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > >> DEBUG_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os > >> RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > >> DEBUG_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os > >> RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO -Os -Wno-unused-but-set-variable -Wno-unused-const-variable > >> > >> (The ARM and AARCH64 DEBUG/RELEASE GCC options don't seem to be > >> affected, as they have relied on inherited -- not open-coded -- "-Os" > >> options from much earlier. So now they do not suffer from this > >> duplication.) > >> > >> The point of this patch was a kind of "normalization", so I think the work isn't complete until the duplication is undone, i.e., the explicit "-Os" flag is removed from the last twelve defines. > >> > >> Can you submit a follow-up patch please? > > > > I have not received an answer, and I'm not aware of a follow-up patch being on the list; so now I've filed: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=2928 > > > > Thanks > > Laszlo > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-27 15:25 ` Leif Lindholm @ 2020-08-28 16:56 ` Laszlo Ersek 2020-08-28 19:15 ` Leif Lindholm 0 siblings, 1 reply; 31+ messages in thread From: Laszlo Ersek @ 2020-08-28 16:56 UTC (permalink / raw) To: Leif Lindholm Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd, Ard Biesheuvel On 08/27/20 17:25, Leif Lindholm wrote: > On Thu, Aug 27, 2020 at 16:55:11 +0200, Laszlo Ersek wrote: >> On 08/27/20 10:32, Pierre Gondois wrote: >>> Hello Laszlo, >>> I thought Leif wanted to revert this modification. Should I apply >> your requested changes, or should this patch be reverted? >> >> The *other* patch in this series has indeed been reverted: >> >> - original commit: dbd546a32d5a ("BaseTools: Add gcc flag to warn on >> void* pointer arithmetic", 2020-07-21) >> >> - revert: 91e4bcb313f0 ("Revert "BaseTools: Add gcc flag to warn on >> void* pointer arithmetic"", 2020-07-24) >> >> I'm not sure what the intent was ultimately with this patch though. >> (I.e., keep it or revert it.) Personally I'm not calling for a revert; >> I'd just like the "-Os" duplication to be eliminated. Also it doesn't >> need to occur for this stable tag, just eventually. >> >> Leif, please comment! > > I did propose reverting it. But I asked for Ard's feedback on the > reason for why we had the break in the flags-chain, in case he > remembered (and he was on holiday at the time). > > Basically, I'm wondering whether we're better off changing this > behaviour or simply nuking GCC48. I use GCC48 daily -- it's the system compiler on RHEL7. My laptop runs RHEL7 -- I value stability above all. Thanks Laszlo ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-28 16:56 ` Laszlo Ersek @ 2020-08-28 19:15 ` Leif Lindholm 2020-08-31 13:22 ` Ard Biesheuvel 0 siblings, 1 reply; 31+ messages in thread From: Leif Lindholm @ 2020-08-28 19:15 UTC (permalink / raw) To: Laszlo Ersek Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd, Ard Biesheuvel On Fri, Aug 28, 2020 at 18:56:45 +0200, Laszlo Ersek wrote: > >> Leif, please comment! > > > > I did propose reverting it. But I asked for Ard's feedback on the > > reason for why we had the break in the flags-chain, in case he > > remembered (and he was on holiday at the time). > > > > Basically, I'm wondering whether we're better off changing this > > behaviour or simply nuking GCC48. > > I use GCC48 daily -- it's the system compiler on RHEL7. My laptop runs > RHEL7 -- I value stability above all. That explains why it's still in then :) OK, so then cleaning it up would be nice. / Leif ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-28 19:15 ` Leif Lindholm @ 2020-08-31 13:22 ` Ard Biesheuvel 2020-08-31 13:43 ` 回复: " gaoliming 2020-08-31 14:03 ` Laszlo Ersek 0 siblings, 2 replies; 31+ messages in thread From: Ard Biesheuvel @ 2020-08-31 13:22 UTC (permalink / raw) To: Leif Lindholm, Laszlo Ersek Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd On 8/28/20 9:15 PM, Leif Lindholm wrote: > On Fri, Aug 28, 2020 at 18:56:45 +0200, Laszlo Ersek wrote: >>>> Leif, please comment! >>> >>> I did propose reverting it. But I asked for Ard's feedback on the >>> reason for why we had the break in the flags-chain, in case he >>> remembered (and he was on holiday at the time). >>> >>> Basically, I'm wondering whether we're better off changing this >>> behaviour or simply nuking GCC48. >> >> I use GCC48 daily -- it's the system compiler on RHEL7. My laptop runs >> RHEL7 -- I value stability above all. > > That explains why it's still in then :) > > OK, so then cleaning it up would be nice. > Apologies, I had missed this discussion. The reason the GCC flags are defined the way the are is because the GCC4x toolchains for x86 were completely disjoint from the UNIXGCC and other ones that were defined for ARM and IA64. I agree that some cleanup would be nice, but if we are only going to reshuffle tools_def variable assignments, I'm not sure it is worth it. What I would like to see is a bit more structure, and some justification why we are using all those different options, as i am not convinced they are still needed even for GCC as far back as v4.8. Also, even though I understand Laszlo's point about being able to use the RHEL 7 system compiler, using other toolchains is trivially easy with EDK2 (this is the whole point), and mainline EDK2 is arguably a development tree, not a stable production tree for ~5 year old firmware builds, and so I do think we should get rid of GCC48 even before RHEL7 goes EOL. I'd even go so far as suggesting that [at some point in the not too distant future], we rename GCC5 to GCCELFLTO (or something less obnoxious) and phase out all the version based GCC toolchains entirely. We might do the same for CLANG38 (the ELF based one), and depending on whether PE/COFF linker support ever materializes for ARM, have a single Clang+PE based toolchain as well. ^ permalink raw reply [flat|nested] 31+ messages in thread
* 回复: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-31 13:22 ` Ard Biesheuvel @ 2020-08-31 13:43 ` gaoliming 2020-08-31 14:03 ` Laszlo Ersek 1 sibling, 0 replies; 31+ messages in thread From: gaoliming @ 2020-08-31 13:43 UTC (permalink / raw) To: devel, ard.biesheuvel, 'Leif Lindholm', 'Laszlo Ersek' Cc: 'Pierre Gondois', bob.c.feng, liming.gao, 'Tomas Pilar', 'nd' Ard: > -----邮件原件----- > 发件人: bounce+27952+64834+4905953+8761045@groups.io > <bounce+27952+64834+4905953+8761045@groups.io> 代表 Ard > Biesheuvel > 发送时间: 2020年8月31日 21:22 > 收件人: Leif Lindholm <leif@nuviainc.com>; Laszlo Ersek > <lersek@redhat.com> > 抄送: Pierre Gondois <Pierre.Gondois@arm.com>; devel@edk2.groups.io; > bob.c.feng@intel.com; liming.gao@intel.com; Tomas Pilar > <Tomas.Pilar@arm.com>; nd <nd@arm.com> > 主题: Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags > > On 8/28/20 9:15 PM, Leif Lindholm wrote: > > On Fri, Aug 28, 2020 at 18:56:45 +0200, Laszlo Ersek wrote: > >>>> Leif, please comment! > >>> > >>> I did propose reverting it. But I asked for Ard's feedback on the > >>> reason for why we had the break in the flags-chain, in case he > >>> remembered (and he was on holiday at the time). > >>> > >>> Basically, I'm wondering whether we're better off changing this > >>> behaviour or simply nuking GCC48. > >> > >> I use GCC48 daily -- it's the system compiler on RHEL7. My laptop runs > >> RHEL7 -- I value stability above all. > > > > That explains why it's still in then :) > > > > OK, so then cleaning it up would be nice. > > > > Apologies, I had missed this discussion. > > The reason the GCC flags are defined the way the are is because the > GCC4x toolchains for x86 were completely disjoint from the UNIXGCC and > other ones that were defined for ARM and IA64. > > I agree that some cleanup would be nice, but if we are only going to > reshuffle tools_def variable assignments, I'm not sure it is worth it. > What I would like to see is a bit more structure, and some justification > why we are using all those different options, as i am not convinced they > are still needed even for GCC as far back as v4.8. > > Also, even though I understand Laszlo's point about being able to use > the RHEL 7 system compiler, using other toolchains is trivially easy > with EDK2 (this is the whole point), and mainline EDK2 is arguably a > development tree, not a stable production tree for ~5 year old firmware > builds, and so I do think we should get rid of GCC48 even before RHEL7 > goes EOL. > I have the same feeling on VS tool chain. Now, very old VS2008 tool chain is still in tools_def.txt. > I'd even go so far as suggesting that [at some point in the not too > distant future], we rename GCC5 to GCCELFLTO (or something less > obnoxious) and phase out all the version based GCC toolchains entirely. > We might do the same for CLANG38 (the ELF based one), and depending on > whether PE/COFF linker support ever materializes for ARM, have a single > Clang+PE based toolchain as well. > This is a good idea to unify tool chain. But, the impact is big. To keep combability, GCC5 and GCCELFLTO can be defined together and share the same setting. Thanks Liming > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-31 13:22 ` Ard Biesheuvel 2020-08-31 13:43 ` 回复: " gaoliming @ 2020-08-31 14:03 ` Laszlo Ersek 2020-08-31 14:37 ` Ard Biesheuvel 1 sibling, 1 reply; 31+ messages in thread From: Laszlo Ersek @ 2020-08-31 14:03 UTC (permalink / raw) To: Ard Biesheuvel, Leif Lindholm Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd On 08/31/20 15:22, Ard Biesheuvel wrote: > mainline EDK2 is arguably a development tree I agree. > not a stable production tree for ~5 year old firmware builds I agree with that too. But I don't think GCC48 is "holding back" edk2. I don't know of a firmware feature that suffers because I'd like to be able to build the tree with GCC48. (LTO is not a firmware feature; and NOOPT builds, which are important, don't / shouldn't enable LTO anyway.) I do agree that maintaining the BaseTools stuff that's related to GCC48 is a burden, technically speaking. Is it a big burden? Should I attempt to handle related issues? Official Software Collections / Developer Toolset add-ons exist for RHEL7: https://developers.redhat.com/products/developertoolset/overview https://access.redhat.com/documentation/en-us/red_hat_developer_toolset/9/html/user_guide/chap-red_hat_developer_toolset#sect-Red_Hat_Developer_Toolset-Compatibility I've played with them in the past. They weren't a good fit for me, as I recall. Anyway, I can check them out again, if I must. > and so I do think we should get rid of GCC48 even before RHEL7 goes > EOL. We might want to explore the Debian / Ubuntu status too (LTS). Thanks, Laszlo ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-31 14:03 ` Laszlo Ersek @ 2020-08-31 14:37 ` Ard Biesheuvel 2020-08-31 16:18 ` Laszlo Ersek 0 siblings, 1 reply; 31+ messages in thread From: Ard Biesheuvel @ 2020-08-31 14:37 UTC (permalink / raw) To: Laszlo Ersek, Leif Lindholm Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd On 8/31/20 4:03 PM, Laszlo Ersek wrote: > On 08/31/20 15:22, Ard Biesheuvel wrote: > >> mainline EDK2 is arguably a development tree > > I agree. > >> not a stable production tree for ~5 year old firmware builds > > I agree with that too. > > But I don't think GCC48 is "holding back" edk2. I don't know of a > firmware feature that suffers because I'd like to be able to build the > tree with GCC48. > True. > (LTO is not a firmware feature; and NOOPT builds, which are important, > don't / shouldn't enable LTO anyway.) > > I do agree that maintaining the BaseTools stuff that's related to GCC48 > is a burden, technically speaking. Is it a big burden? Should I attempt > to handle related issues? > Not necessarily. For obvious reasons, my mental picture of the historical situation is more clear when it comes to ARM, and GCC48 is the last version that uses the large code model, and which shipped with a version of binutils that was buggy, requiring extra restrictions wrt binary layout in GenFw) [which brings me to another GCC toolchain versioning issue, which is that binutils is released on a separate schedule, and the '48' in GCC48 does not clearly specify the binutils version] Not sure how Leif feels about this, but i wouldn't mind retaining GCC48 support only for IA32/X64. Or alternatively, make it a per-platform decision (which it ultimately is anyway, given that every platform ships with platform specific code that could have issues with older toolchains) > Official Software Collections / Developer Toolset add-ons exist for > RHEL7: > > https://developers.redhat.com/products/developertoolset/overview > https://access.redhat.com/documentation/en-us/red_hat_developer_toolset/9/html/user_guide/chap-red_hat_developer_toolset#sect-Red_Hat_Developer_Toolset-Compatibility > > I've played with them in the past. They weren't a good fit for me, as I > recall. Anyway, I can check them out again, if I must. > >> and so I do think we should get rid of GCC48 even before RHEL7 goes >> EOL. > > We might want to explore the Debian / Ubuntu status too (LTS). > Agreed. But one final remark on the distro/system toolchain situation: distros have good reasons for sticking with a single GCC version to build the world, but I wonder if any of them hold for UEFI builds such as OVMF, which runs entirely in its own context, and is mangled by GenFw so the ELF objects are never even consumed directly. So as I see it, the *only* benefit of retaining GCC48 support is for the convenience of developers that use 'mature' distros like RHEL 7 and prefer to use the compiler that happens to be installed. I am not saying this is not a good enough reason, just something we have to realize. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-31 14:37 ` Ard Biesheuvel @ 2020-08-31 16:18 ` Laszlo Ersek 2020-08-31 16:27 ` Laszlo Ersek 0 siblings, 1 reply; 31+ messages in thread From: Laszlo Ersek @ 2020-08-31 16:18 UTC (permalink / raw) To: Ard Biesheuvel, Leif Lindholm Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd On 08/31/20 16:37, Ard Biesheuvel wrote: > Not sure how Leif feels about this, but i wouldn't mind retaining > GCC48 support only for IA32/X64. I've never considered GCC48 usable for ARM/AARCH64. I already use GCC48 for IA32/X64 only. Anyway... it looks like I am able to consume gcc-9 ("devtoolset-9-gcc-9.3.1-2.el7.x86_64") and compatible binutils ("devtoolset-9-binutils-2.32-16.el7.x86_64"), for building edk2 with the GCC5 toolchain tag on RHEL7. I've done a few tests with OVMF; nothing seems to break. > Agreed. But one final remark on the distro/system toolchain situation: > distros have good reasons for sticking with a single GCC version to > build the world, but I wonder if any of them hold for UEFI builds such > as OVMF, which runs entirely in its own context, and is mangled by > GenFw so the ELF objects are never even consumed directly. So as I see > it, the *only* benefit of retaining GCC48 support is for the > convenience of developers that use 'mature' distros like RHEL 7 and > prefer to use the compiler that happens to be installed. I am not > saying this is not a good enough reason, just something we have to > realize. You're not wrong -- I'd just like to clarify that "prefer" is not always a simple personal choice. Building gcc + binutils from source (and finding the right versions in the first place) is traditionally not one of the easier "I'll just build this for myself" endeavors. (With gcc rebuilding itself several times etc.) Furthermore, given that "devtoolset-9" is a "software collection" (managed by the "scl" cmdline utility) at least in my env, its usage is *really* inconvenient in some circumstances. It modifies a bunch of environment variables (of course), but (AIUI) the recommended / supported end-user interface is to let "scl" start a command or subshell for you. The mode when it outputs a bunch of "export" statements for one's shell to "source" requires gymnastics, and it does not seem officially supported. I find it unfortunate that it wants to become the parent of whatever I'm trying to run. Add in the fact that single-line invocations require unwieldy quoting: """ scl enable example 'less --version' runs command 'less --version' in the environment with collection 'example' enabled scl enable foo bar bash runs bash instance with foo and bar Software Collections enabled """ Obviously the following form would be preferred: scl enable example -- less --version Because this form would not require quoting, just prefixing. Anyway... I've now hacked those scripts of mine that needed hacking, so I guess I'm no longer tied to GCC48 for IA32/X64 either. Thanks, Laszlo ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-31 16:18 ` Laszlo Ersek @ 2020-08-31 16:27 ` Laszlo Ersek 2020-08-31 17:14 ` Ard Biesheuvel 0 siblings, 1 reply; 31+ messages in thread From: Laszlo Ersek @ 2020-08-31 16:27 UTC (permalink / raw) To: Ard Biesheuvel, Leif Lindholm Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd On 08/31/20 18:18, Laszlo Ersek wrote: > Obviously the following form would be preferred: > > scl enable example -- less --version > > Because this form would not require quoting, just prefixing. I need to slow down when reading manual pages. Now that I've actually *tried* the above, it works! And upon re-reading the manual, it's even *documented*; just not in the examples section. Where's my brown paper bag again... Laszlo ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags 2020-08-31 16:27 ` Laszlo Ersek @ 2020-08-31 17:14 ` Ard Biesheuvel 0 siblings, 0 replies; 31+ messages in thread From: Ard Biesheuvel @ 2020-08-31 17:14 UTC (permalink / raw) To: Laszlo Ersek, Leif Lindholm Cc: Pierre Gondois, devel@edk2.groups.io, bob.c.feng@intel.com, liming.gao@intel.com, Tomas Pilar, nd On 8/31/20 6:27 PM, Laszlo Ersek wrote: > On 08/31/20 18:18, Laszlo Ersek wrote: > >> Obviously the following form would be preferred: >> >> scl enable example -- less --version >> >> Because this form would not require quoting, just prefixing. > > I need to slow down when reading manual pages. Now that I've actually > *tried* the above, it works! And upon re-reading the manual, it's even > *documented*; just not in the examples section. > > Where's my brown paper bag again... > :-) Happy to hear that you managed, and that you are on board with deprecating GCC48. Given our successful efforts to make the GCC5 profile work with all GCC versions since the 5.x series, I think we should consider dropping GCC48 *and* GCC49, as well as CLANG35 (which only supports ARM anyway, and is superseded by CLANG38 which should also support any Clang version released since) To Liming's point, renaming the profiles may have additional impact which we are not prepared to deal with, so that could be a separate matter. But removing GCC48, GCC49 and CLANG35 reduces the CI effort, and allows us to focus our support on profiles that are actually in wide use. ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-08-31 17:14 UTC | newest] Thread overview: 31+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-07 8:35 [PATCH V2 0/2] Add gcc flag for void* pointer arithmetics PierreGondois 2020-07-07 8:35 ` [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic PierreGondois 2020-07-16 9:07 ` [edk2-devel] " Yuwei Chen 2020-07-20 4:10 ` Bob Feng 2020-07-22 18:05 ` [edk2-devel] " Leif Lindholm 2020-07-22 21:13 ` Andrew Fish 2020-07-23 1:56 ` Bob Feng 2020-07-23 2:49 ` Andrew Fish 2020-07-23 9:33 ` Leif Lindholm 2020-07-24 3:56 ` Bob Feng 2020-07-24 9:01 ` PierreGondois 2020-07-24 11:05 ` Leif Lindholm 2020-07-24 11:03 ` Leif Lindholm 2020-07-07 8:35 ` [PATCH V2 2/2] BaseTools: Factorize GCC flags PierreGondois 2020-07-20 4:11 ` Bob Feng 2020-07-30 12:08 ` [edk2-devel] " Leif Lindholm 2020-07-22 11:03 ` Laszlo Ersek 2020-07-22 11:24 ` Laszlo Ersek 2020-08-26 16:42 ` Laszlo Ersek 2020-08-27 8:32 ` PierreGondois 2020-08-27 14:55 ` Laszlo Ersek 2020-08-27 15:25 ` Leif Lindholm 2020-08-28 16:56 ` Laszlo Ersek 2020-08-28 19:15 ` Leif Lindholm 2020-08-31 13:22 ` Ard Biesheuvel 2020-08-31 13:43 ` 回复: " gaoliming 2020-08-31 14:03 ` Laszlo Ersek 2020-08-31 14:37 ` Ard Biesheuvel 2020-08-31 16:18 ` Laszlo Ersek 2020-08-31 16:27 ` Laszlo Ersek 2020-08-31 17:14 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox