From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x229.google.com (mail-it0-x229.google.com [IPv6:2607:f8b0:4001:c0b::229]) (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 EE2742095896D for ; Wed, 5 Jul 2017 10:31:26 -0700 (PDT) Received: by mail-it0-x229.google.com with SMTP id k192so91563612ith.1 for ; Wed, 05 Jul 2017 10:33:06 -0700 (PDT) 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; bh=iYlRhq77PueZCT8VmVqCo9WtDNHXHj2199jIzYsOUT4=; b=d8KEjXVLLtC5s9fDnLBg3YuV8ZW/o4HSn0fAOmXOis3a+48iAcFTYlHMN17hgcwpJ6 JJOXSksyyB/zZ9/1aLBRCOc1cqUhBA55toe0MAdY3W5m88PS+7UyilYg86Z8K2EIvK7E RX0QJUv9fMcAQAfyD2m38JjbPqJAQArDWAKA4= 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; bh=iYlRhq77PueZCT8VmVqCo9WtDNHXHj2199jIzYsOUT4=; b=C8kkGEfkGyUU4pGZQShM5lUwkOkzabCDnyGImJaFnhw56G+u7FDS4VB+u07KmQKCkK ENptAV7gVXNOfZJgsWIEhYvgSufzm9Nf9QmQE8/uDAbVRpOiIwr3FRKEjkPGpNw51R1s pzac2dn6QUSmm+YGqTgUY4u9/b1mhNLB4gbL6SxEUW4L8q9LSB7BZsm6Q1JMbyMaexmC 7uHKmQ8W5dF1us7DCOebIu+wQTNlV6XMxabFn6qzBsZGpTjLQAAgthA9o3SfiK5q0cF/ asR541YlY6wVi6bmK9DR6auFHm8zFPcdbDHgHu+g39q0fCi2CgIF1HDi/nFztQ3H61tJ 7zYg== X-Gm-Message-State: AKS2vOyh1y7QSVP8FFarn27mBm6jh9SUVuf79tWGYCiPfphzIHbq9XxK Fmx9Oiw6dDO74NWGqPjhWXJkWIvz49Ymn+1jXg== X-Received: by 10.107.59.84 with SMTP id i81mr41567104ioa.72.1499275985737; Wed, 05 Jul 2017 10:33:05 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.134.134 with HTTP; Wed, 5 Jul 2017 10:33:05 -0700 (PDT) In-Reply-To: <05a02e88-873d-35ec-e419-e75187555abc@redhat.com> References: <20170705164218.25814-1-lersek@redhat.com> <05a02e88-873d-35ec-e419-e75187555abc@redhat.com> From: Ard Biesheuvel Date: Wed, 5 Jul 2017 18:33:05 +0100 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Liming Gao , Leif Lindholm 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:31:27 -0000 Content-Type: text/plain; charset="UTF-8" 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