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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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