Hi Leif, Thanks for the review.
The work on this started in 2022, so the copyright dates should be correct. Rest of comments addressed below. Mike > -----Original Message----- > From: Leif Lindholm <quic_llind...@quicinc.com> > Sent: Wednesday, April 5, 2023 11:17 AM > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com> > Cc: Johnson, Chris N <chris.n.john...@intel.com>; Andrew Fish > <af...@apple.com>; Michael Kubacki > <mikub...@linux.microsoft.com>; Sean Brogan <sean.bro...@microsoft.com> > Subject: Re: [edk2-devel] [Patch v2 01/12] UnitTestFrameworkPkg: Add subhook > submodule required for gmock > > On Tue, Apr 04, 2023 at 11:22:09 -0700, Michael D Kinney wrote: > > From: Chris Johnson <chris.n.john...@intel.com> > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4389 > > > > Add subhook submodule that is required to hook internal functions > > when using gmock. > > > > https://github.com/Zeex/subhook > > > > Add SubhookLib library class and SubhookLib library instance. > > Include the SUBHOOK_STATIC define in the SubhookLib INF file so > > it builds as a static library. Also include the SUBHOOK_STATIC > > define in SubhookLib.h so all modules using SubhookLib properly > > link SubhookLib as a static library. > > > > Cc: Andrew Fish <af...@apple.com> > > Cc: Leif Lindholm <quic_llind...@quicinc.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Michael Kubacki <mikub...@linux.microsoft.com> > > Cc: Sean Brogan <sean.bro...@microsoft.com> > > Signed-off-by: Chris Johnson <chris.n.john...@intel.com> > > I have some nitpicks below, which you can take into account or > not. Regardless: > > Reviewed-by: Leif Lindholm <quic_llind...@quicinc.com> > > > --- > > .gitmodules | 3 ++ > > ReadMe.rst | 1 + > > .../Include/Library/SubhookLib.h | 15 ++++++++ > > .../Library/SubhookLib/SubhookLib.inf | 36 +++++++++++++++++++ > > .../Library/SubhookLib/SubhookLib.uni | 11 ++++++ > > .../Library/SubhookLib/subhook | 1 + > > .../Test/UnitTestFrameworkPkgHostTest.dsc | 1 + > > UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec | 2 ++ > > .../UnitTestFrameworkPkgHost.dsc.inc | 1 + > > 9 files changed, 71 insertions(+) > > create mode 100644 UnitTestFrameworkPkg/Include/Library/SubhookLib.h > > create mode 100644 UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.inf > > create mode 100644 UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.uni > > create mode 160000 UnitTestFrameworkPkg/Library/SubhookLib/subhook > > > > diff --git a/.gitmodules b/.gitmodules > > index 8011a88d9d25..fe8a43be93ba 100644 > > --- a/.gitmodules > > +++ b/.gitmodules > > @@ -23,3 +23,6 @@ > > [submodule "UnitTestFrameworkPkg/Library/GoogleTestLib/googletest"] > > path = UnitTestFrameworkPkg/Library/GoogleTestLib/googletest > > url = https://github.com/google/googletest.git > > +[submodule "UnitTestFrameworkPkg/Library/SubhookLib/subhook"] > > + path = UnitTestFrameworkPkg/Library/SubhookLib/subhook > > + url = https://github.com/Zeex/subhook.git > > diff --git a/ReadMe.rst b/ReadMe.rst > > index 497d96355908..91b9cf3c5e50 100644 > > --- a/ReadMe.rst > > +++ b/ReadMe.rst > > @@ -94,6 +94,7 @@ that are covered by additional licenses. > > - `MdeModulePkg/Universal/RegularExpressionDxe/oniguruma > <https://github.com/kkos/oniguruma/blob/abfc8ff81df4067f309032467785e06975678f0d/COPYING>`__ > > - `UnitTestFrameworkPkg/Library/CmockaLib/cmocka > > <https://github.com/tianocore/edk2- > cmocka/blob/f5e2cd77c88d9f792562888d2b70c5a396bfbf7a/COPYING>`__ > > - `UnitTestFrameworkPkg/Library/GoogleTestLib/googletest > <https://github.com/google/googletest/blob/86add13493e5c881d7e4ba77fb91c1f57752b3a4/LICENSE>`__ > > +- `UnitTestFrameworkPkg/Library/SubhookLib/subhook > <https://github.com/Zeex/subhook/blob/83d4e1ebef3588fae48b69a7352cc21801cb70bc/LICENSE.txt>`__ > > - `RedfishPkg/Library/JsonLib/jansson > <https://github.com/akheron/jansson/blob/2882ead5bb90cf12a01b07b2c2361e24960fae02/LICENSE>`__ > > > > The EDK II Project is composed of packages. The maintainers for each > > package > > diff --git a/UnitTestFrameworkPkg/Include/Library/SubhookLib.h > > b/UnitTestFrameworkPkg/Include/Library/SubhookLib.h > > new file mode 100644 > > index 000000000000..46783adfccfb > > --- /dev/null > > +++ b/UnitTestFrameworkPkg/Include/Library/SubhookLib.h > > @@ -0,0 +1,15 @@ > > +/** @file > > + SubhookLib class with APIs from the subhook project > > + > > + Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > 2023? > > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#ifndef SUBHOOK_LIB_H_ > > +#define SUBHOOK_LIB_H_ > > + > > +#define SUBHOOK_STATIC > > +#include <subhook.h> > > + > > +#endif > > diff --git a/UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.inf > b/UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.inf > > new file mode 100644 > > index 000000000000..e8e8ffb90750 > > --- /dev/null > > +++ b/UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.inf > > @@ -0,0 +1,36 @@ > > +## @file > > +# This module provides Subhook Library implementation. > > +# > > +# Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > 2023? > > > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > +# > > +## > > + > > +[Defines] > > + INF_VERSION = 0x00010005 > > A bit ancient for a new module. Will update > > > + BASE_NAME = SubhookLib > > + MODULE_UNI_FILE = SubhookLib.uni > > + FILE_GUID = 70E03378-E140-46A8-8E65-7719DA14A240 > > + MODULE_TYPE = BASE > > + VERSION_STRING = 0.1 > > + LIBRARY_CLASS = SubhookLib|HOST_APPLICATION > > + > > +# > > +# VALID_ARCHITECTURES = IA32 X64 > > Surely not true? The LIBRARY_CLASS statement shows that this lib is only For HOST_APPLICATION which is only supported today on Windows/Linux applications for x86. In addition, this the subhook submodule that this lib wraps Is only for IA32/X64. It has a tiny disassembler and patcher For IA32/X64 instructions only. > > > +# > > + > > +[Sources] > > + subhook/subhook.c > > + > > +[Packages] > > + UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec > > + > > +[BuildOptions] > > + MSFT:*_*_*_CC_FLAGS == /c /EHsc /Zi /DSUBHOOK_STATIC > > + MSFT:NOOPT_*_*_CC_FLAGS = /Od > > + > > + GCC:*_*_*_CC_FLAGS == -g -c > > + > > + GCC:NOOPT_*_*_CC_FLAGS = -O0 > > Isn't this the default set in tools_def.template? I will check. We have to build host based unit tests with NOOPT target which disables optimizations. There is some complexity with HOST_APPLICATION module types that are not covered by tools_def.txt, but this one module is type BASE. It may be useful to support this lib in target environments too, so limiting to HOST_APPLICATION may be something we want to remove and then all flags would come from tools_def.txt. > > > + GCC:*_*_IA32_CC_FLAGS = -m32 > > + GCC:*_*_X64_CC_FLAGS = -m64 > > diff --git a/UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.uni > b/UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.uni > > new file mode 100644 > > index 000000000000..eb61f034047e > > --- /dev/null > > +++ b/UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.uni > > @@ -0,0 +1,11 @@ > > +// /** @file > > +// This module provides Subhook Library implementation. > > +// > > +// Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> > > 2023? > > > +// SPDX-License-Identifier: BSD-2-Clause-Patent > > +// > > +// **/ > > + > > +#string STR_MODULE_ABSTRACT #language en-US "Subhook Library > > implementation" > > + > > +#string STR_MODULE_DESCRIPTION #language en-US "This module > > provides Subhook Library implementation." > > diff --git a/UnitTestFrameworkPkg/Library/SubhookLib/subhook > > b/UnitTestFrameworkPkg/Library/SubhookLib/subhook > > new file mode 160000 > > index 000000000000..83d4e1ebef35 > > --- /dev/null > > +++ b/UnitTestFrameworkPkg/Library/SubhookLib/subhook > > @@ -0,0 +1 @@ > > +Subproject commit 83d4e1ebef3588fae48b69a7352cc21801cb70bc > > diff --git a/UnitTestFrameworkPkg/Test/UnitTestFrameworkPkgHostTest.dsc > b/UnitTestFrameworkPkg/Test/UnitTestFrameworkPkgHostTest.dsc > > index 708ef7f9ab35..722509c8f26f 100644 > > --- a/UnitTestFrameworkPkg/Test/UnitTestFrameworkPkgHostTest.dsc > > +++ b/UnitTestFrameworkPkg/Test/UnitTestFrameworkPkgHostTest.dsc > > @@ -33,6 +33,7 @@ [Components] > > # > > UnitTestFrameworkPkg/Library/CmockaLib/CmockaLib.inf > > UnitTestFrameworkPkg/Library/GoogleTestLib/GoogleTestLib.inf > > + UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.inf > > Could we move this after Posix to keep the alphabetical sorting? Will fix. > > / > Leif > > > UnitTestFrameworkPkg/Library/Posix/DebugLibPosix/DebugLibPosix.inf > > > > UnitTestFrameworkPkg/Library/Posix/MemoryAllocationLibPosix/MemoryAllocationLibPosix.inf > > UnitTestFrameworkPkg/Library/UnitTestLib/UnitTestLibCmocka.inf > > diff --git a/UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec > > b/UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec > > index 14e387d63a0f..30b489915d4a 100644 > > --- a/UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec > > +++ b/UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec > > @@ -20,6 +20,7 @@ [Includes] > > Library/CmockaLib/cmocka/include > > Library/GoogleTestLib/googletest/googletest/include > > Library/GoogleTestLib/googletest/googlemock/include > > + Library/SubhookLib/subhook > > > > [Includes.Common.Private] > > PrivateInclude > > @@ -34,6 +35,7 @@ [LibraryClasses] > > ## @libraryclass GoogleTest infrastructure > > # > > GoogleTestLib|Include/Library/GoogleTestLib.h > > + SubhookLib|Include/Library/SubhookLib.h > > > > [LibraryClasses.Common.Private] > > ## @libraryclass Provides a unit test result report > > diff --git a/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc > b/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc > > index 7f5dfa30ed60..e77897bd326f 100644 > > --- a/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc > > +++ b/UnitTestFrameworkPkg/UnitTestFrameworkPkgHost.dsc.inc > > @@ -15,6 +15,7 @@ [LibraryClasses.common.HOST_APPLICATION] > > > > CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLibNull/BaseCacheMaintenanceLibNull.inf > > CmockaLib|UnitTestFrameworkPkg/Library/CmockaLib/CmockaLib.inf > > > > GoogleTestLib|UnitTestFrameworkPkg/Library/GoogleTestLib/GoogleTestLib.inf > > + SubhookLib|UnitTestFrameworkPkg/Library/SubhookLib/SubhookLib.inf > > > > UnitTestLib|UnitTestFrameworkPkg/Library/UnitTestLib/UnitTestLibCmocka.inf > > > > DebugLib|UnitTestFrameworkPkg/Library/Posix/DebugLibPosix/DebugLibPosix.inf > > > > MemoryAllocationLib|UnitTestFrameworkPkg/Library/Posix/MemoryAllocationLibPosix/MemoryAllocationLibPosix.inf > > -- > > 2.39.1.windows.1 > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#102569): https://edk2.groups.io/g/devel/message/102569 Mute This Topic: https://groups.io/mt/98066293/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-