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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----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.html
> >
> > 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/LockBoxNullLib.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/PeiExtractGuidedSectionLib/PeiEx
> tractGuidedSectionLib.inf
> > > +
> > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM]
> > >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.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/SmmLockBoxDxeLib.in
> f
> > >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemo
> ryAllocationLib.inf
> > >
> ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedSectionLib/DxeE
> xtractGuidedSectionLib.inf
> > >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf
> > >
> > > +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_DRIVER]
> > > +
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.in
> f
> > > +
> > >  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> > >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemo
> ryAllocationLib.inf
> > >
> DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
> > > -
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.in
> f
> > >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsule
> Lib.inf
> > >
> > >
> +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses.X64.DXE_RUNTI
> ME_DRIVER]
> > > +
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.in
> f
> > > +
> > >  [LibraryClasses.common.SMM_CORE]
> > >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > >
> MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemoryAllocatio
> nLib/PiSmmCoreMemoryAllocationLib.inf
> > > @@ -143,13 +149,17 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
> > >
> MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMe
> moryAllocationLib.inf
> > >
> MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTabl
> eLib.inf
> > >
> SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesT
> ableLib.inf
> > > +  SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf
> > > +
> > >
> +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64.DXE_SMM_DRI
> VER]
> > >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxSmmLib.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/UefiMemoryAllocationLib/UefiMemo
> ryAllocationLib.inf
> > >
> DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
> > > +
> > > +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEFI_DRIVER]
> > >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.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