[PATCH 1/2] BaseTools/tools_def GCC: ARM/AARCH64: replace -save-temps with -pipe

[PATCH 1/2] BaseTools/tools_def GCC: ARM/AARCH64: replace -save-temps with -pipe

[Ard Biesheuvel]

For historical reasons, GCC builds for ARM and AARCH64 pass the
-save-temps command line option to GCC, which instructs the compiler
to preserve intermediate files, i.e., preprocessor output and generated
assembler. Given that this clutters up the Build directory, and slows
down the build, let's replace it with -pipe, which explicitly tells the
compiler to keep all intermediate representations in memory only.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 BaseTools/Conf/tools_def.template | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 04a1bcb210ab..7a58ce365ed2 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -4399,7 +4399,7 @@ DEFINE GCC46_X64_DLINK_FLAGS         = DEF(GCC45_X64_DLINK_FLAGS)
 DEFINE GCC46_X64_DLINK2_FLAGS        = DEF(GCC45_X64_DLINK2_FLAGS)
 DEFINE GCC46_ASM_FLAGS               = DEF(GCC45_ASM_FLAGS)
 DEFINE GCC46_ARM_ASM_FLAGS           = $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ASM_FLAGS) -mlittle-endian
-DEFINE GCC46_ARM_CC_FLAGS            = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ARM_CC_FLAGS) -fstack-protector -mword-relocations -save-temps
+DEFINE GCC46_ARM_CC_FLAGS            = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ARM_CC_FLAGS) -fstack-protector -mword-relocations -pipe
 DEFINE GCC46_ARM_CC_XIPFLAGS         = -D__ARM_FEATURE_UNALIGNED=0
 DEFINE GCC46_ARM_DLINK_FLAGS         = DEF(GCC_ARM_DLINK_FLAGS) -Wl,--oformat=elf32-littlearm
 DEFINE GCC46_ARM_DLINK2_FLAGS        = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
@@ -4418,7 +4418,7 @@ DEFINE GCC47_ARM_ASM_FLAGS           = DEF(GCC46_ARM_ASM_FLAGS)
 DEFINE GCC47_AARCH64_ASM_FLAGS       = $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ASM_FLAGS) -mlittle-endian
 DEFINE GCC47_ARM_CC_FLAGS            = DEF(GCC46_ARM_CC_FLAGS)
 DEFINE GCC47_ARM_CC_XIPFLAGS         = DEF(GCC_ARM_CC_XIPFLAGS)
-DEFINE GCC47_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) -mcmodel=large DEF(GCC_AARCH64_CC_FLAGS) -save-temps
+DEFINE GCC47_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) -mcmodel=large DEF(GCC_AARCH64_CC_FLAGS) -pipe
 DEFINE GCC47_AARCH64_CC_XIPFLAGS     = DEF(GCC_AARCH64_CC_XIPFLAGS)
 DEFINE GCC47_ARM_DLINK_FLAGS         = DEF(GCC46_ARM_DLINK_FLAGS)
 DEFINE GCC47_ARM_DLINK2_FLAGS        = DEF(GCC46_ARM_DLINK2_FLAGS)
@@ -4462,7 +4462,7 @@ DEFINE GCC49_ARM_ASM_FLAGS           = DEF(GCC48_ARM_ASM_FLAGS)
 DEFINE GCC49_AARCH64_ASM_FLAGS       = DEF(GCC48_AARCH64_ASM_FLAGS)
 DEFINE GCC49_ARM_CC_FLAGS            = DEF(GCC48_ARM_CC_FLAGS)
 DEFINE GCC49_ARM_CC_XIPFLAGS         = DEF(GCC48_ARM_CC_XIPFLAGS)
-DEFINE GCC49_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -save-temps
+DEFINE GCC49_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -pipe
 DEFINE GCC49_AARCH64_CC_XIPFLAGS     = DEF(GCC48_AARCH64_CC_XIPFLAGS)
 DEFINE GCC49_ARM_DLINK_FLAGS         = DEF(GCC48_ARM_DLINK_FLAGS)
 DEFINE GCC49_ARM_DLINK2_FLAGS        = DEF(GCC48_ARM_DLINK2_FLAGS)
