* [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error @ 2017-04-01 8:38 Song, BinX 2017-04-03 16:16 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Song, BinX @ 2017-04-01 8:38 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: Gao, Liming - 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 -- 2.10.2.windows.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error 2017-04-01 8:38 [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error Song, BinX @ 2017-04-03 16:16 ` Laszlo Ersek 2017-04-03 16:21 ` Ard Biesheuvel 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2017-04-03 16:16 UTC (permalink / raw) To: Song, BinX, edk2-devel@lists.01.org; +Cc: Gao, Liming, Ard Biesheuvel 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? Thanks, Laszlo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error 2017-04-03 16:16 ` Laszlo Ersek @ 2017-04-03 16:21 ` Ard Biesheuvel 2017-04-03 16:34 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Ard Biesheuvel @ 2017-04-03 16:21 UTC (permalink / raw) To: Laszlo Ersek; +Cc: Song, BinX, edk2-devel@lists.01.org, Gao, Liming 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. 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error 2017-04-03 16:21 ` Ard Biesheuvel @ 2017-04-03 16:34 ` Laszlo Ersek 2017-04-05 4:52 ` Gao, Liming 0 siblings, 1 reply; 6+ messages in thread From: Laszlo Ersek @ 2017-04-03 16:34 UTC (permalink / raw) To: Ard Biesheuvel; +Cc: Song, BinX, edk2-devel@lists.01.org, Gao, Liming 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error 2017-04-03 16:34 ` Laszlo Ersek @ 2017-04-05 4:52 ` Gao, Liming 2017-04-05 8:03 ` Laszlo Ersek 0 siblings, 1 reply; 6+ messages in thread From: Gao, Liming @ 2017-04-05 4:52 UTC (permalink / raw) To: Laszlo Ersek, Ard Biesheuvel; +Cc: Song, BinX, edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error 2017-04-05 4:52 ` Gao, Liming @ 2017-04-05 8:03 ` Laszlo Ersek 0 siblings, 0 replies; 6+ messages in thread From: Laszlo Ersek @ 2017-04-05 8:03 UTC (permalink / raw) To: Gao, Liming, Ard Biesheuvel; +Cc: Song, BinX, edk2-devel@lists.01.org On 04/05/17 06:52, Gao, Liming wrote: > 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? I think so, yes. Laszlo > > 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-05 8:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-01 8:38 [PATCH] MdeModulePkg: Fix GCC48/GCC49 build error Song, BinX 2017-04-03 16:16 ` Laszlo Ersek 2017-04-03 16:21 ` Ard Biesheuvel 2017-04-03 16:34 ` Laszlo Ersek 2017-04-05 4:52 ` Gao, Liming 2017-04-05 8:03 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox