* [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize
@ 2017-07-05 16:42 Laszlo Ersek
2017-07-05 16:45 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2017-07-05 16:42 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Liming Gao,
Yonghong Zhu
GNU Binutils produce a PE debug directory with one
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY:
- the Type field of the entry is EFI_IMAGE_DEBUG_TYPE_CODEVIEW,
- the FileOffset field of the entry points right past the entry itself,
- the data structure placed at FileOffset is a CV_INFO_PDB20 structure,
with an "NB10" signature.
This is all correct, except GNU Binutils include the pointed-to
CV_INFO_PDB20 structure in the size of the debug directory (that is,
Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size).
That's a bug.
The malformed debug directory size causes the loop in GenFw's
ZeroDebugData() function to process the CV_INFO_PDB20 structure as a set
of EFI_IMAGE_DEBUG_DIRECTORY_ENTRY elements, which crashes GenFw.
This problem was exposed by commit e4129b0e5897 ("BaseTools: Update GenFw
to clear unused debug entry generated by VS tool chain", 2017-06-19).
Work around the Binutils issue by noticing when an
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset points back into the debug
directory. (This can never happen with a well-formed PE file.) In this
case, truncate DebugDirectoryEntrySize such that the debug directory will
end right before the debug structure pointed-to by
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset.
Tested with OVMF:
- gcc-4.8.5-14.el7.x86_64
- binutils-2.25.1-27.base.el7.x86_64
and with ArmVirtPkg:
- gcc-aarch64-linux-gnu-6.1.1-2.el7.x86_64
- binutils-aarch64-linux-gnu-2.27-3.el7.x86_64
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Yonghong Zhu <yonghong.zhu@intel.com>
Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Reported-by: Leif Lindholm <leif.lindholm@linaro.org>
Ref: http://mid.mail-archive.com/a1de67a8-57c2-908e-dd4d-9726d60fb388@redhat.com
Ref: http://mid.mail-archive.com/20170705134136.GB26676@bivouac.eciton.net
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
Repo: https://github.com/lersek/edk2.git
Branch: binutils_debugdirsize_workaround
BaseTools/Source/C/GenFw/GenFw.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
index 6569460f34f7..a79f485ee681 100644
--- a/BaseTools/Source/C/GenFw/GenFw.c
+++ b/BaseTools/Source/C/GenFw/GenFw.c
@@ -2771,6 +2771,7 @@ Returns:
UINT32 Index;
UINT32 DebugDirectoryEntryRva;
UINT32 DebugDirectoryEntrySize;
+ UINT32 TruncatedDebugDirectorySize;
UINT32 DebugDirectoryEntryFileOffset;
UINT32 ExportDirectoryEntryRva;
UINT32 ExportDirectoryEntryFileOffset;
@@ -2893,6 +2894,25 @@ Returns:
DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
Index = 0;
for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
+ //
+ // Work around GNU Binutils bug: if the debug information pointed-to by
+ // DebugEntry was incorrectly included in DebugDirectoryEntrySize, then
+ // the debug directory doesn't actually extend past the pointed-to debug
+ // information. Truncate DebugDirectoryEntrySize accordingly.
+ //
+ if (DebugEntry->FileOffset >= DebugDirectoryEntryFileOffset &&
+ DebugEntry->FileOffset < (DebugDirectoryEntryFileOffset +
+ DebugDirectoryEntrySize)) {
+ TruncatedDebugDirectorySize = (DebugEntry->FileOffset -
+ DebugDirectoryEntryFileOffset);
+ VerboseMsg (
+ "truncating debug directory size from %u to %u",
+ DebugDirectoryEntrySize,
+ TruncatedDebugDirectorySize
+ );
+ DebugDirectoryEntrySize = TruncatedDebugDirectorySize;
+ }
+
DebugEntry->TimeDateStamp = 0;
if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
--
2.13.1.3.g8be5a757fa67
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize
2017-07-05 16:42 [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize Laszlo Ersek
@ 2017-07-05 16:45 ` Ard Biesheuvel
2017-07-05 17:28 ` Laszlo Ersek
2017-07-05 17:33 ` Laszlo Ersek
0 siblings, 2 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2017-07-05 16:45 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Gerd Hoffmann, Leif Lindholm, Liming Gao,
Yonghong Zhu
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. So I don't
see how this could be a binutils bug.
> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY:
> - the Type field of the entry is EFI_IMAGE_DEBUG_TYPE_CODEVIEW,
> - the FileOffset field of the entry points right past the entry itself,
> - the data structure placed at FileOffset is a CV_INFO_PDB20 structure,
> with an "NB10" signature.
>
> This is all correct, except GNU Binutils include the pointed-to
> CV_INFO_PDB20 structure in the size of the debug directory (that is,
> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size).
> That's a bug.
>
> The malformed debug directory size causes the loop in GenFw's
> ZeroDebugData() function to process the CV_INFO_PDB20 structure as a set
> of EFI_IMAGE_DEBUG_DIRECTORY_ENTRY elements, which crashes GenFw.
>
> This problem was exposed by commit e4129b0e5897 ("BaseTools: Update GenFw
> to clear unused debug entry generated by VS tool chain", 2017-06-19).
>
> Work around the Binutils issue by noticing when an
> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset points back into the debug
> directory. (This can never happen with a well-formed PE file.) In this
> case, truncate DebugDirectoryEntrySize such that the debug directory will
> end right before the debug structure pointed-to by
> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset.
>
> Tested with OVMF:
> - gcc-4.8.5-14.el7.x86_64
> - binutils-2.25.1-27.base.el7.x86_64
>
> and with ArmVirtPkg:
> - gcc-aarch64-linux-gnu-6.1.1-2.el7.x86_64
> - binutils-aarch64-linux-gnu-2.27-3.el7.x86_64
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Yonghong Zhu <yonghong.zhu@intel.com>
> Reported-by: Gerd Hoffmann <kraxel@redhat.com>
> Reported-by: Leif Lindholm <leif.lindholm@linaro.org>
> Ref: http://mid.mail-archive.com/a1de67a8-57c2-908e-dd4d-9726d60fb388@redhat.com
> Ref: http://mid.mail-archive.com/20170705134136.GB26676@bivouac.eciton.net
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> Repo: https://github.com/lersek/edk2.git
> Branch: binutils_debugdirsize_workaround
>
> BaseTools/Source/C/GenFw/GenFw.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
> index 6569460f34f7..a79f485ee681 100644
> --- a/BaseTools/Source/C/GenFw/GenFw.c
> +++ b/BaseTools/Source/C/GenFw/GenFw.c
> @@ -2771,6 +2771,7 @@ Returns:
> UINT32 Index;
> UINT32 DebugDirectoryEntryRva;
> UINT32 DebugDirectoryEntrySize;
> + UINT32 TruncatedDebugDirectorySize;
> UINT32 DebugDirectoryEntryFileOffset;
> UINT32 ExportDirectoryEntryRva;
> UINT32 ExportDirectoryEntryFileOffset;
> @@ -2893,6 +2894,25 @@ Returns:
> DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
> Index = 0;
> for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
> + //
> + // Work around GNU Binutils bug: if the debug information pointed-to by
> + // DebugEntry was incorrectly included in DebugDirectoryEntrySize, then
> + // the debug directory doesn't actually extend past the pointed-to debug
> + // information. Truncate DebugDirectoryEntrySize accordingly.
> + //
> + if (DebugEntry->FileOffset >= DebugDirectoryEntryFileOffset &&
> + DebugEntry->FileOffset < (DebugDirectoryEntryFileOffset +
> + DebugDirectoryEntrySize)) {
> + TruncatedDebugDirectorySize = (DebugEntry->FileOffset -
> + DebugDirectoryEntryFileOffset);
> + VerboseMsg (
> + "truncating debug directory size from %u to %u",
> + DebugDirectoryEntrySize,
> + TruncatedDebugDirectorySize
> + );
> + DebugDirectoryEntrySize = TruncatedDebugDirectorySize;
> + }
> +
> DebugEntry->TimeDateStamp = 0;
> if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
> memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
> --
> 2.13.1.3.g8be5a757fa67
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize
2017-07-05 16:45 ` Ard Biesheuvel
@ 2017-07-05 17:28 ` Laszlo Ersek
2017-07-05 17:33 ` Ard Biesheuvel
2017-07-05 17:33 ` Laszlo Ersek
1 sibling, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2017-07-05 17:28 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Liming Gao, Leif Lindholm
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.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize
2017-07-05 17:28 ` Laszlo Ersek
@ 2017-07-05 17:33 ` Ard Biesheuvel
2017-07-05 17:49 ` Laszlo Ersek
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2017-07-05 17:33 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Liming Gao, Leif Lindholm
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize
2017-07-05 16:45 ` Ard Biesheuvel
2017-07-05 17:28 ` Laszlo Ersek
@ 2017-07-05 17:33 ` Laszlo Ersek
2017-07-05 17:37 ` Ard Biesheuvel
1 sibling, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2017-07-05 17:33 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-01, Gerd Hoffmann, Leif Lindholm, Liming Gao,
Yonghong Zhu
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. So I don't
> see how this could be a binutils bug.
Geez, you are totally right. From
"BaseTools/Source/C/GenFw/Elf64Convert.c":
> STATIC
> VOID
> WriteDebug64 (
> VOID
> )
> {
> UINT32 Len;
> EFI_IMAGE_OPTIONAL_HEADER_UNION *NtHdr;
> EFI_IMAGE_DATA_DIRECTORY *DataDir;
> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *Dir;
> EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY *Nb10;
>
> Len = strlen(mInImageName) + 1;
>
> Dir = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY*)(mCoffFile + mDebugOffset);
> Dir->Type = EFI_IMAGE_DEBUG_TYPE_CODEVIEW;
> Dir->SizeOfData = sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) + Len;
> Dir->RVA = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> Dir->FileOffset = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>
> Nb10 = (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY*)(Dir + 1);
> Nb10->Signature = CODEVIEW_SIGNATURE_NB10;
> strcpy ((char *)(Nb10 + 1), mInImageName);
>
>
> 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);
> }
The last assignment has the bug. It should be
DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize
2017-07-05 17:33 ` Laszlo Ersek
@ 2017-07-05 17:37 ` Ard Biesheuvel
2017-07-05 17:52 ` Laszlo Ersek
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2017-07-05 17:37 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Gerd Hoffmann, Leif Lindholm, Liming Gao,
Yonghong Zhu
On 5 July 2017 at 18:33, 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. So I don't
>> see how this could be a binutils bug.
>
> Geez, you are totally right. From
> "BaseTools/Source/C/GenFw/Elf64Convert.c":
>
>
>> STATIC
>> VOID
>> WriteDebug64 (
>> VOID
>> )
>> {
>> UINT32 Len;
>> EFI_IMAGE_OPTIONAL_HEADER_UNION *NtHdr;
>> EFI_IMAGE_DATA_DIRECTORY *DataDir;
>> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *Dir;
>> EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY *Nb10;
>>
>> Len = strlen(mInImageName) + 1;
>>
>> Dir = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY*)(mCoffFile + mDebugOffset);
>> Dir->Type = EFI_IMAGE_DEBUG_TYPE_CODEVIEW;
>> Dir->SizeOfData = sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) + Len;
>> Dir->RVA = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>> Dir->FileOffset = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>
>> Nb10 = (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY*)(Dir + 1);
>> Nb10->Signature = CODEVIEW_SIGNATURE_NB10;
>> strcpy ((char *)(Nb10 + 1), mInImageName);
>>
>>
>> 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);
>> }
>
> The last assignment has the bug. It should be
>
> DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>
OK, I will take that as an affirmative answer to my question. Are you
sending a patch?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize
2017-07-05 17:33 ` Ard Biesheuvel
@ 2017-07-05 17:49 ` Laszlo Ersek
0 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2017-07-05 17:49 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Liming Gao, Leif Lindholm
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize
2017-07-05 17:37 ` Ard Biesheuvel
@ 2017-07-05 17:52 ` Laszlo Ersek
0 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2017-07-05 17:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-01, Gerd Hoffmann, Leif Lindholm, Liming Gao,
Yonghong Zhu
On 07/05/17 19:37, Ard Biesheuvel wrote:
> On 5 July 2017 at 18:33, 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. So I don't
>>> see how this could be a binutils bug.
>>
>> Geez, you are totally right. From
>> "BaseTools/Source/C/GenFw/Elf64Convert.c":
>>
>>
>>> STATIC
>>> VOID
>>> WriteDebug64 (
>>> VOID
>>> )
>>> {
>>> UINT32 Len;
>>> EFI_IMAGE_OPTIONAL_HEADER_UNION *NtHdr;
>>> EFI_IMAGE_DATA_DIRECTORY *DataDir;
>>> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *Dir;
>>> EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY *Nb10;
>>>
>>> Len = strlen(mInImageName) + 1;
>>>
>>> Dir = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY*)(mCoffFile + mDebugOffset);
>>> Dir->Type = EFI_IMAGE_DEBUG_TYPE_CODEVIEW;
>>> Dir->SizeOfData = sizeof(EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY) + Len;
>>> Dir->RVA = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>> Dir->FileOffset = mDebugOffset + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>>
>>> Nb10 = (EFI_IMAGE_DEBUG_CODEVIEW_NB10_ENTRY*)(Dir + 1);
>>> Nb10->Signature = CODEVIEW_SIGNATURE_NB10;
>>> strcpy ((char *)(Nb10 + 1), mInImageName);
>>>
>>>
>>> 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);
>>> }
>>
>> The last assignment has the bug. It should be
>>
>> DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>>
>
> OK, I will take that as an affirmative answer to my question. Are you
> sending a patch?
>
You send it please, just give me some "Co-debugged-by:" or whatever. :)
Also, please add a ref to the mailing list thread.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-07-05 17:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-05 16:42 [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize Laszlo Ersek
2017-07-05 16:45 ` Ard Biesheuvel
2017-07-05 17:28 ` Laszlo Ersek
2017-07-05 17:33 ` Ard Biesheuvel
2017-07-05 17:49 ` Laszlo Ersek
2017-07-05 17:33 ` Laszlo Ersek
2017-07-05 17:37 ` Ard Biesheuvel
2017-07-05 17:52 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox