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 <[email protected]> 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: [email protected] [mailto:[email protected]] >> Sent: Friday, August 11, 2017 1:08 PM >> To: Gao, Liming <[email protected]> >> Cc: Kinney, Michael D <[email protected]>; edk2- >> [email protected] >> Subject: Re: [edk2] [Patch] BaseTools: Fix Segmentation fault: 11 when build >> AppPkg with XCODE5 >> >> >>> On Aug 10, 2017, at 9:48 PM, Gao, Liming <[email protected]> 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: [email protected] [mailto:[email protected]] >>> Sent: Friday, August 11, 2017 12:59 AM >>> To: Gao, Liming <[email protected]> >>> Cc: Zhu, Yonghong <[email protected]>; Kinney, Michael D >> <[email protected]>; [email protected] >>> Subject: Re: [edk2] [Patch] BaseTools: Fix Segmentation fault: 11 when build >> AppPkg with XCODE5 >>> >>> >>> On Aug 10, 2017, at 3:38 AM, Gao, Liming >> <[email protected]<mailto:[email protected]>> 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: [email protected]<mailto:[email protected]> [mailto:[email protected]] >>> Sent: Tuesday, August 8, 2017 12:26 AM >>> To: Zhu, Yonghong >> <[email protected]<mailto:[email protected]>> >>> Cc: [email protected]<mailto:[email protected]>; Gao, Liming >> <[email protected]<mailto:[email protected]>>; Kinney, Michael D >> <[email protected]<mailto:[email protected]>> >>> 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 >> <[email protected]<mailto:[email protected]><mailto:yongho >> [email protected]>> 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 >> <[email protected]<mailto:[email protected]><mailto:liming.gao@int >> el.com>> >>> Cc: Michael D Kinney >> <[email protected]<mailto:[email protected]><mailto:m >> [email protected]>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Andrew Fish >> <[email protected]<mailto:[email protected]><mailto:[email protected]>> >>> --- >>> 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 >>> [email protected]<mailto:[email protected]> >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> [email protected] >>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
