Re: [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error

["Gao, Liming" <liming.gao@intel.com>]

I agree to move this option to common GCC option. 

And, now GCC5 has this option. That means if the platform pass GCC5 build, it should not be impacted in GCC48/GCC49 by this change. Right?

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 4, 2017 12:34 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Song, BinX <binx.song@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error
> 
> On 04/03/17 18:21, Ard Biesheuvel wrote:
> > On 3 April 2017 at 17:16, Laszlo Ersek <lersek@redhat.com> wrote:
> >> adding Ard
> >>
> >> On 04/01/17 10:38, Song, BinX wrote:
> >>> - Fix GCC48/GCC49 build error
> >>>
> >>> Cc: Liming Gao <liming.gao@intel.com>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Bell Song <binx.song@intel.com>
> >>> ---
> >>>  .../Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf    | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
> b/MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
> >>> index 578f97f..4c9aff5 100644
> >>> --- a/MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
> >>> +++ b/MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
> >>> @@ -54,3 +54,6 @@
> >>>    DebugLib
> >>>    BaseMemoryLib
> >>>    ExtractGuidedSectionLib
> >>> +
> >>> +[BuildOptions]
> >>> +  GCC:*_*_*_CC_FLAGS = -fno-builtin
> >>>
> >>
> >> In "BaseTools/Conf/tools_def.template", we currently have:
> >>
> >> DEFINE GCC_ARM_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) [...] -fno-builtin [...]
> >> DEFINE GCC_AARCH64_CC_FLAGS        = DEF(GCC_ALL_CC_FLAGS) [...] -fno-builtin [...]
> >>
> >> DEFINE GCC5_IA32_CC_FLAGS            = DEF(GCC49_IA32_CC_FLAGS) -fno-builtin
> >> DEFINE GCC5_X64_CC_FLAGS             = DEF(GCC49_X64_CC_FLAGS) -fno-builtin
> >>
> >> Now, GCC5_IA32_CC_FLAGS goes back to GCC44_ALL_CC_FLAGS, via:
> >> - GCC49_IA32_CC_FLAGS
> >> - GCC48_IA32_CC_FLAGS
> >> - GCC47_IA32_CC_FLAGS
> >> - GCC46_IA32_CC_FLAGS
> >> - GCC45_IA32_CC_FLAGS
> >> - GCC44_IA32_CC_FLAGS
> >> - GCC44_ALL_CC_FLAGS
> >>
> >> (similarly for GCC5_X64_CC_FLAGS.)
> >>
> >> So, instead of this patch for BrotliCustomDecompressLib, how about:
> >>
> >> - moving "-fno-builtin" from
> >>     GCC_ARM_CC_FLAGS and
> >>     GCC_AARCH64_CC_FLAGS
> >>   to
> >>     GCC_ALL_CC_FLAGS, and
> >>
> >> - moving "-fno-builtin" from
> >>     GCC5_IA32_CC_FLAGS and
> >>     GCC5_X64_CC_FLAGS
> >>   to
> >>     GCC44_ALL_CC_FLAGS?
> >>
> >> Do we have any reason for permitting builtins at all?
> >>
> >
> > Well, one thing I noticed the other day is that GCC does not
> > 'recognize' memcpy() and memset() when -fno-builtin is defined, which
> > means trivial memcpys and memsets will not be inlined.
> 
> But memcpy() and memset(), as written, cannot be called in edk2 anyway
> 
> - explicitly, because we don't allow that,
> 
> - implicitly (via struct assignment or initialization, for example),
> because we forbid that as well -- source code is supposed to use
> CopyMem(), CopyGuid(), and the like.
> 
> So, yes, memcpy() and memset() would not be inlined with my suggestion,
> but edk2 code shouldn't exist in the first place that leads to the
> generation of memcpy() and memset() calls.
> 
> > I guess that
> > argues for not permitting it at all, but it also means adding it
> > unconditionally may affect how code is currently generated for some
> > platforms.
> 
> If said code doesn't conform to the above requirement, then yes, it
> could happen.
> 
> Thanks!
> Laszlo