On 07/22/20 13:03, Laszlo Ersek wrote:
> Hi Pierre,
>
> On 07/07/20 10:35, PierreGondois wrote:
>> From: Pierre Gondois <[email protected]>
>>
>> GCC48_ALL_CC_FLAGS has no dependency on GCC_ALL_CC_FLAGS.
>> By definition, there should be such dependency.
>>
>> The outcomes of this patch is that GCC48_ALL_CC_FLAGS and
>> other dependent configurations will inherit from the
>> additional "-Os" flag.
>> The "-Os" flag optimizes a build in size, not breaking any
>> build. In a gcc command line, the last optimization flag
>> has precedence. This means that this "-Os" flag will be
>> overriden by a more specific optimization configuration,
>> provided that this more specific flag is appended at the
>> end of the CC_FLAGS.
>>
>> Signed-off-by: Pierre Gondois <[email protected]>
>> Suggested-by: Tomas Pilar <[email protected]>
>> ---
>>
>> The changes can be seen at:
>> https://github.com/PierreARM/edk2/commits/831_Add_gcc_flag_warning_v2
>>
>> Notes:
>> v2:
>> - Make GCC48_ALL_CC_FLAGS dependent on
>> GCC_ALL_CC_FLAGS. [Tomas]
>>
>> BaseTools/Conf/tools_def.template | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template
>> b/BaseTools/Conf/tools_def.template
>> index
>> 397b011ba38f97f81f314f8641ac8bb95d5a2197..a1fd27b1adba8769949b7d628d7fbed49fe24267
>> 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -1952,7 +1952,7 @@ DEFINE GCC_RISCV64_RC_FLAGS = -I binary -O
>> elf64-littleriscv -B riscv
>> # GCC Build Flag for included header file list generation
>> DEFINE GCC_DEPS_FLAGS = -MMD -MF [email protected]
>>
>> -DEFINE GCC48_ALL_CC_FLAGS = -g -fshort-wchar -fno-builtin
>> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections
>> -fdata-sections -include AutoGen.h -fno-common
>> -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
>> +DEFINE GCC48_ALL_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS)
>> -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
>> DEFINE GCC48_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z
>> common-page-size=0x20
>> DEFINE GCC48_IA32_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m32
>> -march=i586 -malign-double -fno-stack-protector -D EFI32
>> -fno-asynchronous-unwind-tables -Wno-address
>> DEFINE GCC48_X64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS) -m64
>> -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))"
>> -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie
>> -fno-asynchronous-unwind-tables -Wno-address
>>
>
> As the commit message states, this change makes GCC48_ALL_CC_FLAGS inherit
> "-Os".
>
> It is true that all the NOOPT_GCC flags override "-Os" with "-O0":
>
> NOOPT_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -O0
> NOOPT_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -O0
> NOOPT_GCC48_ARM_CC_FLAGS = DEF(GCC48_ARM_CC_FLAGS) -O0
> NOOPT_GCC48_AARCH64_CC_FLAGS = DEF(GCC48_AARCH64_CC_FLAGS) -O0
> NOOPT_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -O0
> NOOPT_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -O0
> NOOPT_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -O0
> NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0
> NOOPT_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -O0
> NOOPT_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -O0
> NOOPT_GCC5_ARM_CC_FLAGS = DEF(GCC5_ARM_CC_FLAGS) -O0
> NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0
>
> However, *some* of the DEBUG and RELEASE flags now have two "-Os" flags:
>
> DEBUG_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os
> RELEASE_GCC48_IA32_CC_FLAGS = DEF(GCC48_IA32_CC_FLAGS) -Os
> -Wno-unused-but-set-variable
> DEBUG_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os
> RELEASE_GCC48_X64_CC_FLAGS = DEF(GCC48_X64_CC_FLAGS) -Os
> -Wno-unused-but-set-variable
> DEBUG_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os
> RELEASE_GCC49_IA32_CC_FLAGS = DEF(GCC49_IA32_CC_FLAGS) -Os
> -Wno-unused-but-set-variable -Wno-unused-const-variable
> DEBUG_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os
> RELEASE_GCC49_X64_CC_FLAGS = DEF(GCC49_X64_CC_FLAGS) -Os
> -Wno-unused-but-set-variable -Wno-unused-const-variable
> DEBUG_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os
> RELEASE_GCC5_IA32_CC_FLAGS = DEF(GCC5_IA32_CC_FLAGS) -flto -Os
> -Wno-unused-but-set-variable -Wno-unused-const-variable
> DEBUG_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO
> -Os
> RELEASE_GCC5_X64_CC_FLAGS = DEF(GCC5_X64_CC_FLAGS) -flto -DUSING_LTO
> -Os -Wno-unused-but-set-variable -Wno-unused-const-variable
>
> (The ARM and AARCH64 DEBUG/RELEASE GCC options don't seem to be affected, as
> they have relied on inherited -- not open-coded -- "-Os" options from much
> earlier. So now they do not suffer from this duplication.)
We do have some AARCH64 duplication here:
DEFINE GCC49_AARCH64_CC_FLAGS = DEF(GCC48_ALL_CC_FLAGS)
DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small
because GCC48_ALL_CC_FLAGS is now based on GCC_ALL_CC_FLAGS, and
GCC_AARCH64_CC_FLAGS is *also* based on GCC_ALL_CC_FLAGS.
However, this duplication is not introduced by this patch -- it goes back to
commit 63e1c23b2255 ("BaseTools/AARCH64: use large code model for GCC <= 4.8",
2015-10-02). So it's a separate topic.
Thanks
Laszlo
>
> The point of this patch was a kind of "normalization", so I think the work
> isn't complete until the duplication is undone, i.e., the explicit "-Os" flag
> is removed from the last twelve defines.
>
> Can you submit a follow-up patch please?
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#63109): https://edk2.groups.io/g/devel/message/63109
Mute This Topic: https://groups.io/mt/75351533/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
