[PATCH V2 0/2] Add gcc flag for void* pointer arithmetics

[PATCH V2 0/2] Add gcc flag for void* pointer arithmetics

[PierreGondois]

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

[PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[PierreGondois]

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

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Yuwei Chen]

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‰

Re: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Bob Feng]

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

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Andrew Fish]

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

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Bob Feng]

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

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Andrew Fish]

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

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Bob Feng]

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

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[PierreGondois]

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.

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Leif Lindholm]

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.

Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic

[Leif Lindholm]

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

[PATCH V2 2/2] BaseTools: Factorize GCC flags

[PierreGondois]

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

Re: [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Bob Feng]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Laszlo Ersek]

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
> 

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[PierreGondois]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Laszlo Ersek]

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
> 

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Leif Lindholm]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Ard Biesheuvel]

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.

回复: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[gaoliming]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Ard Biesheuvel]

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.

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Laszlo Ersek]

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

Re: [edk2-devel] [PATCH V2 2/2] BaseTools: Factorize GCC flags

[Ard Biesheuvel]

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.