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.
+
+#include "TestCheckSum.cpp"
+
+int
+main (
+  int   argc,
+  char  *argv[]
+  )
+{
+  testing::InitGoogleTest (&argc, argv);
+  return RUN_ALL_TESTS ();
+}
diff --git a/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp b/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
new file mode 100644
index 000000000000..fa97f11dfa9c
--- /dev/null
+++ b/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
@@ -0,0 +1,64 @@
+/** @file
+  Unit tests for BaseLib's checksum capabilities.
+
+  Copyright (c) 2023 Pedro Falcato. All rights reserved<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <gtest/gtest.h>
+extern "C" {
+#include <Base.h>
+#include <Library/BaseLib.h>
+}
+
+// Precomputed crc32c and crc16-ansi for "hello" (without the null byte)
+constexpr STATIC UINT32  mHelloCrc32c = 0x9A71BB4C;
+constexpr STATIC UINT16  mHelloCrc16  = 0x34F6;
+
+TEST (Crc32c, BasicCheck) {
+  // Note: The magic numbers below are precomputed checksums
+  // Check for basic operation on even and odd numbers of bytes
+  EXPECT_EQ (CalculateCrc32c ("hello", 5, 0), mHelloCrc32c);
+  EXPECT_EQ (
+    CalculateCrc32c ("longlonglonglonglong", sizeof ("longlonglonglonglong") - 1, 0),
+    0xC50F869D
+    );
+  EXPECT_EQ (CalculateCrc32c ("h", 1, 0), 0xB96298FC);
+
+  // Check if a checksum with no bytes correctly yields 0
+  EXPECT_EQ (CalculateCrc32c ("", 0, 0), 0U);
+}
+
+TEST (Crc32c, MultipartCheck) {
+  // Test multi-part crc32c calculation. So that given a string of bytes
+  // s[N], crc32c(s, N, 0) == crc32c(s[N - 1], 1, crc32c(s, N - 1, 0))
+  // and all other sorts of combinations one might imagine.
+  UINT32  val;
+
+  val = CalculateCrc32c ("hel", 3, 0);
+  EXPECT_EQ (CalculateCrc32c (&"hello"[3], 2, val), mHelloCrc32c);
+}
+
+TEST (Crc16, BasicCheck) {
+  // Note: The magic numbers below are precomputed checksums
+  // Check for basic operation on even and odd numbers of bytes
+  EXPECT_EQ (CalculateCrc16Ansi ("hello", 5, CRC16ANSI_INIT), mHelloCrc16);
+  EXPECT_EQ (
+    CalculateCrc16Ansi ("longlonglonglonglong", sizeof ("longlonglonglonglong") - 1, CRC16ANSI_INIT),
+    0xF723
+    );
+  EXPECT_EQ (CalculateCrc16Ansi ("h", 1, CRC16ANSI_INIT), 0xAEBE);
+
+  // Check if a checksum with no bytes correctly yields CRC16ANSI_INIT
+  EXPECT_EQ (CalculateCrc16Ansi ("", 0, CRC16ANSI_INIT), CRC16ANSI_INIT);
+}
+
+TEST (Crc16, MultipartCheck) {
+  // Test multi-part crc16 calculation. So that given a string of bytes
+  // s[N], crc16(s, N, 0) == crc16(s[N - 1], 1, crc16(s, N - 1, 0))
+  // and all other sorts of combinations one might imagine.
+  UINT16  val;
+
+  val = CalculateCrc16Ansi ("hel", 3, CRC16ANSI_INIT);
+  EXPECT_EQ (CalculateCrc16Ansi (&"hello"[3], 2, val), mHelloCrc16);
+}
diff --git a/MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTests64.cpp b/MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTests64.cpp
index 6efdd3be7c5e..9fc60c5c71f3 100644
--- a/MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTests64.cpp
+++ b/MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTests64.cpp
@@ -10,8 +10,8 @@
 
 #include <gtest/gtest.h>
 extern "C" {
-  #include <Base.h>
-  #include <Library/SafeIntLib.h>
+#include <Base.h>
+#include <Library/SafeIntLib.h>
 }
 
 TEST (ConversionTestSuite, TestSafeInt32ToUintn) {
diff --git a/MdePkg/Test/MdePkgHostTest.dsc b/MdePkg/Test/MdePkgHostTest.dsc
index b92b564d434a..583f8fc0ddd8 100644
--- a/MdePkg/Test/MdePkgHostTest.dsc
+++ b/MdePkg/Test/MdePkgHostTest.dsc
@@ -20,6 +20,7 @@
 !include UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc
 
 [LibraryClasses]
+  BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
   SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
   DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibBase.inf
 
@@ -31,6 +32,10 @@
   MdePkg/Test/UnitTest/Library/BaseLib/BaseLibUnitTestsHost.inf
   MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/GoogleTestBaseSafeIntLib.inf
   MdePkg/Test/UnitTest/Library/DevicePathLib/TestDevicePathLibHost.inf
+  #
+  # BaseLib tests
+  #
+  MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
 
   #
   # Build HOST_APPLICATION Libraries
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111885): https://edk2.groups.io/g/devel/message/111885
Mute This Topic: https://groups.io/mt/102886794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

> +
> +#include "TestCheckSum.cpp"
> +
> +int
> +main (
> +  int   argc,
> +  char  *argv[]
> +  )
> +{
> +  testing::InitGoogleTest (&argc, argv);
> +  return RUN_ALL_TESTS ();
> +}
> diff --git a/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
> b/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
> new file mode 100644
> index 000000000000..fa97f11dfa9c
> --- /dev/null
> +++ b/MdePkg/Test/GoogleTest/Library/BaseLib/TestCheckSum.cpp
> @@ -0,0 +1,64 @@
> +/** @file
> +  Unit tests for BaseLib's checksum capabilities.
> +
> +  Copyright (c) 2023 Pedro Falcato. All rights reserved<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include <gtest/gtest.h>
> +extern "C" {
> +#include <Base.h>
> +#include <Library/BaseLib.h>
> +}
> +
> +// Precomputed crc32c and crc16-ansi for "hello" (without the null byte)
> +constexpr STATIC UINT32  mHelloCrc32c = 0x9A71BB4C;
> +constexpr STATIC UINT16  mHelloCrc16  = 0x34F6;
> +
> +TEST (Crc32c, BasicCheck) {
> +  // Note: The magic numbers below are precomputed checksums
> +  // Check for basic operation on even and odd numbers of bytes
> +  EXPECT_EQ (CalculateCrc32c ("hello", 5, 0), mHelloCrc32c);
> +  EXPECT_EQ (
> +    CalculateCrc32c ("longlonglonglonglong", sizeof
> ("longlonglonglonglong") - 1, 0),
> +    0xC50F869D
> +    );
> +  EXPECT_EQ (CalculateCrc32c ("h", 1, 0), 0xB96298FC);
> +
> +  // Check if a checksum with no bytes correctly yields 0
> +  EXPECT_EQ (CalculateCrc32c ("", 0, 0), 0U);
> +}
> +
> +TEST (Crc32c, MultipartCheck) {
> +  // Test multi-part crc32c calculation. So that given a string of bytes
> +  // s[N], crc32c(s, N, 0) == crc32c(s[N - 1], 1, crc32c(s, N - 1, 0))
> +  // and all other sorts of combinations one might imagine.
> +  UINT32  val;
> +
> +  val = CalculateCrc32c ("hel", 3, 0);
> +  EXPECT_EQ (CalculateCrc32c (&"hello"[3], 2, val), mHelloCrc32c);
> +}
> +
> +TEST (Crc16, BasicCheck) {
> +  // Note: The magic numbers below are precomputed checksums
> +  // Check for basic operation on even and odd numbers of bytes
> +  EXPECT_EQ (CalculateCrc16Ansi ("hello", 5, CRC16ANSI_INIT),
> mHelloCrc16);
> +  EXPECT_EQ (
> +    CalculateCrc16Ansi ("longlonglonglonglong", sizeof
> ("longlonglonglonglong") - 1, CRC16ANSI_INIT),
> +    0xF723
> +    );
> +  EXPECT_EQ (CalculateCrc16Ansi ("h", 1, CRC16ANSI_INIT), 0xAEBE);
> +
> +  // Check if a checksum with no bytes correctly yields CRC16ANSI_INIT
> +  EXPECT_EQ (CalculateCrc16Ansi ("", 0, CRC16ANSI_INIT), CRC16ANSI_INIT);
> +}
> +
> +TEST (Crc16, MultipartCheck) {
> +  // Test multi-part crc16 calculation. So that given a string of bytes
> +  // s[N], crc16(s, N, 0) == crc16(s[N - 1], 1, crc16(s, N - 1, 0))
> +  // and all other sorts of combinations one might imagine.
> +  UINT16  val;
> +
> +  val = CalculateCrc16Ansi ("hel", 3, CRC16ANSI_INIT);
> +  EXPECT_EQ (CalculateCrc16Ansi (&"hello"[3], 2, val), mHelloCrc16);
> +}
> diff --git
> a/MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTes
> ts64.cpp
> b/MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTes
> ts64.cpp
> index 6efdd3be7c5e..9fc60c5c71f3 100644
> ---
> a/MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTes
> ts64.cpp
> +++
> b/MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/SafeIntLibUintnIntnUnitTes
> ts64.cpp
> @@ -10,8 +10,8 @@
> 
>  #include <gtest/gtest.h>
>  extern "C" {
> -  #include <Base.h>
> -  #include <Library/SafeIntLib.h>
> +#include <Base.h>
> +#include <Library/SafeIntLib.h>
>  }
> 
>  TEST (ConversionTestSuite, TestSafeInt32ToUintn) {
> diff --git a/MdePkg/Test/MdePkgHostTest.dsc
> b/MdePkg/Test/MdePkgHostTest.dsc
> index b92b564d434a..583f8fc0ddd8 100644
> --- a/MdePkg/Test/MdePkgHostTest.dsc
> +++ b/MdePkg/Test/MdePkgHostTest.dsc
> @@ -20,6 +20,7 @@
>  !include UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc
> 
>  [LibraryClasses]
> +  BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
>    SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
> 
> DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibBase.inf
> 
> @@ -31,6 +32,10 @@
>    MdePkg/Test/UnitTest/Library/BaseLib/BaseLibUnitTestsHost.inf
> 
> MdePkg/Test/GoogleTest/Library/BaseSafeIntLib/GoogleTestBaseSafeIntLib.inf
>    MdePkg/Test/UnitTest/Library/DevicePathLib/TestDevicePathLibHost.inf
> +  #
> +  # BaseLib tests
> +  #
> +  MdePkg/Test/GoogleTest/Library/BaseLib/GoogleTestBaseLib.inf
> 
>    #
>    # Build HOST_APPLICATION Libraries
> --
> 2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111911): https://edk2.groups.io/g/devel/message/111911
Mute This Topic: https://groups.io/mt/102886794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Nov 30, 2023 at 9:31 PM Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> 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

Thanks for the guidance!
I got it to work by changing DLINK and DLINK2. Sent out a patch set
addressing that. When that goes through (hopefully), we'll be able to
respin these tests without the #include hack.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111918): https://edk2.groups.io/g/devel/message/111918
Mute This Topic: https://groups.io/mt/102886794/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-