Re: [PATCH 4/8] ShellPkg/AcpiView: Create a logging facility

["Gao, Zhichao" <zhichao.gao@intel.com>]

See below.

> -----Original Message-----
> From: Tomas Pilar <Tomas.Pilar@arm.com>
> Sent: Monday, June 29, 2020 11:20 PM
> To: devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com; nd@arm.com; Ni, Ray <ray.ni@intel.com>; Gao,
> Zhichao <zhichao.gao@intel.com>
> Subject: [PATCH 4/8] ShellPkg/AcpiView: Create a logging facility
> 
> Extract error and warning logging into separate methods. Fold highlighting and
> other output properties into the logging methods.
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Tomas Pilar <tomas.pilar@arm.com>
> ---
>  .../UefiShellAcpiViewCommandLib/AcpiParser.c  |   5 +-
>  .../UefiShellAcpiViewCommandLib/AcpiViewLog.c | 230
> +++++++++++++++++  .../UefiShellAcpiViewCommandLib/AcpiViewLog.h | 233
> ++++++++++++++++++
>  .../UefiShellAcpiViewCommandLib.inf           |   2 +
>  4 files changed, 466 insertions(+), 4 deletions(-)  create mode 100644
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewLog.c
>  create mode 100644
> ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewLog.h
> 
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> index 7017fa93efae..b88594cf3865 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c
> @@ -11,13 +11,10 @@
>  #include "AcpiParser.h"
>  #include "AcpiView.h"
>  #include "AcpiViewConfig.h"
> +#include "AcpiViewLog.h"
> 
>  STATIC UINT32   gIndent;
> 
> -// Publicly accessible error and warning counters.
> -UINT32   mTableErrorCount;
> -UINT32   mTableWarningCount;
> -
>  STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
> 
>  /**
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewLog.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewLog.c
> new file mode 100644
> index 000000000000..9b9aaa855fdc
> --- /dev/null
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewLog.c
> @@ -0,0 +1,230 @@
> +/** @file
> +  'acpiview' logging and output facility
> +
> +  Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#include "AcpiViewLog.h"
> +#include "AcpiViewConfig.h"
> +#include "AcpiParser.h"
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +static CHAR16 mOutputBuffer [MAX_OUTPUT_SIZE] = { 0 };
> +
> +// String descriptions of error types
> +static const CHAR16* mErrorTypeDesc [ACPI_ERROR_MAX] = {
> +  L"Not an error",        ///< ACPI_ERROR_NONE
> +  L"Generic",             ///< ACPI_ERROR_GENERIC
> +  L"Checksum",            ///< ACPI_ERROR_CSUM
> +  L"Parsing",             ///< ACPI_ERROR_PARSE
> +  L"Length",              ///< ACPI_ERROR_LENGTH
> +  L"Value",               ///< ACPI_ERROR_VALUE
> +  L"Cross-check",         ///< ACPI_ERROR_CROSS
> +};
> +
> +// Publicly accessible error and warning counters.
> +UINT32   mTableErrorCount;
> +UINT32   mTableWarningCount;
> +
> +/**
> +  Change the attributes of the standard output console
> +  to change the colour of the text according to the given
> +  severity of a log message.
> +
> +  @param[in] Severity          The severity of the log message that is being
> +                               annotated with changed colour text.
> +  @param[in] OriginalAttribute The current attributes of ConOut that will be
> modified.
> +**/
> +static
> +VOID
> +EFIAPI
> +ApplyColor (
> +  IN ACPI_LOG_SEVERITY Severity,
> +  IN UINTN             OriginalAttribute
> +  )
> +{
> +  if (!mConfig.ColourHighlighting) {
> +    return;
> +  }
> +
> +  // Strip the foreground colour
> +  UINTN NewAttribute = OriginalAttribute & 0xF0;
> +
> +  // Add specific foreground colour based on severity  switch
> + (Severity) {  case ACPI_DEBUG:
> +    NewAttribute |= EFI_DARKGRAY;
> +    break;
> +  case ACPI_HIGHLIGHT:
> +    NewAttribute |= EFI_LIGHTBLUE;
> +    break;
> +  case ACPI_GOOD:
> +    NewAttribute |= EFI_GREEN;
> +    break;
> +  case ACPI_ITEM:
> +  case ACPI_WARN:
> +    NewAttribute |= EFI_YELLOW;
> +    break;
> +  case ACPI_BAD:
> +  case ACPI_ERROR:
> +  case ACPI_FATAL:
> +    NewAttribute |= EFI_RED;
> +    break;
> +  case ACPI_INFO:
> +  default:
> +    NewAttribute |= OriginalAttribute;
> +    break;
> +  }
> +
> +  gST->ConOut->SetAttribute (gST->ConOut, NewAttribute); }
> +
> +/**
> +  Restore ConOut text attributes.
> +
> +  @param[in] OriginalAttribute The attribute set that will be restored.
> +**/
> +static
> +VOID
> +EFIAPI
> +RestoreColor(
> +  IN UINTN OriginalAttribute
> +  )
> +{
> +  gST->ConOut->SetAttribute (gST->ConOut, OriginalAttribute); }
> +
> +/**
> +  Formats and prints an ascii string to screen.
> +
> +  @param[in] Format String that will be formatted and printed.
> +  @param[in] Marker The marker for variable parameters to be formatted.
> +**/
> +static
> +VOID
> +EFIAPI
> +AcpiViewVSOutput (
> +  IN const CHAR16  *Format,
> +  IN VA_LIST Marker
> +  )
> +{
> +  UnicodeVSPrint (mOutputBuffer, sizeof(mOutputBuffer), Format,
> +Marker);
> +  gST->ConOut->OutputString(gST->ConOut, mOutputBuffer); }
> +
> +/**
> +  Formats and prints and ascii string to screen.
> +
> +  @param[in] Format String that will be formatted and printed.
> +  @param[in] ...    A variable number of parameters that will be formatted.
> +**/
> +VOID
> +EFIAPI
> +AcpiViewOutput (
> +  IN const CHAR16 *Format,
> +  IN ...
> +)

