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

["Leif Lindholm" <leif@nuviainc.com>]

On Fri, Jul 24, 2020 at 09:01:16 +0000, Pierre Gondois wrote:
> Hello everyone,
> 
> Thank you for reverting the patch. I should have tested building on more platforms.
> I can fix the pieces of code highlighted by the -Wpointer-arith flag
> on ARM platforms. For now I saw 10-15 places that would need to be
> modified.
> If this is not a huge task I will try to do it for other platforms.

That would certainly be most appreciated.
If not, I will get to it eventually, but it might drag out a few
weeks.

Regards,

Leif

> Regards,
> Pierre
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bob Feng via groups.io
> Sent: 24 July 2020 04:56
> To: Leif Lindholm <leif@nuviainc.com>; devel@edk2.groups.io; afish@apple.com
> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com
> Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic
> 
> What would be your suggestion on the approach to apply this build option to all architectures? Change the tool_def.txt firstly and push platform owner to fix the corresponding firmware code, or changing the tool_def.txt later, until this change will not break any platform build?
> 
> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Thursday, July 23, 2020 5:33 PM
> To: devel@edk2.groups.io; afish@apple.com
> Cc: Feng, Bob C <bob.c.feng@intel.com>; PierreGondois <pierre.gondois@arm.com>; Gao, Liming <liming.gao@intel.com>; tomas@nuviainc.com
> Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void* pointer arithmetic
> 
> Hi Andrew,
> 
> Agreed.
> 
> I also think this should be applied across all architectures, not just ARM/AARCH64. Since Visual Studio has never been been able to compile the affected code, I expect impact to Ia32/X64 to be minimal.
> 
> Regards,
> 
> Leif
> 
> On Wed, Jul 22, 2020 at 19:49:01 -0700, Andrew Fish via groups.io wrote:
> > Bob,
> >
> > It also looks like clang could use this flag as the default seems to
> > be to follow the GCC behavior.
> >
> > Thanks,
> >
> > Andrew Fish
> >
> > > On Jul 22, 2020, at 6:56 PM, Bob Feng <bob.c.feng@intel.com> wrote:
> > >
> > > Hi Leif
> > >
> > > I agree to revert that patch for now and I sent a revert patch for review. After resolving the build break issue for ARM/AARCH64 platforms in edk2-platforms, and make sure there is no platform build break with this patch, we will push it again.
> > >
> > > Thanks,
> > > Bob
> > >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> > > <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of
> > > Leif Lindholm
> > > Sent: Thursday, July 23, 2020 2:06 AM
> > > To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Feng, Bob C
> > > <bob.c.feng@intel.com <mailto:bob.c.feng@intel.com>>
> > > Cc: PierreGondois <pierre.gondois@arm.com
> > > <mailto:pierre.gondois@arm.com>>; Gao, Liming <liming.gao@intel.com
> > > <mailto:liming.gao@intel.com>>; tomas@nuviainc.com
> > > <mailto:tomas@nuviainc.com>
> > > Subject: Re: [edk2-devel] [PATCH V2 1/2] BaseTools: Add gcc flag to
> > > warn on void* pointer arithmetic
> > >
> > > Hi Bob,
> > >
> > > This patch also breaks about half of the ARM/AARCH64 platforms in edk2-platforms. I agree it should go in at a later stage, but for now, can we please revert it?
> > >
> > > Regards,
> > >
> > > Leif
> > >
> > > On Mon, Jul 20, 2020 at 04:10:27 +0000, Bob Feng wrote:
> > >> Reviewed-by: Bob Feng<bob.c.feng@intel.com>
> > >>
> > >>
> > >> -----Original Message-----
> > >> From: PierreGondois <pierre.gondois@arm.com>
> > >> Sent: Tuesday, July 7, 2020 4:35 PM
> > >> To: devel@edk2.groups.io
> > >> Cc: Pierre Gondois <Pierre.Gondois@arm.com>; Feng, Bob C
> > >> <bob.c.feng@intel.com>; Gao, Liming <liming.gao@intel.com>;
> > >> tomas.pilar@arm.com; nd@arm.com
> > >> Subject: [PATCH V2 1/2] BaseTools: Add gcc flag to warn on void*
> > >> pointer arithmetic
> > >>
> > >> From: Pierre Gondois <pierre.gondois@arm.com>
> > >>
> > >> By default, gcc allows void* pointer arithmetic.
> > >> This is a GCC extension.
> > >> However:
> > >> - the C reference manual states that void*
> > >>   pointer "cannot be operands of addition
> > >>   or subtraction operators". Cf s5.3.1
> > >>   "Generic Pointers";
> > >> - Visual studio compiler treat such operation as
> > >>   an error.
> > >>
> > >> To prevent such pointer arithmetic, the "-Wpointer-arith"
> > >> flag should be set for all GCC versions.
> > >>
> > >> The "-Wpointer-arith"  allows to:
> > >>  "Warn about anything that depends on the "size of"
> > >>  a function type or of void. GNU C assigns these  types a size of
> > >> 1, for convenience in calculations  with void * pointers and
> > >> pointers to functions."
> > >>
> > >> This flag is available since GCC2.95.3 which came out in 2001.
> > >>
> > >> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> > >> ---
> > >>
> > >> The changes can be seen at:
> > >> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_
> > >> v2
> > >>
> > >> Notes:
> > >>    v1:
> > >>     - Add "-Wpointer-arith" gcc flag. [Pierre]
> > >>    v2:
> > >>     - Only add the flag for ARM and AARCH64. [Tomas]
> > >>
> > >> BaseTools/Conf/tools_def.template | 6 +++---
> > >> 1 file changed, 3 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/BaseTools/Conf/tools_def.template
> > >> b/BaseTools/Conf/tools_def.template
> > >> index
> > >> 8aeb8a2a6417e41c5660cda5066f52adc8cc3089..397b011ba38f97f81f314f864
> > >> 1ac
> > >> 8bb95d5a2197 100755
> > >> --- a/BaseTools/Conf/tools_def.template
> > >> +++ b/BaseTools/Conf/tools_def.template
> > >> @@ -1,7 +1,7 @@
> > >> #
> > >> #  Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > >> reserved.<BR>  #  Portions copyright (c) 2008 - 2009, Apple Inc.
> > >> All rights reserved.<BR> -#  Portions copyright (c) 2011 - 2019, ARM Ltd.
> > >> All rights reserved.<BR>
> > >> +#  Portions copyright (c) 2011 - 2020, ARM Ltd. All rights
> > >> +reserved.<BR>
> > >> #  Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<BR>  #  (C) Copyright 2020, Hewlett Packard Enterprise Development LP<BR>  #  Copyright (c) Microsoft Corporation
> > >> @@ -1921,9 +1921,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG     = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N
> > >> DEFINE GCC_ALL_CC_FLAGS            = -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common
> > >> DEFINE GCC_IA32_CC_FLAGS           = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe
> > >> DEFINE GCC_X64_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe
> > >> -DEFINE GCC_ARM_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie
> > >> +DEFINE GCC_ARM_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie
> > >> DEFINE GCC_ARM_CC_XIPFLAGS         = -mno-unaligned-access
> > >> -DEFINE GCC_AARCH64_CC_FLAGS        = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char  -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18
> > >> +DEFINE GCC_AARCH64_CC_FLAGS        = DEF(GCC_ALL_CC_FLAGS) -Wpointer-arith -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18
> > >> DEFINE GCC_AARCH64_CC_XIPFLAGS     = -mstrict-align -mgeneral-regs-only
> > >> DEFINE GCC_DLINK_FLAGS_COMMON      = -nostdlib --pie
> > >> DEFINE GCC_DLINK2_FLAGS_COMMON     = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/GccBase.lds
> > >> --
> > >> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> > >>
> > >>
> > >>
> > >>
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> 
> 
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.