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

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

On 6 December 2017 at 14:28, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.
>

Yes. I reluctantly added the -Wno-varargs option, for instance,
because we didn't need that for Clang 3.5, but more recent Clang
chokes on some questionable uses in EDK2, e.g.,

/home/ard/build/edk2/MdePkg/Library/UefiLib/UefiLib.c:1530:19: error:
passing an object that undergoes default argument promotion to
'va_start' has undefined behavior [-Werror,-Wvarargs]
  VA_START (Args, Iso639Language);
                  ^
/home/ard/build/edk2/MdePkg/Library/UefiLib/UefiLib.c:1517:19: note:
parameter of type 'BOOLEAN' (aka 'unsigned char') is declared here
  IN BOOLEAN      Iso639Language,
                  ^
1 error generated.

So indeed, I tried to be conservative in disabling warnings.

>
>>> ... 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.