From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 03F322119A46D for ; Mon, 10 Dec 2018 10:19:14 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 75B3181107; Mon, 10 Dec 2018 18:19:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-124-101.rdu2.redhat.com [10.10.124.101]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0EBCA5D717; Mon, 10 Dec 2018 18:19:11 +0000 (UTC) To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , "Gao, Liming" References: <20181210141339.17108-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <530714fc-c9f2-d38e-0ad1-0a67b967c52a@redhat.com> Date: Mon, 10 Dec 2018 19:19:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 10 Dec 2018 18:19:13 +0000 (UTC) Subject: Re: [PATCH] BaseTools/tools_def AARCH64: move GCC49/GGC5 to 4 KB section alignment X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Dec 2018 18:19:14 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/10/18 19:12, Ard Biesheuvel wrote: > On Mon, 10 Dec 2018 at 19:08, Laszlo Ersek wrote: >> >> On 12/10/18 15:13, Ard Biesheuvel wrote: >>> Since 4 KB section alignment is required when mapping PE/COFF images >>> with strict permissions, update the default section alignment when >>> using GCC49 and GCC5 in RELEASE mode. Note that XIP modules such as >>> SEC, PEIMs or PEI core are not affected by this change, since the >>> override to 32 byte aligment remains in effect. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> BaseTools/Conf/tools_def.template | 24 +++++++++++--------- >>> 1 file changed, 13 insertions(+), 11 deletions(-) >>> >>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template >>> index e0e68fd7fb49..5d34333dc54f 100755 >>> --- a/BaseTools/Conf/tools_def.template >>> +++ b/BaseTools/Conf/tools_def.template >>> @@ -4263,11 +4263,11 @@ 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) >>> +DEFINE GCC49_AARCH64_CC_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC44_ALL_CC_FLAGS) DEF(GCC_AARCH64_CC_FLAGS) -mcmodel=small >>> 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) >>> -DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) >>> +DEFINE GCC49_AARCH64_DLINK_FLAGS = DEF(GCC48_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 >>> DEFINE GCC49_AARCH64_DLINK2_FLAGS = DEF(GCC48_AARCH64_DLINK2_FLAGS) >>> DEFINE GCC49_ARM_ASLDLINK_FLAGS = DEF(GCC48_ARM_ASLDLINK_FLAGS) >>> DEFINE GCC49_AARCH64_ASLDLINK_FLAGS = DEF(GCC48_AARCH64_ASLDLINK_FLAGS) >>> @@ -5034,15 +5034,16 @@ RELEASE_GCC49_ARM_CC_FLAGS = DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v >>> *_GCC49_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS) >>> *_GCC49_AARCH64_CC_XIPFLAGS = DEF(GCC49_AARCH64_CC_XIPFLAGS) >>> >>> - DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small >>> - DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 >>> + DEBUG_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 >>> + DEBUG_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) >>> DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 >>> >>> -RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=tiny >>> +RELEASE_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-unused-const-variable >>> RELEASE_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) >>> +RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 >>> >>> - NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small >>> - NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0 >>> + NOOPT_GCC49_AARCH64_CC_FLAGS = DEF(GCC49_AARCH64_CC_FLAGS) -O0 >>> + NOOPT_GCC49_AARCH64_DLINK_FLAGS = DEF(GCC49_AARCH64_DLINK_FLAGS) -O0 >>> NOOPT_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0 >>> >>> #################################################################################### >>> @@ -5189,14 +5190,15 @@ RELEASE_GCC5_ARM_DLINK_FLAGS = DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS >>> *_GCC5_AARCH64_VFRPP_FLAGS = $(ARCHCC_FLAGS) $(PLATFORM_FLAGS) DEF(GCC_VFRPP_FLAGS) >>> *_GCC5_AARCH64_CC_XIPFLAGS = DEF(GCC5_AARCH64_CC_XIPFLAGS) >>> >>> - DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -mcmodel=small >>> - DEBUG_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -flto -Os -L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-through=-llto-aarch64 -Wno-lto-type-mismatch -mcmodel=small >>> + DEBUG_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable >>> + DEBUG_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 >>> 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 -Wno-unused-const-variable -mcmodel=tiny >>> +RELEASE_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -flto -Wno-unused-but-set-variable -Wno-unused-const-variable >>> 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 >>> +RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 >>> >>> - NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 -mcmodel=small >>> + NOOPT_GCC5_AARCH64_CC_FLAGS = DEF(GCC5_AARCH64_CC_FLAGS) -O0 >>> NOOPT_GCC5_AARCH64_DLINK_FLAGS = DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000 -O0 >>> NOOPT_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20 -O0 >>> >>> >> >> Looking at this patch with "--word-diff" helps a little (it's too bad we >> can't break these mile long option sequences to multiple lines). So: >> >> * For GCC49: >> >> - The compiler option "-mcmodel=small" is upstreamed from both >> NOOPT_GCC49_AARCH64_CC_FLAGS and DEBUG_GCC49_AARCH64_CC_FLAGS to >> GCC49_AARCH64_CC_FLAGS. This is a no-op for DEBUG and NOOPT, however the >> option will now also apply to RELEASE. >> >> - As a consequence, "-mcmodel=tiny" is not usable for RELEASE, and it is >> removed. >> >> - The same upstreaming occurs for "-z common-page-size=0x1000", in >> GCC49_AARCH64_DLINK_FLAGS. RELEASE needs no fixup, it simply benefits >> from the change. However, RELEASE_GCC49_AARCH64_DLINK_XIPFLAGS needs an >> override, to stick with the original 0x20 alignment. >> >> * For GCC5: >> >> - GCC5_AARCH64_CC_FLAGS is defined as GCC49_AARCH64_CC_FLAGS, therefore >> the "-mcmodel=small" inheritance from the above is automatic. Only the >> DEBUG/NOOPT removals, and the RELEASE fixup are needed. >> >> - GCC5_AARCH64_DLINK_FLAGS is defined as GCC49_AARCH64_DLINK_FLAGS, >> therefore the same inheritance pattern applies to "-z >> common-page-size=0x1000". I'm noticing an omission here however: "-z >> common-page-size=0x1000" should have been removed from >> NOOPT_GCC5_AARCH64_DLINK_FLAGS as well. >> >> - The RELEASE_GCC5_AARCH64_DLINK_XIPFLAGS override is spelled out the same. >> >> >> So... do you agree that NOOPT_GCC5_AARCH64_DLINK_FLAGS should have been >> trimmed as well? >> > > Yes, well spotted. > >> Furthermore, I suggest squeezing the word RELEASE into the subject, >> given that the change is only observable in RELEASE builds. >> > > Sure, I'll add that as well. > Thanks! Personally I don't need to see a repost to believe you. :) With those updates: Reviewed-by: Laszlo Ersek Thanks! Laszlo