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

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [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 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 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: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