[PATCH] MdeModulePkg: Fix GCC48/GCC49 build error

[PATCH] MdeModulePkg: Fix GCC48/GCC49 build error

[Song, BinX]

- 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

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

[Laszlo Ersek]

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

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

[Ard Biesheuvel]

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.

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

[Laszlo Ersek]

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

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

[Gao, Liming]

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

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

[Laszlo Ersek]

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
>