-- 
2.7.4

[PATCH 2/2] BaseTools/tools_def: AARCH64: disable LTO type mismatch warnings

[Ard Biesheuvel]

On AARCH64, any code that may execute with the MMU off needs to be built
with -mstrict-align, given that unaligned accesses are not allowed unless
the MMU is enabled. This does not only affect SEC and PEI modules, but
also static libraries of the BASE type, which may be linked into such
modules, as well as into modules of other types. As it turns out, the
presence of -mstrict-align is reflected in the internal representations
of the types defined in those libraries.

When -fstrict-aliasing is passed to GCC, it assumes that pointers to
objects of different types cannot refer to the same memory location, and
attempts to exploit this fact when optimizing the code. Since such
assumptions are only valid under very strict conditions which are not
guaranteed to be met in EDK2, we disable this optimization by passing
-fno-strict-aliasing by default.

When LTO is in effect, this applies equally to the code generation that
may occur at link time, which is why the linker warns about unexpected
differences in type definitions between the intermediate representations
that are present in the object files being linked. This may result in
warnings such as the one below, even if -fno-strict-aliasing is used:

  MdePkg/Include/Library/BaseLib.h:1712:1:
  warning: type of 'StrToGuid' does not match original declaration
  [-Wlto-type-mismatch]
   StrToGuid (
   ^
  MdePkg/Library/BaseLib/SafeString.c:1506:1:
  note: 'StrToGuid' was previously declared here
   StrToGuid (
   ^
  MdePkg/Library/BaseLib/SafeString.c:1506:1:
  note: code may be misoptimized unless -fno-strict-aliasing is used

This warning is inadvertently triggered when linking BASE libraries built
with -mstrict-align into modules of types other than SEC or PEI, since the
types are subtly different, even though the use of code that maintains
strict alignment in a module that does not care about this is unlikely to
cause problems. And even if it did, it would still only affect code built
with -fstrict-aliasing enabled, which we disable unconditionally. So let's
just silence the warning by passing -Wno-lto-type-mismatch.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 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 7a58ce365ed2..9a3ba9defb12 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -5407,7 +5407,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS     = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
   DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
 
 RELEASE_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -mcmodel=tiny -fomit-frame-pointer
-RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64
+RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
 
   NOOPT_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
   NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
-- 
2.7.4

Re: [PATCH 2/2] BaseTools/tools_def: AARCH64: disable LTO type mismatch warnings

[Laszlo Ersek]

On 06/20/17 20:43, Ard Biesheuvel wrote:
> On AARCH64, any code that may execute with the MMU off needs to be built
> with -mstrict-align, given that unaligned accesses are not allowed unless
> the MMU is enabled. This does not only affect SEC and PEI modules, but
> also static libraries of the BASE type, which may be linked into such
> modules, as well as into modules of other types. As it turns out, the
> presence of -mstrict-align is reflected in the internal representations
> of the types defined in those libraries.
> 
> When -fstrict-aliasing is passed to GCC, it assumes that pointers to
> objects of different types cannot refer to the same memory location, and
> attempts to exploit this fact when optimizing the code. Since such
> assumptions are only valid under very strict conditions which are not
> guaranteed to be met in EDK2, we disable this optimization by passing
> -fno-strict-aliasing by default.
> 
> When LTO is in effect, this applies equally to the code generation that
> may occur at link time, which is why the linker warns about unexpected
> differences in type definitions between the intermediate representations
> that are present in the object files being linked. This may result in
> warnings such as the one below, even if -fno-strict-aliasing is used:
> 
>   MdePkg/Include/Library/BaseLib.h:1712:1:
>   warning: type of 'StrToGuid' does not match original declaration
>   [-Wlto-type-mismatch]
>    StrToGuid (
>    ^
>   MdePkg/Library/BaseLib/SafeString.c:1506:1:
>   note: 'StrToGuid' was previously declared here
>    StrToGuid (
>    ^
>   MdePkg/Library/BaseLib/SafeString.c:1506:1:
>   note: code may be misoptimized unless -fno-strict-aliasing is used
> 
> This warning is inadvertently triggered when linking BASE libraries built
> with -mstrict-align into modules of types other than SEC or PEI, since the
> types are subtly different, even though the use of code that maintains
> strict alignment in a module that does not care about this is unlikely to
> cause problems. And even if it did, it would still only affect code built
> with -fstrict-aliasing enabled, which we disable unconditionally. So let's
> just silence the warning by passing -Wno-lto-type-mismatch.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  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 7a58ce365ed2..9a3ba9defb12 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -5407,7 +5407,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS     = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
>    DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>  
>  RELEASE_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -mcmodel=tiny -fomit-frame-pointer
> -RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64
> +RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
>  
>    NOOPT_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
>    NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 1/2] BaseTools/tools_def GCC: ARM/AARCH64: replace -save-temps with -pipe

[Laszlo Ersek]

On 06/20/17 20:43, Ard Biesheuvel wrote:
> For historical reasons, GCC builds for ARM and AARCH64 pass the
> -save-temps command line option to GCC, which instructs the compiler
> to preserve intermediate files, i.e., preprocessor output and generated
> assembler. Given that this clutters up the Build directory, and slows
> down the build, let's replace it with -pipe, which explicitly tells the
> compiler to keep all intermediate representations in memory only.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Conf/tools_def.template | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index 04a1bcb210ab..7a58ce365ed2 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -4399,7 +4399,7 @@ DEFINE GCC46_X64_DLINK_FLAGS         = DEF(GCC45_X64_DLINK_FLAGS)
>  DEFINE GCC46_X64_DLINK2_FLAGS        = DEF(GCC45_X64_DLINK2_FLAGS)
>  DEFINE GCC46_ASM_FLAGS               = DEF(GCC45_ASM_FLAGS)
>  DEFINE GCC46_ARM_ASM_FLAGS           = $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ASM_FLAGS) -mlittle-endian
> -DEFINE GCC46_ARM_CC_FLAGS            = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ARM_CC_FLAGS) -fstack-protector -mword-relocations -save-temps
> +DEFINE GCC46_ARM_CC_FLAGS            = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ARM_CC_FLAGS) -fstack-protector -mword-relocations -pipe
>  DEFINE GCC46_ARM_CC_XIPFLAGS         = -D__ARM_FEATURE_UNALIGNED=0
>  DEFINE GCC46_ARM_DLINK_FLAGS         = DEF(GCC_ARM_DLINK_FLAGS) -Wl,--oformat=elf32-littlearm
>  DEFINE GCC46_ARM_DLINK2_FLAGS        = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
> @@ -4418,7 +4418,7 @@ DEFINE GCC47_ARM_ASM_FLAGS           = DEF(GCC46_ARM_ASM_FLAGS)
>  DEFINE GCC47_AARCH64_ASM_FLAGS       = $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ASM_FLAGS) -mlittle-endian
>  DEFINE GCC47_ARM_CC_FLAGS            = DEF(GCC46_ARM_CC_FLAGS)
>  DEFINE GCC47_ARM_CC_XIPFLAGS         = DEF(GCC_ARM_CC_XIPFLAGS)
> -DEFINE GCC47_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) -mcmodel=large DEF(GCC_AARCH64_CC_FLAGS) -save-temps
> +DEFINE GCC47_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) -mcmodel=large DEF(GCC_AARCH64_CC_FLAGS) -pipe
>  DEFINE GCC47_AARCH64_CC_XIPFLAGS     = DEF(GCC_AARCH64_CC_XIPFLAGS)
>  DEFINE GCC47_ARM_DLINK_FLAGS         = DEF(GCC46_ARM_DLINK_FLAGS)
>  DEFINE GCC47_ARM_DLINK2_FLAGS        = DEF(GCC46_ARM_DLINK2_FLAGS)
> @@ -4462,7 +4462,7 @@ DEFINE GCC49_ARM_ASM_FLAGS           = DEF(GCC48_ARM_ASM_FLAGS)
>  DEFINE GCC49_AARCH64_ASM_FLAGS       = DEF(GCC48_AARCH64_ASM_FLAGS)
>  DEFINE GCC49_ARM_CC_FLAGS            = DEF(GCC48_ARM_CC_FLAGS)
>  DEFINE GCC49_ARM_CC_XIPFLAGS         = DEF(GCC48_ARM_CC_XIPFLAGS)
> -DEFINE GCC49_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -save-temps
> +DEFINE GCC49_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -pipe
>  DEFINE GCC49_AARCH64_CC_XIPFLAGS     = DEF(GCC48_AARCH64_CC_XIPFLAGS)
>  DEFINE GCC49_ARM_DLINK_FLAGS         = DEF(GCC48_ARM_DLINK_FLAGS)
>  DEFINE GCC49_ARM_DLINK2_FLAGS        = DEF(GCC48_ARM_DLINK2_FLAGS)
> 