Please align the ')'.

> +{
> +  VA_LIST Marker;
> +  VA_START (Marker, Format);
> +
> +  AcpiViewVSOutput (Format, Marker);
> +
> +  VA_END (Marker);
> +}
> +
> +
> +/**
> +  Prints the base file name given a full file path.
> +
> +  @param[in] FullName Fully qualified file path **/ VOID EFIAPI
> +PrintFileName (
> +  IN const CHAR8* FullName
> +  )
> +{
> +  const CHAR8* Cursor;
> +  UINTN Size;
> +
> +  Cursor = FullName;
> +  Size = 0;
> +
> +  // Find the end point of the string.
> +  while (*Cursor && Cursor < FullName + MAX_OUTPUT_SIZE)
> +    Cursor++;
> +
> +  // Find the rightmost path separator.
> +  while (*Cursor != '\\' && *Cursor != '/' && Cursor > FullName) {
> +    Cursor--;
> +    Size++;
> +  }
> +
> +  // Print base file name.
> +  AcpiViewOutput (L"%.*a", Size - 1, Cursor + 1); }
> +
> +/**
> +  AcpiView output and logging function. Will log the event to
> +  configured output (currently screen) and annotate with colour
> +  and extra metadata.
> +
> +  @param[in] FileName      The full filename of the source file where this
> +                           event occured.
> +  @param[in] FunctionName  The name of the function where this event occured.
> +  @param[in] LineNumber    The line number in the source code where this event
> +                           occured.
> +  @param[in] Severity      The severity of the event that occured.
> +  @param[in] Format        The format of the string describing the event.
> +  @param[in] ...           The variable number of parameters that will format the
> +                           string supplied in Format.
> +**/
> +VOID
> +EFIAPI
> +AcpiViewLog (
> +  IN const CHAR8       *FileName,
> +  IN const CHAR8       *FunctionName,
> +  IN UINTN             LineNumber,
> +  IN ACPI_ERROR_TYPE   Error,
> +  IN ACPI_LOG_SEVERITY Severity,
> +  IN const CHAR16      *Format,
> +  ...)

