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

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

Leif,

Looking at the DSC Spec in looks like the priority
change from Aug 9 was to invert the priority 4 and
priority 5 items:

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]

If BaseTools were updated to make (5) higher priority than (4),
then the previous behavior would be restored and no DSC file 
changes would be required.  This means an arch specific mapping
for all module types is higher priority than a module specific
mapping for all archs.

We can update DSC Spec to match the behavior that has been
implemented for a long time.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
> On Behalf Of Leif Lindholm
> Sent: Tuesday, August 27, 2019 1:59 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Baptiste
> Gerondeau <baptiste.gerondeau@linaro.org>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2-devel] [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/LockBoxNu
> llLib.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/PeiMemoryAllocationLi
> b/PeiMemoryAllocationLib.inf
> > >
> > >
> ExtractGuidedSectionLib|MdePkg/Library/PeiExtractGuidedS
> ectionLib/Pe
> > > iExtractGuidedSectionLib.inf
> > > +
> > > +[LibraryClasses.IA32.PEIM,LibraryClasses.X64.PEIM]
> > >
> > >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox
> PeiLib.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/SmmLockBox
> DxeLib.inf
> > >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationL
> ib/UefiMemoryAllocationLib.inf
> > >
> ExtractGuidedSectionLib|MdePkg/Library/DxeExtractGuidedS
> ectionLib/DxeExtractGuidedSectionLib.inf
> > >
> > >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCaps
> uleLib.inf
> > >
> > >
> +[LibraryClasses.IA32.DXE_DRIVER,LibraryClasses.X64.DXE_
> DRIVER]
> > > +
> > >
> +LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBo
> xDxeLib.inf
> > > +
> > >  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> > >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > >
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationL
> ib/UefiMemoryAllocationLib.inf
> > >
> DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibC
> onOut.inf
> > > -
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox
> DxeLib.inf
> > >
> > >
> CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRunt
> imeCapsuleLi
> > > b.inf
> > >
> > >
> +[LibraryClasses.IA32.DXE_RUNTIME_DRIVER,LibraryClasses.
> X64.DXE_RUNT
> > > +IME_DRIVER]
> > > +
> > >
> +LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBo
> xDxeLib.inf
> > > +
> > >  [LibraryClasses.common.SMM_CORE]
> > >    HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> > >
> > >
> MemoryAllocationLib|MdeModulePkg/Library/PiSmmCoreMemory
> AllocationLi
> > > b/PiSmmCoreMemoryAllocationLib.inf
> > > @@ -143,13 +149,17 @@
> [LibraryClasses.common.DXE_SMM_DRIVER]
> > >
> MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLi
> b/SmmMemoryAllocationLib.inf
> > >
> MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmS
> ervicesTableLib.inf
> > >
> > >
> SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/S
> mmServicesTa
> > > bleLib.inf
> > > +  SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf
> > > +
> > >
> +[LibraryClasses.IA32.DXE_SMM_DRIVER,LibraryClasses.X64.
> DXE_SMM_DRIV
> > > +ER]
> > >
> > >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox
> SmmLib.inf
> > > -  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/UefiMemoryAllocationL
> ib/UefiMemoryAllocationLib.inf
> > >
> DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibC
> onOut.inf
> > > +
> > >
> +[LibraryClasses.IA32.UEFI_DRIVER,LibraryClasses.X64.UEF
> I_DRIVER]
> > >
> > >
> LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBox
> DxeLib.inf
> > >
> > >  [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.
> 
> Best Regards,
> 
> Leif
> 
>