Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize

[Laszlo Ersek <lersek@redhat.com>]

On 07/05/17 19:33, Ard Biesheuvel wrote:
> On 5 July 2017 at 18:28, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 07/05/17 18:45, Ard Biesheuvel wrote:
>>> On 5 July 2017 at 17:42, Laszlo Ersek <lersek@redhat.com> 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