Align the ')' in the next line.

> +{
> +  VA_LIST         Marker;
> +  UINTN           OriginalAttribute;
> +
> +  OriginalAttribute = gST->ConOut->Mode->Attribute;  ApplyColor
> + (Severity, OriginalAttribute);  VA_START (Marker, Format);
> +
> +  switch (Severity) {
> +  case ACPI_FATAL:
> +    AcpiViewOutput (L"FATAL ");
> +    break;
> +  case ACPI_ERROR:
> +    AcpiViewOutput (L"ERROR[%s] ", mErrorTypeDesc[Error]);
> +    mTableErrorCount++;
> +    break;
> +  case ACPI_WARN:
> +    AcpiViewOutput (L"WARN ");
> +    mTableWarningCount++;
> +    break;
> +  default:
> +    break;
> +  }
> +
> +  if (Severity >= ACPI_WARN) {
> +    AcpiViewOutput (L"(");
> +    PrintFileName (FileName);
> +    AcpiViewOutput (L":%d) ", LineNumber);  }
> +
> +  AcpiViewVSOutput (Format, Marker);
> +  AcpiViewOutput (L"\n");
> +
> +  VA_END (Marker);
> +  RestoreColor (OriginalAttribute);
> +}
> +
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewLog.h
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewLog.h
> new file mode 100644
> index 000000000000..77049cd8eec2
> --- /dev/null
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiViewLog.h
> @@ -0,0 +1,233 @@
> +/** @file
> +  Header file for 'acpiview' logging and output facility
> +
> +  Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#ifndef ACPI_VIEW_LOG_H_
> +#define ACPI_VIEW_LOG_H_
> +
> +#include <Library/PrintLib.h>
> +
> +/*
> +  Categories of errors that can be logged by AcpiView.
> +*/
> +typedef enum {
> +  ACPI_ERROR_NONE,        ///< Not an error
> +  ACPI_ERROR_GENERIC,     ///< An unspecified error
> +  ACPI_ERROR_CSUM,        ///< A checksum was invalid
> +  ACPI_ERROR_PARSE,       ///< Failed to parse item
> +  ACPI_ERROR_LENGTH,      ///< Size of a thing is incorrect
> +  ACPI_ERROR_VALUE,       ///< The value of a field was incorrect
> +  ACPI_ERROR_CROSS,       ///< A constraint on multiple items was violated
> +  ACPI_ERROR_MAX
> +} ACPI_ERROR_TYPE;
> +
> +/*
> +  Different severities of events that can be logged.
> +*/
> +typedef enum {
> +  ACPI_DEBUG,       ///< Will not be shown unless specified on command line
> +  ACPI_INFO,        ///< Standard output
> +  ACPI_GOOD,        ///< Unspecified good outcome, green colour
> +  ACPI_BAD,         ///< Unspecified bad outcome, red colour
> +  ACPI_ITEM,        ///< Used when describing multiple items
> +  ACPI_HIGHLIGHT,   ///< A new context or section has been entered
> +  ACPI_WARN,        ///< An unusual event happened
> +  ACPI_ERROR,       ///< Acpi table is not conformant
> +  ACPI_FATAL        ///< This will abort program execution.
> +} ACPI_LOG_SEVERITY;
> +
> +// Publicly accessible error and warning counters.
> +extern UINT32   mTableErrorCount;
> +extern UINT32   mTableWarningCount;
> +
> +/**
> +  AcpiView output and logging function. Will log the event to
> +  configured output (currently screen) and annotate with colour
> +  and extra metadata.
> +
> +  @param[in] FileName      The full filename of the source file where this
> +                           event occured.
> +  @param[in] FunctionName  The name of the function where this event occured.
> +  @param[in] LineNumber    The line number in the source code where this event
> +                           occured.
> +  @param[in] Severity      The severity of the event that occured.
> +  @param[in] Error         The type of the erorr reported. May be
> ACPI_ERROR_NONE if the event
> +                           is not an error.
> +  @param[in] Format        The format of the string describing the event.
> +  @param[in] ...           The variable number of parameters that will format the
> +                           string supplied in Format.
> +**/
> +VOID
> +EFIAPI
> +AcpiViewLog (
> +  IN const CHAR8       *FileName,
> +  IN const CHAR8       *FunctionName,
> +  IN UINTN             LineNumber,
> +  IN ACPI_ERROR_TYPE   Error,
> +  IN ACPI_LOG_SEVERITY Severity,
> +  IN const CHAR16            *Format,
> +  ...
> +  );

