Re: [edk2-devel] [Patch v2 01/12] UnitTestFrameworkPkg: Add subhook submodule required for gmock

["Michael D Kinney" <michael.d.kinney@intel.com>]

One more update included below.

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, April 5, 2023 2:35 PM
> To: Leif Lindholm <quic_llindhol@quicinc.com>; devel@edk2.groups.io
> Cc: Johnson, Chris N <chris.n.johnson@intel.com>; Andrew Fish <afish@apple.com>; Michael Kubacki <mikuback@linux.microsoft.com>;
> Sean Brogan <sean.brogan@microsoft.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [Patch v2 01/12] UnitTestFrameworkPkg: Add subhook submodule required for gmock
> 
> 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_llindhol@quicinc.com>
> > Sent: Wednesday, April 5, 2023 11:17 AM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Johnson, Chris N <chris.n.johnson@intel.com>; Andrew Fish <afish@apple.com>; Michael Kubacki
> > <mikuback@linux.microsoft.com>; Sean Brogan <sean.brogan@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.johnson@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 <afish@apple.com>
> > > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > Signed-off-by: Chris Johnson <chris.n.johnson@intel.com>
> >
> > I have some nitpicks below, which you can take into account or
> > not. Regardless:
> >
> > Reviewed-by: Leif Lindholm <quic_llindhol@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.


I have simplified the [BuildOptions] section.  Since the first use for these
uses a '==', all flags inherited from tools_def.txt or the DSC file are 
thrown out.  This is required to build this submodule that does not like
the EDK II default Base.h definitions.  The only use case today for this 
component is in host-based unit testing to support gmock.  It may be 
possible to extend the support of the lib to target based environments
for IA32/X64, but would require more work on getting the build to be
compatible.  For now, I have changed the MODULE_TYPE to HOST_APPLICATION
as that is the only env supported now.

> 
> >
> > > +  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
> > >
> > >
> > >
> > > 
> > >
> > >