"-pipe" isn't used anywhere else in "tools_def.template". Can we imagine
a platform where cross-compiling to aarch64 with gcc works otherwise,
but "-pipe" might break that? Cygwin perhaps? (No clue, honestly.)

For consistency with the rest of "tools_def.template", I'd suggest
simply dropping "-save-temps", and thinking about "-pipe" separately
(and then for all the GCC toolchains and for all arches). But, I don't
feel particularly strongly about this.

Thanks
Laszlo

Re: [PATCH 2/2] BaseTools/tools_def: AARCH64: disable LTO type mismatch warnings

[Leif Lindholm]

On Tue, Jun 20, 2017 at 08:43:54PM +0200, Ard Biesheuvel wrote:
> On AARCH64, any code that may execute with the MMU off needs to be built
> with -mstrict-align, given that unaligned accesses are not allowed unless
> the MMU is enabled. This does not only affect SEC and PEI modules, but
> also static libraries of the BASE type, which may be linked into such
> modules, as well as into modules of other types. As it turns out, the
> presence of -mstrict-align is reflected in the internal representations
> of the types defined in those libraries.
> 
> When -fstrict-aliasing is passed to GCC, it assumes that pointers to
> objects of different types cannot refer to the same memory location, and
> attempts to exploit this fact when optimizing the code. Since such
> assumptions are only valid under very strict conditions which are not
> guaranteed to be met in EDK2, we disable this optimization by passing
> -fno-strict-aliasing by default.

