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

[Laszlo Ersek <lersek@redhat.com>]

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
>