[edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation

[edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation

[Pedro Falcato]

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4609

CalculateCrc16Ansi is currently miscalculating all checksums and causing
ext4 mount failures on older (~13 year old) filesystems.

This patchset:
1) Fixes CalculateCrc16Ansi to properly calculate checksums
   This is a breaking change.
2) Adds google test tests for BaseLib. They were immensely helpful in
  making sure things were correct, while iterating quickly.

Boot tested on a freshly baked "old filesystem" using a script[1],
and tested for further correctness using unit tests.

[1] https://gist.github.com/heatd/6adaae8288e270897975d9321c5e8f41

Pedro Falcato (2):
  MdePkg/BaseLib: Fix CRC16-ANSI calculation
  MdePkg/Test: Add google tests for BaseLib

 MdePkg/Include/Library/BaseLib.h              |  5 ++
 MdePkg/Library/BaseLib/CheckSum.c             |  4 +-
 .../Library/BaseLib/GoogleTestBaseLib.inf     | 31 +++++++++
 .../Library/BaseLib/TestBaseLibMain.cpp       | 23 +++++++
 .../Library/BaseLib/TestCheckSum.cpp          | 64 +++++++++++++++++++
 .../SafeIntLibUintnIntnUnitTests64.cpp        |  4 +-
 MdePkg/Test/MdePkgHostTest.dsc                |  5 ++
 7 files changed, 132 insertions(+), 4 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

-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111883): https://edk2.groups.io/g/devel/message/111883
Mute This Topic: https://groups.io/mt/102886792/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] MdePkg/BaseLib: Fix CRC16-ANSI calculation

[Pedro Falcato]

The current CalculateCrc16Ansi implementation does the following:
1) Invert the passed checksum
2) Calculate the new checksum by going through data and using the
   lookup table
3) Invert it back again

This emulated my design for CalculateCrc32c, where 0 is
passed as the initial checksum, and it inverts in the end.
However, CRC16 does not invert the checksum on input and output.
So this is incorrect.

Fix the problem by not inverting input checksums nor output checksums.
Callers should now pass CRC16ANSI_INIT as the initial value instead of
"0". This is a breaking change.

This problem was found out-of-list when older ext4 filesystems
(that use crc16 checksums) failed to mount with "corruption".

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4609
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>
---
 MdePkg/Include/Library/BaseLib.h  | 5 +++++
 MdePkg/Library/BaseLib/CheckSum.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 5d7067ee854e..728e89d2bf44 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4599,6 +4599,11 @@ CalculateCrc16Ansi (
   IN  UINT16      InitialValue
   );
 
