* [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
@ 2017-07-03 5:21 Liming Gao
2017-07-03 5:28 ` Zhu, Yonghong
2017-07-05 12:42 ` Laszlo Ersek
0 siblings, 2 replies; 6+ messages in thread
From: Liming Gao @ 2017-07-03 5:21 UTC (permalink / raw)
To: edk2-devel
https://bugzilla.tianocore.org/show_bug.cgi?id=600
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
BaseTools/Source/C/GenFw/GenFw.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
index 22e4e72..6569460 100644
--- a/BaseTools/Source/C/GenFw/GenFw.c
+++ b/BaseTools/Source/C/GenFw/GenFw.c
@@ -2770,6 +2770,7 @@ Returns:
{
UINT32 Index;
UINT32 DebugDirectoryEntryRva;
+ UINT32 DebugDirectoryEntrySize;
UINT32 DebugDirectoryEntryFileOffset;
UINT32 ExportDirectoryEntryRva;
UINT32 ExportDirectoryEntryFileOffset;
@@ -2781,12 +2782,14 @@ Returns:
EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;
EFI_IMAGE_SECTION_HEADER *SectionHeader;
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
+ EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
UINT32 *NewTimeStamp;
//
// Init variable.
//
DebugDirectoryEntryRva = 0;
+ DebugDirectoryEntrySize = 0;
ExportDirectoryEntryRva = 0;
ResourceDirectoryEntryRva = 0;
DebugDirectoryEntryFileOffset = 0;
@@ -2822,6 +2825,7 @@ Returns:
if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
DebugDirectoryEntryRva = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
+ DebugDirectoryEntrySize = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
if (ZeroDebugFlag) {
Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
@@ -2841,6 +2845,7 @@ Returns:
if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
DebugDirectoryEntryRva = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
+ DebugDirectoryEntrySize = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
if (ZeroDebugFlag) {
Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
@@ -2886,11 +2891,23 @@ Returns:
if (DebugDirectoryEntryFileOffset != 0) {
DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
- DebugEntry->TimeDateStamp = 0;
- mImageTimeStamp = 0;
- if (ZeroDebugFlag) {
- memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
- memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
+ Index = 0;
+ for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
+ DebugEntry->TimeDateStamp = 0;
+ if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
+ memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
+ memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
+ }
+ if (DebugEntry->Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
+ RsdsEntry = (EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *) (FileBuffer + DebugEntry->FileOffset);
+ if (RsdsEntry->Signature == CODEVIEW_SIGNATURE_RSDS) {
+ RsdsEntry->Unknown = 0;
+ RsdsEntry->Unknown2 = 0;
+ RsdsEntry->Unknown3 = 0;
+ RsdsEntry->Unknown4 = 0;
+ RsdsEntry->Unknown5 = 0;
+ }
+ }
}
}
--
2.8.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
2017-07-03 5:21 [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain Liming Gao
@ 2017-07-03 5:28 ` Zhu, Yonghong
2017-07-05 12:42 ` Laszlo Ersek
1 sibling, 0 replies; 6+ messages in thread
From: Zhu, Yonghong @ 2017-07-03 5:28 UTC (permalink / raw)
To: Gao, Liming, edk2-devel@lists.01.org
Reviewed-by: Yonghong Zhu <yonghong.zhu@intel.com>
Best Regards,
Zhu Yonghong
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Liming Gao
Sent: Monday, July 03, 2017 1:21 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
https://bugzilla.tianocore.org/show_bug.cgi?id=600
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
BaseTools/Source/C/GenFw/GenFw.c | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
index 22e4e72..6569460 100644
--- a/BaseTools/Source/C/GenFw/GenFw.c
+++ b/BaseTools/Source/C/GenFw/GenFw.c
@@ -2770,6 +2770,7 @@ Returns:
{
UINT32 Index;
UINT32 DebugDirectoryEntryRva;
+ UINT32 DebugDirectoryEntrySize;
UINT32 DebugDirectoryEntryFileOffset;
UINT32 ExportDirectoryEntryRva;
UINT32 ExportDirectoryEntryFileOffset;
@@ -2781,12 +2782,14 @@ Returns:
EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;
EFI_IMAGE_SECTION_HEADER *SectionHeader;
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
+ EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
UINT32 *NewTimeStamp;
//
// Init variable.
//
DebugDirectoryEntryRva = 0;
+ DebugDirectoryEntrySize = 0;
ExportDirectoryEntryRva = 0;
ResourceDirectoryEntryRva = 0;
DebugDirectoryEntryFileOffset = 0;
@@ -2822,6 +2825,7 @@ Returns:
if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
DebugDirectoryEntryRva = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
+ DebugDirectoryEntrySize =
+ Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
if (ZeroDebugFlag) {
Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0; @@ -2841,6 +2845,7 @@ Returns:
if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
DebugDirectoryEntryRva = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
+ DebugDirectoryEntrySize =
+ Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
if (ZeroDebugFlag) {
Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0; @@ -2886,11 +2891,23 @@ Returns:
if (DebugDirectoryEntryFileOffset != 0) {
DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
- DebugEntry->TimeDateStamp = 0;
- mImageTimeStamp = 0;
- if (ZeroDebugFlag) {
- memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
- memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
+ Index = 0;
+ for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
+ DebugEntry->TimeDateStamp = 0;
+ if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
+ memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
+ memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
+ }
+ if (DebugEntry->Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
+ RsdsEntry = (EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *) (FileBuffer + DebugEntry->FileOffset);
+ if (RsdsEntry->Signature == CODEVIEW_SIGNATURE_RSDS) {
+ RsdsEntry->Unknown = 0;
+ RsdsEntry->Unknown2 = 0;
+ RsdsEntry->Unknown3 = 0;
+ RsdsEntry->Unknown4 = 0;
+ RsdsEntry->Unknown5 = 0;
+ }
+ }
}
}
--
2.8.0.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
2017-07-03 5:21 [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain Liming Gao
2017-07-03 5:28 ` Zhu, Yonghong
@ 2017-07-05 12:42 ` Laszlo Ersek
2017-07-05 13:29 ` Laszlo Ersek
2017-07-05 13:41 ` Leif Lindholm
1 sibling, 2 replies; 6+ messages in thread
From: Laszlo Ersek @ 2017-07-05 12:42 UTC (permalink / raw)
To: Liming Gao, edk2-devel; +Cc: Gerd Hoffmann, Ard Biesheuvel
Hi Liming,
Gerd's Jenkins CI reported a GenFw segfault, with this patch applied.
I can reproduce the segfault locally. This is the command line:
> GenFw \
> -e DXE_RUNTIME_DRIVER \
> -o .../Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.efi \
> Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.dll
Backtrace from gdb:
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff789c814 in __memset_sse2 () from /lib64/libc.so.6
> Missing separate debuginfos, use: debuginfo-install glibc-2.17-194.el7.x86_64 libuuid-2.23.2-39.el7.x86_64
> (gdb) where
> #0 0x00007ffff789c814 in __memset_sse2 () from /lib64/libc.so.6
> #1 0x0000000000407492 in ZeroDebugData (FileBuffer=0x636e50 "MZ", ZeroDebugFlag=0 '\000') at GenFw.c:2898
> #2 0x0000000000406a4d in main (argc=0, argv=0x7fffffffd0b8) at GenFw.c:2591
See below for the SIGSEGV location:
On 07/03/17 07:21, Liming Gao wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=600
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Liming Gao <liming.gao@intel.com>
> ---
> BaseTools/Source/C/GenFw/GenFw.c | 27 ++++++++++++++++++++++-----
> 1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
> index 22e4e72..6569460 100644
> --- a/BaseTools/Source/C/GenFw/GenFw.c
> +++ b/BaseTools/Source/C/GenFw/GenFw.c
> @@ -2770,6 +2770,7 @@ Returns:
> {
> UINT32 Index;
> UINT32 DebugDirectoryEntryRva;
> + UINT32 DebugDirectoryEntrySize;
> UINT32 DebugDirectoryEntryFileOffset;
> UINT32 ExportDirectoryEntryRva;
> UINT32 ExportDirectoryEntryFileOffset;
> @@ -2781,12 +2782,14 @@ Returns:
> EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;
> EFI_IMAGE_SECTION_HEADER *SectionHeader;
> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
> + EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
> UINT32 *NewTimeStamp;
>
> //
> // Init variable.
> //
> DebugDirectoryEntryRva = 0;
> + DebugDirectoryEntrySize = 0;
> ExportDirectoryEntryRva = 0;
> ResourceDirectoryEntryRva = 0;
> DebugDirectoryEntryFileOffset = 0;
> @@ -2822,6 +2825,7 @@ Returns:
> if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
> DebugDirectoryEntryRva = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
> + DebugDirectoryEntrySize = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
> if (ZeroDebugFlag) {
> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
> @@ -2841,6 +2845,7 @@ Returns:
> if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
> DebugDirectoryEntryRva = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
> + DebugDirectoryEntrySize = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
> if (ZeroDebugFlag) {
> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
> @@ -2886,11 +2891,23 @@ Returns:
>
> if (DebugDirectoryEntryFileOffset != 0) {
> DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
> - DebugEntry->TimeDateStamp = 0;
> - mImageTimeStamp = 0;
> - if (ZeroDebugFlag) {
> - memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
> - memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
> + Index = 0;
> + for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
> + DebugEntry->TimeDateStamp = 0;
> + if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
> + memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
This memset() is the culprit.
According to gdb (all values decimal),
- Index = 1,
- DebugDirectoryEntrySize = 237,
- ZeroDebugFlag = 0.
These values look suspicious, because
sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) is 28, and
DebugDirectoryEntrySize (237) is not an integral multiple of that.
This is the first EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element:
> (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[0]
> $11 = {Characteristics = 0, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 2, SizeOfData = 209, RVA = 33292, FileOffset = 33292}
This is the second one (which triggers the crash, Index=1):
> (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[1]
> $12 = {Characteristics = 808534606, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 0, SizeOfData = 1836017711, RVA = 1634479973, FileOffset = 796094307}
The second element is obviously garbage (FileOffset = 796094307, and
then the memset() wanders off into the weeds).
This is the contents of the DataDirectory:
> (gdb) print Optional64Hdr->DataDirectory
> $22 = {{VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_EXPORT
> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_IMPORT
> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE
> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_EXCEPTION
> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_SECURITY
> {VirtualAddress = 36864, Size = 4096}, // EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC
> {VirtualAddress = 33264, Size = 237}, // EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_COPYRIGHT
> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_GLOBALPTR
> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_TLS
> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_LOAD_CONFIG
> {VirtualAddress = 0, Size = 0},
> {VirtualAddress = 0, Size = 0},
> {VirtualAddress = 0, Size = 0},
> {VirtualAddress = 0, Size = 0},
> {VirtualAddress = 0, Size = 0}}
According to <https://msdn.microsoft.com/en-us/library/ms809762.aspx>,
the format (element type) of the debug directory is specific to the
toolchain; the article says,
> To determine the number of entries in the Microsoft linker-generated
> debug directory, divide the size of the debug directory (found in the
> size field of the data directory entry) by the size of an
> IMAGE_DEBUG_DIRECTORY structure
Here's a hexdump of the debug directory (237 bytes starting from
(FileBuffer+33264)):
00000000 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00 |................|
00000010 d1 00 00 00 0c 82 00 00 0c 82 00 00 4e 42 31 30 |Ñ...........NB10|
00000020 00 00 00 00 00 00 00 00 00 00 00 00 2f 68 6f 6d |............/hom|
00000030 65 2f 6c 61 63 6f 73 2f 73 72 63 2f 75 70 73 74 |e/lacos/src/upst|
00000040 72 65 61 6d 2f 65 64 6b 32 2f 42 75 69 6c 64 2f |ream/edk2/Build/|
00000050 4f 76 6d 66 58 36 34 2f 4e 4f 4f 50 54 5f 47 43 |OvmfX64/NOOPT_GC|
00000060 43 34 38 2f 58 36 34 2f 4d 64 65 4d 6f 64 75 6c |C48/X64/MdeModul|
00000070 65 50 6b 67 2f 55 6e 69 76 65 72 73 61 6c 2f 52 |ePkg/Universal/R|
00000080 65 70 6f 72 74 53 74 61 74 75 73 43 6f 64 65 52 |eportStatusCodeR|
00000090 6f 75 74 65 72 2f 52 75 6e 74 69 6d 65 44 78 65 |outer/RuntimeDxe|
000000a0 2f 52 65 70 6f 72 74 53 74 61 74 75 73 43 6f 64 |/ReportStatusCod|
000000b0 65 52 6f 75 74 65 72 52 75 6e 74 69 6d 65 44 78 |eRouterRuntimeDx|
000000c0 65 2f 44 45 42 55 47 2f 52 65 70 6f 72 74 53 74 |e/DEBUG/ReportSt|
000000d0 61 74 75 73 43 6f 64 65 52 6f 75 74 65 72 52 75 |atusCodeRouterRu|
000000e0 6e 74 69 6d 65 44 78 65 2e 64 6c 6c 00 |ntimeDxe.dll.|
The bytes 0x4e 0x42 0x31 0x30 ("NB10"), at offset 28, can be seen above
as "Characteristics = 808534606". Googling this value, it seems to be a
signature / magic value: CVINFO_PDB20_CVSIGNATURE.
Then, VirtualAddress = 33264 is 0x81F0 hex, and "objdump -x" reports:
> Sections:
> Idx Name Size VMA LMA File off Algn Flags
> [...]
> 2 .build-id 00000024 00000000000081f0 00000000000081f0 000081f0 2**2 CONTENTS, READONLY
> [...]
> SYMBOL TABLE:
> [...]
> 00000000000081f0 l d .build-id 0000000000000000 .build-id
> [...]
Also, I found a function called pe_bfd_read_buildid() in the GNU
Binutils source that works with the PE_DEBUG_DATA entry of the data
directory.
In "BaseTools/Scripts/GccBase.lds", we have:
> /*
> * Retain the GNU build id but in a non-allocatable section so GenFw
> * does not copy it into the PE/COFF image.
> */
> .build-id (INFO) : { *(.note.gnu.build-id) }
This is from commit 7fd5d619806d ("BaseTools GCC: drop GNU notes section
from EFI image", 2016-07-27).
So... I don't really know what's happening here, but the DEBUG entry of
the data directory (i.e., the debug directory) doesn't seem to be
structured like GenFw expects.
> + memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
> + }
> + if (DebugEntry->Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
> + RsdsEntry = (EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *) (FileBuffer + DebugEntry->FileOffset);
> + if (RsdsEntry->Signature == CODEVIEW_SIGNATURE_RSDS) {
> + RsdsEntry->Unknown = 0;
> + RsdsEntry->Unknown2 = 0;
> + RsdsEntry->Unknown3 = 0;
> + RsdsEntry->Unknown4 = 0;
> + RsdsEntry->Unknown5 = 0;
> + }
> + }
> }
> }
>
>
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
2017-07-05 12:42 ` Laszlo Ersek
@ 2017-07-05 13:29 ` Laszlo Ersek
2017-07-05 15:29 ` Laszlo Ersek
2017-07-05 13:41 ` Leif Lindholm
1 sibling, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-07-05 13:29 UTC (permalink / raw)
To: Liming Gao, edk2-devel; +Cc: Ard Biesheuvel
On 07/05/17 14:42, Laszlo Ersek wrote:
> Hi Liming,
>
> Gerd's Jenkins CI reported a GenFw segfault, with this patch applied.
>
> I can reproduce the segfault locally. This is the command line:
>
>> GenFw \
>> -e DXE_RUNTIME_DRIVER \
>> -o .../Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.efi \
>> Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.dll
>
> Backtrace from gdb:
>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x00007ffff789c814 in __memset_sse2 () from /lib64/libc.so.6
>> Missing separate debuginfos, use: debuginfo-install glibc-2.17-194.el7.x86_64 libuuid-2.23.2-39.el7.x86_64
>> (gdb) where
>> #0 0x00007ffff789c814 in __memset_sse2 () from /lib64/libc.so.6
>> #1 0x0000000000407492 in ZeroDebugData (FileBuffer=0x636e50 "MZ", ZeroDebugFlag=0 '\000') at GenFw.c:2898
>> #2 0x0000000000406a4d in main (argc=0, argv=0x7fffffffd0b8) at GenFw.c:2591
>
> See below for the SIGSEGV location:
>
> On 07/03/17 07:21, Liming Gao wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=600
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>> ---
>> BaseTools/Source/C/GenFw/GenFw.c | 27 ++++++++++++++++++++++-----
>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
>> index 22e4e72..6569460 100644
>> --- a/BaseTools/Source/C/GenFw/GenFw.c
>> +++ b/BaseTools/Source/C/GenFw/GenFw.c
>> @@ -2770,6 +2770,7 @@ Returns:
>> {
>> UINT32 Index;
>> UINT32 DebugDirectoryEntryRva;
>> + UINT32 DebugDirectoryEntrySize;
>> UINT32 DebugDirectoryEntryFileOffset;
>> UINT32 ExportDirectoryEntryRva;
>> UINT32 ExportDirectoryEntryFileOffset;
>> @@ -2781,12 +2782,14 @@ Returns:
>> EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;
>> EFI_IMAGE_SECTION_HEADER *SectionHeader;
>> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
>> + EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
>> UINT32 *NewTimeStamp;
>>
>> //
>> // Init variable.
>> //
>> DebugDirectoryEntryRva = 0;
>> + DebugDirectoryEntrySize = 0;
>> ExportDirectoryEntryRva = 0;
>> ResourceDirectoryEntryRva = 0;
>> DebugDirectoryEntryFileOffset = 0;
>> @@ -2822,6 +2825,7 @@ Returns:
>> if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
>> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
>> DebugDirectoryEntryRva = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
>> + DebugDirectoryEntrySize = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
>> if (ZeroDebugFlag) {
>> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
>> Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
>> @@ -2841,6 +2845,7 @@ Returns:
>> if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
>> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
>> DebugDirectoryEntryRva = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
>> + DebugDirectoryEntrySize = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
>> if (ZeroDebugFlag) {
>> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
>> Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
>> @@ -2886,11 +2891,23 @@ Returns:
>>
>> if (DebugDirectoryEntryFileOffset != 0) {
>> DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
>> - DebugEntry->TimeDateStamp = 0;
>> - mImageTimeStamp = 0;
>> - if (ZeroDebugFlag) {
>> - memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
>> - memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
>> + Index = 0;
>> + for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
>> + DebugEntry->TimeDateStamp = 0;
>> + if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
>> + memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
>
> This memset() is the culprit.
>
> According to gdb (all values decimal),
> - Index = 1,
> - DebugDirectoryEntrySize = 237,
> - ZeroDebugFlag = 0.
>
> These values look suspicious, because
> sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) is 28, and
> DebugDirectoryEntrySize (237) is not an integral multiple of that.
>
> This is the first EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element:
>
>> (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[0]
>> $11 = {Characteristics = 0, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 2, SizeOfData = 209, RVA = 33292, FileOffset = 33292}
>
> This is the second one (which triggers the crash, Index=1):
>
>> (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[1]
>> $12 = {Characteristics = 808534606, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 0, SizeOfData = 1836017711, RVA = 1634479973, FileOffset = 796094307}
>
> The second element is obviously garbage (FileOffset = 796094307, and
> then the memset() wanders off into the weeds).
>
> This is the contents of the DataDirectory:
>
>> (gdb) print Optional64Hdr->DataDirectory
>> $22 = {{VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_EXPORT
>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_IMPORT
>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE
>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_EXCEPTION
>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_SECURITY
>> {VirtualAddress = 36864, Size = 4096}, // EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC
>> {VirtualAddress = 33264, Size = 237}, // EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_COPYRIGHT
>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_GLOBALPTR
>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_TLS
>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_LOAD_CONFIG
>> {VirtualAddress = 0, Size = 0},
>> {VirtualAddress = 0, Size = 0},
>> {VirtualAddress = 0, Size = 0},
>> {VirtualAddress = 0, Size = 0},
>> {VirtualAddress = 0, Size = 0}}
>
> According to <https://msdn.microsoft.com/en-us/library/ms809762.aspx>,
> the format (element type) of the debug directory is specific to the
> toolchain; the article says,
>
>> To determine the number of entries in the Microsoft linker-generated
>> debug directory, divide the size of the debug directory (found in the
>> size field of the data directory entry) by the size of an
>> IMAGE_DEBUG_DIRECTORY structure
>
> Here's a hexdump of the debug directory (237 bytes starting from
> (FileBuffer+33264)):
>
> 00000000 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00 |................|
> 00000010 d1 00 00 00 0c 82 00 00 0c 82 00 00 4e 42 31 30 |Ñ...........NB10|
> 00000020 00 00 00 00 00 00 00 00 00 00 00 00 2f 68 6f 6d |............/hom|
> 00000030 65 2f 6c 61 63 6f 73 2f 73 72 63 2f 75 70 73 74 |e/lacos/src/upst|
> 00000040 72 65 61 6d 2f 65 64 6b 32 2f 42 75 69 6c 64 2f |ream/edk2/Build/|
> 00000050 4f 76 6d 66 58 36 34 2f 4e 4f 4f 50 54 5f 47 43 |OvmfX64/NOOPT_GC|
> 00000060 43 34 38 2f 58 36 34 2f 4d 64 65 4d 6f 64 75 6c |C48/X64/MdeModul|
> 00000070 65 50 6b 67 2f 55 6e 69 76 65 72 73 61 6c 2f 52 |ePkg/Universal/R|
> 00000080 65 70 6f 72 74 53 74 61 74 75 73 43 6f 64 65 52 |eportStatusCodeR|
> 00000090 6f 75 74 65 72 2f 52 75 6e 74 69 6d 65 44 78 65 |outer/RuntimeDxe|
> 000000a0 2f 52 65 70 6f 72 74 53 74 61 74 75 73 43 6f 64 |/ReportStatusCod|
> 000000b0 65 52 6f 75 74 65 72 52 75 6e 74 69 6d 65 44 78 |eRouterRuntimeDx|
> 000000c0 65 2f 44 45 42 55 47 2f 52 65 70 6f 72 74 53 74 |e/DEBUG/ReportSt|
> 000000d0 61 74 75 73 43 6f 64 65 52 6f 75 74 65 72 52 75 |atusCodeRouterRu|
> 000000e0 6e 74 69 6d 65 44 78 65 2e 64 6c 6c 00 |ntimeDxe.dll.|
>
> The bytes 0x4e 0x42 0x31 0x30 ("NB10"), at offset 28, can be seen above
> as "Characteristics = 808534606". Googling this value, it seems to be a
> signature / magic value: CVINFO_PDB20_CVSIGNATURE.
>
> Then, VirtualAddress = 33264 is 0x81F0 hex, and "objdump -x" reports:
>
>> Sections:
>> Idx Name Size VMA LMA File off Algn Flags
>> [...]
>> 2 .build-id 00000024 00000000000081f0 00000000000081f0 000081f0 2**2 CONTENTS, READONLY
>> [...]
>> SYMBOL TABLE:
>> [...]
>> 00000000000081f0 l d .build-id 0000000000000000 .build-id
>> [...]
>
> Also, I found a function called pe_bfd_read_buildid() in the GNU
> Binutils source that works with the PE_DEBUG_DATA entry of the data
> directory.
>
> In "BaseTools/Scripts/GccBase.lds", we have:
>
>> /*
>> * Retain the GNU build id but in a non-allocatable section so GenFw
>> * does not copy it into the PE/COFF image.
>> */
>> .build-id (INFO) : { *(.note.gnu.build-id) }
>
> This is from commit 7fd5d619806d ("BaseTools GCC: drop GNU notes section
> from EFI image", 2016-07-27).
>
> So... I don't really know what's happening here, but the DEBUG entry of
> the data directory (i.e., the debug directory) doesn't seem to be
> structured like GenFw expects.
After looking a bit more at the hexdump, it seems that the first entry
in the debug directory is indeed of type
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY, and its Type field is 2
(EFI_IMAGE_DEBUG_TYPE_CODEVIEW), so GenFw branches to the "RsdsEntry" logic.
The second entry however is bogus -- apparently a codeview structure
(containing the build-id) is placed into the debug directory?
typedef struct _CV_INFO_PDB20
{
char CvHeader[4]; // NB10
char Offset[4]; // zeroes
char Signature[4]; // zeroes
char Age[4]; // zeroes
char PdbFileName[]; // filename
} CV_INFO_PDB20;
This is from GNU Binutils commit 61e2488cd849 ("Add support for
generating and inserting build IDs into COFF binaries.", 2014-04-08).
What I don't understand is why "objdump -p" does not decode the debug
directory for me. :/
Thanks
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
2017-07-05 12:42 ` Laszlo Ersek
2017-07-05 13:29 ` Laszlo Ersek
@ 2017-07-05 13:41 ` Leif Lindholm
1 sibling, 0 replies; 6+ messages in thread
From: Leif Lindholm @ 2017-07-05 13:41 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Liming Gao, edk2-devel, Ard Biesheuvel, Kinney, Michael D,
Andrew Fish, Yonghong Zhu
Many thanks for tracking this down Laszlo - we'd stumbled over it
ourselves this morning.
Liming, Mike, Andrew: this currently makes the BaseTools unusable with
gcc toolchains. Can we revert this commit until this has been
resolved?
Best Regards,
Leif
On Wed, Jul 05, 2017 at 02:42:15PM +0200, Laszlo Ersek wrote:
> Hi Liming,
>
> Gerd's Jenkins CI reported a GenFw segfault, with this patch applied.
>
> I can reproduce the segfault locally. This is the command line:
>
> > GenFw \
> > -e DXE_RUNTIME_DRIVER \
> > -o .../Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.efi \
> > Build/OvmfX64/NOOPT_GCC48/X64/MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe/DEBUG/ReportStatusCodeRouterRuntimeDxe.dll
>
> Backtrace from gdb:
>
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x00007ffff789c814 in __memset_sse2 () from /lib64/libc.so.6
> > Missing separate debuginfos, use: debuginfo-install glibc-2.17-194.el7.x86_64 libuuid-2.23.2-39.el7.x86_64
> > (gdb) where
> > #0 0x00007ffff789c814 in __memset_sse2 () from /lib64/libc.so.6
> > #1 0x0000000000407492 in ZeroDebugData (FileBuffer=0x636e50 "MZ", ZeroDebugFlag=0 '\000') at GenFw.c:2898
> > #2 0x0000000000406a4d in main (argc=0, argv=0x7fffffffd0b8) at GenFw.c:2591
>
> See below for the SIGSEGV location:
>
> On 07/03/17 07:21, Liming Gao wrote:
> > https://bugzilla.tianocore.org/show_bug.cgi?id=600
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Liming Gao <liming.gao@intel.com>
> > ---
> > BaseTools/Source/C/GenFw/GenFw.c | 27 ++++++++++++++++++++++-----
> > 1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c
> > index 22e4e72..6569460 100644
> > --- a/BaseTools/Source/C/GenFw/GenFw.c
> > +++ b/BaseTools/Source/C/GenFw/GenFw.c
> > @@ -2770,6 +2770,7 @@ Returns:
> > {
> > UINT32 Index;
> > UINT32 DebugDirectoryEntryRva;
> > + UINT32 DebugDirectoryEntrySize;
> > UINT32 DebugDirectoryEntryFileOffset;
> > UINT32 ExportDirectoryEntryRva;
> > UINT32 ExportDirectoryEntryFileOffset;
> > @@ -2781,12 +2782,14 @@ Returns:
> > EFI_IMAGE_OPTIONAL_HEADER64 *Optional64Hdr;
> > EFI_IMAGE_SECTION_HEADER *SectionHeader;
> > EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *DebugEntry;
> > + EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *RsdsEntry;
> > UINT32 *NewTimeStamp;
> >
> > //
> > // Init variable.
> > //
> > DebugDirectoryEntryRva = 0;
> > + DebugDirectoryEntrySize = 0;
> > ExportDirectoryEntryRva = 0;
> > ResourceDirectoryEntryRva = 0;
> > DebugDirectoryEntryFileOffset = 0;
> > @@ -2822,6 +2825,7 @@ Returns:
> > if (Optional32Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
> > Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
> > DebugDirectoryEntryRva = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
> > + DebugDirectoryEntrySize = Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
> > if (ZeroDebugFlag) {
> > Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
> > Optional32Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
> > @@ -2841,6 +2845,7 @@ Returns:
> > if (Optional64Hdr->NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_DEBUG && \
> > Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size != 0) {
> > DebugDirectoryEntryRva = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress;
> > + DebugDirectoryEntrySize = Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size;
> > if (ZeroDebugFlag) {
> > Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
> > Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
> > @@ -2886,11 +2891,23 @@ Returns:
> >
> > if (DebugDirectoryEntryFileOffset != 0) {
> > DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
> > - DebugEntry->TimeDateStamp = 0;
> > - mImageTimeStamp = 0;
> > - if (ZeroDebugFlag) {
> > - memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
> > - memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
> > + Index = 0;
> > + for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
> > + DebugEntry->TimeDateStamp = 0;
> > + if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
> > + memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
>
> This memset() is the culprit.
>
> According to gdb (all values decimal),
> - Index = 1,
> - DebugDirectoryEntrySize = 237,
> - ZeroDebugFlag = 0.
>
> These values look suspicious, because
> sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) is 28, and
> DebugDirectoryEntrySize (237) is not an integral multiple of that.
>
> This is the first EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element:
>
> > (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[0]
> > $11 = {Characteristics = 0, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 2, SizeOfData = 209, RVA = 33292, FileOffset = 33292}
>
> This is the second one (which triggers the crash, Index=1):
>
> > (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[1]
> > $12 = {Characteristics = 808534606, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 0, SizeOfData = 1836017711, RVA = 1634479973, FileOffset = 796094307}
>
> The second element is obviously garbage (FileOffset = 796094307, and
> then the memset() wanders off into the weeds).
>
> This is the contents of the DataDirectory:
>
> > (gdb) print Optional64Hdr->DataDirectory
> > $22 = {{VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_EXPORT
> > {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_IMPORT
> > {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE
> > {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_EXCEPTION
> > {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_SECURITY
> > {VirtualAddress = 36864, Size = 4096}, // EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC
> > {VirtualAddress = 33264, Size = 237}, // EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
> > {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_COPYRIGHT
> > {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_GLOBALPTR
> > {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_TLS
> > {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_LOAD_CONFIG
> > {VirtualAddress = 0, Size = 0},
> > {VirtualAddress = 0, Size = 0},
> > {VirtualAddress = 0, Size = 0},
> > {VirtualAddress = 0, Size = 0},
> > {VirtualAddress = 0, Size = 0}}
>
> According to <https://msdn.microsoft.com/en-us/library/ms809762.aspx>,
> the format (element type) of the debug directory is specific to the
> toolchain; the article says,
>
> > To determine the number of entries in the Microsoft linker-generated
> > debug directory, divide the size of the debug directory (found in the
> > size field of the data directory entry) by the size of an
> > IMAGE_DEBUG_DIRECTORY structure
>
> Here's a hexdump of the debug directory (237 bytes starting from
> (FileBuffer+33264)):
>
> 00000000 00 00 00 00 00 00 00 00 00 00 00 00 02 00 00 00 |................|
> 00000010 d1 00 00 00 0c 82 00 00 0c 82 00 00 4e 42 31 30 |Ñ...........NB10|
> 00000020 00 00 00 00 00 00 00 00 00 00 00 00 2f 68 6f 6d |............/hom|
> 00000030 65 2f 6c 61 63 6f 73 2f 73 72 63 2f 75 70 73 74 |e/lacos/src/upst|
> 00000040 72 65 61 6d 2f 65 64 6b 32 2f 42 75 69 6c 64 2f |ream/edk2/Build/|
> 00000050 4f 76 6d 66 58 36 34 2f 4e 4f 4f 50 54 5f 47 43 |OvmfX64/NOOPT_GC|
> 00000060 43 34 38 2f 58 36 34 2f 4d 64 65 4d 6f 64 75 6c |C48/X64/MdeModul|
> 00000070 65 50 6b 67 2f 55 6e 69 76 65 72 73 61 6c 2f 52 |ePkg/Universal/R|
> 00000080 65 70 6f 72 74 53 74 61 74 75 73 43 6f 64 65 52 |eportStatusCodeR|
> 00000090 6f 75 74 65 72 2f 52 75 6e 74 69 6d 65 44 78 65 |outer/RuntimeDxe|
> 000000a0 2f 52 65 70 6f 72 74 53 74 61 74 75 73 43 6f 64 |/ReportStatusCod|
> 000000b0 65 52 6f 75 74 65 72 52 75 6e 74 69 6d 65 44 78 |eRouterRuntimeDx|
> 000000c0 65 2f 44 45 42 55 47 2f 52 65 70 6f 72 74 53 74 |e/DEBUG/ReportSt|
> 000000d0 61 74 75 73 43 6f 64 65 52 6f 75 74 65 72 52 75 |atusCodeRouterRu|
> 000000e0 6e 74 69 6d 65 44 78 65 2e 64 6c 6c 00 |ntimeDxe.dll.|
>
> The bytes 0x4e 0x42 0x31 0x30 ("NB10"), at offset 28, can be seen above
> as "Characteristics = 808534606". Googling this value, it seems to be a
> signature / magic value: CVINFO_PDB20_CVSIGNATURE.
>
> Then, VirtualAddress = 33264 is 0x81F0 hex, and "objdump -x" reports:
>
> > Sections:
> > Idx Name Size VMA LMA File off Algn Flags
> > [...]
> > 2 .build-id 00000024 00000000000081f0 00000000000081f0 000081f0 2**2 CONTENTS, READONLY
> > [...]
> > SYMBOL TABLE:
> > [...]
> > 00000000000081f0 l d .build-id 0000000000000000 .build-id
> > [...]
>
> Also, I found a function called pe_bfd_read_buildid() in the GNU
> Binutils source that works with the PE_DEBUG_DATA entry of the data
> directory.
>
> In "BaseTools/Scripts/GccBase.lds", we have:
>
> > /*
> > * Retain the GNU build id but in a non-allocatable section so GenFw
> > * does not copy it into the PE/COFF image.
> > */
> > .build-id (INFO) : { *(.note.gnu.build-id) }
>
> This is from commit 7fd5d619806d ("BaseTools GCC: drop GNU notes section
> from EFI image", 2016-07-27).
>
> So... I don't really know what's happening here, but the DEBUG entry of
> the data directory (i.e., the debug directory) doesn't seem to be
> structured like GenFw expects.
>
> > + memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
> > + }
> > + if (DebugEntry->Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
> > + RsdsEntry = (EFI_IMAGE_DEBUG_CODEVIEW_RSDS_ENTRY *) (FileBuffer + DebugEntry->FileOffset);
> > + if (RsdsEntry->Signature == CODEVIEW_SIGNATURE_RSDS) {
> > + RsdsEntry->Unknown = 0;
> > + RsdsEntry->Unknown2 = 0;
> > + RsdsEntry->Unknown3 = 0;
> > + RsdsEntry->Unknown4 = 0;
> > + RsdsEntry->Unknown5 = 0;
> > + }
> > + }
> > }
> > }
> >
> >
>
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain
2017-07-05 13:29 ` Laszlo Ersek
@ 2017-07-05 15:29 ` Laszlo Ersek
0 siblings, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2017-07-05 15:29 UTC (permalink / raw)
To: Liming Gao, edk2-devel
Cc: Ard Biesheuvel, Leif Lindholm (Linaro address), Zhu, Yonghong
I understand it now. The article at
<http://www.debuginfo.com/articles/debuginfomatch.html> was of great help.
On 07/05/17 15:29, Laszlo Ersek wrote:
> On 07/05/17 14:42, Laszlo Ersek wrote:
>> On 07/03/17 07:21, Liming Gao wrote:
>>> @@ -2886,11 +2891,23 @@ Returns:
>>>
>>> if (DebugDirectoryEntryFileOffset != 0) {
>>> DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset);
>>> - DebugEntry->TimeDateStamp = 0;
>>> - mImageTimeStamp = 0;
>>> - if (ZeroDebugFlag) {
>>> - memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
>>> - memset (DebugEntry, 0, sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY));
>>> + Index = 0;
>>> + for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) {
>>> + DebugEntry->TimeDateStamp = 0;
>>> + if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
>>> + memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData);
>>
>> This memset() is the culprit.
>>
>> According to gdb (all values decimal),
>> - Index = 1,
>> - DebugDirectoryEntrySize = 237,
>> - ZeroDebugFlag = 0.
>>
>> These values look suspicious, because
>> sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY) is 28, and
>> DebugDirectoryEntrySize (237) is not an integral multiple of that.
>>
>> This is the first EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element:
>>
>>> (gdb) print ((EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset))[0]
>>> $11 = {Characteristics = 0, TimeDateStamp = 0, MajorVersion = 0, MinorVersion = 0, Type = 2, SizeOfData = 209, RVA = 33292, FileOffset = 33292}
Notice this. The first entry (which is valid, with type =
EFI_IMAGE_DEBUG_TYPE_CODEVIEW), points at offset 33292, and the
pointed-to data (which is a CodeView information block) has size 209.
This means that the first byte *past* the CodeView information is at
offset 33292 + 209 = 33501.
Now, look at the data dictionary again:
>>> (gdb) print Optional64Hdr->DataDirectory
>>> $22 = {{VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_EXPORT
>>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_IMPORT
>>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_RESOURCE
>>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_EXCEPTION
>>> {VirtualAddress = 0, Size = 0}, // EFI_IMAGE_DIRECTORY_ENTRY_SECURITY
>>> {VirtualAddress = 36864, Size = 4096}, // EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC
>>> {VirtualAddress = 33264, Size = 237}, // EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
Notice this: 33264 + 237 = 33501. The same end offset!
And then: 237 - 209 = 28, which is exactly the size of
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.
So, what happens is that the GNU linker (or some other bin-util that
produces the input file for GenFw)
(1) creates a valid EFI_IMAGE_DEBUG_DIRECTORY_ENTRY, of type
EFI_IMAGE_DEBUG_TYPE_CODEVIEW, with size = 28,
(2) this element points to a valid CV_INFO_PDB20 structure (having NB10
for signature), with size = 209,
(3) The CV_INFO_PDB20 structure *immediately* follows the
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY element that points to it -- this is all
fine,
(4) *however*, the pointed-to CV_INFO_PDB20 element (from (3)) is *also*
included in the size of the debug directory, even though the size of the
debug directory should *only* describe the debug directory entry from (1)!
So, this is most certainly a GNU Binutils bug.
We can catch it though: as soon as we find an
EFI_IMAGE_DEBUG_DIRECTORY_ENTRY whose FileOffset field points into the
debug directory itself, we know that the debug directory's size has to
be truncated at once to that offset.
Let me see if I can write a patch for this.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-05 15:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-03 5:21 [Patch] BaseTools: Update GenFw to clear unused debug entry generated by VS tool chain Liming Gao
2017-07-03 5:28 ` Zhu, Yonghong
2017-07-05 12:42 ` Laszlo Ersek
2017-07-05 13:29 ` Laszlo Ersek
2017-07-05 15:29 ` Laszlo Ersek
2017-07-05 13:41 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox