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

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

[Supreeth Venkatesh]

All,

Recently, I updated to latest edk2 master (yesterday's) and I am actually encountering assert in ShellPkg/Library/UefiShellLib/UefiShellLib.c

if (mUnicodeCollationProtocol == NULL) {
    Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid, NULL, (VOID**)&mUnicodeCollationProtocol);
    ASSERT_EFI_ERROR (Status);
  }

It was working few weeks back and has now stopped working.
It's probably because  the platform is unable to locate this protocol "UnicodeCollationProtocol".
Is Anyone else facing the same issue?
Does the new ShellPkg requires additional packages/infs to be included in the platform description file to work with latest changes  to get past this error?

Thanks,
Supreeth
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

[Carsey, Jaben]

Tapan,

Could this be a side effect of your patch?  Should we allow the UefiShellLib to continue without this protocol and then return an error only when the OpenFile is called?

Also, it looks like we never properly initialize mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing else...

-Jaben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Supreeth Venkatesh
> Sent: Tuesday, October 04, 2016 3:51 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source on the
> Reference Platform
> Importance: High
> 
> All,
> 
> Recently, I updated to latest edk2 master (yesterday's) and I am actually
> encountering assert in ShellPkg/Library/UefiShellLib/UefiShellLib.c
> 
> if (mUnicodeCollationProtocol == NULL) {
>     Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid, NULL,
> (VOID**)&mUnicodeCollationProtocol);
>     ASSERT_EFI_ERROR (Status);
>   }
> 
> It was working few weeks back and has now stopped working.
> It's probably because  the platform is unable to locate this protocol
> "UnicodeCollationProtocol".
> Is Anyone else facing the same issue?
> Does the new ShellPkg requires additional packages/infs to be included in
> the platform description file to work with latest changes  to get past this
> error?
> 
> Thanks,
> Supreeth
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Shah, Tapan]

It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is necessary for even other Shell libraries other than UefiShellLib in order to have Shell parser and other command line processing to work properly. That's why I added ASSERT if UefiShellLib fails to locate the protocol.
 
Reference platform should have EnglishDxe module to have this protocol installed properly.

  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf

-----Original Message-----
From: Carsey, Jaben [mailto:jaben.carsey@intel.com] 
Sent: Wednesday, October 05, 2016 10:41 AM
To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben <jaben.carsey@intel.com>
Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform

Tapan,

Could this be a side effect of your patch?  Should we allow the UefiShellLib to continue without this protocol and then return an error only when the OpenFile is called?

Also, it looks like we never properly initialize mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing else...

-Jaben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Supreeth Venkatesh
> Sent: Tuesday, October 04, 2016 3:51 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source 
> on the Reference Platform
> Importance: High
> 
> All,
> 
> Recently, I updated to latest edk2 master (yesterday's) and I am 
> actually encountering assert in 
> ShellPkg/Library/UefiShellLib/UefiShellLib.c
> 
> if (mUnicodeCollationProtocol == NULL) {
>     Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid, 
> NULL, (VOID**)&mUnicodeCollationProtocol);
>     ASSERT_EFI_ERROR (Status);
>   }
> 
> It was working few weeks back and has now stopped working.
> It's probably because  the platform is unable to locate this protocol 
> "UnicodeCollationProtocol".
> Is Anyone else facing the same issue?
> Does the new ShellPkg requires additional packages/infs to be included 
> in the platform description file to work with latest changes  to get 
> past this error?
> 
> Thanks,
> Supreeth
> IMPORTANT NOTICE: The contents of this email and any attachments are 
> confidential and may also be privileged. If you are not the intended 
> recipient, please notify the sender immediately and do not disclose 
> the contents to any other person, use it for any purpose, or store or 
> copy the information in any medium. Thank you.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Daniil Egranov]

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 following dependency in UefiShellLib.inf fixes ASSERT problem:

[Depex]
   gEfiUnicodeCollation2ProtocolGuid


Thanks,

Daniil


On 10/05/2016 11:02 AM, Shah, Tapan wrote:
> It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is necessary for even other Shell libraries other than UefiShellLib in order to have Shell parser and other command line processing to work properly. That's why I added ASSERT if UefiShellLib fails to locate the protocol.
>   
> Reference platform should have EnglishDxe module to have this protocol installed properly.
>
>    MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
>
> -----Original Message-----
> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> Sent: Wednesday, October 05, 2016 10:41 AM
> To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
> Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben <jaben.carsey@intel.com>
> Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform
>
> Tapan,
>
> Could this be a side effect of your patch?  Should we allow the UefiShellLib to continue without this protocol and then return an error only when the OpenFile is called?
>
> Also, it looks like we never properly initialize mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing else...
>
> -Jaben
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Supreeth Venkatesh
>> Sent: Tuesday, October 04, 2016 3:51 PM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
>> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source
>> on the Reference Platform
>> Importance: High
>>
>> All,
>>
>> Recently, I updated to latest edk2 master (yesterday's) and I am
>> actually encountering assert in
>> ShellPkg/Library/UefiShellLib/UefiShellLib.c
>>
>> if (mUnicodeCollationProtocol == NULL) {
>>      Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid,
>> NULL, (VOID**)&mUnicodeCollationProtocol);
>>      ASSERT_EFI_ERROR (Status);
>>    }
>>
>> It was working few weeks back and has now stopped working.
>> It's probably because  the platform is unable to locate this protocol
>> "UnicodeCollationProtocol".
>> Is Anyone else facing the same issue?
>> Does the new ShellPkg requires additional packages/infs to be included
>> in the platform description file to work with latest changes  to get
>> past this error?
>>
>> Thanks,
>> Supreeth
>> IMPORTANT NOTICE: The contents of this email and any attachments are
>> confidential and may also be privileged. If you are not the intended
>> recipient, please notify the sender immediately and do not disclose
>> the contents to any other person, use it for any purpose, or store or
>> copy the information in any medium. Thank you.
>> _______________________________________________
>> 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

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

[Tim Lewis]

Daniil --

Per your point about modules: Adding a dependency expression to a library should NOT have any effect on an application, since applications do not have dependency expressions. 

So this would indicate that you are trying to link a driver against the UefiShellLib. Is this correct?

