Re: [PATCH v3 1/1] MdePkg: Use ANSI colors to indicate debug message severity

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

Rebecca,

BasePrintLib is used for much more than just DEBUG() macros.  This features should
not modify the behavior of BasePrintLib.

For example, a single module that links to BasePrintLib can use those services
for non DebugLib services with no expectation that ANSI sequences are added
and that same module can link against DebugLib for DEBUG() macro support that
expects ANSI sequences to be added.  This is a conflict.

The scope of this change should be only the DebugLib instance.

Mike

> -----Original Message-----
> From: Rebecca Cran <rebecca@quicinc.com>
> Sent: Friday, September 2, 2022 4:23 AM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu,
> Zhiguang <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>
> Cc: Rebecca Cran <rebecca@quicinc.com>
> Subject: [PATCH v3 1/1] MdePkg: Use ANSI colors to indicate debug message severity
> 
> There currently isn't a way to differentiate the different
> levels of DEBUG output: DEBUG_ERROR, DEBUG_WARN, DEBUG_INFO
> etc.
> 
> To improve this, wrap DEBUG_ERROR and DEBUG_WARN level
> messages in ANSI color code escape sequences. DEBUG_ERROR
> messages will be displayed in red text, and DEBUG_WARN
> in yellow.
> 
> Only enable this new functionality if the FeatureFlag
> gEfiMdePkgTokenSpaceGuid.PcdDebugAnsiSeqSupport
> is set to TRUE. By default it's FALSE.
> 
> Signed-off-by: Rebecca Cran <rebecca@quicinc.com>
> ---
>  MdePkg/MdePkg.dec                                                |   6 +
>  MdePkg/MdePkg.dsc                                                |   2 +
>  MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf |   2 +-
>  MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf         |   2 +-
>  MdePkg/Include/Library/PrintLib.h                                |  39 ++++++
>  MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c                 |   4 +
>  MdePkg/Library/BasePrintLib/PrintLib.c                           | 128 ++++++++++++++++++++
>  MdePkg/Library/UefiDebugLibConOut/DebugLib.c                     |   4 +
>  8 files changed, 185 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index f1ebf9e251c1..00b50cedd510 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -4,6 +4,7 @@
>  # It also provides the definitions(including PPIs/PROTOCOLs/GUIDs) of
>  # EFI1.10/UEFI2.7/PI1.7 and some Industry Standards.
>  #
> +# Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.<BR>
>  # Copyright (c) 2007 - 2022, Intel Corporation. All rights reserved.<BR>
>  # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>  # (C) Copyright 2016 - 2021 Hewlett Packard Enterprise Development LP<BR>
> @@ -1973,6 +1974,11 @@ [PcdsFeatureFlag]
>    # @Prompt Validate ORDERED_COLLECTION structure
>    gEfiMdePkgTokenSpaceGuid.PcdValidateOrderedCollection|FALSE|BOOLEAN|0x0000002a
> 
> +  ## Indicates if DEBUG output should use ANSI sequences.<BR><BR>
> +  #   TRUE  - Will use ANSI sequences in DEBUG output.<BR>
> +  #   FALSE - Will not use ANSI sequences in DEBUG output.<BR>
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugAnsiSeqSupport|FALSE|BOOLEAN|0x0000002f
> +
>  [PcdsFixedAtBuild]
>    ## Status code value for indicating a watchdog timer has expired.
>    # EFI_COMPUTING_UNIT_HOST_PROCESSOR | EFI_CU_HP_EC_TIMER_EXPIRED
> diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
> index cc1ac196a931..1ecbe6526ac9 100644
> --- a/MdePkg/MdePkg.dsc
> +++ b/MdePkg/MdePkg.dsc
> @@ -1,6 +1,7 @@
>  ## @file
>  # EFI/PI MdePkg Package
>  #
> +# Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.<BR>
>  # Copyright (c) 2007 - 2022, Intel Corporation. All rights reserved.<BR>
>  # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>  # (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
> @@ -25,6 +26,7 @@ [Defines]
> 
>  [PcdsFeatureFlag]
>    gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport|TRUE
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugAnsiSeqSupport|FALSE

The DEC default value is FALSE, so this line is not required.

