Andrew:
I get your point. I agree edk2 firmware to support old EFI image. And, I
review the change in BasePeCoffLib. But, I think it is unnecessary. Below is
the current logic. On the first loop, DebugEntry.Type is
EFI_IMAGE_DEBUG_TYPE_CODEVIEW, then will return. It doesn't enter into second
loop. For mtoc debug entry, its DebugEntry.Type is also
EFI_IMAGE_DEBUG_TYPE_CODEVIEW, its DEBUG_CODEVIEW_ ENTRY Signature is
CODEVIEW_SIGNATURE_MTOC. So, it will be handled in this logic.
for (Index = 0; Index < DebugDirectoryEntry->Size; Index += sizeof
(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY)) {
//
// Read next debug directory entry
//
Size = sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
...
//
// From PeCoff spec, when DebugEntry.RVA == 0 means this debug info
will not load into memory.
// Here we will always load EFI_IMAGE_DEBUG_TYPE_CODEVIEW type debug
info. so need adjust the
// ImageContext->ImageSize when DebugEntry.RVA == 0.
//
if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
ImageContext->DebugDirectoryEntryRva = (UINT32)
(DebugDirectoryEntryRva + Index);
if (DebugEntry.RVA == 0 && DebugEntry.FileOffset != 0) {
ImageContext->ImageSize += DebugEntry.SizeOfData;
}
return RETURN_SUCCESS;
}
}
Thanks
Liming
>-----Original Message-----
>From: af...@apple.com [mailto:af...@apple.com]
>Sent: Friday, August 11, 2017 1:08 PM
>To: Gao, Liming <liming....@intel.com>
>Cc: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-
>de...@lists.01.org
>Subject: Re: [edk2] [Patch] BaseTools: Fix Segmentation fault: 11 when build
>AppPkg with XCODE5
>
>
>> On Aug 10, 2017, at 9:48 PM, Gao, Liming <liming....@intel.com> wrote:
>>
>> Andrew:
>> Edk2 Build system calls GenFw to generate EFI image in build phase. Even if
>this image is not built into BIOS image, its EFI image will be generated by
>GenFw. So, only if this EFI image is built from EDK2 project, it can be updated
>by GenFw tool. You can see this step in build_rule.txt to convert .dll to .efi
>image.
>>
>
>Gao,
>
>We can fix the future for edk2, but we can't change the past, or change other
>build systems. It is not safe to assume that every EFI executable that the DXE
>Core will see is as new as the firmware, or was even built via the edk2. Option
>ROMs and OS loaders are good examples of this issue.
>
>Thanks,
>
>Andrew Fish
>
>> Thanks
>> Liming
>> From: af...@apple.com [mailto:af...@apple.com]
>> Sent: Friday, August 11, 2017 12:59 AM
>> To: Gao, Liming <liming....@intel.com>
>> Cc: Zhu, Yonghong <yonghong....@intel.com>; Kinney, Michael D
><michael.d.kin...@intel.com>; edk2-devel@lists.01.org
>> Subject: Re: [edk2] [Patch] BaseTools: Fix Segmentation fault: 11 when build
>AppPkg with XCODE5
>>
>>
>> On Aug 10, 2017, at 3:38 AM, Gao, Liming
><liming....@intel.com<mailto:liming....@intel.com>> wrote:
>>
>> Andrew:
>> If this is a mtoc bug, I suggest to update GenFw to always correct it in the
>generated EFI image. If so, the EFI image is always correct. There is no change
>requirement in PeCoff library in MdePkg.
>>
>>
>> Liming,
>>
>> EFI supports loading PE/COFF images that are not built at the same time as
>the platform firmware (UEFI Shell, OS loader), and that is why I added the fix
>to the PE/COFF library code.
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>
>> Thanks
>> Liming
>> From: af...@apple.com<mailto:af...@apple.com> [mailto:af...@apple.com]
>> Sent: Tuesday, August 8, 2017 12:26 AM
>> To: Zhu, Yonghong
><yonghong....@intel.com<mailto:yonghong....@intel.com>>
>> Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Gao, Liming
><liming....@intel.com<mailto:liming....@intel.com>>; Kinney, Michael D
><michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>
>> Subject: Re: [Patch] BaseTools: Fix Segmentation fault: 11 when build
>AppPkg with XCODE5
>>
>> Should that be:
>> Contributed-under: TianoCore Contribution Agreement 1.1
>>
>> I also noticed the PeCoff lib is going to loop and reload the .debug suction
>due to this mtoc bug, so it would be good to harden that code too.
>>
>> git diff MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> index 8d1daba..1e4c67e 100644
>> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> @@ -771,6 +771,8 @@ PeCoffLoaderGetImageInfo (
>> }
>>
>> return RETURN_SUCCESS;
>> + } else if (DebugEntry.Type == CODEVIEW_SIGNATURE_MTOC) {
>> + return RETURN_SUCCESS;
>> }
>> }
>> }
>> @@ -862,6 +864,8 @@ PeCoffLoaderGetImageInfo (
>> if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) {
>> ImageContext->DebugDirectoryEntryRva = (UINT32)
>(DebugDirectoryEntryRva + Index);
>> return RETURN_SUCCESS;
>> + } else if (DebugEntry.Type == CODEVIEW_SIGNATURE_MTOC) {
>> + return RETURN_SUCCESS;
>> }
>> }
>> }
>>
>>
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=663
>> Contributed-under: TianoCore Contribution Agreement 1.1
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>
>> On Aug 6, 2017, at 9:00 PM, Yonghong Zhu
><yonghong....@intel.com<mailto:yonghong....@intel.com><mailto:yongho
>ng....@intel.com>> wrote:
>>
>> it is a bug in mtoc setting the size of the debug directory entry to
>> the size of the .debug section, not the size of the
>> EFI_IMAGE_DEBUG_DIRECTORY_ENTRY. It was causing a loop to iterate and
>> get bogus EFI_IMAGE_DEBUG_DIRECTORY_ENTRY data and pass that to
>> memset() and boom.
>>
>> Cc: Liming Gao
><liming....@intel.com<mailto:liming....@intel.com><mailto:liming.gao@int
>el.com>>
>> Cc: Michael D Kinney
><michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com><mailto:m
>ichael.d.kin...@intel.com>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Andrew Fish
><af...@apple.com<mailto:af...@apple.com><mailto:af...@apple.com>>
>> ---
>> BaseTools/Source/C/GenFw/GenFw.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/BaseTools/Source/C/GenFw/GenFw.c
>b/BaseTools/Source/C/GenFw/GenFw.c
>> index 246deb0..af60c92 100644
>> --- a/BaseTools/Source/C/GenFw/GenFw.c
>> +++ b/BaseTools/Source/C/GenFw/GenFw.c
>> @@ -2813,10 +2813,11 @@ Returns:
>> //
>> // Get Debug, Export and Resource EntryTable RVA address.
>> // Resource Directory entry need to review.
>> //
>> Optional32Hdr = (EFI_IMAGE_OPTIONAL_HEADER32 *) ((UINT8*) FileHdr +
>sizeof (EFI_IMAGE_FILE_HEADER));
>> + Optional64Hdr = (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr
>+ sizeof (EFI_IMAGE_FILE_HEADER));
>> if (Optional32Hdr->Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *)
>Optional32Hdr + FileHdr->SizeOfOptionalHeader);
>> if (Optional32Hdr->NumberOfRvaAndSizes >
>EFI_IMAGE_DIRECTORY_ENTRY_EXPORT && \
>> Optional32Hdr-
>>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].Size != 0) {
>> ExportDirectoryEntryRva = Optional32Hdr-
>>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].VirtualAddress;
>> @@ -2833,11 +2834,10 @@ Returns:
>> Optional32Hdr-
>>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = 0;
>> Optional32Hdr-
>>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].VirtualAddress = 0;
>> }
>> }
>> } else {
>> - Optional64Hdr = (EFI_IMAGE_OPTIONAL_HEADER64 *) ((UINT8*) FileHdr
>+ sizeof (EFI_IMAGE_FILE_HEADER));
>> SectionHeader = (EFI_IMAGE_SECTION_HEADER *) ((UINT8 *)
>Optional64Hdr + FileHdr->SizeOfOptionalHeader);
>> if (Optional64Hdr->NumberOfRvaAndSizes >
>EFI_IMAGE_DIRECTORY_ENTRY_EXPORT && \
>> Optional64Hdr-
>>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].Size != 0) {
>> ExportDirectoryEntryRva = Optional64Hdr-
>>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_EXPORT].VirtualAddress;
>> }
>> @@ -2907,10 +2907,20 @@ Returns:
>> RsdsEntry->Unknown = 0;
>> RsdsEntry->Unknown2 = 0;
>> RsdsEntry->Unknown3 = 0;
>> RsdsEntry->Unknown4 = 0;
>> RsdsEntry->Unknown5 = 0;
>> + } else if (RsdsEntry->Signature == CODEVIEW_SIGNATURE_MTOC) {
>> + // MTOC sets DebugDirectoryEntrySize to size of the .debug
>> section,
>so fix it.
>> + if (!ZeroDebugFlag) {
>> + if (Optional32Hdr->Magic ==
>EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>> + Optional32Hdr-
>>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = sizeof
>(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>> + } else {
>> + Optional64Hdr-
>>DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size = sizeof
>(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>> + }
>> + }
>> + break;
>> }
>> }
>> }
>> }
>>
>> --
>> 2.6.1.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
