Liming, Your right I'm off by one. The signature is in the thing EFI_IMAGE_DEBUG_DIRECTORY_ENTRY points to.
I do have a question. Why does the PE/COFF code have an extra step. The TE images are converted PE/COFF images so would not you need this code in both places? Is there some other assumption in play here? https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L769 // // 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; } https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L862 if (DebugEntry.Type == EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { ImageContext->DebugDirectoryEntryRva = (UINT32) (DebugDirectoryEntryRva + Index); return RETURN_SUCCESS; } Thanks, Andrew Fish > On Aug 10, 2017, at 10:49 PM, Gao, Liming <liming....@intel.com> wrote: > > 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