Just a comment - you don't necessarily have to update this excellent
commit mesage, but:
-fstrict-aliasing is GCC default, because it is a restriction in the C
language. Because it's a bit non-obvious, things can go hilariously
wrong in very non-obvious ways, and the potential optimization gains
are ulikely to be generally relevant, -fno-strict-aliasing is a
sensible thing to always have set (like we do).

> When LTO is in effect, this applies equally to the code generation that
> may occur at link time, which is why the linker warns about unexpected
> differences in type definitions between the intermediate representations
> that are present in the object files being linked. This may result in
> warnings such as the one below, even if -fno-strict-aliasing is used:
> 
>   MdePkg/Include/Library/BaseLib.h:1712:1:
>   warning: type of 'StrToGuid' does not match original declaration
>   [-Wlto-type-mismatch]
>    StrToGuid (
>    ^
>   MdePkg/Library/BaseLib/SafeString.c:1506:1:
>   note: 'StrToGuid' was previously declared here
>    StrToGuid (
>    ^
>   MdePkg/Library/BaseLib/SafeString.c:1506:1:
>   note: code may be misoptimized unless -fno-strict-aliasing is used
> 
> This warning is inadvertently triggered when linking BASE libraries built
> with -mstrict-align into modules of types other than SEC or PEI, since the
> types are subtly different, even though the use of code that maintains
> strict alignment in a module that does not care about this is unlikely to
> cause problems. And even if it did, it would still only affect code built
> with -fstrict-aliasing enabled, which we disable unconditionally. So let's
> just silence the warning by passing -Wno-lto-type-mismatch.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  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 7a58ce365ed2..9a3ba9defb12 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -5407,7 +5407,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS     = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
>    DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>  
>  RELEASE_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -mcmodel=tiny -fomit-frame-pointer
> -RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64
> +RELEASE_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch
>  
>    NOOPT_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small
>    NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
> -- 
> 2.7.4
> 

Re: [PATCH 2/2] BaseTools/tools_def: AARCH64: disable LTO type mismatch warnings

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>Sent: Wednesday, June 21, 2017 4:00 AM
>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Zhu,
>Yonghong <yonghong.zhu@intel.com>; lersek@redhat.com
>Subject: Re: [PATCH 2/2] BaseTools/tools_def: AARCH64: disable LTO type
>mismatch warnings
>
>On Tue, Jun 20, 2017 at 08:43:54PM +0200, Ard Biesheuvel wrote:
>> On AARCH64, any code that may execute with the MMU off needs to be
>built
>> with -mstrict-align, given that unaligned accesses are not allowed unless
>> the MMU is enabled. This does not only affect SEC and PEI modules, but
>> also static libraries of the BASE type, which may be linked into such
>> modules, as well as into modules of other types. As it turns out, the
>> presence of -mstrict-align is reflected in the internal representations
>> of the types defined in those libraries.
>>
>> When -fstrict-aliasing is passed to GCC, it assumes that pointers to
>> objects of different types cannot refer to the same memory location, and
>> attempts to exploit this fact when optimizing the code. Since such
>> assumptions are only valid under very strict conditions which are not
>> guaranteed to be met in EDK2, we disable this optimization by passing
>> -fno-strict-aliasing by default.
>
>Just a comment - you don't necessarily have to update this excellent
>commit mesage, but:
>-fstrict-aliasing is GCC default, because it is a restriction in the C
>language. Because it's a bit non-obvious, things can go hilariously
>wrong in very non-obvious ways, and the potential optimization gains
>are ulikely to be generally relevant, -fno-strict-aliasing is a
>sensible thing to always have set (like we do).
>
>> When LTO is in effect, this applies equally to the code generation that
>> may occur at link time, which is why the linker warns about unexpected
>> differences in type definitions between the intermediate representations
>> that are present in the object files being linked. This may result in
>> warnings such as the one below, even if -fno-strict-aliasing is used:
>>
>>   MdePkg/Include/Library/BaseLib.h:1712:1:
>>   warning: type of 'StrToGuid' does not match original declaration
>>   [-Wlto-type-mismatch]
>>    StrToGuid (
>>    ^
>>   MdePkg/Library/BaseLib/SafeString.c:1506:1:
>>   note: 'StrToGuid' was previously declared here
>>    StrToGuid (
>>    ^
>>   MdePkg/Library/BaseLib/SafeString.c:1506:1:
>>   note: code may be misoptimized unless -fno-strict-aliasing is used
>>
>> This warning is inadvertently triggered when linking BASE libraries built
>> with -mstrict-align into modules of types other than SEC or PEI, since the
>> types are subtly different, even though the use of code that maintains
>> strict alignment in a module that does not care about this is unlikely to
>> cause problems. And even if it did, it would still only affect code built
>> with -fstrict-aliasing enabled, which we disable unconditionally. So let's
>> just silence the warning by passing -Wno-lto-type-mismatch.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
>Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
>> ---
>>  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 7a58ce365ed2..9a3ba9defb12 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -5407,7 +5407,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS     =
>DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
>>    DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>>
>>  RELEASE_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -
>flto -Wno-unused-but-set-variable -mcmodel=tiny -fomit-frame-pointer
>> -RELEASE_GCC5_AARCH64_DLINK_FLAGS =
>DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -
>L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-
>through=-llto-aarch64
>> +RELEASE_GCC5_AARCH64_DLINK_FLAGS =
>DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -
>L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-
>through=-llto-aarch64 -Wno-lto-type-mismatch
>>
>>    NOOPT_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -
>O0 -mcmodel=small
>>    NOOPT_GCC5_AARCH64_DLINK_FLAGS =
>DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
>> --
>> 2.7.4
>>

Re: [PATCH 1/2] BaseTools/tools_def GCC: ARM/AARCH64: replace -save-temps with -pipe

[Ard Biesheuvel]

On 20 June 2017 at 21:28, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/20/17 20:43, Ard Biesheuvel wrote:
>> For historical reasons, GCC builds for ARM and AARCH64 pass the
>> -save-temps command line option to GCC, which instructs the compiler
>> to preserve intermediate files, i.e., preprocessor output and generated
>> assembler. Given that this clutters up the Build directory, and slows
>> down the build, let's replace it with -pipe, which explicitly tells the
>> compiler to keep all intermediate representations in memory only.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  BaseTools/Conf/tools_def.template | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
>> index 04a1bcb210ab..7a58ce365ed2 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -4399,7 +4399,7 @@ DEFINE GCC46_X64_DLINK_FLAGS         = DEF(GCC45_X64_DLINK_FLAGS)
>>  DEFINE GCC46_X64_DLINK2_FLAGS        = DEF(GCC45_X64_DLINK2_FLAGS)
>>  DEFINE GCC46_ASM_FLAGS               = DEF(GCC45_ASM_FLAGS)
>>  DEFINE GCC46_ARM_ASM_FLAGS           = $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ASM_FLAGS) -mlittle-endian
>> -DEFINE GCC46_ARM_CC_FLAGS            = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ARM_CC_FLAGS) -fstack-protector -mword-relocations -save-temps
>> +DEFINE GCC46_ARM_CC_FLAGS            = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ARM_CC_FLAGS) -fstack-protector -mword-relocations -pipe
>>  DEFINE GCC46_ARM_CC_XIPFLAGS         = -D__ARM_FEATURE_UNALIGNED=0
>>  DEFINE GCC46_ARM_DLINK_FLAGS         = DEF(GCC_ARM_DLINK_FLAGS) -Wl,--oformat=elf32-littlearm
>>  DEFINE GCC46_ARM_DLINK2_FLAGS        = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
>> @@ -4418,7 +4418,7 @@ DEFINE GCC47_ARM_ASM_FLAGS           = DEF(GCC46_ARM_ASM_FLAGS)
>>  DEFINE GCC47_AARCH64_ASM_FLAGS       = $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_ASM_FLAGS) -mlittle-endian
>>  DEFINE GCC47_ARM_CC_FLAGS            = DEF(GCC46_ARM_CC_FLAGS)
>>  DEFINE GCC47_ARM_CC_XIPFLAGS         = DEF(GCC_ARM_CC_XIPFLAGS)
>> -DEFINE GCC47_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) -mcmodel=large DEF(GCC_AARCH64_CC_FLAGS) -save-temps
>> +DEFINE GCC47_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) -mcmodel=large DEF(GCC_AARCH64_CC_FLAGS) -pipe
>>  DEFINE GCC47_AARCH64_CC_XIPFLAGS     = DEF(GCC_AARCH64_CC_XIPFLAGS)
>>  DEFINE GCC47_ARM_DLINK_FLAGS         = DEF(GCC46_ARM_DLINK_FLAGS)
>>  DEFINE GCC47_ARM_DLINK2_FLAGS        = DEF(GCC46_ARM_DLINK2_FLAGS)
>> @@ -4462,7 +4462,7 @@ DEFINE GCC49_ARM_ASM_FLAGS           = DEF(GCC48_ARM_ASM_FLAGS)
>>  DEFINE GCC49_AARCH64_ASM_FLAGS       = DEF(GCC48_AARCH64_ASM_FLAGS)
>>  DEFINE GCC49_ARM_CC_FLAGS            = DEF(GCC48_ARM_CC_FLAGS)
>>  DEFINE GCC49_ARM_CC_XIPFLAGS         = DEF(GCC48_ARM_CC_XIPFLAGS)
>> -DEFINE GCC49_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -save-temps
>> +DEFINE GCC49_AARCH64_CC_FLAGS        = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -pipe
>>  DEFINE GCC49_AARCH64_CC_XIPFLAGS     = DEF(GCC48_AARCH64_CC_XIPFLAGS)
>>  DEFINE GCC49_ARM_DLINK_FLAGS         = DEF(GCC48_ARM_DLINK_FLAGS)
>>  DEFINE GCC49_ARM_DLINK2_FLAGS        = DEF(GCC48_ARM_DLINK2_FLAGS)
>>
>
> "-pipe" isn't used anywhere else in "tools_def.template". Can we imagine
> a platform where cross-compiling to aarch64 with gcc works otherwise,
> but "-pipe" might break that? Cygwin perhaps? (No clue, honestly.)
>
> For consistency with the rest of "tools_def.template", I'd suggest
> simply dropping "-save-temps", and thinking about "-pipe" separately
> (and then for all the GCC toolchains and for all arches). But, I don't
> feel particularly strongly about this.
>

