From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0486E1A1E1E for ; Wed, 5 Oct 2016 15:17:23 -0700 (PDT) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 05 Oct 2016 15:17:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,451,1473145200"; d="scan'208";a="887132350" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga003.jf.intel.com with ESMTP; 05 Oct 2016 15:17:24 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 5 Oct 2016 15:17:22 -0700 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.167]) by FMSMSX110.amr.corp.intel.com ([169.254.14.41]) with mapi id 14.03.0248.002; Wed, 5 Oct 2016 15:17:22 -0700 From: "Carsey, Jaben" To: Tim Lewis , "afish@apple.com" , Laszlo Ersek CC: edk2-devel-01 , Leif Lindholm , "Carsey, Jaben" Thread-Topic: [edk2] Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform Thread-Index: AdIekL1EjMpAqwOYRWKk8YKyvfLMLgAjbAlwAACkBtAAFfCNAAAA2Y4AAAEEU4AAAD2LAAAOXvpQ//+ShACAAAPuAIAAAEoAgAACDwCAAHKqMP//lKmAgABqSdA= Date: Wed, 5 Oct 2016 22:17:22 +0000 Message-ID: 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> <7236196A5DF6C040855A6D96F556A53F3F4043@msmail.insydesw.com.tw> In-Reply-To: <7236196A5DF6C040855A6D96F556A53F3F4043@msmail.insydesw.com.tw> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZmJmNzQxZjMtMTQxNS00OTk0LWEyYzEtY2IyZmY5ZDI5Mzk5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ik8zdVQ0UWtQSHpoYStxb3htTFlhUmhzUE9kRWN2TklHK2xsdGoxS21PZUk9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.1.200.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 22:17:24 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, As Tim clarified, the API already fail when the shell protocol was not pres= ent and the shell already needs the UnicodeCollation protocol. The goal here is not to allow the library to function without the shell pro= tocol (and thereby the UnicodeCollation protocol). The goal is to allow th= e constructor to complete successfully. So that a DXE driver (or other driv= er) can be loaded into memory and wait for the shell to be present to incre= ase 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 she= ll anyways. This was already true since the protocol was not present for t= he 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 ; afish@apple.com; Laszlo Ersek > > Cc: edk2-devel-01 ; Leif Lindholm > > Subject: RE: [edk2] Assert in ShellPkg with latest tianocore edk2 source = on > the Reference Platform > Importance: High >=20 > Jaben -- >=20 > Here is the relevant piece of code. There is *no way* for Unicode Collati= on to > execute without the Shell Protocol existing! There is no way for the Shel= l > Protocol to exist without (at least) Unicode Collation or Unicode Collati= on 2 > from existing. >=20 > 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. >=20 > Status =3D gEfiShellProtocol->OpenFileByName(FileName, > FileHandle, > OpenMode); > if ((mUnicodeCollationProtocol->StriColl (mUnicodeCollationProtocol, > (CHAR16*)FileName, L"NUL") !=3D 0) && > (mUnicodeCollationProtocol->StriColl (mUnicodeCollationProtocol, > (CHAR16*)FileName, L"NULL") !=3D 0) && > !EFI_ERROR(Status) && ((OpenMode & EFI_FILE_MODE_CREATE) !=3D 0)= ){ > FileInfo =3D FileFunctionMap.GetFileInfo(*FileHandle); << WITHOUT A > SHELL 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; > } >=20 >=20 >=20 > -----Original Message----- > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > 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 >=20 > That is not enough. That gets called by the constructor. We need to not > check in either constructor or the constructor worker. >=20 > That difference only matters for the shell binary itself. >=20 > -Jaben >=20 > > -----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 > > Cc: Carsey, Jaben ; edk2-devel-01 > devel@ml01.01.org>; Leif Lindholm > > 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 pointe= rs 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 > > Cc: Tim Lewis ; Carsey, Jaben > > ; Shah, Tapan ; Daniil > > Egranov ; edk2-devel-01 > devel@ml01.01.org>; Leif Lindholm > > 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 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 (=3D 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 b= e 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 ; Laszlo Ersek > > >> ; Andrew Fish ; Daniil Egranov > > >> > > >> Cc: Carsey, Jaben ; edk2-devel-01 > > >> ; Leif Lindholm > > >> 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 ; 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 > > >>> 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 ; Daniil Egranov > > >>> > > >>> Cc: Carsey, Jaben ; edk2-devel-01 > >>> devel@ml01.01.org>; Leif Lindholm ; Shah, > > >>> Tapan > > >>> 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 > > >>>>> > > >>> 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