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 AE9FC2095896C for ; Wed, 5 Jul 2017 10:47:59 -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 C8AC83B73E; Wed, 5 Jul 2017 17:49:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C8AC83B73E Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com C8AC83B73E Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-25.phx2.redhat.com [10.3.116.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9D93517104; Wed, 5 Jul 2017 17:49:37 +0000 (UTC) To: Ard Biesheuvel Cc: edk2-devel-01 , Liming Gao , Leif Lindholm References: <20170705164218.25814-1-lersek@redhat.com> <05a02e88-873d-35ec-e419-e75187555abc@redhat.com> From: Laszlo Ersek Message-ID: <428b442e-5e60-d2f0-a1b4-92cb9cb32e98@redhat.com> Date: Wed, 5 Jul 2017 19:49:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.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.30]); Wed, 05 Jul 2017 17:49:39 +0000 (UTC) Subject: Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize 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, 05 Jul 2017 17:47:59 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/05/17 19:33, Ard Biesheuvel wrote: > On 5 July 2017 at 18:28, Laszlo Ersek wrote: >> On 07/05/17 18:45, Ard Biesheuvel wrote: >>> On 5 July 2017 at 17:42, Laszlo Ersek wrote: >>>> GNU Binutils produce a PE debug directory with one >>> >>> This sentence already confuses me. This crash is reproducible on ARM, >>> but the ARM toolchains are strictly ELF based, and all PE/COFF data >>> structures are created by GenFw itself, never by binutils. >> >> According to binutils commit 61e2488cd849: >> >> Add support for generating and inserting build IDs into COFF binaries. >> >> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=61e2488cd849 >> >> the write_build_id() function from that commit does produce PE/COFF >> artifacts. >> >> /* Construct a debug directory entry which points to an immediately following CodeView record. */ >> >> /* Record the location of the debug directory in the data directory. */ >> >> I can't exactly say where the bug is (it may have been added later -- >> I'm not a binutils developer), and the code I quoted above might not >> even be related to the symptoms we're seeing at all, but binutils can >> definitely generate PE stuff. >> >> Plus, the mal-sized debug directory in the GenFw-crasher DLL files seems >> to fall onto a section called ".build-id". >> >> OTOH, after reviewing the commands from Gerd's Jenkins log that lead to >> the GenFw crash on "SecMain.dll", I think you are right... All of these >> commands use ELF formats, apparently: >> >>> "gcc" \ >>> -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=SecMainStrings \ >>> -m32 \ >>> -march=i586 \ >>> -malign-double \ >>> -fno-stack-protector \ >>> -D EFI32 \ >>> -fno-asynchronous-unwind-tables \ >>> -Wno-address \ >>> -Os \ >>> -mno-mmx \ >>> -mno-sse \ >>> -D DISABLE_NEW_DEPRECATED_INTERFACES \ >>> -c \ >>> -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/./SecMain.obj \ >>> -IOvmfPkg/Sec/Ia32 \ >>> -IOvmfPkg/Sec \ >>> -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \ >>> -IMdePkg \ >>> -IMdePkg/Include \ >>> -IMdePkg/Include/Ia32 \ >>> -IMdeModulePkg \ >>> -IMdeModulePkg/Include \ >>> -IUefiCpuPkg \ >>> -IUefiCpuPkg/Include \ >>> -IOvmfPkg \ >>> -IOvmfPkg/Include \ >>> OvmfPkg/Sec/SecMain.c >>> >>> "gcc" \ >>> -E \ >>> -x assembler-with-cpp \ >>> -include Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.h \ >>> -IOvmfPkg/Sec/Ia32 \ >>> -IOvmfPkg/Sec \ >>> -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \ >>> -IMdePkg \ >>> -IMdePkg/Include \ >>> -IMdePkg/Include/Ia32 \ >>> -IMdeModulePkg \ >>> -IMdeModulePkg/Include \ >>> -IUefiCpuPkg \ >>> -IUefiCpuPkg/Include \ >>> -IOvmfPkg \ >>> -IOvmfPkg/Include \ >>> OvmfPkg/Sec/Ia32/SecEntry.nasm \ >>> > Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.i >>> >>> Trim \ >>> --trim-long \ >>> --source-code \ >>> -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.iii \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.i >>> >>> "nasm" \ >>> -IOvmfPkg/Sec/Ia32/ \ >>> -f elf32 \ >>> -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.obj \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/Ia32/SecEntry.iii >>> >>> "gcc" \ >>> -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=SecMainStrings \ >>> -m32 \ >>> -march=i586 \ >>> -malign-double \ >>> -fno-stack-protector \ >>> -D EFI32 \ >>> -fno-asynchronous-unwind-tables \ >>> -Wno-address \ >>> -Os \ >>> -mno-mmx \ >>> -mno-sse \ >>> -D DISABLE_NEW_DEPRECATED_INTERFACES \ >>> -c \ >>> -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/./AutoGen.obj \ >>> -IOvmfPkg/Sec/Ia32 \ >>> -IOvmfPkg/Sec \ >>> -IBuild/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG \ >>> -IMdePkg \ >>> -IMdePkg/Include \ >>> -IMdePkg/Include/Ia32 \ >>> -IMdeModulePkg \ >>> -IMdeModulePkg/Include \ >>> -IUefiCpuPkg \ >>> -IUefiCpuPkg/Include \ >>> -IOvmfPkg \ >>> -IOvmfPkg/Include \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/AutoGen.c >>> >>> "ar" \ >>> cr \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/SecMain.lib \ >>> @Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/OUTPUT/object_files.lst >>> >>> "gcc" \ >>> -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.dll \ >>> -nostdlib \ >>> -Wl,-n,-q,--gc-sections \ >>> -z common-page-size=0x40 \ >>> -Wl,--entry,_ModuleEntryPoint \ >>> -u _ModuleEntryPoint \ >>> -Wl,-Map,Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.map \ >>> -Wl,-m,elf_i386,--oformat=elf32-i386 \ >>> -Wl,--start-group,@Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/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=SecMainStrings \ >>> -m32 \ >>> -march=i586 \ >>> -malign-double \ >>> -fno-stack-protector \ >>> -D EFI32 \ >>> -fno-asynchronous-unwind-tables \ >>> -Wno-address \ >>> -Os \ >>> -mno-mmx \ >>> -mno-sse \ >>> -D DISABLE_NEW_DEPRECATED_INTERFACES \ >>> -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 \ >>> -Wl,--script=BaseTools/Scripts/GccBase.lds >>> >>> "objcopy" >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.dll >>> >>> cp \ >>> -f \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.dll \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.debug >>> >>> objcopy \ >>> --strip-unneeded \ >>> -R .eh_frame \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.dll >>> >>> objcopy \ >>> --add-gnu-debuglink=Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.debug \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.dll >>> >>> cp \ >>> -f \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.debug \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/SecMain.debug >>> >>> "GenFw" \ >>> -e SEC \ >>> -o Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.efi \ >>> Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.dll >>> >>> GNUmakefile:379: recipe for target 'Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.efi' failed >>> make: *** [Build/OvmfIa32/DEBUG_GCC49/IA32/OvmfPkg/Sec/SecMain/DEBUG/SecMain.efi] Segmentation fault (core dumped) >> >> What I don't understand though is, if GenFw creates the debug directory >> contents in the first place, then why clear it separately later (which >> currently crashes); why not just skip pulling stuff into the debug >> directory? >> >> Anyway, this was just an experiment on my part, I don't mind if the >> regressive patch is reverted first. >> > > GenFw can take both PE/COFF and ELF files as input, and in the latter > case, it performs the PE/COFF conversion itself. > > I tried the patch below, and it seems to get rid of the segfault. > Could you please confirm? > > > diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c > b/BaseTools/Source/C/GenFw/Elf32Convert.c > index f7b084dc9b84..14fe4a285857 100644 > --- a/BaseTools/Source/C/GenFw/Elf32Convert.c > +++ b/BaseTools/Source/C/GenFw/Elf32Convert.c > @@ -1142,7 +1142,7 @@ WriteDebug32 ( > NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset); > DataDir = &NtHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG]; > DataDir->VirtualAddress = mDebugOffset; > - DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); > + DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); > } > > STATIC > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c > b/BaseTools/Source/C/GenFw/Elf64Convert.c > index 7eed7b92d30f..c39bdff063ab 100644 > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c > @@ -1095,7 +1095,7 @@ WriteDebug64 ( > NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset); > DataDir = &NtHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG]; > DataDir->VirtualAddress = mDebugOffset; > - DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); > + DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); > } > > STATIC > I swear I found the same (and sent my previous email) before getting yours. :) (I mirror my IMAP stuff every 5 minutes.) So yes, this works. (Checked with OVMF IA32 and IA32X64.) Thanks, Laszlo