This is valid for drivers that produce the new Dynamic commands. That is why the UefiShellLib INF allows DXE_RUNTIME_DRIVER and DXE_DRIVER. DXE_RUNTIME_DRIVER and DXE_DRIVER should call ShellInitialize() (this is what the Shell does for commands). Unfortunately, the UnicodeCollation initializer is not a part of this.

Jaben --- I suggest that moving the UnicodeCollation initializer into ShellLibConstructorWorker would probably solve the problem for a well-constructed shell extension driver. 

Tim 

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Daniil Egranov
Sent: Wednesday, October 05, 2016 12:24 PM
To: Shah, Tapan <tapandshah@hpe.com>; Carsey, Jaben <jaben.carsey@intel.com>; Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Leif Lindholm <Leif.Lindholm@arm.com>
Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform

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 following dependency in UefiShellLib.inf fixes ASSERT problem:

[Depex]
   gEfiUnicodeCollation2ProtocolGuid


Thanks,

Daniil


On 10/05/2016 11:02 AM, Shah, Tapan wrote:
> It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is necessary for even other Shell libraries other than UefiShellLib in order to have Shell parser and other command line processing to work properly. That's why I added ASSERT if UefiShellLib fails to locate the protocol.
>   
> Reference platform should have EnglishDxe module to have this protocol installed properly.
>
>    
> MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
>
> -----Original Message-----
> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> Sent: Wednesday, October 05, 2016 10:41 AM
> To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-devel-01 
> <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
> Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben 
> <jaben.carsey@intel.com>
> Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on 
> the Reference Platform
>
> Tapan,
>
> Could this be a side effect of your patch?  Should we allow the UefiShellLib to continue without this protocol and then return an error only when the OpenFile is called?
>
> Also, it looks like we never properly initialize mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing else...
>
> -Jaben
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
>> Of Supreeth Venkatesh
>> Sent: Tuesday, October 04, 2016 3:51 PM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
>> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source 
>> on the Reference Platform
>> Importance: High
>>
>> All,
>>
>> Recently, I updated to latest edk2 master (yesterday's) and I am 
>> actually encountering assert in 
>> ShellPkg/Library/UefiShellLib/UefiShellLib.c
>>
>> if (mUnicodeCollationProtocol == NULL) {
>>      Status = gBS->LocateProtocol 
>> (&gEfiUnicodeCollation2ProtocolGuid,
>> NULL, (VOID**)&mUnicodeCollationProtocol);
>>      ASSERT_EFI_ERROR (Status);
>>    }
>>
>> It was working few weeks back and has now stopped working.
>> It's probably because  the platform is unable to locate this protocol 
>> "UnicodeCollationProtocol".
>> Is Anyone else facing the same issue?
>> Does the new ShellPkg requires additional packages/infs to be 
>> included in the platform description file to work with latest changes  
>> to get past this error?
>>
>> Thanks,
>> Supreeth
>> IMPORTANT NOTICE: The contents of this email and any attachments are 
>> confidential and may also be privileged. If you are not the intended 
>> recipient, please notify the sender immediately and do not disclose 
>> the contents to any other person, use it for any purpose, or store or 
>> copy the information in any medium. Thank you.
>> _______________________________________________
>> 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

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

[Andrew Fish]

> 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.  

Thanks,

Andre wFish


> The following dependency in UefiShellLib.inf fixes ASSERT problem:
> 
> [Depex]
>  gEfiUnicodeCollation2ProtocolGuid
> 
> 
> Thanks,
> 
> Daniil
> 
> 
> On 10/05/2016 11:02 AM, Shah, Tapan wrote:
>> It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is necessary for even other Shell libraries other than UefiShellLib in order to have Shell parser and other command line processing to work properly. That's why I added ASSERT if UefiShellLib fails to locate the protocol.
>>  Reference platform should have EnglishDxe module to have this protocol installed properly.
>> 
>>   MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
>> 
>> -----Original Message-----
>> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
>> Sent: Wednesday, October 05, 2016 10:41 AM
>> To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben <jaben.carsey@intel.com>
>> Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform
>> 
>> Tapan,
>> 
>> Could this be a side effect of your patch?  Should we allow the UefiShellLib to continue without this protocol and then return an error only when the OpenFile is called?
>> 
>> Also, it looks like we never properly initialize mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing else...
>> 
>> -Jaben
>> 
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Supreeth Venkatesh
>>> Sent: Tuesday, October 04, 2016 3:51 PM
>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
>>> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source
>>> on the Reference Platform
>>> Importance: High
>>> 
>>> All,
>>> 
>>> Recently, I updated to latest edk2 master (yesterday's) and I am
>>> actually encountering assert in
>>> ShellPkg/Library/UefiShellLib/UefiShellLib.c
>>> 
>>> if (mUnicodeCollationProtocol == NULL) {
>>>     Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid,
>>> NULL, (VOID**)&mUnicodeCollationProtocol);
>>>     ASSERT_EFI_ERROR (Status);
>>>   }
>>> 
>>> It was working few weeks back and has now stopped working.
>>> It's probably because  the platform is unable to locate this protocol
>>> "UnicodeCollationProtocol".
>>> Is Anyone else facing the same issue?
>>> Does the new ShellPkg requires additional packages/infs to be included
>>> in the platform description file to work with latest changes  to get
>>> past this error?
>>> 
>>> Thanks,
>>> Supreeth
>>> IMPORTANT NOTICE: The contents of this email and any attachments are
>>> confidential and may also be privileged. If you are not the intended
>>> recipient, please notify the sender immediately and do not disclose
>>> the contents to any other person, use it for any purpose, or store or
>>> copy the information in any medium. Thank you.
>>> _______________________________________________
>>> 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

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

[Laszlo Ersek]

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

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

[Shah, Tapan]

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

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

[Carsey, Jaben]

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

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

[Shah, Tapan]

Yes, it has to be case insensitive per the latest Shell spec.

-----Original Message-----
From: Carsey, Jaben [mailto:jaben.carsey@intel.com] 
Sent: Wednesday, October 05, 2016 3: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: 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 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

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

[Tim Lewis]

Jaben --

In which cases would you have the Shell protocol present and not have the Unicode Collation protocol?

By my count, the Shell itself cannot function without it (see ProcessCommandLine()). 

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

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

[Laszlo Ersek]

On 10/05/16 22:24, Shah, Tapan wrote:
> 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).

What function is that precisely? The hunk headers in the patch don't show.

