From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 982EF7803CD for ; Thu, 30 Nov 2023 20:16:01 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=spPAMcrp2RMRpiDbRpZrczRbn/Fhs1petHmrr+W3+EU=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1701375360; v=1; b=IztzDakgJkM7VT0/xFEtXnEPTHIB+3IDAAiLMb6f8QhuYhhZM9otuawZWVbHiNxr7JasG3lF MbGCKMQVqY5otfRTM1mwKPh/D9ObuelCioicXgThDAe8R1/7lpkiWOt/Wui89Cjvp7BcHzUfWrQ CVbUt6FSvYBVFwBg50Uyam9E= X-Received: by 127.0.0.2 with SMTP id 7c28YY7687511xvz9TppAv1J; Thu, 30 Nov 2023 12:16:00 -0800 X-Received: from mail-ua1-f49.google.com (mail-ua1-f49.google.com [209.85.222.49]) by mx.groups.io with SMTP id smtpd.web11.4061.1701375359528386970 for ; Thu, 30 Nov 2023 12:15:59 -0800 X-Received: by mail-ua1-f49.google.com with SMTP id a1e0cc1a2514c-7c537501594so420505241.1 for ; Thu, 30 Nov 2023 12:15:59 -0800 (PST) X-Gm-Message-State: H4qUyyTy8bCq6mArGbZtDhpcx7686176AA= X-Google-Smtp-Source: AGHT+IHZ0EBcBn3OGLWqzBDT3mNkeUDpxEDnILw+OmTXHFUjAkxNlJGmKYjgLnLrrXPoOsxt73xKLdkg2IBztN59k5c= X-Received: by 2002:a05:6102:cd4:b0:462:ad06:c90b with SMTP id g20-20020a0561020cd400b00462ad06c90bmr27757786vst.18.1701375358071; Thu, 30 Nov 2023 12:15:58 -0800 (PST) MIME-Version: 1.0 References: <20231130024611.67135-1-pedro.falcato@gmail.com> <20231130024611.67135-3-pedro.falcato@gmail.com> In-Reply-To: From: "Pedro Falcato" Date: Thu, 30 Nov 2023 20:15:46 +0000 Message-ID: Subject: Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: Savva Mitrofanov , "Gao, Liming" , "Liu, Zhiguang" Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,pedro.falcato@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=IztzDakg; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Thu, Nov 30, 2023 at 7:32=E2=80=AFPM Michael D Kinney wrote: > > With one comment below addressed > > Reviewed-by: Michael D Kinney > > > -----Original Message----- > > From: Pedro Falcato > > Sent: Wednesday, November 29, 2023 6:46 PM > > To: devel@edk2.groups.io > > Cc: Savva Mitrofanov ; Pedro Falcato > > ; Gao, Liming ; Kinn= ey, > > Michael D ; Liu, Zhiguang > > > > 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 chec= k > > for correctness on various inputs using precomputed hashes. > > > > Signed-off-by: Pedro Falcato > > Cc: Liming Gao > > Cc: Michael D Kinney > > Cc: Zhiguang Liu > > --- > > .../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.i= nf > > 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 =3D 0x00010005 > > + BASE_NAME =3D GoogleTestBaseLib > > + FILE_GUID =3D 34D8CBBA-2442-455F-8454-5B06B12A8B62 > > + MODULE_TYPE =3D HOST_APPLICATION > > + VERSION_STRING =3D 1.0 > > + > > +# > > +# The following information is for reference only and not required by = the > > build tools. > > +# > > +# VALID_ARCHITECTURES =3D 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
> > + SPDX-License-Identifier: BSD-2-Clause-Patent > > +**/ > > + > > +#include > > + > > +// 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 --=20 Pedro -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-