Re: [edk2-devel] BaseTools/AutoGen: declare ProcessLibraryConstructorList() for SEC modules

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

Hi Laszlo,

I need to review the proposed BaseTools/AutoGen change to see what options
are available for compatibility.

My main concern is downstream consumers that may break immediately with
a change like this and we need a way for them to be informed and have
time to update their components just like you outline a sequence to update
the edk2 repo components.

Thanks,

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 7, 2024 7:40 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel-groups-
> io <devel@edk2.groups.io>
> Cc: Abdul Lateef Attar <abdattar@amd.com>; Abner Chang
> <abner.chang@amd.com>; Warkentin, Andrei <andrei.warkentin@intel.com>;
> Andrew Fish <afish@apple.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; S, Ashraf Ali <ashraf.ali.s@intel.com>;
> Bibo Mao <maobibo@loongson.cn>; Feng, Bob C <bob.c.feng@intel.com>;
> West, Catharine <catharine.west@intel.com>; Chao Li
> <lichao@loongson.cn>; Chiu, Chasel <chasel.chiu@intel.com>; Duggapu,
> Chinni B <chinni.b.duggapu@intel.com>; Duke Zhai <duke.zhai@amd.com>;
> Aktas, Erdem <erdemaktas@google.com>; Eric Xing <eric.xing@amd.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Guo, Gua <gua.guo@intel.com>; Dong,
> Guo <guo.dong@intel.com>; Igniculus Fu <igniculus.fu@amd.com>; Lu,
> James <james.lu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kelly
> Steele <kelly.steele@intel.com>; Ken Yao <ken.yao@amd.com>; Leif
> Lindholm <quic_llindhol@quicinc.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Michael Roth <michael.roth@amd.com>; Xu,
> Min M <min.m.xu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Paul Grimes <paul.grimes@amd.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Rebecca Cran <rebecca@bsdio.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Sami Mujawar <sami.mujawar@arm.com>;
> Rhodes, Sean <sean@starlabs.systems>; Zeng, Star <star.zeng@intel.com>;
> Sunil V L <sunilvl@ventanamicro.com>; Mohapatra, Susovan
> <susovan.mohapatra@intel.com>; Kuo, Ted <ted.kuo@intel.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; USER0FISH <libing1202@outlook.com>;
> Xianglai li <lixianglai@loongson.cn>; Chen, Christine
> <yuwei.chen@intel.com>; caiyuqing379 <caiyuqing_hz@outlook.com>; dahogn
> <dahogn@hotmail.com>; meng-cz <mengcz1126@gmail.com>
> Subject: Re: BaseTools/AutoGen: declare ProcessLibraryConstructorList()
> for SEC modules
> 
> On 2/7/24 02:16, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > The main issue is that a non-backwards compatible change is being
> > proposed to BaseTools.
> >
> > In order for this to work, BaseTools needs to support both the
> > old style and new style SEC autogen.
> >
> > The guarantee of backwards compatibility is something that is
> required
> > for BaseTools.  Similar to constrains on compilers/linkers.  New
> features
> > can be added, but the old behavior has to always be supported.
> Removing
> > old behavior takes a very long time.
> 
> This change consists of three parts:
> 
> (1) generate the SEC ProcessLibraryConstructorList() declaration in
> "AutoGen.h"
> 
> (2) in those modules that declare SEC ProcessLibraryConstructorList()
> themselves *properly*, remove the (now superfluous, but certainly *not
> conflicting*) manual declarations
> 
> (3) in those modules that declare SEC ProcessLibraryConstructorList()
> themselves *incorrectly*, either manually, or by way of including
> <Library/PeimEntryPoint.h>, remove the *now-conflicting* manual
> declarations / inclusions.
> 
> For modules that fall in category (2), the AutoGen change (1) is
> compatible. The AutoGen change (1) is incompatible only with modules
> that fall into category (3) -- that is, with modules that already
> declare SEC ProcessLibraryConstructorList() incorrectly.
> 
> In other words, the compatibility we're talking about here is *bug
> compatibility*.
> 
> If bug compat is a goal, then the AutoGen patch cannot be merged at
> all.
> 
> If bug compat is not a goal, then:
> 
> (a) the AutoGen change could be / should be merged into the separate
> BaseTools repository at first,
> 
> (b) for edk2, I should rework this series such that:
> 
> - modules from category (3) be brought to category (2) at first (i.e.,
> replacing the bogus calls / manual declarations with proper calls /
> manual declarations),
> 
> - the edk2-basetools dependency in "pip-requirements.txt" be bumped
> from
> 0.1.48 to the new version,
> 
> - finally, the manual (but otherwise, now-correct) declarations in the
> modules of category (2) be eliminated.
> 
> Therefore, my question is: do we intend to preserve bug compatibility?
> 
> Thanks!
> Laszlo
> 
> 
> 
> >
> > Mike
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Tuesday, February 6, 2024 4:51 PM
> >> To: edk2-devel-groups-io <devel@edk2.groups.io>
> >> Cc: Abdul Lateef Attar <abdattar@amd.com>; Abner Chang
> >> <abner.chang@amd.com>; Warkentin, Andrei
> <andrei.warkentin@intel.com>;
> >> Andrew Fish <afish@apple.com>; Ard Biesheuvel
> >> <ardb+tianocore@kernel.org>; S, Ashraf Ali <ashraf.ali.s@intel.com>;
> >> Bibo Mao <maobibo@loongson.cn>; Feng, Bob C <bob.c.feng@intel.com>;
> >> West, Catharine <catharine.west@intel.com>; Chao Li
> >> <lichao@loongson.cn>; Chiu, Chasel <chasel.chiu@intel.com>; Duggapu,
> >> Chinni B <chinni.b.duggapu@intel.com>; Duke Zhai
> <duke.zhai@amd.com>;
> >> Aktas, Erdem <erdemaktas@google.com>; Eric Xing <eric.xing@amd.com>;
> >> Gerd Hoffmann <kraxel@redhat.com>; Guo, Gua <gua.guo@intel.com>;
> Dong,
> >> Guo <guo.dong@intel.com>; Igniculus Fu <igniculus.fu@amd.com>; Lu,
> >> James <james.lu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Kelly
> >> Steele <kelly.steele@intel.com>; Ken Yao <ken.yao@amd.com>; Leif
> >> Lindholm <quic_llindhol@quicinc.com>; Liming Gao
> >> <gaoliming@byosoft.com.cn>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Michael Roth <michael.roth@amd.com>;
> Xu,
> >> Min M <min.m.xu@intel.com>; Desimone, Nathaniel L
> >> <nathaniel.l.desimone@intel.com>; Paul Grimes <paul.grimes@amd.com>;
> >> Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>;
> >> Rebecca Cran <rebecca@bsdio.com>; Chaganty, Rangasai V
> >> <rangasai.v.chaganty@intel.com>; Sami Mujawar
> <sami.mujawar@arm.com>;
> >> Rhodes, Sean <sean@starlabs.systems>; Zeng, Star
> <star.zeng@intel.com>;
> >> Sunil V L <sunilvl@ventanamicro.com>; Mohapatra, Susovan
> >> <susovan.mohapatra@intel.com>; Kuo, Ted <ted.kuo@intel.com>; Tom
> >> Lendacky <thomas.lendacky@amd.com>; USER0FISH
> <libing1202@outlook.com>;
> >> Xianglai li <lixianglai@loongson.cn>; Chen, Christine
> >> <yuwei.chen@intel.com>; caiyuqing379 <caiyuqing_hz@outlook.com>;
> dahogn
> >> <dahogn@hotmail.com>; meng-cz <mengcz1126@gmail.com>
> >> Subject: BaseTools/AutoGen: declare ProcessLibraryConstructorList()
> for
> >> SEC modules
> >>
> >> Bugzillas:
> >> - https://bugzilla.tianocore.org/show_bug.cgi?id=990
> >> - https://bugzilla.tianocore.org/show_bug.cgi?id=991
> >>
> >> Personal build:
> >> - https://github.com/tianocore/edk2/pull/5354
> >>
> >> I'll follow up with two series, in response to this message. One of
> >> those sets makes AutoGen declare ProcessLibraryConstructorList() for
> >> SEC
> >> modules (as Liming suggested), and cleans up the buggy or
> superfluous
> >> manual declarations in edk2. The other set cleans up edk2-platforms
> >> similarly. The edk2 set is intended to be bisectable. The edk2-
> >> platforms
> >> set can't be like that (separate repo anyway). I build-tested
> >> everything
> >> that I could, but the coverage is not complete in edk2-platforms.
> >>
> >> Note that the personal build (PR#5354) failed. The reason is that CI
> >> uses an out-of-tree BaseTools installation / package (version 0.1.48
> at
> >> the moment). This just goes on to show why it's a bad idea to
> develop
> >> BaseTools in separation from edk2: even though the edk2 series *is*
> >> fully bisectable, it's impossible to get it through CI (because CI
> >> ignores the BaseTools update inside the series); and if we get the
> >> central BaseTools patch from the edk2 series into "that other"
> >> (separate) project *first*, then the series will not be bisectable
> any
> >> longer (because the BaseTools/AutoGen update at the front, without
> >> further patches in the series applied, will trigger build errors).
> >>
> >> Laszlo
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115270): https://edk2.groups.io/g/devel/message/115270
Mute This Topic: https://groups.io/mt/104210524/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-