Yeah, that's a fair point. The fact that -save-temps is specific to
ARM does not mean we should perpetuate that with -pipe.

Leif, if you agree, I will drop the addition of -pipe from this patch,
and we can revisit it later for all GCC flavors.

-- 
Ard.

Re: [PATCH 1/2] BaseTools/tools_def GCC: ARM/AARCH64: replace -save-temps with -pipe

[Leif Lindholm]

On Wed, Jun 21, 2017 at 01:31:15PM +0200, Ard Biesheuvel wrote:
> > "-pipe" isn't used anywhere else in "tools_def.template". Can we imagine
> > a platform where cross-compiling to aarch64 with gcc works otherwise,
> > but "-pipe" might break that? Cygwin perhaps? (No clue, honestly.)
> >
> > For consistency with the rest of "tools_def.template", I'd suggest
> > simply dropping "-save-temps", and thinking about "-pipe" separately
> > (and then for all the GCC toolchains and for all arches). But, I don't
> > feel particularly strongly about this.
> >
> 
> Yeah, that's a fair point. The fact that -save-temps is specific to
> ARM does not mean we should perpetuate that with -pipe.
> 
> Leif, if you agree, I will drop the addition of -pipe from this patch,
> and we can revisit it later for all GCC flavors.

Much as I would like to find out if there really are any platforms
that still have issues with -pipe (in which case, why wouldn't GCC
just use temp files anyway?), I guess that is a sensible approach.

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
for the dropping of -save-temps.

