Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds (more)

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hao Wu,

Please provide a patch to BaseTools to restore the previous behavior.

We need that to review the complexity of the change to determine
what to do.  Without that information today, the release date 
of this stable tag is at risk.

Mike

> -----Original Message-----
> From: Wu, Hao A
> Sent: Tuesday, August 27, 2019 6:40 PM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Laszlo
> Ersek <lersek@redhat.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
> Baptiste Gerondeau <baptiste.gerondeau@linaro.org>; Wang,
> Jian J <jian.j.wang@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: RE: [PATCH 1/1] MdeModulePkg: fix !x86 builds
> (more)
> 
> > -----Original Message-----
> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > Sent: Wednesday, August 28, 2019 4:59 AM
> > To: Laszlo Ersek
> > Cc: devel@edk2.groups.io; Andrew Fish; Kinney, Michael
> D; Baptiste
> > Gerondeau; Wang, Jian J; Wu, Hao A; Feng, Bob C; Gao,
> Liming
> > Subject: Re: [PATCH 1/1] MdeModulePkg: fix !x86 builds
> (more)
> >
> > +Bob, Liming,
> >
> > On Tue, Aug 27, 2019 at 09:26:05PM +0200, Laszlo Ersek
> wrote:
> > > Hi Leif,
> > >
> > > On 08/27/19 14:43, Leif Lindholm wrote:
> > > > Commit 4a1f6b85c184
> > > > ("MdeModulePkg: add LockBoxNullLib for !IA32/X64 in
> .dsc") added
> > > > an ARM/AARCH64 resolution for LockBoxLib. However,
> this failed to
> > > > address the overrides provided for PEIM,
> DXE_DRIVER,
> > > > DXE_RUNTIME_DRIVER, DXE_SMM_DRIVER and UEFI_DRIVER,
> so any
> > modules
> > > > of those classes still failed to build.
> > > >
> > > > Break these out properly into their own
> LibraryClasses sections.
> > > >
> > > > Resolves BZ:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2134
> > > >
> > > > Signed-off-by: Leif Lindholm
> <leif.lindholm@linaro.org>
> > > > Reported-by: Baptiste Gerondeau
> <baptiste.gerondeau@linaro.org>
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > > ---
> > > >
> > > > I don't understand how the above would appear to
> work back when I
> > > > submitted the previous patch but not work now, but
> I haven't dug
> > > > into it deeper. Including the x86-specific
> LockBoxLib in the
> > > > .common section is however clearly not correct.
> > >
> > > I agree with you that the present situation is not
> correct.
> > >
> > > According to:
> > >
> > >   https://edk2-docs.gitbooks.io/edk-ii-dsc-
> >
> specification/2_dsc_overview/26_[libraryclasses]_section_
> processing.ht
> > ml
> > >
> > > the library class resolutions take effect in the
> following order
> > > (entries near the top have higher priority):
> > >
> > > > 1. <LibraryClasses> associated with the INF file in
> the
> > > > [Components]
> > section
> > > > 2. [LibraryClasses.$(Arch).$(MODULE_TYPE),
> > LibraryClasses.$(Arch).$(MODULE_TYPE)]
> > > > 3. [LibraryClasses.$(Arch).$(MODULE_TYPE)]
> > > > 4. [LibraryClasses.common.$(MODULE_TYPE)]
> > > > 5. [LibraryClasses.$(Arch)]
> > > > 6. [LibraryClasses.common] or [LibraryClasses]
> > >
> > > (Side comment 1: levels #2 and #3 look very similar;
> I think the
> > > difference is that #2 is supposed to be a multi-arch
> and/or
> > > multi-module-type section, while #3 is a single-arch
> and
> > > single-module-type section.)
> > >
> > > Commit 4a1f6b85c184 ("MdeModulePkg: add
> LockBoxNullLib for !IA32/X64
> > in
> > > .dsc", 2019-03-27) provided a LockBoxLib resolution
> at level #5:
> >
> > Yes.
> >
> > > > [LibraryClasses.ARM, LibraryClasses.AARCH64]
> > >
> > > However, the other LockBoxLib resolutions are at
> level #4:
> > >
> > > > [LibraryClasses.common.PEIM]
> > > > [LibraryClasses.common.DXE_DRIVER]
> > > > [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> > > > [LibraryClasses.common.DXE_SMM_DRIVER]
> > > > [LibraryClasses.common.UEFI_DRIVER]
> > >
> > > So the latter taking priority is actually specified
> behavior.
> >
> > Hmm. That's not great.
> > Anyway, I stopped being lazy and did a bisect.
> >
> > The culprit is
> > e8449e1d8e3b ("BaseTools: Decouple AutoGen Objects"),
> marked as
> > resolving
> https://bugzilla.tianocore.org/show_bug.cgi?id=1875.
> >
> > This also affects SignedCapsulePkg/SignedCapsulePkg.dsc
> (although once
> > addressed, AARCH64 also needs a NULL entry added for
> > CompilerIntrinsicsLib.
> >
> > > (Side comment 2: EBC is in the same boat, from commit
> cbcccd2c9d93
> > > ("Update Code to pass EBC compiler", 2013-05-13):
> > >
> > > > [LibraryClasses.EBC]
> > > >
> > > >
> LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNul
> lLib.inf
> > > )
> > >
> > > As to why this breakage was not exposed right at
> commit 4a1f6b85c184
> > > -- I have no idea. Perhaps it was hidden by a
> BaseTools issue that
> > > has been fixed meanwhile.
> >
> > Yes.
> > But it is also a fundamental change in tool behaviour
> introduced on 9
> > August. I am really uncomfortable about this making it
> into the
> > release this week - but I also believe this is the
> foundation for the
> > multiprocess autogen.
> >
> > Since you have very helpfully analyzed *what* changed
> ... would the
> > better "fix" for 2019.08 be to intentionally break the
> new code to
> > conform to the old behaviour - and then revert that
> patch after the
> > tag?
> >
> > If we do that, this patch could then wait and indeed be
> merged as part
> > of the same set.
> >
> > > On 08/27/19 14:43, Leif Lindholm wrote:
> > > > I think a fix for this issue needs to go into
> 2019.08,
> > >
> > > I agree the problem should be fixed in 2019.08 --
> taking your word
> > > that commit 4a1f6b85c184 *appeared* to fix the
> MdeModulePkg.dsc
> > > build for ARM/AARCH64, we now have a regression since
> that commit
> > > (dated 2019-03-27).
> > >
> > > > but if someone has a prettier suggestion, I am not
> wedded to this one.
> > >
> > > I think this is good enough. The lib class
> resolutions are raised to
> > > level #2, but they will no longer match ARM /
> AARCH64, so your
> > > level#5 addition from commit 4a1f6b85c184 will take
> effect.
> > >
> > > >
> > > >  MdeModulePkg/MdeModulePkg.dsc | 16 +++++++++++++--
> -
> > > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/MdeModulePkg.dsc
> > b/MdeModulePkg/MdeModulePkg.dsc
> > > > index 4320839abfb5..15ba96cecbed 100644
> > > > --- a/MdeModulePkg/MdeModulePkg.dsc
> > > > +++ b/MdeModulePkg/MdeModulePkg.dsc
> > > > @@ -109,6 +109,8 @@ [LibraryClasses.common.PEIM]
> > > >    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
> > > >
> >
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib
> /PeiMemory
> > AllocationLib.inf
> > > >
> >
> ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedSe
> ctionLib/PeiE
> > ExtractGuidedSectionLib|x
> > tractGuidedSectionLib.inf
> > > > +
> > > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM]
> > > >
> >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxP
> eiLib.inf
> > >
> > > (1) I suggest replacing "," with ", ". (That's more
> consistent with
> > > preexistent section headers in the DSC file.) Applies
> to the other
> > > new section headers too.
> >
> > Yes, fair point.
> >
> > > >
> > > >  [LibraryClasses.common.DXE_CORE]
> > > > @@ -118,18 +120,22 @@
> [LibraryClasses.common.DXE_CORE]
> > > >
> > > >  [LibraryClasses.common.DXE_DRIVER]
> > > >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > > > -
> >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD
> xeLib.in
> > f
> > > >
> >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLi
> b/UefiMemo
> > ryAllocationLib.inf
> > > >
> >
> ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSe
> ctionLib/DxeE
> > xtractGuidedSectionLib.inf
> > > >
> >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsu
> leLib.inf
> > > >
> > > >
> +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_D
> RIVER]
> > > > +
> >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD
> xeLib.in
> > f
> > > > +
> > > >  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> > > >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > > >
> >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLi
> b/UefiMemo
> > ryAllocationLib.inf
> > > >
> >
> DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibCo
> nOut.inf
> > > > -
> >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD
> xeLib.in
> > f
> > > >
> >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRunti
> meCapsule
> > Lib.inf
> > > >
> > > >
> >
> +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses.X
> 64.DXE_RUNTI
> > ME_DRIVER]
> > > > +
> >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD
> xeLib.in
> > f
> > > > +
> > > >  [LibraryClasses.common.SMM_CORE]
> > > >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > > >
> >
> MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryA
> llocatio
> > nLib/PiSmmCoreMemoryAllocationLib.inf
> > > > @@ -143,13 +149,17 @@
> [LibraryClasses.common.DXE_SMM_DRIVER]
> > > >
> >
> MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib
> /SmmMe
> > moryAllocationLib.inf
> > > >
> >
> MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmSe
> rvicesTabl
> > eLib.inf
> > > >
> >
> SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/Sm
> mServicesT
> > ableLib.inf
> > > > +  SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf
> > > > +
> > > >
> >
> +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64.D
> XE_SMM_DRI
> > VER]
> > > >
> >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxS
> mmLib.i
> > nf
> > > > -  SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf
> > > >
> > >
> > > I wonder if this is really necessary. I would assume
> all the
> > > DXE_SMM_DRIVER modules are listed under
> > >
> > >   [Components.IA32, Components.X64]
> > >
> > > already. But, this hunk certainly doesn't hurt.
> >
> > Well, this fixes the current issue. I completely agree
> the file could
> > benefit from some overall restructuring.
> >
> > > >  [LibraryClasses.common.UEFI_DRIVER]
> > > >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > > >
> >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLi
> b/UefiMemo
> > ryAllocationLib.inf
> > > >
> >
> DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibCo
> nOut.inf
> > > > +
> > > >
> +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEFI
> _DRIVER]
> > > >
> >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxD
> xeLib.in
> > f
> > > >
> > > >  [LibraryClasses.common.UEFI_APPLICATION]
> > > >
> > >
> > > With (1) fixed (feel free to correct that just before
> pushing):
> > >
> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> > Thanks!
> >
> > > Do wait for maintainer review, of course.
> >
> > Of course.
> 
> 
> Per my understanding to the analysis from Leif, Laszlo
> and Mike, the patch will depend on the precedence of the
> below rules:
> 
> * [LibraryClasses.common.$(MODULE_TYPE)]
> * [LibraryClasses.$(Arch)]
> 
> So for now, we should wait for the final call on the
> above open, right?
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Best Regards,
> >
> > Leif