Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib

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

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.falcato@gmail.com>
> Sent: Thursday, November 30, 2023 12:16 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Savva Mitrofanov <savvamtr@gmail.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@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.kinney@intel.com> wrote:
> >
> > With one comment below addressed
> >
> > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > > -----Original Message-----
> > > From: Pedro Falcato <pedro.falcato@gmail.com>
> > > Sent: Wednesday, November 29, 2023 6:46 PM
> > > To: devel@edk2.groups.io
> > > Cc: Savva Mitrofanov <savvamtr@gmail.com>; Pedro Falcato
> > > <pedro.falcato@gmail.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Kinney,
> > > Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang
> > > <zhiguang.liu@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.falcato@gmail.com>
> > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Zhiguang Liu <zhiguang.liu@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-