/
    Leif

Re: [PATCH 1/2] BaseTools/tools_def GCC: ARM/AARCH64: replace -save-temps with -pipe

[Ard Biesheuvel]

On 21 June 2017 at 12:42, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Jun 21, 2017 at 01:31:15PM +0200, Ard Biesheuvel wrote:
>> > "-pipe" isn't used anywhere else in "tools_def.template". Can we imagine
>> > a platform where cross-compiling to aarch64 with gcc works otherwise,
>> > but "-pipe" might break that? Cygwin perhaps? (No clue, honestly.)
>> >
>> > For consistency with the rest of "tools_def.template", I'd suggest
>> > simply dropping "-save-temps", and thinking about "-pipe" separately
>> > (and then for all the GCC toolchains and for all arches). But, I don't
>> > feel particularly strongly about this.
>> >
>>
>> Yeah, that's a fair point. The fact that -save-temps is specific to
>> ARM does not mean we should perpetuate that with -pipe.
>>
>> Leif, if you agree, I will drop the addition of -pipe from this patch,
>> and we can revisit it later for all GCC flavors.
>
> Much as I would like to find out if there really are any platforms
> that still have issues with -pipe (in which case, why wouldn't GCC
> just use temp files anyway?), I guess that is a sensible approach.
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> for the dropping of -save-temps.
>

