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

[Laszlo Ersek <lersek@redhat.com>]

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