From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::243; helo=mail-io0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x243.google.com (mail-io0-x243.google.com [IPv6:2607:f8b0:4001:c06::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7B1FF2217CE28 for ; Wed, 6 Dec 2017 06:27:47 -0800 (PST) Received: by mail-io0-x243.google.com with SMTP id s37so2850562ioe.10 for ; Wed, 06 Dec 2017 06:32:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=GxoMzSqB7mtfIhfIZ9BMmcXhyh+LtSJSnB++rwgt0Po=; b=H3VwX43skOvpTZADgnTL1xXy3VsL0vx6b6BmqOcZuXl4T/7NV+TnQ+Z0a+UO8/MANk m2RlgAnX+qhaveGXaQhJ5zrRi4AlTIM+Y+QmNzOnlyxQTIw0/0lOQmKIz0E//gv/q6vp SAeS6Z3tPxxw4vdUaWp+UzFm0LQN5TKWMx8hs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=GxoMzSqB7mtfIhfIZ9BMmcXhyh+LtSJSnB++rwgt0Po=; b=sIGX+Fbfq28+R4MFHQiwZqwwjfOTjnjbZih1yZNvaPdesE1IPb6etNFzlGva5bWHhu hQn0vsHG1pYFgnC2f+HwKqeQcRqLsqokBKmyuL3QS3G4oI3WaWfFdJdDjTQljg35VcoC OtnjZhg6Oofn04JjFk5FSC6QxOyOGLjgW7FBDzSv7uFmAcJ8QsnkgQHVgIu5gh0psozh avREQfuQkqvTqyeG9jUvPTu3K7BLb8Xl4AaItrjmus3ZPtzZW0Q177ZCHUNnYh20aDki COFf++w+GgezERTEuarFu3H/FN0q4IBBaEyrSdQMsmuCr58jDFPNJzemFRWiMQYV1Ayr mkAA== X-Gm-Message-State: AJaThX4Vz1L8zD3eJPMHGhcHpuGM5F/28xXH5NnRAuIEXA9nx4Hp4jBH R/qBZKI/pe5Nd6cVnQSIfNeBYwUXulEtTFlplpI8+g== X-Google-Smtp-Source: AGs4zMZiTjTMX2dfZiV7t+mDycSf40UtlYQERvO6/aVWTVVCBplm5iSwtuLK3H4fC+HpbeswkzH+fTaNTccLRecwfKw= X-Received: by 10.107.174.222 with SMTP id n91mr32282777ioo.43.1512570738757; Wed, 06 Dec 2017 06:32:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Wed, 6 Dec 2017 06:32:17 -0800 (PST) In-Reply-To: <5eb876bf-7362-0ab6-7931-76bd1b7ffd64@redhat.com> References: <20171206113917.6166-1-ard.biesheuvel@linaro.org> <5eb876bf-7362-0ab6-7931-76bd1b7ffd64@redhat.com> From: Ard Biesheuvel Date: Wed, 6 Dec 2017 14:32:17 +0000 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm , "Gao, Liming" , "Zhu, Yonghong" , "Shi, Steven" , Evan Lloyd Subject: Re: [PATCH] BaseTools/tools_def: add CLANG38 LTO versions for AARCH64 and ARM X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Dec 2017 14:27:47 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 6 December 2017 at 14:28, Laszlo Ersek wrote: > On 12/06/17 15:16, Ard Biesheuvel wrote: >> On 6 December 2017 at 14:12, Laszlo Ersek 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 >>>> --- >>>> 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 =3D ENV(DTC_PREFIX)= dtc >>>> # Intel(r) ACPI Compiler from >>>> # https://acpica.org/downloads >>>> # CLANG38 -Linux- Requires: >>>> -# Clang v3.8, LLVMgold plugin and GNU bin= utils 2.26 targeting x86_64-linux-gnu >>>> -# Clang v3.9 or later, LLVMgold plugin an= d GNU binutils 2.28 targeting x86_64-linux-gnu >>>> +# Clang v3.8, LLVMgold plugin and GNU bin= utils 2.26 targeting x86_64-linux-gnu, aarch64-linux-gnu or arm-linux-gnuea= bi >>>> +# Clang v3.9 or later, LLVMgold plugin an= d GNU binutils 2.28 targeting x86_64-linux-gnu, aarch64-linux-gnu or arm-li= nux-gnueabi >>>> # Optional: >>>> # Required to build platforms or ACPI tab= les: >>>> # Intel(r) ACPI Compiler from >>>> @@ -5652,6 +5652,7 @@ RELEASE_CLANG35_AARCH64_CC_FLAGS =3D DEF(CLANG35= _AARCH64_CC_FLAGS) $(ARCHCC_FLAGS) >>>> *_CLANG38_*_MAKE_PATH =3D make >>>> *_CLANG38_*_*_DLL =3D ENV(CLANG38_DLL) >>>> *_CLANG38_*_ASL_PATH =3D DEF(UNIX_IASL_BIN) >>>> +*_CLANG38_*_DTC_PATH =3D DEF(DTC_BIN) >>>> >>>> *_CLANG38_*_APP_FLAGS =3D >>>> *_CLANG38_*_ASL_FLAGS =3D DEF(IASL_FLAGS) >>>> @@ -5663,7 +5664,8 @@ DEFINE CLANG38_X64_PREFIX =3D ENV(CLAN= G38_BIN) >>>> DEFINE CLANG38_IA32_TARGET =3D -target i686-pc-linux-gnu >>>> DEFINE CLANG38_X64_TARGET =3D -target x86_64-pc-linux-gnu >>>> >>>> -DEFINE CLANG38_ALL_CC_FLAGS =3D DEF(GCC44_ALL_CC_FLAGS) -Wno-= empty-body -fno-stack-protector -mms-bitfields -Wno-address -Wno-shift-nega= tive-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-fl= oat -ftrap-function=3Dundefined_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 =3D -Wno-parentheses-equality -Wn= o-tautological-compare -Wno-tautological-constant-out-of-range-compare -Wno= -empty-body -Wno-varargs >>>> +DEFINE CLANG38_ALL_CC_FLAGS =3D DEF(GCC44_ALL_CC_FLAGS) DEF(C= LANG38_WARNING_OVERRIDES) -fno-stack-protector -mms-bitfields -Wno-address = -Wno-shift-negative-value -Wno-unknown-pragmas -Wno-incompatible-library-re= declaration -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -msoft-float = -mno-implicit-float -ftrap-function=3Dundefined_behavior_has_been_optimize= d_away_by_clang -funsigned-char -fno-ms-extensions -Wno-null-dereference -W= no-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 (kep= t >>> 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 > Thanks.