回复: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug entry read error after it is found

["gaoliming" <gaoliming@byosoft.com.cn>]

Ard:

> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Ard
> Biesheuvel
> 发送时间: 2023年4月27日 22:47
> 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn
> 抄送: Michael Kubacki <michael.kubacki@microsoft.com>
> 主题: Re: [edk2-devel] [PATCH] MdePkg BasePeCoffLib: Ignore the debug
> entry read error after it is found
> 
> On Thu, 27 Apr 2023 at 07:17, gaoliming via groups.io
> <gaoliming=byosoft.com.cn@groups.io> wrote:
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4425
> >
> > This change is to support the pre-built EFI image on the old Edk2 code.
> > Old Edk2 GenFw tool generates the wrong debug entry in EFI image.
> > Those pre-built images can be loaded before d6457b309.
> >
> > Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> > ---
> >  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > index 4b71176a0c..4636842eeb 100644
> > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > @@ -735,7 +735,7 @@ PeCoffLoaderGetImageInfo (
> >                                       &Size,
> >                                       &DebugEntry
> >                                       );
> > -          if (RETURN_ERROR (Status) || (Size != ReadSize)) {
> > +          if ((ImageContext->DebugDirectoryEntryRva == 0) &&
> (RETURN_ERROR (Status) || (Size != ReadSize))) {
> 
> I'm not sure I understand how this fixes anything. Why is the
> condition 'RVA == 0' significant here?
> 
> Would something like the below perhaps work as well?
> 

DebugDirectoryEntryRva will set to the real value if DEBUG_TYPE_CODEVIEW is found. 
If its value is not zero, it means DebugEntry has been found. This is not an error, and don't need to report error here. 

> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -755,6 +755,17 @@ PeCoffLoaderGetImageInfo (
>                ImageContext->ImageSize += DebugEntry.SizeOfData;
>              }
> 
> +            //
> +            // Implementations of GenFw before commit
> 60e85a39fe49071 will
> +            // concatenate the debug directory entry and the codeview
> entry,
> +            // and erroneously put the combined size into the debug
> directory
> +            // entry's size field. If this is the case, no other relevant
> +            // directory entries can exist, and we can terminate here.
> +            //
> +            if (DebugEntry.FileOffset ==
> (DebugDirectoryEntryFileOffset + Index)) {
> +              break;
> +            }
> +
d6457b309 adds DEBUG_TYPE_EX_DLLCHARACTERISTICS support. So, there may be two DEBUG entries in one EFI image. 
One is DEBUG_TYPE_CODEVIEW, another is DEBUG_TYPE_EX_DLLCHARACTERISTICS. 

Here break after DEBUG_TYPE_CODEVIEW is found. So, is there the chance to search DEBUG_TYPE_EX_DLLCHARACTERISTICS?

Thanks
Liming
>              continue;
>            }
> 
> 
> 
>