Re: [edk2-devel] [PATCH v2 02/12] MdeModulePkg: Define the VariablePolicyLib

["Laszlo Ersek" <lersek@redhat.com>]

On 05/12/20 08:46, Michael Kubacki wrote:
> From: Bret Barkelew <brbarkel@microsoft.com>
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=2522
>
> VariablePolicy is an updated interface to
> replace VarLock and VarCheckProtocol.
>
> Add the VariablePolicyLib library that implements
> the portable business logic for the VariablePolicy
> engine.
>
> Also add host-based CI test cases for the lib.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c                     |   46 +
>  MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c               |   85 +
>  MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c                               |  806 +++++++
>  MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.c   | 2285 ++++++++++++++++++++
>  MdeModulePkg/Include/Library/VariablePolicyLib.h                                         |  207 ++
>  MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf                             |   44 +
>  MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni                             |   12 +
>  MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf                   |   52 +
>  MdeModulePkg/Library/VariablePolicyLib/VariablePolicyUnitTest/VariablePolicyUnitTest.inf |   41 +
>  MdeModulePkg/MdeModulePkg.dec                                                            |    3 +
>  MdeModulePkg/MdeModulePkg.dsc                                                            |    5 +
>  MdeModulePkg/Test/MdeModulePkgHostTest.dsc                                               |   11 +
>  12 files changed, 3597 insertions(+)
>
> diff --git a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
> new file mode 100644
> index 000000000000..ad2ee0b2fb8f
> --- /dev/null
> +++ b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
> @@ -0,0 +1,46 @@
> +/** @file -- VariablePolicyExtraInitNull.c
> +This file contains extra init and deinit routines that don't do anything
> +extra.
> +
> +Copyright (c) Microsoft Corporation.
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +
> +
> +/**
> +  An extra init hook that enables the RuntimeDxe library instance to
> +  register VirtualAddress change callbacks. Among other things.
> +
> +  @retval     EFI_SUCCESS   Everything is good. Continue with init.
> +  @retval     Others        Uh... don't continue.
> +
> +**/
> +EFI_STATUS
> +VariablePolicyExtraInit (
> +  VOID
> +  )
> +{
> +  // NULL implementation.
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  An extra deinit hook that enables the RuntimeDxe library instance to
> +  register VirtualAddress change callbacks. Among other things.
> +
> +  @retval     EFI_SUCCESS   Everything is good. Continue with deinit.
> +  @retval     Others        Uh... don't continue.
> +
> +**/
> +EFI_STATUS
> +VariablePolicyExtraDeinit (
> +  VOID
> +  )
> +{
> +  // NULL implementation.
> +  return EFI_SUCCESS;
> +}
> diff --git a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c
> new file mode 100644
> index 000000000000..8e63f4eb66a0
> --- /dev/null
> +++ b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c
> @@ -0,0 +1,85 @@
> +/** @file -- VariablePolicyExtraInitRuntimeDxe.c
> +This file contains extra init and deinit routines that register and unregister
> +VariableAddressChange callbacks.
> +
> +Copyright (c) Microsoft Corporation.
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +
> +extern EFI_GET_VARIABLE   mGetVariableHelper = NULL;
> +extern UINT8              *mPolicyTable = NULL;

These two lines trigger a build failure with GCC48:

> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c:13:27: error: 'mGetVariableHelper' initialized and declared 'extern' [-Werror]
>  extern EFI_GET_VARIABLE   mGetVariableHelper = NULL;
>                            ^
> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntimeDxe.c:14:28: error: 'mPolicyTable' initialized and declared 'extern' [-Werror]
>  extern UINT8              *mPolicyTable = NULL;
>                             ^
> cc1: all warnings being treated as errors

Unfortunately, this is a gcc bug:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45977

More precisely, it is not a bug that gcc "warns" about this (it *would* be a
gcc bug no-questions-asked if the diagnostic would be a hard error even without
"-Werror").

Instead, the gcc bug at present is that gcc doesn't offer a
"-Wno-error=FOOBAR"-style option for disabling "error treatment" for this
specific warning.

So please rework this in order to avoid triggering the gcc problem.

Thanks!
Laszlo