Align the parameter Format with other parameters.

> +
> +/**
> +  Formats and prints and ascii string to screen.
> +
> +  @param[in] Format String that will be formatted and printed.
> +  @param[in] ...    A variable number of parameters that will be formatted.
> +**/
> +VOID
> +EFIAPI
> +AcpiViewOutput (
> +  IN const CHAR16 *Format,
> +  IN ...
> +  );
> +
> +/**
> +  Check that a buffer has not been overrun by a member. Can be invoked
> +  using the BufferOverrun macro that fills in local source metadata
> +  (first three parameters) for logging purposes.
> +
> +  @param[in] FileName        Source file where this invocation is made
> +  @param[in] FunctionName    Name of the local symbol
> +  @param[in] LineNumber      Source line number of the local call
> +  @param[in] ItemDescription User friendly name for the member being
> checked
> +  @param[in] Position        Memory address of the member
> +  @param[in] Length          Length of the member
> +  @param[in] Buffer          Memory address of the buffer where member resides
> +  @param[in] BufferSize      Size of the buffer where member resides
> +
> +  @retval TRUE               Buffer was overrun
> +  @retval FALSE              Buffer is safe
> +**/
> +static
> +inline
> +BOOLEAN
> +EFIAPI
> +MemberIntegrityInternal (
> +  IN const CHAR8  *FileName,
> +  IN const CHAR8  *FunctionName,
> +  IN UINTN        LineNumber,
> +  IN const CHAR8 *ItemDescription,
> +  IN UINTN        Offset,
> +  IN UINTN        Length,
> +  IN VOID         *Buffer,
> +  IN UINTN        BufferSize)
> +{
> +  if (Length == 0) {
> +    AcpiViewLog (
> +      FileName,
> +      FunctionName,
> +      LineNumber,
> +      ACPI_ERROR_LENGTH,
> +      ACPI_ERROR,
> +      L"%a at %p in buffer %p+%x has zero size!",
> +      ItemDescription,
> +      (UINT8 *)Buffer + Offset,
> +      Buffer,
> +      BufferSize);
> +    return TRUE;
> +  }
> +
> +  if (Offset + Length > BufferSize) {
> +    AcpiViewLog (
> +      FileName,
> +      FunctionName,
> +      LineNumber,
> +      ACPI_ERROR_LENGTH,
> +      ACPI_ERROR,
> +      L"%a %p+%x overruns buffer %p+%x",
> +      ItemDescription,
> +      (UINT8 *) Buffer + Offset,
> +      Length,
> +      Buffer,
> +      BufferSize);
> +  }
> +
> +  return (Offset + Length > BufferSize); }
> +
> +// Determine if a member located at Offset into a Buffer lies entirely
> +// within the BufferSize given the member size is Length // Evaluates
> +to TRUE and logs error if buffer is overrun or if Length is zero
> +#define AssertMemberIntegrity(Offset, Length, Buffer, BufferSize) \
> +  MemberIntegrityInternal (__FILE__, __func__, __LINE__, #Length,
> +Offset, Length, Buffer, BufferSize)
> +
> +
> +/**
> +  Checks that a boolean constraint evaluates correctly. Can be invoked
> +  using the CheckConstraint macro that fills in the source code metadata.
> +
> +  @param[in] FileName        Source file where this invocation is made
> +  @param[in] FunctionName    Name of the local symbol
> +  @param[in] LineNumber      Source line number of the local call
> +  @param[in] ConstraintText  The Source code of the constraint
> +  @param[in] Specification   The specification that imposes the constraint
> +  @param[in] Constraint      The actual constraint
> +  @
> +
> +  @retval TRUE               Constraint is violated
> +  @retval FALSE              Constraint is not violated
> +**/
> +static
> +inline
> +BOOLEAN
> +EFIAPI
> +CheckConstraintInternal (
> +  IN const CHAR8       *FileName,
> +  IN const CHAR8       *FunctionName,
> +  IN UINTN             LineNumber,
> +  IN const CHAR8       *ConstraintText,
> +  IN const CHAR16      *Specification,
> +  IN BOOLEAN           Constraint,
> +  IN ACPI_LOG_SEVERITY Severity
> +)
> +{
> +  if (!Constraint) {
> +    AcpiViewLog (
> +      FileName,
> +      FunctionName,
> +      LineNumber,
> +      Severity == ACPI_ERROR ? ACPI_ERROR_VALUE : ACPI_ERROR_NONE,
> +      Severity,
> +      L"(%a) Constraint was violated: %a",
> +      Specification,
> +      ConstraintText);
> +  }
> +
> +  // Return TRUE if constraint was violated
> +  return !Constraint;
> +}

