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

["Pedro Falcato" <pedro.falcato@gmail.com>]

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 (#111913): https://edk2.groups.io/g/devel/message/111913
Mute This Topic: https://groups.io/mt/102886794/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-