Thanks, pushed as fa6080138c08

Re: [PATCH 2/2] BaseTools/tools_def: AARCH64: disable LTO type mismatch warnings

[Ard Biesheuvel]

On 21 June 2017 at 02:11, Gao, Liming <liming.gao@intel.com> wrote:
> Reviewed-by: Liming Gao <liming.gao@intel.com>
>

Thanks, pushed as 9ba8baae15ca


>>-----Original Message-----
>>From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
>>Sent: Wednesday, June 21, 2017 4:00 AM
>>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>Cc: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Zhu,
>>Yonghong <yonghong.zhu@intel.com>; lersek@redhat.com
>>Subject: Re: [PATCH 2/2] BaseTools/tools_def: AARCH64: disable LTO type
>>mismatch warnings
>>
>>On Tue, Jun 20, 2017 at 08:43:54PM +0200, Ard Biesheuvel wrote:
>>> On AARCH64, any code that may execute with the MMU off needs to be
>>built
>>> with -mstrict-align, given that unaligned accesses are not allowed unless
>>> the MMU is enabled. This does not only affect SEC and PEI modules, but
>>> also static libraries of the BASE type, which may be linked into such
>>> modules, as well as into modules of other types. As it turns out, the
>>> presence of -mstrict-align is reflected in the internal representations
>>> of the types defined in those libraries.
>>>
>>> When -fstrict-aliasing is passed to GCC, it assumes that pointers to
>>> objects of different types cannot refer to the same memory location, and
>>> attempts to exploit this fact when optimizing the code. Since such
>>> assumptions are only valid under very strict conditions which are not
>>> guaranteed to be met in EDK2, we disable this optimization by passing
>>> -fno-strict-aliasing by default.
>>
>>Just a comment - you don't necessarily have to update this excellent
>>commit mesage, but:
>>-fstrict-aliasing is GCC default, because it is a restriction in the C
>>language. Because it's a bit non-obvious, things can go hilariously
>>wrong in very non-obvious ways, and the potential optimization gains
>>are ulikely to be generally relevant, -fno-strict-aliasing is a
>>sensible thing to always have set (like we do).
>>
>>> When LTO is in effect, this applies equally to the code generation that
>>> may occur at link time, which is why the linker warns about unexpected
>>> differences in type definitions between the intermediate representations
>>> that are present in the object files being linked. This may result in
>>> warnings such as the one below, even if -fno-strict-aliasing is used:
>>>
>>>   MdePkg/Include/Library/BaseLib.h:1712:1:
>>>   warning: type of 'StrToGuid' does not match original declaration
>>>   [-Wlto-type-mismatch]
>>>    StrToGuid (
>>>    ^
>>>   MdePkg/Library/BaseLib/SafeString.c:1506:1:
>>>   note: 'StrToGuid' was previously declared here
>>>    StrToGuid (
>>>    ^
>>>   MdePkg/Library/BaseLib/SafeString.c:1506:1:
>>>   note: code may be misoptimized unless -fno-strict-aliasing is used
>>>
>>> This warning is inadvertently triggered when linking BASE libraries built
>>> with -mstrict-align into modules of types other than SEC or PEI, since the
>>> types are subtly different, even though the use of code that maintains
>>> strict alignment in a module that does not care about this is unlikely to
>>> cause problems. And even if it did, it would still only affect code built
>>> with -fstrict-aliasing enabled, which we disable unconditionally. So let's
>>> just silence the warning by passing -Wno-lto-type-mismatch.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>>Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>>> ---
>>>  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 7a58ce365ed2..9a3ba9defb12 100755
>>> --- a/BaseTools/Conf/tools_def.template
>>> +++ b/BaseTools/Conf/tools_def.template
>>> @@ -5407,7 +5407,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS     =
>>DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
>>>    DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>>>
>>>  RELEASE_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -
>>flto -Wno-unused-but-set-variable -mcmodel=tiny -fomit-frame-pointer
>>> -RELEASE_GCC5_AARCH64_DLINK_FLAGS =
>>DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -
>>L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-
>>through=-llto-aarch64
>>> +RELEASE_GCC5_AARCH64_DLINK_FLAGS =
>>DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -
>>L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-
>>through=-llto-aarch64 -Wno-lto-type-mismatch
>>>
>>>    NOOPT_GCC5_AARCH64_CC_FLAGS    = DEF(GCC5_AARCH64_CC_FLAGS) -
>>O0 -mcmodel=small
>>>    NOOPT_GCC5_AARCH64_DLINK_FLAGS =
>>DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0
>>> --
>>> 2.7.4
>>>