Re: [PATCH] BaseTools/tools_def: add CLANG38 LTO versions for AARCH64 and ARM

[Laszlo Ersek <lersek@redhat.com>]

On 12/06/17 15:16, Ard Biesheuvel wrote:
> On 6 December 2017 at 14:12, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 12/06/17 12:39, Ard Biesheuvel wrote:
>>> Extend the CLANG38 toolchain definition so it can be used for
>>> ARM and AARCH64 as well. Note that this requires llvm-ar and
>>> the LLVMgold.so linker plugin.
>>>
>>> In preparation of doing the same for GCC5, this toolchain version
>>> also departs from the custom of using -O0 for DEBUG builds, which
>>> makes them needlessly slow. Instead, let's add a NOOPT flavor as
>>> well, and enable optimization for DEBUG like the other architectures
>>> do. (Note that this will require some trivial changes to the platform
>>> description files)
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  BaseTools/Conf/tools_def.template | 98 +++++++++++++++++++-
>>>  1 file changed, 95 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
>>> index 91b135c2e569..6ee720d7586e 100755
>>> --- a/BaseTools/Conf/tools_def.template
>>> +++ b/BaseTools/Conf/tools_def.template
>>> @@ -399,8 +399,8 @@ DEFINE DTC_BIN                 = ENV(DTC_PREFIX)dtc
>>>  #                               Intel(r) ACPI Compiler from
>>>  #                               https://acpica.org/downloads
>>>  #   CLANG38  -Linux-  Requires:
>>> -#                             Clang v3.8, LLVMgold plugin and GNU binutils 2.26 targeting x86_64-linux-gnu
>>> -#                             Clang v3.9 or later, LLVMgold plugin and GNU binutils 2.28 targeting x86_64-linux-gnu
>>> +#                             Clang v3.8, LLVMgold plugin and GNU binutils 2.26 targeting x86_64-linux-gnu, aarch64-linux-gnu or arm-linux-gnueabi
>>> +#                             Clang v3.9 or later, LLVMgold plugin and GNU binutils 2.28 targeting x86_64-linux-gnu, aarch64-linux-gnu or arm-linux-gnueabi
>>>  #                        Optional:
>>>  #                             Required to build platforms or ACPI tables:
>>>  #                               Intel(r) ACPI Compiler from
>>> @@ -5652,6 +5652,7 @@ RELEASE_CLANG35_AARCH64_CC_FLAGS = DEF(CLANG35_AARCH64_CC_FLAGS) $(ARCHCC_FLAGS)
>>>  *_CLANG38_*_MAKE_PATH               = make
>>>  *_CLANG38_*_*_DLL                   = ENV(CLANG38_DLL)
>>>  *_CLANG38_*_ASL_PATH                = DEF(UNIX_IASL_BIN)
>>> +*_CLANG38_*_DTC_PATH                = DEF(DTC_BIN)
>>>
>>>  *_CLANG38_*_APP_FLAGS               =
>>>  *_CLANG38_*_ASL_FLAGS               = DEF(IASL_FLAGS)
>>> @@ -5663,7 +5664,8 @@ DEFINE CLANG38_X64_PREFIX           = ENV(CLANG38_BIN)
>>>  DEFINE CLANG38_IA32_TARGET          = -target i686-pc-linux-gnu
>>>  DEFINE CLANG38_X64_TARGET           = -target x86_64-pc-linux-gnu
>>>
>>> -DEFINE CLANG38_ALL_CC_FLAGS         = DEF(GCC44_ALL_CC_FLAGS) -Wno-empty-body -fno-stack-protector -mms-bitfields -Wno-address -Wno-shift-negative-value -Wno-parentheses-equality -Wno-unknown-pragmas -Wno-tautological-constant-out-of-range-compare -Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -msoft-float -mno-implicit-float  -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -funsigned-char -fno-ms-extensions -Wno-null-dereference -Wno-tautological-compare -Wno-unknown-warning-option -Wno-varargs
>>> +DEFINE CLANG38_WARNING_OVERRIDES    = -Wno-parentheses-equality -Wno-tautological-compare -Wno-tautological-constant-out-of-range-compare -Wno-empty-body  -Wno-varargs
>>> +DEFINE CLANG38_ALL_CC_FLAGS         = DEF(GCC44_ALL_CC_FLAGS) DEF(CLANG38_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields -Wno-address -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library-redeclaration -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -msoft-float -mno-implicit-float  -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang -funsigned-char -fno-ms-extensions -Wno-null-dereference -Wno-unknown-warning-option
>>
>> At a quite superficial look, the patch seems OK to me. I'm just curious
>> about one thing: what decides if a -Wno... option goes into
>> CLANG38_WARNING_OVERRIDES? You left some -Wno... options out of it (kept
>> them explicitly in the CLANG38_ALL_CC_FLAGS define).
>>
> 
> I split off some warning overrides that are shared between all
> architectures, and left the x86 only ones in CLANG38_ALL_CC_FLAGS

That's what I suspected; it just seemed strange that we wanted to
suppress e.g. "-Waddress" for x86 only.

... I do see the point though -- suppressing warnings is always a messy
business, so whenever we can opt for *enabling* warnings, we should. Not
inheriting a -Wno... flag does just that.


>> ... It seems like the CLANG38_ALL_CC_FLAGS is not used for the ARM /
>> AARCH64 toolchains. "ALL" becomes sort of a misnomer then. Is that OK?
>>
> 
> Yes. In tools_def land, ALL still means 'both IA32 and X64', and I
> have wasted too many cycles on trying to refactor the GCC toolchains
> to care about it.

Fair enough.

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

Thanks!
Laszlo