I think the shell DSC be updated to catch build errors in the NULL libs.
I am good with Mike's proposed solution below, but not very concerned with the
method to catch the error.
Thanks
-Jaben
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Thursday, August 01, 2019 1:30 PM
> To: Sami Mujawar <sami.muja...@arm.com>; devel@edk2.groups.io;
> Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>;
> Gao, Zhichao <zhichao....@intel.com>
> Subject: RE: [edk2-devel] [Patch] ShellPkg/AcpiView: Fix IA32 link error
>
> Hi Sami,
>
> I agree with your feedback. I saw that there was a larger
> patch set for the ShellPkg, so I let that complete before
> returning to this topic.
>
> The reason that I noticed this issue in the first place is
> when I added the acpiview command to a platform and there
> was an IA32 build failure. It would be better if the ShellPkg
> build caught this issue. Adding the acpiview command to
> the standard shell build adds 50K to an uncompressed DEBUG
> IA32 build.
>
> 869,312 Shell_7C04A583-9E3E-4f1c-AD65-E05268D0B4D1.efi
> 920,928 Shell_EA4BB293-2D7F-4456-A681-1F22F42CD0BC.efi
>
> Should acpiview be added to the standard ShellPkg build,
> or should I add an extra build of the Shell to the
> ShellPkg DSC file with a different GUID to make sure
> the shell builds when all NULL libs are included without
> any !if statements. For example:
>
> ShellPkg/Application/Shell/Shell.inf {
> <Defines>
> FILE_GUID = EA4BB293-2D7F-4456-A681-1F22F42CD0BC
> <PcdsFixedAtBuild>
> gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> <LibraryClasses>
>
> NULL|ShellPkg/Library/UefiShellLevel2CommandsLib/UefiShellLevel2Comma
> ndsLib.inf
>
> NULL|ShellPkg/Library/UefiShellLevel1CommandsLib/UefiShellLevel1Comma
> ndsLib.inf
>
> NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3Comma
> ndsLib.inf
>
> NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1Com
> mandsLib.inf
>
> NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1Com
> mandsLib.inf
>
> NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Com
> mandsLib.inf
>
> NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1
> CommandsLib.inf
>
> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2
> CommandsLib.inf
>
> NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCo
> mmandLib.inf
> }
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: Sami Mujawar [mailto:sami.muja...@arm.com]
> > Sent: Thursday, July 11, 2019 1:24 AM
> > To: devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kin...@intel.com>
> > Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray
> > <ray...@intel.com>; Gao, Zhichao
> > <zhichao....@intel.com>
> > Subject: RE: [edk2-devel] [Patch] ShellPkg/AcpiView:
> > Fix IA32 link error
> >
> > Hi Mike,
> >
> > Since LocalityCount is 64-bit wide the SLIT validation
> > code could possibly end up in an infinite loop. I am
> > not aware of a platform that has a large enough
> > LocalityCount to hit this condition. However, would it
> > be good to have a check that limits the validation to
> > MAX_UINT32?
> >
> > e.g. Something like
> > if (LocalityCount < MAX_UINT32) {
> > // Validate
> > for (Count = 0; Count < LocalityCount; Count++) {
> > for (Index = 0; Index < LocalityCount; Index++) {
> > ...
> > } else {
> > Print (L"INFO: Skipping validation of System
> > Localities as locality count is > MAX_UINT32\n"); }
> >
> > Regards,
> >
> > Sami Mujawar
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On
> > Behalf Of Michael D Kinney via Groups.Io
> > Sent: 10 July 2019 11:35 PM
> > To: devel@edk2.groups.io
> > Cc: Jaben Carsey <jaben.car...@intel.com>; Ray Ni
> > <ray...@intel.com>; Zhichao Gao <zhichao....@intel.com>
> > Subject: [edk2-devel] [Patch] ShellPkg/AcpiView: Fix
> > IA32 link error
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1970
> >
> > Update local variable in ParseAcpiSlot() to be UINT32
> > instead of UINT64 to avoid 64-bit multiply operation in
> > the SLIT_ELEMENT() macro.
> >
> > Cc: Jaben Carsey <jaben.car...@intel.com>
> > Cc: Ray Ni <ray...@intel.com>
> > Cc: Zhichao Gao <zhichao....@intel.com>
> > Signed-off-by: Michael D Kinney
> > <michael.d.kin...@intel.com>
> > ---
> >
> > .../UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser
> > .c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/
> > Slit/SlitParser.c
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/
> > Slit/SlitParser.c
> > index 1f9dac66ee..af85c9aa1c 100644
> > ---
> > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/
> > Slit/SlitParser.c
> > +++
> > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/
> > Slit/SlitPars
> > +++ er.c
> > @@ -57,8 +57,8 @@ ParseAcpiSlit (
> > )
> > {
> > UINT32 Offset;
> > - UINT64 Count;
> > - UINT64 Index;
> > + UINT32 Count;
> > + UINT32 Index;
> > UINT64 LocalityCount;
> > UINT8* LocalityPtr;
> > CHAR16 Buffer[80]; // Used for AsciiName param of
> > ParseAcpi
> > --
> > 2.21.0.windows.1
> >
> >
> >
> >
> > 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44807): https://edk2.groups.io/g/devel/message/44807
Mute This Topic: https://groups.io/mt/32421790/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
