Hi Pedro, I agree that silent failures are terrible.
The issue is documented with the requirement to use #include here: https://github.com/tianocore/edk2/tree/master/UnitTestFrameworkPkg#googletest-configuration Unit tests are built with their own DSC configuration extensions. Does adding --whole-archive to the GCC SLINK settings of the following file resolve the issue? https://github.com/tianocore/edk2/blob/master/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc Mike > -----Original Message----- > From: Pedro Falcato <pedro.falc...@gmail.com> > Sent: Thursday, November 30, 2023 12:16 PM > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com> > Cc: Savva Mitrofanov <savva...@gmail.com>; Gao, Liming > <gaolim...@byosoft.com.cn>; Liu, Zhiguang <zhiguang....@intel.com> > Subject: Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for > BaseLib > > On Thu, Nov 30, 2023 at 7:32 PM Michael D Kinney > <michael.d.kin...@intel.com> wrote: > > > > With one comment below addressed > > > > Reviewed-by: Michael D Kinney <michael.d.kin...@intel.com> > > > > > -----Original Message----- > > > From: Pedro Falcato <pedro.falc...@gmail.com> > > > Sent: Wednesday, November 29, 2023 6:46 PM > > > To: devel@edk2.groups.io > > > Cc: Savva Mitrofanov <savva...@gmail.com>; Pedro Falcato > > > <pedro.falc...@gmail.com>; Gao, Liming <gaolim...@byosoft.com.cn>; > Kinney, > > > Michael D <michael.d.kin...@intel.com>; Liu, Zhiguang > > > <zhiguang....@intel.com> > > > Subject: [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib > > > > > > Add GoogleTestBaseLib, which contains gtest unit tests for BaseLib. > > > For now, only add checksum tests for CRC32C and CRC16; these tests > check > > > for correctness on various inputs using precomputed hashes. > > > > > > Signed-off-by: Pedro Falcato <pedro.falc...@gmail.com> > > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > > Cc: Zhiguang Liu <zhiguang....@intel.com> > > > --- > > > .../Library/BaseLib/GoogleTestBaseLib.inf | 31 +++++++++ > > > .../Library/BaseLib/TestBaseLibMain.cpp | 23 +++++++ > > > .../Library/BaseLib/TestCheckSum.cpp | 64 +++++++++++++++++++ > > > .../SafeIntLibUintnIntnUnitTests64.cpp | 4 +- > > > MdePkg/Test/MdePkgHostTest.dsc | 5 ++ > > > 5 files changed, 125 insertions(+), 2 deletions(-) > > > create mode 100644 > > > MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf > > > create mode 100644 > > > MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp > > > create mode 100644 > > > MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp > > > > > > diff --git > a/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf > > > b/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf > > > new file mode 100644 > > > index 000000000000..c859e5f86b9e > > > --- /dev/null > > > +++ b/MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf > > > @@ -0,0 +1,31 @@ > > > +## @file > > > +# Host OS based Application that unit tests BaseLib using Google Test > > > +# > > > +# Copyright (c) 2023, Pedro Falcato. All rights reserved. > > > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > > +## > > > + > > > +[Defines] > > > + INF_VERSION = 0x00010005 > > > + BASE_NAME = GoogleTestBaseLib > > > + FILE_GUID = 34D8CBBA-2442-455F-8454-5B06B12A8B62 > > > + MODULE_TYPE = HOST_APPLICATION > > > + VERSION_STRING = 1.0 > > > + > > > +# > > > +# The following information is for reference only and not required by > the > > > build tools. > > > +# > > > +# VALID_ARCHITECTURES = IA32 X64 > > > +# > > > + > > > +[Sources] > > > + TestCheckSum.cpp > > > + TestBaseLibMain.cpp > > > + > > > +[Packages] > > > + MdePkg/MdePkg.dec > > > + UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec > > > + > > > +[LibraryClasses] > > > + GoogleTestLib > > > + BaseLib > > > diff --git a/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp > > > b/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp > > > new file mode 100644 > > > index 000000000000..1a9941492be6 > > > --- /dev/null > > > +++ b/MdePkg/Test/GoogleTest/Library/BaseLib/TestBaseLibMain.cpp > > > @@ -0,0 +1,23 @@ > > > +/** @file > > > + Main routine for BaseLib google tests. > > > + > > > + Copyright (c) 2023 Pedro Falcato. All rights reserved<BR> > > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > > +**/ > > > + > > > +#include <gtest/gtest.h> > > > + > > > +// Note: Until we can --whole-archive libs, we're forced to include > > > secondary files from the main one. > > > +// Yuck. > > > > Though I agree with this comment, it does not need to be in the source > > code. > > > > Not sure I understand how --whole-archive can help, so please start > > a new discussion or enter a BZ with the details. > > That's alright, I can give you a quick rundown (a whole separate > discussion is a bit too hardcore): > > The EDK2 build system builds all these modules into .lib/static libraries. > When linkers (I can only speak for UNIX linkers, but AFAIK Windows has > a similar behavior) use static libraries they use them to resolve > outstanding[1] undefined symbols. They do this by opening an ar > file[2], which is essentially a collection of .o in a single file, > looking at the archive's symbol table and only processing object files > they actually need. > > This actually works fine for most usage of static libraries, where you > manually pull symbols you need, or in EFI where the linker pulls in > EfiMain(), which pulls the rest of the object files in. The problem is > that Google Test, in all of its C++ glory, has the test registration > done in global constructors for global objects (that are created in > the TEST(Blah, TestSomething) macro invocation). These constructors > are not referenced elsewhere, so the linker doesn't pull those object > files; this is bad, it means none of those objects will ever be seen > by the linker, so you just drop tests. > > This whole problem can be fixed by doing /WHOLEARCHIVE or > --Wl,--whole-archive on the static libraries; in that case, the linker > forcibly includes all of the static library's object files (even > unreferenced ones!) in the link. This fixes our case as then > constructors are referenced, so the tests are properly registered and > run. This is a "hardcore" switch that we don't want to force on most > components, so it should ideally be worked around with some > to-be-named .DSC property + conditional whole-archive based on that > property. > > If you can agree that this is the way forward, I'm more than happy to > drop all of this background into a BZ and link that from the comment. > But I'd really like to make some sort of reference to this problem in > source, because I spent more time trying to figure out why all my > tests were disappearing than actually writing tests[3]. And do note > that the original gtest example you committed > (MdePkg/Test/GoogleTest/Library/BaseSafeIntLib) also has this problem, > SafeIntLibUintnIntnUnitTests{32, 64} are *not* included in the final > executable. This problem is completely silent, and incredibly annoying > to walk through at first. > > > [1] side-sidenote: LLVM's LLD does not require the "outstanding" part > (ld libc.a a.o just works), but the rest still applies perfectly > [2] traditionally .a on UNIX, .lib on windows and EDK2. but the format > is (AFAIK) similar, and the idea certainly applies > [3] There are actually two paragraphs on this in UnitTestFrameworkPkg, > I just didn't see those at first, but someone in UEFI talkbox was able > to point that readme out > > -- > Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111914): https://edk2.groups.io/g/devel/message/111914 Mute This Topic: https://groups.io/mt/102886794/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
