Re: Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform

["Carsey, Jaben" <jaben.carsey@intel.com>]

Laszlo,

As Tim clarified, the API already fail when the shell protocol was not present and the shell already needs the UnicodeCollation protocol.

The goal here is not to allow the library to function without the shell protocol (and thereby the UnicodeCollation protocol).  The goal is to allow the constructor to complete successfully. So that a DXE driver (or other driver) can be loaded into memory and wait for the shell to be present to increase shell functionality.

The reasons is that a non-functional ShellOpenFileByName() in the time gap between them is ok since it will fail as soon as the lib calls into the shell anyways.  This was already true since the protocol was not present for the API to work with...

-Jaben

> -----Original Message-----
> From: Tim Lewis [mailto:tim.lewis@insyde.com]
> Sent: Wednesday, October 05, 2016 2:33 PM
> To: Carsey, Jaben <jaben.carsey@intel.com>; afish@apple.com; Laszlo Ersek
> <lersek@redhat.com>
> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>; Leif Lindholm
> <Leif.Lindholm@arm.com>
> Subject: RE: [edk2] Assert in ShellPkg with latest tianocore edk2 source on
> the Reference Platform
> Importance: High
> 
> Jaben --
> 
> Here is the relevant piece of code. There is *no way* for Unicode Collation to
> execute without the Shell Protocol existing! There is no way for the Shell
> Protocol to exist without (at least) Unicode Collation or Unicode Collation 2
> from existing.
> 
> So adding a check to the function does not help it, because in every case that
> counts (where shell protocol is produced by some sort of standard shell),
> Unicode Collation already exists.
> 
>     Status = gEfiShellProtocol->OpenFileByName(FileName,
>                                                FileHandle,
>                                                OpenMode);
>     if ((mUnicodeCollationProtocol->StriColl (mUnicodeCollationProtocol,
> (CHAR16*)FileName, L"NUL") != 0) &&
>         (mUnicodeCollationProtocol->StriColl (mUnicodeCollationProtocol,
> (CHAR16*)FileName, L"NULL") != 0) &&
>          !EFI_ERROR(Status) && ((OpenMode & EFI_FILE_MODE_CREATE) != 0)){
>       FileInfo = FileFunctionMap.GetFileInfo(*FileHandle); << WITHOUT A
> SHELL IT WILL FAIL HERE
>       ASSERT(FileInfo != NULL);
>       FileInfo->Attribute = Attributes;
>       Status2 = FileFunctionMap.SetFileInfo(*FileHandle, FileInfo);
>       FreePool(FileInfo);
>       if (EFI_ERROR (Status2)) {
>         gEfiShellProtocol->CloseFile(*FileHandle);
>       }
>       Status = Status2;
>     }
> 
> 
> 
> -----Original Message-----
> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> Sent: Wednesday, October 05, 2016 2:17 PM
> To: Tim Lewis <tim.lewis@insyde.com>; afish@apple.com; Laszlo Ersek
> <lersek@redhat.com>
> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>; Leif Lindholm
> <Leif.Lindholm@arm.com>; Carsey, Jaben <jaben.carsey@intel.com>
> Subject: RE: [edk2] Assert in ShellPkg with latest tianocore edk2 source on
> the Reference Platform
> 
> That is not enough.  That gets called by the constructor.  We need to not
> check in either constructor or the constructor worker.
> 
> That difference only matters for the shell binary itself.
> 
> -Jaben
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Tim Lewis
> > Sent: Wednesday, October 05, 2016 2:07 PM
> > To: afish@apple.com; Laszlo Ersek <lersek@redhat.com>
> > Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01 <edk2-
> > devel@ml01.01.org>; Leif Lindholm <Leif.Lindholm@arm.com>
> > Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2
> > source on the Reference Platform
> > Importance: High
> >
> > If he has a DXE driver that consumes the shell lib (a valid case),
> > then my original suggestion stands: move it to
> > ShellLibConstructorWorker. This is where the ShellLib's internal pointers to
> the Shell protocol are initialized.
> >
> > Tim
> >
> > -----Original Message-----
> > From: afish@apple.com [mailto:afish@apple.com]
> > Sent: Wednesday, October 05, 2016 1:59 PM
> > To: Laszlo Ersek <lersek@redhat.com>
> > Cc: Tim Lewis <tim.lewis@insyde.com>; Carsey, Jaben
> > <jaben.carsey@intel.com>; Shah, Tapan <tapandshah@hpe.com>; Daniil
> > Egranov <daniil.egranov@arm.com>; edk2-devel-01 <edk2-
> > devel@ml01.01.org>; Leif Lindholm <Leif.Lindholm@arm.com>
> > Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2
> > source on the Reference Platform
> >
> >
> > > On Oct 5, 2016, at 1:58 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> > >
> > > On 10/05/16 22:44, Tim Lewis wrote:
> > >> Jaben --
> > >>
> > >> In which cases would you have the Shell protocol present and not
> > >> have
> > the Unicode Collation protocol?
> > >
> > > That's exactly the question!
> > >
> > > According to the spec, at least if the system cannot boot from a
> > > disk device, then the collation protocol need not be present.
> > >
> > >> By my count, the Shell itself cannot function without it (see
> > ProcessCommandLine()).
> > >
> > > In which case though I don't understand why Daniil experiences this
> > > change (= commit 583448b441650) as a regression on Juno. If the
> > > shell couldn't function without the collation protocol even before
> > > commit 583448b441650, then the shell must never have run on Juno --
> > > because it appears to lack collation -- and then this change cannot be a
> regression.
> > >
> >
> > Looks like he has a DXE Driver that consume the ShellLib.
> >
> > Thanks,
> >
> > Andrew Fish
> >
> > > Thanks
> > > Laszlo
> > >
> > >
> > >>
> > >> Tim
> > >>
> > >>
> > >> -----Original Message-----
> > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > >> Of Carsey, Jaben
> > >> Sent: Wednesday, October 05, 2016 1:35 PM
> > >> To: Shah, Tapan <tapandshah@hpe.com>; Laszlo Ersek
> > >> <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Daniil Egranov
> > >> <daniil.egranov@arm.com>
> > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01
> > >> <edk2-devel@ml01.01.org>; Leif Lindholm <Leif.Lindholm@arm.com>
> > >> Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2
> > >> source on the Reference Platform
> > >>
> > >> That should work.  We also need to initialize the variable to NULL
> > >> in the
> > constructor.
> > >>
> > >> Do we need to be case insensitive for this?  Can we use memcmp
> > >> instead
> > of the prototol?
> > >>
> > >> -Jaben
> > >>
> > >>> -----Original Message-----
> > >>> From: Shah, Tapan [mailto:tapandshah@hpe.com]
> > >>> Sent: Wednesday, October 05, 2016 1:25 PM
> > >>> To: Laszlo Ersek <lersek@redhat.com>; Andrew Fish
> > <afish@apple.com>;
> > >>> Daniil Egranov <daniil.egranov@arm.com>
> > >>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01 <edk2-
> > >>> devel@ml01.01.org>; Leif Lindholm <Leif.Lindholm@arm.com>
> > >>> Subject: RE: [edk2] Assert in ShellPkg with latest tianocore edk2
> > >>> source on the Reference Platform
> > >>> Importance: High
> > >>>
> > >>> How about moving protocol locate from constructor to the actual
> > >>> function level and returning an error if it fails to locate (See
> > >>> attached
> > patch file).
> > >>>
> > >>> Tapan
> > >>>
> > >>> -----Original Message-----
> > >>> From: Laszlo Ersek [mailto:lersek@redhat.com]
> > >>> Sent: Wednesday, October 05, 2016 3:18 PM
> > >>> To: Andrew Fish <afish@apple.com>; Daniil Egranov
> > >>> <daniil.egranov@arm.com>
> > >>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01 <edk2-
> > >>> devel@ml01.01.org>; Leif Lindholm <Leif.Lindholm@arm.com>; Shah,
> > >>> Tapan <tapandshah@hpe.com>
> > >>> Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2
> > >>> source on the Reference Platform
> > >>>
> > >>> On 10/05/16 21:48, Andrew Fish wrote:
> > >>>>
> > >>>>> On Oct 5, 2016, at 12:24 PM, Daniil Egranov
> > >>>>> <daniil.egranov@arm.com>
> > >>> wrote:
> > >>>>>
> > >>>>> I have the same ASSERT issue on Juno platform even the
> > >>>>> EnglishDxe.inf is
> > >>> included to the platform build. If UefiShellLib has such
> > >>> dependency on the protocol then according to EDKII Module Writer's
> > >>> Guide you need to specify the dependency on protocol in the module
> > >>> .inf to ensure the drivers proper load sequence.
> > >>>>>
> > >>>>> 8.6 Dependency Expressions
> > >>>>> A dependency expression specifies the protocols that the DXE
> > >>>>> driver requires to execute. In EDK II, it is specified in the
> > >>>>> [Depex] section of INF
> > >>> file.
> > >>>>>
> > >>>>
> > >>>> The Dependency Expression is for DXE Drivers that are dispatched
> > >>>> by the
> > >>> DXE Core. A UEFI Driver from an option ROM or an Application does
> > >>> not get dispatched by the dispatch and the Depex will not help.
> > >>> The Depex ends up being a section in the FV and it has nothing to
> > >>> do with the PE/COFF image of the an application or option ROM.
> > >>>>
> > >>>> IMHO the shell should try as hard as possible to function and
> > >>>> should not
> > >>> ASSERT if some newer Protocol is missing.
> > >>>
> > >>> (Background: commit 583448b441650.)
> > >>>
> > >>> In this specific case, the protocol dependency seems possible to
> > eliminate:
> > >>>
> > >>> -
> > Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c
> > >>> includes the CharToUpper() function, which is minimal -- could
> > >>> even be copied or moved over -- and can translate the ASCII subset
> > >>> of UCS-2,
> > >>>
> > >>> - once the ASCII characters of the input string have been
> > >>> translated to upper case, the result could be searched for /
> > >>> compared against
> > L"NULL"
> > >>> using BaseLib.h APIs.
> > >>>
> > >>> (Given that L"NULL" only contains characters from the ASCII
> > >>> subset, it suffices to upper-case ASCII chars in the input string.
> > >>> No non-ASCII character in the BMP can translate to ASCII N/U/L via
> > >>> upcasing, even with the real collation protocol (I trust), and no
> > >>> other ASCII characters than n/u/l will translate to N/U/L. This
> > >>> means that we won't miss an instance of NULL because we didn't
> > >>> upcate it (because it was non-ascii), and it also means we won't
> > >>> mistakenly match something non-NULL as NULL.)
> > >>>
> > >>> Just my two cents.
> > >>>
> > >>> Laszlo
> > >>
> > >> _______________________________________________
> > >> 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
> > >>
> > >
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel