From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from msmail.insydesw.com.tw (ms.insydesw.com [211.75.113.220]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9CA281A1E2A for ; Wed, 5 Oct 2016 14:33:04 -0700 (PDT) Received: from msmail.insydesw.com.tw ([fe80::74f7:f173:f4aa:9a05]) by msmail.insydesw.com.tw ([fe80::74f7:f173:f4aa:9a05%11]) with mapi id 14.01.0438.000; Thu, 6 Oct 2016 05:33:02 +0800 From: Tim Lewis To: "Carsey, Jaben" , "afish@apple.com" , Laszlo Ersek CC: edk2-devel-01 , Leif Lindholm Thread-Topic: [edk2] Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform Thread-Index: AdIekL1EjMpAqwOYRWKk8YKyvfLMLgAjbAlwAACkBtD//7QPAIAABswAgAAII4CAAAHsAIAAAr6A//94KYCAAI6EAIAAAEoA//94TQCAAIy7gP//d6vw Date: Wed, 5 Oct 2016 21:33:01 +0000 Message-ID: <7236196A5DF6C040855A6D96F556A53F3F4043@msmail.insydesw.com.tw> References: <93F01BC9-4B02-467E-B900-65C6775BB0F3@apple.com> <618fd786-24d4-653c-d6b8-cb774654c644@redhat.com> <7236196A5DF6C040855A6D96F556A53F3F3F6D@msmail.insydesw.com.tw> <7236196A5DF6C040855A6D96F556A53F3F3FD7@msmail.insydesw.com.tw> In-Reply-To: Accept-Language: en-US, zh-TW X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [192.168.100.107] MIME-Version: 1.0 Subject: Re: Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Oct 2016 21:33:05 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 Sh= ell Protocol to exist without (at least) Unicode Collation or Unicode Colla= tion 2 from existing. So adding a check to the function does not help it, because in every case t= hat counts (where shell protocol is produced by some sort of standard shell= ), Unicode Collation already exists. Status =3D gEfiShellProtocol->OpenFileByName(FileName, FileHandle, OpenMode); if ((mUnicodeCollationProtocol->StriColl (mUnicodeCollationProtocol, (C= HAR16*)FileName, L"NUL") !=3D 0) && (mUnicodeCollationProtocol->StriColl (mUnicodeCollationProtocol, (C= HAR16*)FileName, L"NULL") !=3D 0) && !EFI_ERROR(Status) && ((OpenMode & EFI_FILE_MODE_CREATE) !=3D 0)){ FileInfo =3D FileFunctionMap.GetFileInfo(*FileHandle); << WITHOUT A S= HELL IT WILL FAIL HERE ASSERT(FileInfo !=3D NULL); FileInfo->Attribute =3D Attributes; Status2 =3D FileFunctionMap.SetFileInfo(*FileHandle, FileInfo); FreePool(FileInfo); if (EFI_ERROR (Status2)) { gEfiShellProtocol->CloseFile(*FileHandle); } Status =3D Status2; } -----Original Message----- From: Carsey, Jaben [mailto:jaben.carsey@intel.com]=20 Sent: Wednesday, October 05, 2016 2:17 PM To: Tim Lewis ; afish@apple.com; Laszlo Ersek Cc: edk2-devel-01 ; Leif Lindholm ; Carsey, Jaben 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 c= heck 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=20 > Tim Lewis > Sent: Wednesday, October 05, 2016 2:07 PM > To: afish@apple.com; Laszlo Ersek > Cc: Carsey, Jaben ; edk2-devel-01 devel@ml01.01.org>; Leif Lindholm > Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2=20 > source on the Reference Platform > Importance: High >=20 > If he has a DXE driver that consumes the shell lib (a valid case),=20 > then my original suggestion stands: move it to=20 > ShellLibConstructorWorker. This is where the ShellLib's internal pointers= to the Shell protocol are initialized. >=20 > Tim >=20 > -----Original Message----- > From: afish@apple.com [mailto:afish@apple.com] > Sent: Wednesday, October 05, 2016 1:59 PM > To: Laszlo Ersek > Cc: Tim Lewis ; Carsey, Jaben=20 > ; Shah, Tapan ; Daniil=20 > Egranov ; edk2-devel-01 devel@ml01.01.org>; Leif Lindholm > Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2=20 > source on the Reference Platform >=20 >=20 > > On Oct 5, 2016, at 1:58 PM, Laszlo Ersek wrote: > > > > On 10/05/16 22:44, Tim Lewis wrote: > >> Jaben -- > >> > >> In which cases would you have the Shell protocol present and not=20 > >> have > the Unicode Collation protocol? > > > > That's exactly the question! > > > > According to the spec, at least if the system cannot boot from a=20 > > 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=20 > > change (=3D commit 583448b441650) as a regression on Juno. If the=20 > > shell couldn't function without the collation protocol even before=20 > > commit 583448b441650, then the shell must never have run on Juno --=20 > > because it appears to lack collation -- and then this change cannot be = a regression. > > >=20 > Looks like he has a DXE Driver that consume the ShellLib. >=20 > Thanks, >=20 > Andrew Fish >=20 > > Thanks > > Laszlo > > > > > >> > >> Tim > >> > >> > >> -----Original Message----- > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf=20 > >> Of Carsey, Jaben > >> Sent: Wednesday, October 05, 2016 1:35 PM > >> To: Shah, Tapan ; Laszlo Ersek=20 > >> ; Andrew Fish ; Daniil Egranov=20 > >> > >> Cc: Carsey, Jaben ; edk2-devel-01=20 > >> ; Leif Lindholm > >> Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2=20 > >> source on the Reference Platform > >> > >> That should work. We also need to initialize the variable to NULL=20 > >> in the > constructor. > >> > >> Do we need to be case insensitive for this? Can we use memcmp=20 > >> 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 ; Andrew Fish > ; > >>> Daniil Egranov > >>> Cc: Carsey, Jaben ; edk2-devel-01 >>> devel@ml01.01.org>; Leif Lindholm > >>> Subject: RE: [edk2] Assert in ShellPkg with latest tianocore edk2=20 > >>> source on the Reference Platform > >>> Importance: High > >>> > >>> How about moving protocol locate from constructor to the actual=20 > >>> function level and returning an error if it fails to locate (See=20 > >>> attached > patch file). > >>> > >>> Tapan > >>> > >>> -----Original Message----- > >>> From: Laszlo Ersek [mailto:lersek@redhat.com] > >>> Sent: Wednesday, October 05, 2016 3:18 PM > >>> To: Andrew Fish ; Daniil Egranov=20 > >>> > >>> Cc: Carsey, Jaben ; edk2-devel-01 >>> devel@ml01.01.org>; Leif Lindholm ; Shah,=20 > >>> Tapan > >>> Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2=20 > >>> source on the Reference Platform > >>> > >>> On 10/05/16 21:48, Andrew Fish wrote: > >>>> > >>>>> On Oct 5, 2016, at 12:24 PM, Daniil Egranov=20 > >>>>> > >>> wrote: > >>>>> > >>>>> I have the same ASSERT issue on Juno platform even the=20 > >>>>> EnglishDxe.inf is > >>> included to the platform build. If UefiShellLib has such=20 > >>> dependency on the protocol then according to EDKII Module Writer's=20 > >>> Guide you need to specify the dependency on protocol in the module=20 > >>> .inf to ensure the drivers proper load sequence. > >>>>> > >>>>> 8.6 Dependency Expressions > >>>>> A dependency expression specifies the protocols that the DXE=20 > >>>>> driver requires to execute. In EDK II, it is specified in the=20 > >>>>> [Depex] section of INF > >>> file. > >>>>> > >>>> > >>>> The Dependency Expression is for DXE Drivers that are dispatched=20 > >>>> by the > >>> DXE Core. A UEFI Driver from an option ROM or an Application does=20 > >>> not get dispatched by the dispatch and the Depex will not help.=20 > >>> The Depex ends up being a section in the FV and it has nothing to=20 > >>> 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=20 > >>>> 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=20 > >>> even be copied or moved over -- and can translate the ASCII subset=20 > >>> of UCS-2, > >>> > >>> - once the ASCII characters of the input string have been=20 > >>> translated to upper case, the result could be searched for /=20 > >>> compared against > L"NULL" > >>> using BaseLib.h APIs. > >>> > >>> (Given that L"NULL" only contains characters from the ASCII=20 > >>> subset, it suffices to upper-case ASCII chars in the input string.=20 > >>> No non-ASCII character in the BMP can translate to ASCII N/U/L via=20 > >>> upcasing, even with the real collation protocol (I trust), and no=20 > >>> other ASCII characters than n/u/l will translate to N/U/L. This=20 > >>> means that we won't miss an instance of NULL because we didn't=20 > >>> upcate it (because it was non-ascii), and it also means we won't=20 > >>> 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 > >> > > >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel