public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: Leif Lindholm <quic_llindhol@quicinc.com>,
	"devel@edk2.groups.io" <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
Date: Wed, 5 Apr 2023 21:34:59 +0000	[thread overview]
Message-ID: <CO1PR11MB492981638E1F24CE3C0B14D4D2909@CO1PR11MB4929.namprd11.prod.outlook.com> (raw)
In-Reply-To: <ZC27FrvahCfQ2sVc@qc-i7.hemma.eciton.net>

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.

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

  reply	other threads:[~2023-04-05 21:35 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-04 18:22 [Patch v2 00/12] Add gmock support for host-based unit testing Michael D Kinney
2023-04-04 18:22 ` [Patch v2 01/12] UnitTestFrameworkPkg: Add subhook submodule required for gmock Michael D Kinney
2023-04-05  1:51   ` [edk2-devel] " Michael Kubacki
2023-04-05 18:16   ` Leif Lindholm
2023-04-05 21:34     ` Michael D Kinney [this message]
2023-04-07 21:31       ` Michael D Kinney
2023-04-04 18:22 ` [Patch v2 02/12] .pytool/CISettings.py: Add subhook submodule Michael D Kinney
2023-04-05  1:51   ` Michael Kubacki
2023-04-04 18:22 ` [Patch v2 03/12] UnitTestFrameworkPkg: Add gmock support to GoogleTestLib Michael D Kinney
2023-04-05  1:51   ` Michael Kubacki
2023-04-04 18:22 ` [Patch v2 04/12] UnitTestFrameworkPkg/ReadMe.md: Add gmock documentation Michael D Kinney
2023-04-05  1:51   ` [edk2-devel] " Michael Kubacki
2023-04-04 18:22 ` [Patch v2 05/12] MdePkg: Add gmock examples Michael D Kinney
2023-04-06  1:15   ` 回复: " gaoliming
2023-04-04 18:22 ` [Patch v2 06/12] MdeModulePkg/Library/UefiSortLib: Add GoogleTestLib example Michael D Kinney
2023-04-04 18:22 ` [Patch v2 07/12] SecurityPkg: Add gmock example Michael D Kinney
2023-04-04 18:22 ` [Patch v2 08/12] SecurityPkg/Library/SecureBootVariableLib: Fix VS20xx 4122 errors Michael D Kinney
2023-04-04 18:22 ` [Patch v2 09/12] SecurityPkg/Library/SecureBootVariableLib: HOST_APPLICATION IA32/X64 only Michael D Kinney
2023-04-04 18:22 ` [Patch v2 10/12] MdePkg/Library/BaseLib: " Michael D Kinney
2023-04-06  1:15   ` 回复: " gaoliming
2023-04-04 18:22 ` [Patch v2 11/12] MdeModulePkg: " Michael D Kinney
2023-04-04 18:22 ` [Patch v2 12/12] PrmPkg/Library: " Michael D Kinney
2023-04-05  1:51   ` Michael Kubacki
2023-04-05  7:50   ` [edk2-devel] " Ard Biesheuvel
2023-04-05 12:46     ` Michael Kubacki
2023-04-05 14:27       ` Michael D Kinney
2023-04-05 20:33 ` [edk2-devel] [Patch v2 00/12] Add gmock support for host-based unit testing Oliver Smith-Denny
2023-04-07 21:20 ` Michael D Kinney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CO1PR11MB492981638E1F24CE3C0B14D4D2909@CO1PR11MB4929.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox