* [edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation @ 2023-11-30 2:46 Pedro Falcato 2023-11-30 2:46 ` [edk2-devel] [PATCH 1/2] MdePkg/BaseLib: " Pedro Falcato 2023-11-30 2:46 ` [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib Pedro Falcato 0 siblings, 2 replies; 8+ messages in thread From: Pedro Falcato @ 2023-11-30 2:46 UTC (permalink / raw) To: devel; +Cc: Savva Mitrofanov, 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* [edk2-devel] [PATCH 1/2] MdePkg/BaseLib: Fix CRC16-ANSI calculation 2023-11-30 2:46 [edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation Pedro Falcato @ 2023-11-30 2:46 ` Pedro Falcato 2023-11-30 19:32 ` Michael D Kinney 2023-11-30 2:46 ` [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib Pedro Falcato 1 sibling, 1 reply; 8+ messages in thread From: Pedro Falcato @ 2023-11-30 2:46 UTC (permalink / raw) To: devel Cc: Savva Mitrofanov, Pedro Falcato, Liming Gao, Michael D Kinney, Zhiguang Liu 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] MdePkg/BaseLib: Fix CRC16-ANSI calculation 2023-11-30 2:46 ` [edk2-devel] [PATCH 1/2] MdePkg/BaseLib: " Pedro Falcato @ 2023-11-30 19:32 ` Michael D Kinney 0 siblings, 0 replies; 8+ messages in thread From: Michael D Kinney @ 2023-11-30 19:32 UTC (permalink / raw) To: Pedro Falcato, devel@edk2.groups.io Cc: Savva Mitrofanov, Gao, Liming, Liu, Zhiguang, Kinney, Michael D 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib 2023-11-30 2:46 [edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation Pedro Falcato 2023-11-30 2:46 ` [edk2-devel] [PATCH 1/2] MdePkg/BaseLib: " Pedro Falcato @ 2023-11-30 2:46 ` Pedro Falcato 2023-11-30 19:32 ` Michael D Kinney 1 sibling, 1 reply; 8+ messages in thread From: Pedro Falcato @ 2023-11-30 2:46 UTC (permalink / raw) To: devel Cc: Savva Mitrofanov, Pedro Falcato, Liming Gao, Michael D Kinney, Zhiguang Liu 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib 2023-11-30 2:46 ` [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib Pedro Falcato @ 2023-11-30 19:32 ` Michael D Kinney 2023-11-30 20:15 ` Pedro Falcato 0 siblings, 1 reply; 8+ messages in thread From: Michael D Kinney @ 2023-11-30 19:32 UTC (permalink / raw) To: Pedro Falcato, devel@edk2.groups.io Cc: Savva Mitrofanov, Gao, Liming, Liu, Zhiguang, Kinney, Michael D 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib 2023-11-30 19:32 ` Michael D Kinney @ 2023-11-30 20:15 ` Pedro Falcato 2023-11-30 21:31 ` Michael D Kinney 0 siblings, 1 reply; 8+ messages in thread From: Pedro Falcato @ 2023-11-30 20:15 UTC (permalink / raw) To: devel, michael.d.kinney; +Cc: Savva Mitrofanov, Gao, Liming, Liu, Zhiguang 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib 2023-11-30 20:15 ` Pedro Falcato @ 2023-11-30 21:31 ` Michael D Kinney 2023-11-30 22:44 ` Pedro Falcato 0 siblings, 1 reply; 8+ messages in thread From: Michael D Kinney @ 2023-11-30 21:31 UTC (permalink / raw) To: Pedro Falcato, devel@edk2.groups.io Cc: Savva Mitrofanov, Gao, Liming, Liu, Zhiguang, Kinney, Michael D 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib 2023-11-30 21:31 ` Michael D Kinney @ 2023-11-30 22:44 ` Pedro Falcato 0 siblings, 0 replies; 8+ messages in thread From: Pedro Falcato @ 2023-11-30 22:44 UTC (permalink / raw) To: Kinney, Michael D Cc: devel@edk2.groups.io, Savva Mitrofanov, Gao, Liming, Liu, Zhiguang 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] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-11-30 22:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-30 2:46 [edk2-devel] [PATCH 0/2] MdePkg: Fix CRC16-ANSI calculation Pedro Falcato 2023-11-30 2:46 ` [edk2-devel] [PATCH 1/2] MdePkg/BaseLib: " Pedro Falcato 2023-11-30 19:32 ` Michael D Kinney 2023-11-30 2:46 ` [edk2-devel] [PATCH 2/2] MdePkg/Test: Add google tests for BaseLib Pedro Falcato 2023-11-30 19:32 ` Michael D Kinney 2023-11-30 20:15 ` Pedro Falcato 2023-11-30 21:31 ` Michael D Kinney 2023-11-30 22:44 ` Pedro Falcato
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox