| Message ID | 1409698598-9766-1-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/03/14 00:56, Laszlo Ersek wrote: > Calls to constructors of interdependent library instances are generated in > the correct order only if all library instances in question have > constructors. If some have no constructors, then the rest may see their > constructors called out of order. > > Cycle detection also only works when all library instances have > constructors. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > .../BaseOrderedCollectionRedBlackTreeLib.inf | 2 ++ > .../BaseOrderedCollectionRedBlackTreeLib.c | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > index a68afc8..f846a79 100644 > --- a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > +++ b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > @@ -32,6 +32,8 @@ > VERSION_STRING = 1.0 > LIBRARY_CLASS = OrderedCollectionLib > > + CONSTRUCTOR = BaseOrderedCollectionRedBlackTreeLibConstructor > + > # > # VALID_ARCHITECTURES = IA32 X64 IPF EBC > # > diff --git a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c > index 8d18a4b..23ba8de 100644 > --- a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c > +++ b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c > @@ -1452,3 +1452,13 @@ RedBlackTreeValidate ( > DEBUG ((DEBUG_VERBOSE, "%a: Tree=%p BlackHeight=%Ld Count=%Ld\n", > __FUNCTION__, Tree, (INT64)BlackHeight, (INT64)ForwardCount)); > } > + > + > +RETURN_STATUS > +EFIAPI > +BaseOrderedCollectionRedBlackTreeLibConstructor ( > + VOID > + ) > +{ > + return RETURN_SUCCESS; > +} > Self-NACK for this patchset -- please ignore. I've seen the light. Thanks Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
Liming, Michael, On 09/03/14 00:56, Laszlo Ersek wrote: > Calls to constructors of interdependent library instances are generated in > the correct order only if all library instances in question have > constructors. If some have no constructors, then the rest may see their > constructors called out of order. > > Cycle detection also only works when all library instances have > constructors. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > .../BaseOrderedCollectionRedBlackTreeLib.inf | 2 ++ > .../BaseOrderedCollectionRedBlackTreeLib.c | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > index a68afc8..f846a79 100644 > --- a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > +++ b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > @@ -32,6 +32,8 @@ > VERSION_STRING = 1.0 > LIBRARY_CLASS = OrderedCollectionLib > > + CONSTRUCTOR = BaseOrderedCollectionRedBlackTreeLibConstructor > + > # > # VALID_ARCHITECTURES = IA32 X64 IPF EBC > # > diff --git a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c > index 8d18a4b..23ba8de 100644 > --- a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c > +++ b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c > @@ -1452,3 +1452,13 @@ RedBlackTreeValidate ( > DEBUG ((DEBUG_VERBOSE, "%a: Tree=%p BlackHeight=%Ld Count=%Ld\n", > __FUNCTION__, Tree, (INT64)BlackHeight, (INT64)ForwardCount)); > } > + > + > +RETURN_STATUS > +EFIAPI > +BaseOrderedCollectionRedBlackTreeLibConstructor ( > + VOID > + ) > +{ > + return RETURN_SUCCESS; > +} > This patchset has the same goal as the following code in "MdePkg/Library/UefiLib/UefiLib.c", found by Ard: /** Empty constructor function that is required to resolve dependencies between libraries. ** DO NOT REMOVE ** @param ImageHandle The firmware allocated handle for the EFI image. @param SystemTable A pointer to the EFI System Table. @retval EFI_SUCCESS The constructor executed correctly. **/ EFI_STATUS EFIAPI UefiLibConstructor ( IN EFI_HANDLE ImageHandle, IN EFI_SYSTEM_TABLE *SystemTable ) { return EFI_SUCCESS; } Added in SVN r11181. Thanks Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
Laszlo: Tools behavior tries to avoid the cycle between the library instances. From Library INF file, some ones depend on each other, but their constructor may have no dependency. If they are all considered even if they have no constructor, the cycle will easily happen, and cause tools break. So, BaseTools cares the library instance with the constructor. I agree it is not accurate, because INF file has no information to describe the library constructor dependency. For the failure case, we have to add the empty constructor function as its constructor to resolve the real dependency issue. For your case, I prefer to resolve the real dependency issue. Is the change for BaseOrderedCollectionRedBlackTreeLib enough? If yes, I suggest to only make this change now. If you add more dummy constructor functions, it may cause the cycle dependency happen. Thanks Liming -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Saturday, September 06, 2014 7:42 PM To: edk2-devel@lists.sourceforge.net; Gao, Liming; Kinney, Michael D Subject: Re: [edk2] [PATCH 01/12] MdePkg: BaseOrderedCollectionRedBlackTreeLib: add constructor Liming, Michael, On 09/03/14 00:56, Laszlo Ersek wrote: > Calls to constructors of interdependent library instances are > generated in the correct order only if all library instances in > question have constructors. If some have no constructors, then the > rest may see their constructors called out of order. > > Cycle detection also only works when all library instances have > constructors. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > .../BaseOrderedCollectionRedBlackTreeLib.inf | 2 ++ > .../BaseOrderedCollectionRedBlackTreeLib.c | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git > a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedColle > ctionRedBlackTreeLib.inf > b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedColle > ctionRedBlackTreeLib.inf > index a68afc8..f846a79 100644 > --- > a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedColle > ctionRedBlackTreeLib.inf > +++ b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedC > +++ ollectionRedBlackTreeLib.inf > @@ -32,6 +32,8 @@ > VERSION_STRING = 1.0 > LIBRARY_CLASS = OrderedCollectionLib > > + CONSTRUCTOR = BaseOrderedCollectionRedBlackTreeLibConstructor > + > # > # VALID_ARCHITECTURES = IA32 X64 IPF EBC > # > diff --git > a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedColle > ctionRedBlackTreeLib.c > b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedColle > ctionRedBlackTreeLib.c > index 8d18a4b..23ba8de 100644 > --- > a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedColle > ctionRedBlackTreeLib.c > +++ b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedC > +++ ollectionRedBlackTreeLib.c > @@ -1452,3 +1452,13 @@ RedBlackTreeValidate ( > DEBUG ((DEBUG_VERBOSE, "%a: Tree=%p BlackHeight=%Ld Count=%Ld\n", > __FUNCTION__, Tree, (INT64)BlackHeight, (INT64)ForwardCount)); } > + > + > +RETURN_STATUS > +EFIAPI > +BaseOrderedCollectionRedBlackTreeLibConstructor ( > + VOID > + ) > +{ > + return RETURN_SUCCESS; > +} > This patchset has the same goal as the following code in "MdePkg/Library/UefiLib/UefiLib.c", found by Ard: /** Empty constructor function that is required to resolve dependencies between libraries. ** DO NOT REMOVE ** @param ImageHandle The firmware allocated handle for the EFI image. @param SystemTable A pointer to the EFI System Table. @retval EFI_SUCCESS The constructor executed correctly. **/ EFI_STATUS EFIAPI UefiLibConstructor ( IN EFI_HANDLE ImageHandle, IN EFI_SYSTEM_TABLE *SystemTable ) { return EFI_SUCCESS; } Added in SVN r11181. Thanks Laszlo ------------------------------------------------------------------------------ Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
On 09/09/14 11:18, Gao, Liming wrote: > Tools behavior tries to avoid the cycle between the library > instances. From Library INF file, some ones depend on each other, but > their constructor may have no dependency. If they are all considered > even if they have no constructor, the cycle will easily happen, and > cause tools break. So, BaseTools cares the library instance with the > constructor. I agree it is not accurate, because INF file has no > information to describe the library constructor dependency. For the > failure case, we have to add the empty constructor function as its > constructor to resolve the real dependency issue. > > For your case, I prefer to resolve the real dependency issue. Is the > change for BaseOrderedCollectionRedBlackTreeLib enough? If yes, I > suggest to only make this change now. If you add more dummy > constructor functions, it may cause the cycle dependency happen. This patchset does not address any current problems. I wrote it out of pure caution. I think I understand the issue. Constructors are all-or-nothing. That is, we can't express that a library's constructor depends only on a *subset* of the library's complete set of (runtime) dependencies. Hence, if you add a CONSTRUCTOR to the INF, it might create false init-time dependencies, leading to false cycles, breaking the build unnecessarily. The only way out of this is to omit CONSTRUCTOR from one of the library instances that constitutes the cycle (which will then imply that this library will be initialized as the first one). And then it is the programmer's responsibility to ensure that this library instance in fact impose no direct or indirect init-time dependencies. Okay. Our original problem (independent of this series) was that we just copied (and customized) a library instance that had no CONSTRUCTOR. Our customized version *did* impose (indirect) init-time dependencies, but we didn't notice. Obviously, this library would then be initialized first, and due to the indirect init-time dependencies that we gave it unknowingly, it would crash. Once we realized those dependencies, we added the CONSTRUCTOR, and then the cycle was laid bare. Given that the cycle was *valid*, we addressed the cycle itself, by reorganizing the code, and removing some dependencies for real. My point here is that adding empty constructors immediately at the beginning is *safer*. They cause no problems until they suddenly cause problems -- and when they do, you get a cycle report, and you can work to address the cycle. You can analyze if the dependencies are valid or false. You can split up libraries, create customized copies without dependencies / CONSTRUCTORs, and so on. Whereas, if by default you don't add constructors, then the sudden breakage will not present itself as a nice controlled cycle report. It will show up as a crash, at runtime. You say that you'd prefer to resolve the real dependency issue. We have none, currently. But what speaks against the safer approach, ie. adding all the empty constructors now, and resolving the real dependency issue (*or* the false cycle report) when there is a cycle report? This is a tradeoff between false cycle reports and runtime crashes. - With empty constructors added by default, you get valid cycle reports, and you get false cycle reports. - With empty constructors *omitted* by default, you get valid cycle reports, and you get runtime crashes. Am I wrong? Thanks, Laszlo ------------------------------------------------------------------------------ Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
On 9 September 2014 14:38, Laszlo Ersek <lersek@redhat.com> wrote: > On 09/09/14 11:18, Gao, Liming wrote: > >> Tools behavior tries to avoid the cycle between the library >> instances. From Library INF file, some ones depend on each other, but >> their constructor may have no dependency. If they are all considered >> even if they have no constructor, the cycle will easily happen, and >> cause tools break. So, BaseTools cares the library instance with the >> constructor. I agree it is not accurate, because INF file has no >> information to describe the library constructor dependency. For the >> failure case, we have to add the empty constructor function as its >> constructor to resolve the real dependency issue. >> >> For your case, I prefer to resolve the real dependency issue. Is the >> change for BaseOrderedCollectionRedBlackTreeLib enough? If yes, I >> suggest to only make this change now. If you add more dummy >> constructor functions, it may cause the cycle dependency happen. > > This patchset does not address any current problems. I wrote it out of > pure caution. > > I think I understand the issue. Constructors are all-or-nothing. > > That is, we can't express that a library's constructor depends only on a > *subset* of the library's complete set of (runtime) dependencies. Hence, > if you add a CONSTRUCTOR to the INF, it might create false init-time > dependencies, leading to false cycles, breaking the build unnecessarily. > > The only way out of this is to omit CONSTRUCTOR from one of the library > instances that constitutes the cycle (which will then imply that this > library will be initialized as the first one). And then it is the > programmer's responsibility to ensure that this library instance in fact > impose no direct or indirect init-time dependencies. Okay. > > Our original problem (independent of this series) was that we just > copied (and customized) a library instance that had no CONSTRUCTOR. Our > customized version *did* impose (indirect) init-time dependencies, but > we didn't notice. Obviously, this library would then be initialized > first, and due to the indirect init-time dependencies that we gave it > unknowingly, it would crash. > > Once we realized those dependencies, we added the CONSTRUCTOR, and then > the cycle was laid bare. Given that the cycle was *valid*, we addressed > the cycle itself, by reorganizing the code, and removing some > dependencies for real. > > My point here is that adding empty constructors immediately at the > beginning is *safer*. They cause no problems until they suddenly cause > problems -- and when they do, you get a cycle report, and you can work > to address the cycle. You can analyze if the dependencies are valid or > false. You can split up libraries, create customized copies without > dependencies / CONSTRUCTORs, and so on. > > Whereas, if by default you don't add constructors, then the sudden > breakage will not present itself as a nice controlled cycle report. It > will show up as a crash, at runtime. > > You say that you'd prefer to resolve the real dependency issue. We have > none, currently. But what speaks against the safer approach, ie. adding > all the empty constructors now, and resolving the real dependency issue > (*or* the false cycle report) when there is a cycle report? > > This is a tradeoff between false cycle reports and runtime crashes. > - With empty constructors added by default, you get valid cycle > reports, and you get false cycle reports. > - With empty constructors *omitted* by default, you get valid cycle > reports, and you get runtime crashes. > > Am I wrong? > I think you're wrong :-) We did not copy a library and add a constructor. What we did was pick some arrangement of libraries, and notice that an indirect dependency through UefiBootServicesTableLib resulted in the constructor ordering to get screwed up. Not the same thing. (The first time I got the cycle report was when adding the false dependency on UefiBootServicesTableLib to my SerialPortLib, and we worked our way back from there) So there is a very fundamental issue here that remains unaddressed: the core issue is that a library without a constructor may have ordering constraints depending on which library *implementations* are selected to fulfil its dependencies. The only correct way to deal with this is to treat a library as having a constructor if any of its resolved dependencies have one (or it has one itself).
diff --git a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf index a68afc8..f846a79 100644 --- a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf +++ b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf @@ -32,6 +32,8 @@ VERSION_STRING = 1.0 LIBRARY_CLASS = OrderedCollectionLib + CONSTRUCTOR = BaseOrderedCollectionRedBlackTreeLibConstructor + # # VALID_ARCHITECTURES = IA32 X64 IPF EBC # diff --git a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c index 8d18a4b..23ba8de 100644 --- a/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c +++ b/MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.c @@ -1452,3 +1452,13 @@ RedBlackTreeValidate ( DEBUG ((DEBUG_VERBOSE, "%a: Tree=%p BlackHeight=%Ld Count=%Ld\n", __FUNCTION__, Tree, (INT64)BlackHeight, (INT64)ForwardCount)); } + + +RETURN_STATUS +EFIAPI +BaseOrderedCollectionRedBlackTreeLibConstructor ( + VOID + ) +{ + return RETURN_SUCCESS; +}
Calls to constructors of interdependent library instances are generated in the correct order only if all library instances in question have constructors. If some have no constructors, then the rest may see their constructors called out of order. Cycle detection also only works when all library instances have constructors. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- .../BaseOrderedCollectionRedBlackTreeLib.inf | 2 ++ .../BaseOrderedCollectionRedBlackTreeLib.c | 10 ++++++++++ 2 files changed, 12 insertions(+)