For this two inline functions:
Refer to CCS_2_1 Section 5.1.9: Do not use the in-line assembler.
Refer to CCS_2_1 Section 5.3.7: include file shall not generate the code or define data variables.

Thanks,
Zhichao

> +
> +// Evaluates to TRUE and logs error if a constraint is violated //
> +Constraint internally cast to BOOLEAN using !!(Constraint) #define
> +AssertConstraint(Specification, Constraint) \
> +  CheckConstraintInternal (                           \
> +    __FILE__, __func__, __LINE__, #Constraint, Specification,
> +!!(Constraint), ACPI_ERROR)
> +
> +// Evaluates to TRUE and logs error if a constraint is violated #define
> +WarnConstraint(Specification, Constraint) \
> +  CheckConstraintInternal (                           \
> +    __FILE__, __func__, __LINE__, #Constraint, Specification,
> +Constraint, ACPI_WARN)
> +
> +
> +// Maximum string size that can be printed #define MAX_OUTPUT_SIZE 256
> +
> +#define _AcpiLog(...) AcpiViewLog(__FILE__, __func__, __LINE__,
> +__VA_ARGS__)
> +
> +// Generic Loging macro, needs severity and formatted string #define
> +AcpiLog(Severity, ...) _AcpiLog(ACPI_ERROR_NONE, Severity, __VA_ARGS__)
> +
> +// Log undecorated text, needs formatted string #define AcpiInfo(...)
> +_AcpiLog(ACPI_ERROR_NONE, ACPI_INFO, __VA_ARGS__)
> +
> +// Log error and increment counter, needs error type and formatted
> +string #define AcpiError(Error, ...) _AcpiLog(Error, ACPI_ERROR,
> +__VA_ARGS__)
> +
> +// Log a FATAL error, needs formatted string #define AcpiFatal(...)
> +_AcpiLog(ACPI_ERROR_GENERIC, ACPI_FATAL, __VA_ARGS__)
> +
> +#endif // ACPI_VIEW_LOG_H_
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.inf
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.inf
> index 91459f9ec632..e0586cbccec2 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLi
> b.inf
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewComm
> +++ andLib.inf
> @@ -27,6 +27,8 @@ [Sources.common]
>    AcpiView.h
>    AcpiViewConfig.c
>    AcpiViewConfig.h
> +  AcpiViewLog.h
> +  AcpiViewLog.c
>    Parsers/Bgrt/BgrtParser.c
>    Parsers/Dbg2/Dbg2Parser.c
>    Parsers/Dsdt/DsdtParser.c
> --
> 2.24.1.windows.2
>