+//
+// Initial value for the CRC16-ANSI algorithm, when no prior checksum has been calculated.
+//
+#define CRC16ANSI_INIT  0xffff
+
 /**
    Calculates the CRC32c checksum of the given buffer.
 
diff --git a/MdePkg/Library/BaseLib/CheckSum.c b/MdePkg/Library/BaseLib/CheckSum.c
index b6a076573191..57d324c82b22 100644
--- a/MdePkg/Library/BaseLib/CheckSum.c
+++ b/MdePkg/Library/BaseLib/CheckSum.c
@@ -678,13 +678,13 @@ CalculateCrc16Ansi (
 
   Buf = Buffer;
 
-  Crc = ~InitialValue;
+  Crc = InitialValue;
 
   while (Length-- != 0) {
     Crc = mCrc16LookupTable[(Crc & 0xFF) ^ *(Buf++)] ^ (Crc >> 8);
   }
 
-  return ~Crc;
+  return Crc;
 }
 
 GLOBAL_REMOVE_IF_UNREFERENCED STATIC CONST UINT32  mCrc32cLookupTable[256] = {
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111884): https://edk2.groups.io/g/devel/message/111884
Mute This Topic: https://groups.io/mt/102886793/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation

[Pedro Falcato]

On Thu, Nov 30, 2023 at 2:46 AM Pedro Falcato via groups.io
<pedro.falcato=gmail.com@groups.io> wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4609
>
> CalculateCrc16Ansi is currently miscalculating all checksums and causing
> ext4 mount failures on older (~13 year old) filesystems.
>
> This patchset:
> 1) Fixes CalculateCrc16Ansi to properly calculate checksums
>    This is a breaking change.
> 2) Adds google test tests for BaseLib. They were immensely helpful in
>   making sure things were correct, while iterating quickly.
>
> Boot tested on a freshly baked "old filesystem" using a script[1],
> and tested for further correctness using unit tests.
>
> [1] https://gist.github.com/heatd/6adaae8288e270897975d9321c5e8f41
>
> Pedro Falcato (2):
>   MdePkg/BaseLib: Fix CRC16-ANSI calculation
>   MdePkg/Test: Add google tests for BaseLib
>
>  MdePkg/Include/Library/BaseLib.h              |  5 ++
>  MdePkg/Library/BaseLib/CheckSum.c             |  4 +-
>  .../Library/BaseLib/GoogleTestBaseLib.inf     | 31 +++++++++
>  .../Library/BaseLib/TestBaseLibMain.cpp       | 23 +++++++
>  .../Library/BaseLib/TestCheckSum.cpp          | 64 +++++++++++++++++++
>  .../SafeIntLibUintnIntnUnitTests64.cpp        |  4 +-
>  MdePkg/Test/MdePkgHostTest.dsc                |  5 ++
>  7 files changed, 132 insertions(+), 4 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
>
> --
> 2.43.0

whoops, dropped some CC's here, adding...

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111886): https://edk2.groups.io/g/devel/message/111886
Mute This Topic: https://groups.io/mt/102886792/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael D Kinney]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] MdePkg/BaseLib: Fix CRC16-ANSI calculation

[Michael D Kinney]

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 1/2] MdePkg/BaseLib: Fix CRC16-ANSI calculation
> 
> The current CalculateCrc16Ansi implementation does the following:
> 1) Invert the passed checksum
> 2) Calculate the new checksum by going through data and using the
>    lookup table
> 3) Invert it back again
> 
> This emulated my design for CalculateCrc32c, where 0 is
> passed as the initial checksum, and it inverts in the end.
> However, CRC16 does not invert the checksum on input and output.
> So this is incorrect.
> 
> Fix the problem by not inverting input checksums nor output checksums.
> Callers should now pass CRC16ANSI_INIT as the initial value instead of
> "0". This is a breaking change.
> 
> This problem was found out-of-list when older ext4 filesystems
> (that use crc16 checksums) failed to mount with "corruption".
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4609
> 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>
> ---
>  MdePkg/Include/Library/BaseLib.h  | 5 +++++
>  MdePkg/Library/BaseLib/CheckSum.c | 4 ++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/BaseLib.h
> b/MdePkg/Include/Library/BaseLib.h
> index 5d7067ee854e..728e89d2bf44 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4599,6 +4599,11 @@ CalculateCrc16Ansi (
>    IN  UINT16      InitialValue
>    );
> 
> +//
> +// Initial value for the CRC16-ANSI algorithm, when no prior checksum has
> been calculated.
> +//
> +#define CRC16ANSI_INIT  0xffff
> +
>  /**
>     Calculates the CRC32c checksum of the given buffer.
> 
> diff --git a/MdePkg/Library/BaseLib/CheckSum.c
> b/MdePkg/Library/BaseLib/CheckSum.c
> index b6a076573191..57d324c82b22 100644
> --- a/MdePkg/Library/BaseLib/CheckSum.c
> +++ b/MdePkg/Library/BaseLib/CheckSum.c
> @@ -678,13 +678,13 @@ CalculateCrc16Ansi (
> 
>    Buf = Buffer;
> 
> -  Crc = ~InitialValue;
> +  Crc = InitialValue;
> 
>    while (Length-- != 0) {
>      Crc = mCrc16LookupTable[(Crc & 0xFF) ^ *(Buf++)] ^ (Crc >> 8);
>    }
> 
> -  return ~Crc;
> +  return Crc;
>  }
> 
>  GLOBAL_REMOVE_IF_UNREFERENCED STATIC CONST UINT32  mCrc32cLookupTable[256]
> = {
> --
> 2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111912): https://edk2.groups.io/g/devel/message/111912
Mute This Topic: https://groups.io/mt/102886793/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael D Kinney]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-