> 
>  [PcdsFixedAtBuild]
>    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x0f
> diff --git a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> index 7504faee67f0..8d6ed759e974 100644
> --- a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> +++ b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> @@ -41,4 +41,4 @@ [Pcd]
>    gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue  ## SOMETIMES_CONSUMES
>    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask      ## CONSUMES
>    gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel ## CONSUMES
> -
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugAnsiSeqSupport       ## CONSUMES
> diff --git a/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
> b/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
> index 53bbc8ce3f65..694494ffc7a3 100644
> --- a/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
> +++ b/MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
> @@ -50,4 +50,4 @@ [Pcd]
>    gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue        ## SOMETIMES_CONSUMES
>    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask            ## CONSUMES
>    gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel    ## CONSUMES
> -
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugAnsiSeqSupport          ## CONSUMES
> diff --git a/MdePkg/Include/Library/PrintLib.h b/MdePkg/Include/Library/PrintLib.h
> index 8d523cac528d..06d9761df897 100644
> --- a/MdePkg/Include/Library/PrintLib.h
> +++ b/MdePkg/Include/Library/PrintLib.h
> @@ -2,6 +2,7 @@
>    Provides services to print a formatted string to a buffer. All combinations of
>    Unicode and ASCII strings are supported.
> 
> +Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.<BR>
>  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -931,4 +932,42 @@ SPrintLengthAsciiFormat (
>    IN  VA_LIST      Marker
>    );
> 
> +/**
> +  Wraps a message with ANSI color escape codes.
> +
> +  @param String     The string to wrap.
> +  @param StringLen  The size of the String buffer in characters.
> +  @param ErrorLevel The error level.
> +
> +  @retval RETURN_SUCCESS          The string was successfully updated.
> +  @retval RETURN_BUFFER_TOO_SMALL The buffer is too small.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +AsciiDebugGetColorString (
> +  IN OUT CHAR8  *String,
> +  IN UINTN      StringLen,
> +  IN UINTN      ErrorLevel
> +  );
> +
> +/**
> +  Wraps a message with ANSI color escape codes.
> +
> +  @param String     The string to wrap.
> +  @param StringLen  The size of the String buffer in Unicode characters.
> +  @param ErrorLevel The error level.
> +
> +  @retval RETURN_SUCCESS          The string was successfully updated.
> +  @retval RETURN_BUFFER_TOO_SMALL The buffer is too small.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +UnicodeDebugGetColorString (
> +  IN OUT CHAR16  *String,
> +  IN UINTN       StringLen,
> +  IN UINTN       ErrorLevel
> +  );
> +
>  #endif
> diff --git a/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c b/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
> index bd5686947712..0971c9a57c88 100644
> --- a/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
> +++ b/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
> @@ -125,6 +125,10 @@ DebugPrintMarker (
>      AsciiBSPrint (Buffer, sizeof (Buffer), Format, BaseListMarker);
>    }
> 
> +  if (FeaturePcdGet (PcdDebugAnsiSeqSupport)) {
> +    AsciiDebugGetColorString (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ErrorLevel);
> +  }
> +
>    //
>    // Send the print string to a Serial Port
>    //
> diff --git a/MdePkg/Library/BasePrintLib/PrintLib.c b/MdePkg/Library/BasePrintLib/PrintLib.c
> index e6f4042bb90b..95135d45aff9 100644
> --- a/MdePkg/Library/BasePrintLib/PrintLib.c
> +++ b/MdePkg/Library/BasePrintLib/PrintLib.c
> @@ -1,12 +1,15 @@
>  /** @file
>    Base Print Library instance implementation.
> 
> +  Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.<BR>
>    Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>    Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  **/
> 
> +#include <Library/BaseMemoryLib.h>
> +
>  #include "PrintLibInternal.h"
> 
>  //
> @@ -19,6 +22,19 @@ VA_LIST  gNullVaList;
> 
>  #define ASSERT_UNICODE_BUFFER(Buffer)  ASSERT ((((UINTN) (Buffer)) & 0x01) == 0)
> 
> +//
> +// Define the maximum debug and assert message length that this library supports
> +//
> +#define MAX_DEBUG_MESSAGE_LENGTH  0x100
> +
> +#define ASCII_RED_ESC_SEQ     "\033[31m"
> +#define ASCII_YELLOW_ESC_SEQ  "\033[33m"
> +#define ASCII_END_ESC_SEQ     "\033[0m"
> +
> +#define RED_ESC_SEQ     L"\033[31m"
> +#define YELLOW_ESC_SEQ  L"\033[33m"
> +#define END_ESC_SEQ     L"\033[0m"
> +
>  /**
>    Produces a Null-terminated Unicode string in an output buffer based on
>    a Null-terminated Unicode format string and a VA_LIST argument list.
> @@ -834,3 +850,115 @@ SPrintLengthAsciiFormat (
>  {
>    return BasePrintLibSPrintMarker (NULL, 0, OUTPUT_UNICODE | COUNT_ONLY_NO_PRINT, (CHAR8 *)FormatString, Marker, NULL);
>  }
> +
> +/**
> +  Wraps a message with ANSI color escape codes.
> +
> +  @param String     The string to wrap.
> +  @param StringLen  The size of the String buffer in characters.
> +  @param ErrorLevel The error level.
> +
> +  @retval RETURN_SUCCESS          The string was successfully updated.
> +  @retval RETURN_BUFFER_TOO_SMALL The buffer is too small.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +AsciiDebugGetColorString (
> +  IN OUT CHAR8  *String,
> +  IN UINTN      StringLen,
> +  IN UINTN      ErrorLevel
> +  )
> +{
> +  CHAR8  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> +  UINTN  ReqBufferLen;
> +
> +  ReqBufferLen = AsciiStrLen (String) +
> +                 AsciiStrLen (ASCII_RED_ESC_SEQ) +
> +                 AsciiStrLen (ASCII_END_ESC_SEQ) +
> +                 1;
> +
> +  if (StringLen < ReqBufferLen) {
> +    return RETURN_BUFFER_TOO_SMALL;
> +  }
> +
> +  ZeroMem (Buffer, sizeof (Buffer));
> +
> +  switch (ErrorLevel) {
> +    case DEBUG_WARN:
> +      AsciiStrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ASCII_YELLOW_ESC_SEQ);
> +      break;
> +    case DEBUG_ERROR:
> +      AsciiStrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ASCII_RED_ESC_SEQ);
> +      break;
> +  }
> +
> +  AsciiStrCatS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, String);
> +
> +  switch (ErrorLevel) {
> +    case DEBUG_WARN:
> +    case DEBUG_ERROR:
> +      AsciiStrCatS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ASCII_END_ESC_SEQ);
> +      break;
> +  }
> +
> +  AsciiStrCpyS (String, StringLen, Buffer);
> +
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Wraps a message with ANSI color escape codes.
> +
> +  @param String     The string to wrap.
> +  @param StringLen  The size of the String buffer in Unicode characters.
> +  @param ErrorLevel The error level.
> +
> +  @retval RETURN_SUCCESS          The string was successfully updated.
> +  @retval RETURN_BUFFER_TOO_SMALL The buffer is too small.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +UnicodeDebugGetColorString (
> +  IN OUT CHAR16  *String,
> +  IN UINTN       StringLen,
> +  IN UINTN       ErrorLevel
> +  )
> +{
> +  CHAR16  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> +  UINTN   ReqBufferLen;
> +
> +  ReqBufferLen = StrLen (String) +
> +                 StrLen (RED_ESC_SEQ) +
> +                 StrLen (END_ESC_SEQ) +
> +                 1;
> +
> +  if (StringLen < ReqBufferLen) {
> +    return RETURN_BUFFER_TOO_SMALL;
> +  }
> +
> +  ZeroMem (Buffer, sizeof (Buffer));
> +
> +  switch (ErrorLevel) {
> +    case DEBUG_WARN:
> +      StrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, YELLOW_ESC_SEQ);
> +      break;
> +    case DEBUG_ERROR:
> +      StrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, RED_ESC_SEQ);
> +      break;
> +  }
> +
> +  StrCatS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, String);
> +
> +  switch (ErrorLevel) {
> +    case DEBUG_WARN:
> +    case DEBUG_ERROR:
> +      StrCatS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, END_ESC_SEQ);
> +      break;
> +  }
> +
> +  StrCpyS (String, StringLen, Buffer);
> +
> +  return RETURN_SUCCESS;
> +}
> diff --git a/MdePkg/Library/UefiDebugLibConOut/DebugLib.c b/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
> index 65c8dc2b4654..aac5b6d6e022 100644
> --- a/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
> +++ b/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
> @@ -108,6 +108,10 @@ DebugPrintMarker (
>        UnicodeBSPrintAsciiFormat (Buffer, sizeof (Buffer), Format, BaseListMarker);
>      }
> 
> +    if (FeaturePcdGet (PcdDebugAnsiSeqSupport)) {
> +      UnicodeDebugGetColorString (Buffer, MAX_DEBUG_MESSAGE_LENGTH, ErrorLevel);
> +    }
> +
>      //
>      // Send the print string to the Console Output device
>      //
> --
> 2.30.2