Re: [Patch V2] BaseTools: support the NOOPT target with the GCC tool chains

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 5 October 2016 at 18:56, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/05/16 18:06, Ard Biesheuvel wrote:
>> On 5 October 2016 at 15:48, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 10/05/16 03:30, Yonghong Zhu wrote:
>>>> Update the tools_def.template to add NOOPT support with GCC tool chains.
>>>>
>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
>>>> ---
>>>>  BaseTools/Conf/tools_def.template | 28 ++++++++++++++++++++++++++++
>>>>  1 file changed, 28 insertions(+)
>>>
>>> I thought I understood what was going on, but apparently I was wrong
>>> about that.
>>>
>>> In this patch, we add or modify:
>>> - NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG -- okay
>>> - NOOPT_GCC*_(IA32|X64|ARM|AARCH64)_CC_FLAGS -- okay
>>>
>>> So that part is fine with me. But then we also add / modify:
>>> - NOOPT_GCC(49|5)_AARCH64_DLINK_(FLAGS|XIPFLAGS)
>>> - NOOPT_GCC5_ARM_DLINK_FLAGS
>>>
>>> First I thought the latter set of changes was unnecessary, because "ld"
>>> didn't use "-O". I checked the manual, and I was wrong: "ld" does know /
>>> use "-O". So those changes are fine, I guess.
>>>
>>
>> Yes, especially under LTO, in which case code generation is performed
>> during the link stage, which should adhere to the same rules as the
>> compiler. This not only applies to -O, but also to things like
>> -march/-mcpu and -mstrict-align. This is why we pass all CFLAGS to the
>> linker for the GCC5 LTO builds.
>>
>>> But then: is the patch *complete*? Because I can see some more DLINK
>>> stuff, for IA32 and X64 (not just ARM and AARCH64). Is it okay to ignore
>>> those? For example:
>>>
>>> *_GCC5_IA32_DLINK_FLAGS          = DEF(GCC5_IA32_X64_DLINK_FLAGS) -Os
>>> -Wl,-m,elf_i386,--oformat=elf32-i386
>>>
>>>
>>> *_GCC5_X64_DLINK_FLAGS           = DEF(GCC5_X64_DLINK_FLAGS) -Os
>>>
>>> Where GCC5_X64_DLINK_FLAGS and GCC5_IA32_X64_DLINK_FLAGS even include
>>> -flto. (I don't know if "-flto" hampers source level debugging or not.)
>>>
>>
>> The GCC man page documents -flto as being a bad idea, i.e.,
>>
>> """
>> Link-time optimization does not work well with generation of debugging
>> information.  Combining -flto with -g is currently experimental and
>> expected to produce unexpected results.
>> """
>>
>> (which raises a philosophical question as well, i.e., to which extent
>> expected unexpected results are still unexpected results. But I
>> digress ...)
>>
>> Another note: the DEBUG build for ARM and AARCH64 is essentially NOOPT
>> already, not DEBUG. How does this patch intend to deal with that?
>
> It just copies the DEBUG settings to NOOPT (via deep copy, not by
> reference). I believe that's OK.
>

OK, fair enough. Leif and I can look into this in the future.