If it is a function that is part of some file operation, then, although
the patch is an improvement because the shell won't ASSERT(), it's still
not perfect, because lack of the protocol will break that file operation
completely.

... The containing function seems to be ShellOpenFileByName(). Seems
like a quite central function to render inoperable if the collation
protocol is not present.

Actually, I'm quite confused about conformance to specific versions of
the UEFI spec. In UEFI v2.6, section "2.6.2 Platform-Specific Elements",
point 5, we have

    If a platform includes the ability to boot from a disk device, then
    the EFI_BLOCK_IO_PROTOCOL, the EFI_DISK_IO_PROTOCOL, the
    EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, and the
    EFI_UNICODE_COLLATION_PROTOCOL are required.

In fact this requirement goes back to UEFI v2.3.1c at least. (I didn't
bother to check earlier releases of the spec.) By depending on the
protocol in the shell, we say that the current upstream shell is not
required to open files by name on pre-v2.3.1c systems (or perhaps on
systems older than v2.3.1c), and on systems that cannot boot from a disk
device.

Is this something we can say? How is this kind of compatibility tracked
in edk2?

Thanks
Laszlo

> 
> 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
> 
> 
> uefishelllib_fix.diff
> 
> 
> Index: ShellPkg/Library/UefiShellLib/UefiShellLib.c
> ===================================================================
> --- ShellPkg/Library/UefiShellLib/UefiShellLib.c	(revision 22692)
> +++ ShellPkg/Library/UefiShellLib/UefiShellLib.c	(working copy)
> @@ -292,8 +292,6 @@
>    IN EFI_SYSTEM_TABLE  *SystemTable
>    )
>  {
> -  EFI_STATUS  Status;
> -
>    mEfiShellEnvironment2       = NULL;
>    gEfiShellProtocol           = NULL;
>    gEfiShellParametersProtocol = NULL;
> @@ -300,11 +298,6 @@
>    mEfiShellInterface          = NULL;
>    mEfiShellEnvironment2Handle = NULL;
>  
> -  if (mUnicodeCollationProtocol == NULL) {
> -    Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid, NULL, (VOID**)&mUnicodeCollationProtocol);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> -
>    //
>    // verify that auto initialize is not set false
>    //
> @@ -730,6 +723,14 @@
>                                                 FileHandle,
>                                                 OpenMode);
>  
> +    if (mUnicodeCollationProtocol == NULL) {
> +      Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid, NULL, (VOID**)&mUnicodeCollationProtocol);
> +      if (EFI_ERROR (Status)) {
> +        gEfiShellProtocol->CloseFile (*FileHandle);
> +        return Status;
> +      }
> +    }
> +
>      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)){
> 

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

[Daniil Egranov]

On 10/05/2016 02:48 PM, 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.
>
> Thanks,
>
> Andre wFish
I had to put a context of the ASSERT message in the original message to 
make it more clear:

add-symbol-file 
/home/user1/workspace/juno_16.09/uefi/edk2/Build/ArmJuno/DEBUG_GCC49/AARCH64/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe/DEBUG/ArmJunoDxe.dll 
0xF8AB9000
Loading driver at 0x000F8AB8000 EntryPoint=0x000F8AB9048 ArmJunoDxe.efi

ASSERT_EFI_ERROR (Status = Not Found)
ASSERT [ArmJunoDxe] 
/home/danegr01/workspace/juno_16.09/uefi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c(305): 
!EFI_ERROR (Status)

If driver includes a module which has dependency on one of the 
protocols, should the driver know about this dependency? Probably not. 
It should be inherited from the module. Adding [Depex] to UefiShellLib 
corrected dispatching ArmJunoDxe and EnglishDxe by the Dxe Core.

>
>
>> The following dependency in UefiShellLib.inf fixes ASSERT problem:
>>
>> [Depex]
>>   gEfiUnicodeCollation2ProtocolGuid
>>
>>
>> Thanks,
>>
>> Daniil
>>
>>
>> On 10/05/2016 11:02 AM, Shah, Tapan wrote:
>>> It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is necessary for even other Shell libraries other than UefiShellLib in order to have Shell parser and other command line processing to work properly. That's why I added ASSERT if UefiShellLib fails to locate the protocol.
>>>   Reference platform should have EnglishDxe module to have this protocol installed properly.
>>>
>>>    MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
>>>
>>> -----Original Message-----
>>> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
>>> Sent: Wednesday, October 05, 2016 10:41 AM
>>> To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben <jaben.carsey@intel.com>
>>> Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform
>>>
>>> Tapan,
>>>
>>> Could this be a side effect of your patch?  Should we allow the UefiShellLib to continue without this protocol and then return an error only when the OpenFile is called?
>>>
>>> Also, it looks like we never properly initialize mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing else...
>>>
>>> -Jaben
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>>> Supreeth Venkatesh
>>>> Sent: Tuesday, October 04, 2016 3:51 PM
>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
>>>> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source
>>>> on the Reference Platform
>>>> Importance: High
>>>>
>>>> All,
>>>>
>>>> Recently, I updated to latest edk2 master (yesterday's) and I am
>>>> actually encountering assert in
>>>> ShellPkg/Library/UefiShellLib/UefiShellLib.c
>>>>
>>>> if (mUnicodeCollationProtocol == NULL) {
>>>>      Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid,
>>>> NULL, (VOID**)&mUnicodeCollationProtocol);
>>>>      ASSERT_EFI_ERROR (Status);
>>>>    }
>>>>
>>>> It was working few weeks back and has now stopped working.
>>>> It's probably because  the platform is unable to locate this protocol
>>>> "UnicodeCollationProtocol".
>>>> Is Anyone else facing the same issue?
>>>> Does the new ShellPkg requires additional packages/infs to be included
>>>> in the platform description file to work with latest changes  to get
>>>> past this error?
>>>>
>>>> Thanks,
>>>> Supreeth
>>>> IMPORTANT NOTICE: The contents of this email and any attachments are
>>>> confidential and may also be privileged. If you are not the intended
>>>> recipient, please notify the sender immediately and do not disclose
>>>> the contents to any other person, use it for any purpose, or store or
>>>> copy the information in any medium. Thank you.
>>>> _______________________________________________
>>>> 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

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.

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
> 

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

[Andrew Fish]

> 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
>> 
> 

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

[Laszlo Ersek]

On 10/05/16 22:53, Daniil Egranov wrote:
> 
> 
> On 10/05/2016 02:48 PM, 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.
>>
>> Thanks,
>>
>> Andre wFish
> I had to put a context of the ASSERT message in the original message to
> make it more clear:
> 
> add-symbol-file
> /home/user1/workspace/juno_16.09/uefi/edk2/Build/ArmJuno/DEBUG_GCC49/AARCH64/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe/DEBUG/ArmJunoDxe.dll
> 0xF8AB9000
> Loading driver at 0x000F8AB8000 EntryPoint=0x000F8AB9048 ArmJunoDxe.efi
> 
> ASSERT_EFI_ERROR (Status = Not Found)
> ASSERT [ArmJunoDxe]
> /home/danegr01/workspace/juno_16.09/uefi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c(305):
> !EFI_ERROR (Status)
> 
> If driver includes a module which has dependency on one of the
> protocols, should the driver know about this dependency? Probably not.
> It should be inherited from the module. Adding [Depex] to UefiShellLib
> corrected dispatching ArmJunoDxe and EnglishDxe by the Dxe Core.

You are right; Tim confirmed UefiShellLib is valid to use in
DXE_RUNTIME_DRIVER and DXE_DRIVER modules, and ArmJunoDxe is a DXE_DRIVER.

This additional information also explains why you see the change as a
regression, but have had a working shell all this time -- due to the
missing depex, ArmJunoDxe got dispatched earlier and experienced the
regression, but the shell app itself (being a UEFI application) found
the collation protocol (provided by EnglishDxe) just fine.

So, your suggestion about the depex solves the problem for drivers, on
platforms that provide the collation protocol, but it doesn't solve the
(separate) problem of the shell app proper, on platforms that don't
provide the collation protocol at all.

Thanks
Laszlo

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

[Tim Lewis]

Is it possible that Daniil has UnicodeCollation and not UnicodeCollation2? The shell itself can handle this case, but the ShellLib cannot.

Tim

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, October 05, 2016 1:58 PM
To: Tim Lewis <tim.lewis@insyde.com>; Carsey, Jaben <jaben.carsey@intel.com>; Shah, Tapan <tapandshah@hpe.com>; Andrew Fish <afish@apple.com>; Daniil Egranov <daniil.egranov@arm.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

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.

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
> 

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

[Andrew Fish]

> On Oct 5, 2016, at 1:53 PM, Daniil Egranov <daniil.egranov@arm.com> wrote:
> 
> 
> 
> On 10/05/2016 02:48 PM, 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.
>> 
>> Thanks,
>> 
>> Andre wFish
> I had to put a context of the ASSERT message in the original message to make it more clear:
> 
> add-symbol-file /home/user1/workspace/juno_16.09/uefi/edk2/Build/ArmJuno/DEBUG_GCC49/AARCH64/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe/DEBUG/ArmJunoDxe.dll 0xF8AB9000
> Loading driver at 0x000F8AB8000 EntryPoint=0x000F8AB9048 ArmJunoDxe.efi
> 
> ASSERT_EFI_ERROR (Status = Not Found)
> ASSERT [ArmJunoDxe] /home/danegr01/workspace/juno_16.09/uefi/edk2/ShellPkg/Library/UefiShellLib/UefiShellLib.c(305): !EFI_ERROR (Status)
> 
> If driver includes a module which has dependency on one of the protocols, should the driver know about this dependency? Probably not. It should be inherited from the module. Adding [Depex] to UefiShellLib corrected dispatching ArmJunoDxe and EnglishDxe by the Dxe Core.
> 

Daniil,

Sorry, I was assuming the UefiShellLib only supported UEFI Applications. Thats what I get for not looking at the code :(, my bad. 

https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellLib/UefiShellLib.inf#L23 <https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellLib/UefiShellLib.inf#L23>
  LIBRARY_CLASS                  = ShellLib|UEFI_APPLICATION UEFI_DRIVER DXE_RUNTIME_DRIVER DXE_DRIVER

Given the library supports DXE_RUNTIME_DRIVER and DXE_DRIVER I think you are right it should publish a Depex if it has a hard dependency. 

I'm guessing that your DXE Driver is dispatching prior UEFI Driver that publishes the protocol. 

Thanks,

Andrew Fish


>> 
>> 
>>> The following dependency in UefiShellLib.inf fixes ASSERT problem:
>>> 
>>> [Depex]
>>>  gEfiUnicodeCollation2ProtocolGuid
>>> 
>>> 
>>> Thanks,
>>> 
>>> Daniil
>>> 
>>> 
>>> On 10/05/2016 11:02 AM, Shah, Tapan wrote:
>>>> It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is necessary for even other Shell libraries other than UefiShellLib in order to have Shell parser and other command line processing to work properly. That's why I added ASSERT if UefiShellLib fails to locate the protocol.
>>>>  Reference platform should have EnglishDxe module to have this protocol installed properly.
>>>> 
>>>>   MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
>>>> 
>>>> -----Original Message-----
>>>> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
>>>> Sent: Wednesday, October 05, 2016 10:41 AM
>>>> To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
>>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben <jaben.carsey@intel.com>
>>>> Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform
>>>> 
>>>> Tapan,
>>>> 
>>>> Could this be a side effect of your patch?  Should we allow the UefiShellLib to continue without this protocol and then return an error only when the OpenFile is called?
>>>> 
>>>> Also, it looks like we never properly initialize mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing else...
>>>> 
>>>> -Jaben
>>>> 
>>>>> -----Original Message-----
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>>>> Supreeth Venkatesh
>>>>> Sent: Tuesday, October 04, 2016 3:51 PM
>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
>>>>> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source
>>>>> on the Reference Platform
>>>>> Importance: High
>>>>> 
>>>>> All,
>>>>> 
>>>>> Recently, I updated to latest edk2 master (yesterday's) and I am
>>>>> actually encountering assert in
>>>>> ShellPkg/Library/UefiShellLib/UefiShellLib.c
>>>>> 
>>>>> if (mUnicodeCollationProtocol == NULL) {
>>>>>     Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid,
>>>>> NULL, (VOID**)&mUnicodeCollationProtocol);
>>>>>     ASSERT_EFI_ERROR (Status);
>>>>>   }
>>>>> 
>>>>> It was working few weeks back and has now stopped working.
>>>>> It's probably because  the platform is unable to locate this protocol
>>>>> "UnicodeCollationProtocol".
>>>>> Is Anyone else facing the same issue?
>>>>> Does the new ShellPkg requires additional packages/infs to be included
>>>>> in the platform description file to work with latest changes  to get
>>>>> past this error?
>>>>> 
>>>>> Thanks,
>>>>> Supreeth
>>>>> IMPORTANT NOTICE: The contents of this email and any attachments are
>>>>> confidential and may also be privileged. If you are not the intended
>>>>> recipient, please notify the sender immediately and do not disclose
>>>>> the contents to any other person, use it for any purpose, or store or
>>>>> copy the information in any medium. Thank you.
>>>>> _______________________________________________
>>>>> 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
>> _______________________________________________
>> 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 <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

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

[Laszlo Ersek]

On 10/05/16 22:59, Andrew Fish wrote:
> 
>> 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. 

That's it, thank you. Daniil named ArmJunoDxe.efi in his second (?) email.

Laszlo

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

[Tim Lewis]

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
>> 
> 

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

[Carsey, Jaben]

All,
What are your thoughts about Tapan's suggestion to move the protocol location to the only function that needs it?

Andrew,

We cannot publish a dependency.  There are DXE driver dynamic shell commands that launch before the shell runs and produce the appropriate protocol so that the shell will call into them.  These drivers must be allowed to successfully launch before the shell runs.

Danil,

Can you elaborate what your DXE driver is doing with the library?


-Jaben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Andrew Fish
> Sent: Wednesday, October 05, 2016 2:06 PM
> To: Daniil Egranov <daniil.egranov@arm.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01 <edk2-
> devel@lists.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
> 
> 
> > On Oct 5, 2016, at 1:53 PM, Daniil Egranov <daniil.egranov@arm.com>
> wrote:
> >
> >
> >
> > On 10/05/2016 02:48 PM, 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.
> >>
> >> Thanks,
> >>
> >> Andre wFish
> > I had to put a context of the ASSERT message in the original message to
> make it more clear:
> >
> > add-symbol-file
> /home/user1/workspace/juno_16.09/uefi/edk2/Build/ArmJuno/DEBUG_GC
> C49/AARCH64/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJuno
> Dxe/DEBUG/ArmJunoDxe.dll 0xF8AB9000
> > Loading driver at 0x000F8AB8000 EntryPoint=0x000F8AB9048
> ArmJunoDxe.efi
> >
> > ASSERT_EFI_ERROR (Status = Not Found)
> > ASSERT [ArmJunoDxe]
> /home/danegr01/workspace/juno_16.09/uefi/edk2/ShellPkg/Library/UefiSh
> ellLib/UefiShellLib.c(305): !EFI_ERROR (Status)
> >
> > If driver includes a module which has dependency on one of the protocols,
> should the driver know about this dependency? Probably not. It should be
> inherited from the module. Adding [Depex] to UefiShellLib corrected
> dispatching ArmJunoDxe and EnglishDxe by the Dxe Core.
> >
> 
> Daniil,
> 
> Sorry, I was assuming the UefiShellLib only supported UEFI Applications.
> Thats what I get for not looking at the code :(, my bad.
> 
> https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellL
> ib/UefiShellLib.inf#L23
> <https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShel
> lLib/UefiShellLib.inf#L23>
>   LIBRARY_CLASS                  = ShellLib|UEFI_APPLICATION UEFI_DRIVER
> DXE_RUNTIME_DRIVER DXE_DRIVER
> 
> Given the library supports DXE_RUNTIME_DRIVER and DXE_DRIVER I think
> you are right it should publish a Depex if it has a hard dependency.
> 
> I'm guessing that your DXE Driver is dispatching prior UEFI Driver that
> publishes the protocol.
> 
> Thanks,
> 
> Andrew Fish
> 
> 
> >>
> >>
> >>> The following dependency in UefiShellLib.inf fixes ASSERT problem:
> >>>
> >>> [Depex]
> >>>  gEfiUnicodeCollation2ProtocolGuid
> >>>
> >>>
> >>> Thanks,
> >>>
> >>> Daniil
> >>>
> >>>
> >>> On 10/05/2016 11:02 AM, Shah, Tapan wrote:
> >>>> It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is
> necessary for even other Shell libraries other than UefiShellLib in order to
> have Shell parser and other command line processing to work properly.
> That's why I added ASSERT if UefiShellLib fails to locate the protocol.
> >>>>  Reference platform should have EnglishDxe module to have this
> protocol installed properly.
> >>>>
> >>>>
> MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
> >>>>
> >>>> -----Original Message-----
> >>>> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> >>>> Sent: Wednesday, October 05, 2016 10:41 AM
> >>>> To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-
> devel-01 <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
> >>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben
> <jaben.carsey@intel.com>
> >>>> Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on the
> Reference Platform
> >>>>
> >>>> Tapan,
> >>>>
> >>>> Could this be a side effect of your patch?  Should we allow the
> UefiShellLib to continue without this protocol and then return an error only
> when the OpenFile is called?
> >>>>
> >>>> Also, it looks like we never properly initialize
> mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing
> else...
> >>>>
> >>>> -Jaben
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf Of
> >>>>> Supreeth Venkatesh
> >>>>> Sent: Tuesday, October 04, 2016 3:51 PM
> >>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
> >>>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
> >>>>> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source
> >>>>> on the Reference Platform
> >>>>> Importance: High
> >>>>>
> >>>>> All,
> >>>>>
> >>>>> Recently, I updated to latest edk2 master (yesterday's) and I am
> >>>>> actually encountering assert in
> >>>>> ShellPkg/Library/UefiShellLib/UefiShellLib.c
> >>>>>
> >>>>> if (mUnicodeCollationProtocol == NULL) {
> >>>>>     Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid,
> >>>>> NULL, (VOID**)&mUnicodeCollationProtocol);
> >>>>>     ASSERT_EFI_ERROR (Status);
> >>>>>   }
> >>>>>
> >>>>> It was working few weeks back and has now stopped working.
> >>>>> It's probably because  the platform is unable to locate this protocol
> >>>>> "UnicodeCollationProtocol".
> >>>>> Is Anyone else facing the same issue?
> >>>>> Does the new ShellPkg requires additional packages/infs to be
> included
> >>>>> in the platform description file to work with latest changes  to get
> >>>>> past this error?
> >>>>>
> >>>>> Thanks,
> >>>>> Supreeth
> >>>>> IMPORTANT NOTICE: The contents of this email and any attachments
> are
> >>>>> confidential and may also be privileged. If you are not the intended
> >>>>> recipient, please notify the sender immediately and do not disclose
> >>>>> the contents to any other person, use it for any purpose, or store or
> >>>>> copy the information in any medium. Thank you.
> >>>>> _______________________________________________
> >>>>> 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
> >> _______________________________________________
> >> 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 <mailto:edk2-devel@lists.01.org>
> > https://lists.01.org/mailman/listinfo/edk2-devel
> <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

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

[Carsey, Jaben]

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

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

[Laszlo Ersek]

On 10/05/16 23:05, Andrew Fish wrote:

> I'm guessing that your DXE Driver is dispatching prior UEFI Driver that publishes the protocol. 

BTW... Why is EnglishDxe a UEFI_DRIVER rather than a DXE_DRIVER? It
doesn't conform to the UEFI driver model. (I'm not saying it *must* not
be a UEFI_DRIVER, but what reason does it have for being one?) It also
calls no runtime services, and only the
InstallMultipleProtocolInterfaces() boot service; so I don't think it
relies on all of the architectural protocols either.

Thanks
Laszlo

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

[Andrew Fish]

> On Oct 5, 2016, at 2:15 PM, Carsey, Jaben <jaben.carsey@intel.com> wrote:
> 
> All,
> What are your thoughts about Tapan's suggestion to move the protocol location to the only function that needs it?
> 
> Andrew,
> 
> We cannot publish a dependency.  There are DXE driver dynamic shell commands that launch before the shell runs and produce the appropriate protocol so that the shell will call into them.  These drivers must be allowed to successfully launch before the shell runs.
> 

Jaben,

I'm not saying it was good change...., but the patch added a hard dependency on a protocol so a depex was required for correctness. I agree deferring the work is a better solution. 

Thanks,

Andrew Fish


> Danil,
> 
> Can you elaborate what your DXE driver is doing with the library?
> 
> 
> -Jaben
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Andrew Fish
>> Sent: Wednesday, October 05, 2016 2:06 PM
>> To: Daniil Egranov <daniil.egranov@arm.com>
>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01 <edk2-
>> devel@lists.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
>> 
>> 
>>> On Oct 5, 2016, at 1:53 PM, Daniil Egranov <daniil.egranov@arm.com>
>> wrote:
>>> 
>>> 
>>> 
>>> On 10/05/2016 02:48 PM, 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.
>>>> 
>>>> Thanks,
>>>> 
>>>> Andre wFish
>>> I had to put a context of the ASSERT message in the original message to
>> make it more clear:
>>> 
>>> add-symbol-file
>> /home/user1/workspace/juno_16.09/uefi/edk2/Build/ArmJuno/DEBUG_GC
>> C49/AARCH64/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJuno
>> Dxe/DEBUG/ArmJunoDxe.dll 0xF8AB9000
>>> Loading driver at 0x000F8AB8000 EntryPoint=0x000F8AB9048
>> ArmJunoDxe.efi
>>> 
>>> ASSERT_EFI_ERROR (Status = Not Found)
>>> ASSERT [ArmJunoDxe]
>> /home/danegr01/workspace/juno_16.09/uefi/edk2/ShellPkg/Library/UefiSh
>> ellLib/UefiShellLib.c(305): !EFI_ERROR (Status)
>>> 
>>> If driver includes a module which has dependency on one of the protocols,
>> should the driver know about this dependency? Probably not. It should be
>> inherited from the module. Adding [Depex] to UefiShellLib corrected
>> dispatching ArmJunoDxe and EnglishDxe by the Dxe Core.
>>> 
>> 
>> Daniil,
>> 
>> Sorry, I was assuming the UefiShellLib only supported UEFI Applications.
>> Thats what I get for not looking at the code :(, my bad.
>> 
>> https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellL
>> ib/UefiShellLib.inf#L23
>> <https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShel
>> lLib/UefiShellLib.inf#L23>
>>  LIBRARY_CLASS                  = ShellLib|UEFI_APPLICATION UEFI_DRIVER
>> DXE_RUNTIME_DRIVER DXE_DRIVER
>> 
>> Given the library supports DXE_RUNTIME_DRIVER and DXE_DRIVER I think
>> you are right it should publish a Depex if it has a hard dependency.
>> 
>> I'm guessing that your DXE Driver is dispatching prior UEFI Driver that
>> publishes the protocol.
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>> 
>>>> 
>>>> 
>>>>> The following dependency in UefiShellLib.inf fixes ASSERT problem:
>>>>> 
>>>>> [Depex]
>>>>> gEfiUnicodeCollation2ProtocolGuid
>>>>> 
>>>>> 
>>>>> Thanks,
>>>>> 
>>>>> Daniil
>>>>> 
>>>>> 
>>>>> On 10/05/2016 11:02 AM, Shah, Tapan wrote:
>>>>>> It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is
>> necessary for even other Shell libraries other than UefiShellLib in order to
>> have Shell parser and other command line processing to work properly.
>> That's why I added ASSERT if UefiShellLib fails to locate the protocol.
>>>>>> Reference platform should have EnglishDxe module to have this
>> protocol installed properly.
>>>>>> 
>>>>>> 
>> MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
>>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
>>>>>> Sent: Wednesday, October 05, 2016 10:41 AM
>>>>>> To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-
>> devel-01 <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
>>>>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben
>> <jaben.carsey@intel.com>
>>>>>> Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on the
>> Reference Platform
>>>>>> 
>>>>>> Tapan,
>>>>>> 
>>>>>> Could this be a side effect of your patch?  Should we allow the
>> UefiShellLib to continue without this protocol and then return an error only
>> when the OpenFile is called?
>>>>>> 
>>>>>> Also, it looks like we never properly initialize
>> mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing
>> else...
>>>>>> 
>>>>>> -Jaben
>>>>>> 
>>>>>>> -----Original Message-----
>>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
>> Behalf Of
>>>>>>> Supreeth Venkatesh
>>>>>>> Sent: Tuesday, October 04, 2016 3:51 PM
>>>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
>>>>>>> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source
>>>>>>> on the Reference Platform
>>>>>>> Importance: High
>>>>>>> 
>>>>>>> All,
>>>>>>> 
>>>>>>> Recently, I updated to latest edk2 master (yesterday's) and I am
>>>>>>> actually encountering assert in
>>>>>>> ShellPkg/Library/UefiShellLib/UefiShellLib.c
>>>>>>> 
>>>>>>> if (mUnicodeCollationProtocol == NULL) {
>>>>>>>    Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid,
>>>>>>> NULL, (VOID**)&mUnicodeCollationProtocol);
>>>>>>>    ASSERT_EFI_ERROR (Status);
>>>>>>>  }
>>>>>>> 
>>>>>>> It was working few weeks back and has now stopped working.
>>>>>>> It's probably because  the platform is unable to locate this protocol
>>>>>>> "UnicodeCollationProtocol".
>>>>>>> Is Anyone else facing the same issue?
>>>>>>> Does the new ShellPkg requires additional packages/infs to be
>> included
>>>>>>> in the platform description file to work with latest changes  to get
>>>>>>> past this error?
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Supreeth
>>>>>>> IMPORTANT NOTICE: The contents of this email and any attachments
>> are
>>>>>>> confidential and may also be privileged. If you are not the intended
>>>>>>> recipient, please notify the sender immediately and do not disclose
>>>>>>> the contents to any other person, use it for any purpose, or store or
>>>>>>> copy the information in any medium. Thank you.
>>>>>>> _______________________________________________
>>>>>>> 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
>>>> _______________________________________________
>>>> 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 <mailto:edk2-devel@lists.01.org>
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> <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

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

[Laszlo Ersek]

On 10/05/16 23:15, Carsey, Jaben wrote:
> All,
> What are your thoughts about Tapan's suggestion to move the protocol location to the only function that needs it?

As I wrote elsewhere in the thread, that will eliminate the assert, but
it will break ShellOpenFileByName() for good (under the same
circumstances where the assert is triggered now).

Is a non-functional ShellOpenFileByName() acceptable, if the platform
doesn't provide the collation protocol? In other words, is it okay to
fail to open any file by name just because we can't compare the filename
against NULL because the collation protocol is missing?

Thanks
Laszlo

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

[Tim Lewis]

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

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

[Kinney, Michael D]

Laszlo,

There are many types of UEFI Drivers and they are enumerated in the
UEFI Driver Writer's Guide.  Some follow the UEFI Driver Model and
some do not.

A driver that only consumes/produces UEFI services/protocols without a depex
should be a UEFI Driver.  If there is a depex or use of any PI services/protocols
then it must be one of the DXE driver types.

We actually prefer to categorize drivers as UEFI Driver's if possible, because 
that maximizes the drivers's compatibility with UEFI conformant platforms.

Mike


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Wednesday, October 5, 2016 2: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>
> Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2 source on the
> Reference Platform
> 
> On 10/05/16 23:05, Andrew Fish wrote:
> 
> > I'm guessing that your DXE Driver is dispatching prior UEFI Driver that publishes the
> protocol.
> 
> BTW... Why is EnglishDxe a UEFI_DRIVER rather than a DXE_DRIVER? It
> doesn't conform to the UEFI driver model. (I'm not saying it *must* not
> be a UEFI_DRIVER, but what reason does it have for being one?) It also
> calls no runtime services, and only the
> InstallMultipleProtocolInterfaces() boot service; so I don't think it
> relies on all of the architectural protocols either.
> 
> Thanks
> Laszlo
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Daniil Egranov]

On 10/05/2016 04:15 PM, Carsey, Jaben wrote:
> All,
> What are your thoughts about Tapan's suggestion to move the protocol location to the only function that needs it?
>
> Andrew,
>
> We cannot publish a dependency.  There are DXE driver dynamic shell commands that launch before the shell runs and produce the appropriate protocol so that the shell will call into them.  These drivers must be allowed to successfully launch before the shell runs.
>
> Danil,
>
> Can you elaborate what your DXE driver is doing with the library?

As I can see, it used by ArmJunoDxe for registering new shell command:

   // Install dynamic Shell command to run baremetal binaries.
   Status = ShellDynCmdRunAxfInstall (ImageHandle);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "ArmJunoDxe: Failed to install 
ShellDynCmdRunAxf\n"));
   }


>
>
> -Jaben
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Andrew Fish
>> Sent: Wednesday, October 05, 2016 2:06 PM
>> To: Daniil Egranov <daniil.egranov@arm.com>
>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01 <edk2-
>> devel@lists.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
>>
>>
>>> On Oct 5, 2016, at 1:53 PM, Daniil Egranov <daniil.egranov@arm.com>
>> wrote:
>>>
>>>
>>> On 10/05/2016 02:48 PM, 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.
>>>> Thanks,
>>>>
>>>> Andre wFish
>>> I had to put a context of the ASSERT message in the original message to
>> make it more clear:
>>> add-symbol-file
>> /home/user1/workspace/juno_16.09/uefi/edk2/Build/ArmJuno/DEBUG_GC
>> C49/AARCH64/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJuno
>> Dxe/DEBUG/ArmJunoDxe.dll 0xF8AB9000
>>> Loading driver at 0x000F8AB8000 EntryPoint=0x000F8AB9048
>> ArmJunoDxe.efi
>>> ASSERT_EFI_ERROR (Status = Not Found)
>>> ASSERT [ArmJunoDxe]
>> /home/danegr01/workspace/juno_16.09/uefi/edk2/ShellPkg/Library/UefiSh
>> ellLib/UefiShellLib.c(305): !EFI_ERROR (Status)
>>> If driver includes a module which has dependency on one of the protocols,
>> should the driver know about this dependency? Probably not. It should be
>> inherited from the module. Adding [Depex] to UefiShellLib corrected
>> dispatching ArmJunoDxe and EnglishDxe by the Dxe Core.
>> Daniil,
>>
>> Sorry, I was assuming the UefiShellLib only supported UEFI Applications.
>> Thats what I get for not looking at the code :(, my bad.
>>
>> https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShellL
>> ib/UefiShellLib.inf#L23
>> <https://github.com/tianocore/edk2/blob/master/ShellPkg/Library/UefiShel
>> lLib/UefiShellLib.inf#L23>
>>    LIBRARY_CLASS                  = ShellLib|UEFI_APPLICATION UEFI_DRIVER
>> DXE_RUNTIME_DRIVER DXE_DRIVER
>>
>> Given the library supports DXE_RUNTIME_DRIVER and DXE_DRIVER I think
>> you are right it should publish a Depex if it has a hard dependency.
>>
>> I'm guessing that your DXE Driver is dispatching prior UEFI Driver that
>> publishes the protocol.
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>
>>>>
>>>>> The following dependency in UefiShellLib.inf fixes ASSERT problem:
>>>>>
>>>>> [Depex]
>>>>>   gEfiUnicodeCollation2ProtocolGuid
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Daniil
>>>>>
>>>>>
>>>>> On 10/05/2016 11:02 AM, Shah, Tapan wrote:
>>>>>> It's possible. But I think gEfiUnicodeCollation2ProtocolGuid protocol is
>> necessary for even other Shell libraries other than UefiShellLib in order to
>> have Shell parser and other command line processing to work properly.
>> That's why I added ASSERT if UefiShellLib fails to locate the protocol.
>>>>>>   Reference platform should have EnglishDxe module to have this
>> protocol installed properly.
>>>>>>
>> MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
>>>>>> -----Original Message-----
>>>>>> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
>>>>>> Sent: Wednesday, October 05, 2016 10:41 AM
>>>>>> To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-
>> devel-01 <edk2-devel@lists.01.org>; Shah, Tapan <tapandshah@hpe.com>
>>>>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>; Carsey, Jaben
>> <jaben.carsey@intel.com>
>>>>>> Subject: RE: Assert in ShellPkg with latest tianocore edk2 source on the
>> Reference Platform
>>>>>> Tapan,
>>>>>>
>>>>>> Could this be a side effect of your patch?  Should we allow the
>> UefiShellLib to continue without this protocol and then return an error only
>> when the OpenFile is called?
>>>>>> Also, it looks like we never properly initialize
>> mUnicodeCollationProtocol.  We check for NULL in Constructor, but nothing
>> else...
>>>>>> -Jaben
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
>> Behalf Of
>>>>>>> Supreeth Venkatesh
>>>>>>> Sent: Tuesday, October 04, 2016 3:51 PM
>>>>>>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>>>>>>> Cc: Leif Lindholm <Leif.Lindholm@arm.com>
>>>>>>> Subject: [edk2] Assert in ShellPkg with latest tianocore edk2 source
>>>>>>> on the Reference Platform
>>>>>>> Importance: High
>>>>>>>
>>>>>>> All,
>>>>>>>
>>>>>>> Recently, I updated to latest edk2 master (yesterday's) and I am
>>>>>>> actually encountering assert in
>>>>>>> ShellPkg/Library/UefiShellLib/UefiShellLib.c
>>>>>>>
>>>>>>> if (mUnicodeCollationProtocol == NULL) {
>>>>>>>      Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid,
>>>>>>> NULL, (VOID**)&mUnicodeCollationProtocol);
>>>>>>>      ASSERT_EFI_ERROR (Status);
>>>>>>>    }
>>>>>>>
>>>>>>> It was working few weeks back and has now stopped working.
>>>>>>> It's probably because  the platform is unable to locate this protocol
>>>>>>> "UnicodeCollationProtocol".
>>>>>>> Is Anyone else facing the same issue?
>>>>>>> Does the new ShellPkg requires additional packages/infs to be
>> included
>>>>>>> in the platform description file to work with latest changes  to get
>>>>>>> past this error?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Supreeth
>>>>>>> IMPORTANT NOTICE: The contents of this email and any attachments
>> are
>>>>>>> confidential and may also be privileged. If you are not the intended
>>>>>>> recipient, please notify the sender immediately and do not disclose
>>>>>>> the contents to any other person, use it for any purpose, or store or
>>>>>>> copy the information in any medium. Thank you.
>>>>>>> _______________________________________________
>>>>>>> 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
>>>> _______________________________________________
>>>> 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 <mailto:edk2-devel@lists.01.org>
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> <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

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

[Laszlo Ersek]

On 10/05/16 23:34, Kinney, Michael D wrote:
> Laszlo,
> 
> There are many types of UEFI Drivers and they are enumerated in the
> UEFI Driver Writer's Guide.  Some follow the UEFI Driver Model and
> some do not.
> 
> A driver that only consumes/produces UEFI services/protocols without a depex
> should be a UEFI Driver.  If there is a depex or use of any PI services/protocols
> then it must be one of the DXE driver types.
> 
> We actually prefer to categorize drivers as UEFI Driver's if possible, because 
> that maximizes the drivers's compatibility with UEFI conformant platforms.

Thanks!

It's been a while since I last looked at that part of the guide (chapter
6, UEFI Driver Categories). I guess EnglishDxe then qualifies as a
Service Driver.

... Interestingly, when people asked about automatically running
applications (not drivers) for various kinds of init work, without
staying resident, I occasionally thought about recommending a driver
module that would return an error code intentionally, even on success.
And now I see there's actually a category of drivers for this, called
"Initializing Drivers". So now I wonder why we needed SysPrep#### after
all... Probably because their order had to be specified in some way.

The Root Bridge Driver category is also intersting (under "UEFI Driver
Categories"); the core PciHostBridgeDxe in edk2 is a DXE_DRIVER. (It
does have an elaborate depex though, and not just on architectural
protocols.)

Thanks!
Laszlo


>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>> Sent: Wednesday, October 5, 2016 2: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>
>> Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2 source on the
>> Reference Platform
>>
>> On 10/05/16 23:05, Andrew Fish wrote:
>>
>>> I'm guessing that your DXE Driver is dispatching prior UEFI Driver that publishes the
>> protocol.
>>
>> BTW... Why is EnglishDxe a UEFI_DRIVER rather than a DXE_DRIVER? It
>> doesn't conform to the UEFI driver model. (I'm not saying it *must* not
>> be a UEFI_DRIVER, but what reason does it have for being one?) It also
>> calls no runtime services, and only the
>> InstallMultipleProtocolInterfaces() boot service; so I don't think it
>> relies on all of the architectural protocols either.
>>
>> Thanks
>> Laszlo
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Carsey, Jaben]

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

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

[Laszlo Ersek]

On 10/06/16 00:17, Carsey, Jaben wrote:
> 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...

I got it now. Makes sense. Thanks!
Laszlo