Re: [PATCH 1/3] MdePkg: break #defines out of Uefi/UefiMultiPhase.h

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Dec 14, 2017 at 10:44:04AM +0800, Ni, Ruiyu wrote:
> On 12/13/2017 8:26 PM, Leif Lindholm wrote:
> > Turns out all .vfr files in the tree interacting with DynamicPcds
> > manually copy the same set of EFI_VARIABLE_* definitions, since the rest
> > of UefiMultiPhase.h is incompatible with VfrCompile.
> > 
> > Split these out into a separate header file UefiMultiPhaseDefinitions.h
> > in order to make it possible to include just that portion into .vfr
> > files. Then include that from UefiMultiPhase.h.
> > 
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >   MdePkg/Include/Uefi/UefiMultiPhase.h            | 23 +-----------
> >   MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h | 39 ++++++++++++++++++++
> >   2 files changed, 41 insertions(+), 21 deletions(-)
> > 
> > diff --git a/MdePkg/Include/Uefi/UefiMultiPhase.h b/MdePkg/Include/Uefi/UefiMultiPhase.h
> > index 0dcbb1b9ee..b360c9513b 100644
> > --- a/MdePkg/Include/Uefi/UefiMultiPhase.h
> > +++ b/MdePkg/Include/Uefi/UefiMultiPhase.h
> > @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >   #ifndef __UEFI_MULTIPHASE_H__
> >   #define __UEFI_MULTIPHASE_H__
> > +#include "UefiMultiPhaseDefinitions.h"
> > +
> >   #include <Guid/WinCertificate.h>
> >   ///
> >   /// Enumeration of memory types introduced in UEFI.
> > @@ -156,27 +158,6 @@ typedef struct {
> >   } EFI_TABLE_HEADER;
> >   ///
> > -/// Attributes of variable.
> > -///
> > -#define EFI_VARIABLE_NON_VOLATILE                            0x00000001
> > -#define EFI_VARIABLE_BOOTSERVICE_ACCESS                      0x00000002
> > -#define EFI_VARIABLE_RUNTIME_ACCESS                          0x00000004
> > -///
> > -/// This attribute is identified by the mnemonic 'HR'
> > -/// elsewhere in this specification.
> > -///
> > -#define EFI_VARIABLE_HARDWARE_ERROR_RECORD                   0x00000008
> > -///
> > -/// Attributes of Authenticated Variable
> > -///
> > -#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS   0x00000020
> > -#define EFI_VARIABLE_APPEND_WRITE                            0x00000040
> > -///
> > -/// NOTE: EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and should be considered reserved.
> > -///
> > -#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS              0x00000010
> > -
> > -///
> >   /// AuthInfo is a WIN_CERTIFICATE using the wCertificateType
> >   /// WIN_CERTIFICATE_UEFI_GUID and the CertType
> >   /// EFI_CERT_TYPE_RSA2048_SHA256_GUID. If the attribute specifies
> > diff --git a/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h b/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h
> > new file mode 100644
> > index 0000000000..df55a92dfa
> > --- /dev/null
> > +++ b/MdePkg/Include/Uefi/UefiMultiPhaseDefinitions.h
> > @@ -0,0 +1,39 @@
> > +/** @file
> > +  This includes some definitions introduced in UEFI that will be used in both PEI and DXE phases.
> > +
> > +Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> > +This program and the accompanying materials are licensed and made available under
> > +the terms and conditions of the BSD License that accompanies this distribution.
> > +The full text of the license may be found at
> > +http://opensource.org/licenses/bsd-license.php.
> > +
> > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef __UEFI_MULTIPHASE_DEFS_H__
> > +#define __UEFI_MULTIPHASE_DEFS_H__
> > +
> > +///
> > +/// Attributes of variable.
> > +///
> > +#define EFI_VARIABLE_NON_VOLATILE                            0x00000001
> > +#define EFI_VARIABLE_BOOTSERVICE_ACCESS                      0x00000002
> > +#define EFI_VARIABLE_RUNTIME_ACCESS                          0x00000004
> > +///
> > +/// This attribute is identified by the mnemonic 'HR'
> > +/// elsewhere in this specification.
> > +///
> > +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD                   0x00000008
> > +///
> > +/// Attributes of Authenticated Variable
> > +///
> > +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS   0x00000020
> > +#define EFI_VARIABLE_APPEND_WRITE                            0x00000040
> > +///
> > +/// NOTE: EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated and should be considered reserved.
> > +///
> > +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS              0x00000010
> > +
> > +#endif
> > 
> Can we just move the definitions to UefiBaseTypes.h?

There is nothing in UefiBaseTypes.h that is fundamentally easier to
deal with than what is in UefiMultiPhase.h to begin with.

> Even vfrcompiler still complains, the syntax enhancement to
> vfrcompiler should be simple.

It's (probably) not VfrCompile that would need to change.
The problem is that the build runs the C preprocessor on the .vfr
source files before passing them to VfrCompile. #defines that just
replace text work fine, but typedefs and struct definitions make no
sense in vfr source.

It runs the preprocessor in C mode (for gpp, -x c) which means we
cannot use __ASSEMBLER__ to filter out typedefs and struct
definitions.

We could perhaps switch to -x assembler-with-cpp (and filter
UefiMultiPhase.h on __ASSEMBLER__), like we do for DTC.
It _seems_ to work in my test, but I do not know what a corresponding
change for VS would be.

> I personally do not like creating more and more files.

I am sort of with you on that one.
But I consider it a much lesser evil than duplicated definitions.

/
    Leif