From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 5B46321E49BB0 for ; Tue, 22 Aug 2017 04:56:55 -0700 (PDT) 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 959E07E42E; Tue, 22 Aug 2017 11:59:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 959E07E42E Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-92.phx2.redhat.com [10.3.116.92]) by smtp.corp.redhat.com (Postfix) with ESMTP id D2F987A4D5; Tue, 22 Aug 2017 11:59:23 +0000 (UTC) To: "Shi, Steven" , Ard Biesheuvel Cc: edk2-devel-01 , Alex Williamson , "Justen, Jordan L" , "Gao, Liming" , "Kinney, Michael D" , Paolo Bonzini References: <20170811003426.2332-1-lersek@redhat.com> <20170811003426.2332-2-lersek@redhat.com> <06C8AB66E78EE34A949939824ABE2B313B560EB2@shsmsx102.ccr.corp.intel.com> <787f4528-980e-8c71-2804-0e8be2c935aa@redhat.com> <06C8AB66E78EE34A949939824ABE2B313B56176B@shsmsx102.ccr.corp.intel.com> <092446e6-0900-7eb3-d071-b88abcdadfa9@redhat.com> <06C8AB66E78EE34A949939824ABE2B313B5673A1@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <3c593a67-bffc-45db-e65c-8d0242ddada4@redhat.com> Date: Tue, 22 Aug 2017 13:59:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <06C8AB66E78EE34A949939824ABE2B313B5673A1@shsmsx102.ccr.corp.intel.com> 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.27]); Tue, 22 Aug 2017 11:59:27 +0000 (UTC) Subject: Re: [PATCH 1/1] BaseTools/tools_def.template: revert to large code model for X64/GCC5/LTO 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: Tue, 22 Aug 2017 11:56:55 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/22/17 10:00, Shi, Steven wrote: > It is a link flag misuse in our GCC build toolchains, not > compiler/linker's problem. Below patch can fix the wrong assembly > function relocation type in PIE binary. With below patch, all the > GCC5, GCC49 and GCC48 can build correctly images of OvmfPkgIa32X64 and > OvmfPkgX64 platforms without my previous simple work around in my > side. Please try it in your side. > > Since we are using GCC as linker command, we MUST pass -pie to ld with > "-Wl,-pie", not just "--pie" or "-fpie". > > So, this means we never correctly build small+PIE 64bits code with GCC > toolchains before (CLANG38 is correct). If you failed to enable > PIE/LTO build before, it is worthy to revisit those failures with > "-Wl,-pie". FYI. The first question of Paolo (CC'd) was, when he saw this issue in my last status report, whether we added "-fpie" to the link command line as well. And, I confirmed, we did. This is how I responded to him: > - in "BaseTools/Conf/build_rule.template", there's > > > [Static-Library-File] > > > > *.lib > > > > > > $(MAKE_FILE) > > > > > > $(DEBUG_DIR)(+)$(MODULE_NAME).dll > > > > [...] > > > > > > "$(DLINK)" -o ${dst} $(DLINK_FLAGS) -Wl,--start-group,@$(STATIC_LIBRARY_FILES_LIST),--end-group $(CC_FLAGS) $(DLINK2_FLAGS) > > "$(OBJCOPY)" $(OBJCOPY_FLAGS) ${dst} > > (see "$(CC_FLAGS)") > > - and in the build log, I see > > > "gcc" \ > > -o Build/OvmfX64/DEBUG_GCC5/X64/UefiCpuPkg/CpuMpPei/CpuMpPei/DEBUG/CpuMpPei.dll \ > > -nostdlib \ > > -Wl,-n,-q,--gc-sections \ > > -z common-page-size=0x40 \ > > -Wl,--entry,_ModuleEntryPoint \ > > -u _ModuleEntryPoint \ > > -Wl,-Map,Build/OvmfX64/DEBUG_GCC5/X64/UefiCpuPkg/CpuMpPei/CpuMpPei/DEBUG/CpuMpPei.map \ > > -Wl,-melf_x86_64,--oformat=elf64-x86-64 \ > > -flto \ > > -Os \ > > -Wl,--start-group,@Build/OvmfX64/DEBUG_GCC5/X64/UefiCpuPkg/CpuMpPei/CpuMpPei/OUTPUT/static_library_files.lst,--end-group \ > > -g \ > > -fshort-wchar \ > > -fno-builtin \ > > -fno-strict-aliasing \ > > -Wall \ > > -Werror \ > > -Wno-array-bounds \ > > -ffunction-sections \ > > -fdata-sections \ > > -include AutoGen.h \ > > -fno-common \ > > -DSTRING_ARRAY_NAME=CpuMpPeiStrings \ > > -m64 \ > > -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" \ > > -maccumulate-outgoing-args \ > > -mno-red-zone \ > > -Wno-address \ > > -mcmodel=small \ > > -fpie \ > > -fno-asynchronous-unwind-tables \ > > -Wno-address \ > > -flto \ > > -DUSING_LTO \ > > -Os \ > > -mno-mmx \ > > -mno-sse \ > > -D DISABLE_NEW_DEPRECATED_INTERFACES \ > > -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 \ > > -Wl,--script=BaseTools/Scripts/GccBase.lds \ > > -Wno-error I don't understand why we need "-Wl,-pie" separately. The "gcc" binary should pass it through to "ld" as necessary, IMO. This is what the gcc documentation says: > 3.13 Options for Linking > ======================== > '-pie' > Produce a position independent executable on targets that support > it. For predictable results, you must also specify the same set > of options used for compilation ('-fpie', '-fPIE', or model > suboptions) when you specify this linker option. > > 3.18 Options for Code Generation Conventions > ============================================ > '-fpie' > '-fPIE' > These options are similar to '-fpic' and '-fPIC', but generated > position independent code can be only linked into executables. > Usually these options are used when '-pie' GCC option is used > during linking. > > '-fpie' and '-fPIE' both define the macros '__pie__' and > '__PIE__'. The macros have the value 1 for '-fpie' and 2 for > '-fPIE'. This seems to suggest that "-pie" is the *master* switch (used only when linking), and "-fpie" is a *prerequisite* for it (to be used both when linking and compiling). Is this right? If so, then I think this is a gcc usability bug. We don't generally start our thinking from the linker side. The above implies that the simple (hosted) command line: $ gcc -o example -fpie source1.c source2.c could also result in miscompilation, because "-pie" is not given, only "-fpie". On 08/22/17 10:00, Shi, Steven wrote: > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template > index 1fa3ca3..9e46d65 100755 > --- a/BaseTools/Conf/tools_def.template > +++ b/BaseTools/Conf/tools_def.template > @@ -4375,7 +4375,7 @@ DEFINE GCC44_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z comm > DEFINE GCC44_IA32_X64_ASLDLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable > DEFINE GCC44_IA32_X64_DLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map > DEFINE GCC44_IA32_DLINK2_FLAGS = -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON) > -DEFINE GCC44_X64_DLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64 > +DEFINE GCC44_X64_DLINK_FLAGS = DEF(GCC44_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64 -Wl,-pie > DEFINE GCC44_X64_DLINK2_FLAGS = -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 DEF(GCC_DLINK2_FLAGS_COMMON) > DEFINE GCC44_ASM_FLAGS = DEF(GCC_ASM_FLAGS) > > @@ -4455,7 +4455,7 @@ DEFINE GCC49_IA32_X64_DLINK_COMMON = -nostdlib -Wl,-n,-q,--gc-sections -z comm > DEFINE GCC49_IA32_X64_ASLDLINK_FLAGS = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable > DEFINE GCC49_IA32_X64_DLINK_FLAGS = DEF(GCC49_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map > DEFINE GCC49_IA32_DLINK2_FLAGS = DEF(GCC48_IA32_DLINK2_FLAGS) > -DEFINE GCC49_X64_DLINK_FLAGS = DEF(GCC49_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64 > +DEFINE GCC49_X64_DLINK_FLAGS = DEF(GCC49_IA32_X64_DLINK_FLAGS) -Wl,-melf_x86_64,--oformat=elf64-x86-64 -Wl,-pie > DEFINE GCC49_X64_DLINK2_FLAGS = DEF(GCC48_X64_DLINK2_FLAGS) > DEFINE GCC49_ASM_FLAGS = DEF(GCC48_ASM_FLAGS) > DEFINE GCC49_ARM_ASM_FLAGS = DEF(GCC48_ARM_ASM_FLAGS) Do we think that this was an omission in commit a1b8baccc30b ("BaseTools GCC: use 'gcc' as the linker command for GCC44 and later", 2016-07-23)? Honestly, I'm quite a bit annoyed by this parameter forwarding mess between "gcc" and "ld". If someone can submit a formal patch to fix this, please do so (I also suggest to reassign BZ#671 to BaseTools then). If there's an actual patch I can fetch or apply with git, I'll be happy to test it. Thanks, Laszlo