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

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

Hi Rebecca,

In the edk2 repo, I see the following instances of the DebugPrintMarker() API
where you are currently adding he ANSI color sequences.  Should all of these
be updated?  I did not review the edk2-platforms repo.  And there may be
downstream custom DebugLib instances.  It would better if this feature could 
be enabled for all existing DebugLib instances, but the only common location
is the DEBUG() macro definition in MdePkg/Include/Library/DebugLib.h and 
adding code to that macro adds statements to the module calling the DebugLib
services, which can increase code size.

ArmPkg\Library\SemiHostingDebugLib\DebugLib.c:
   75  VOID
   76: DebugPrintMarker (
   77    IN  UINTN        ErrorLevel,

IntelFsp2Pkg\Library\BaseFspDebugLibSerialPort\DebugLib.c:
   89  VOID
   90: DebugPrintMarker (
   91    IN  UINTN        ErrorLevel,

MdeModulePkg\Library\PeiDxeDebugLibReportStatusCode\DebugLib.c:
   85  VOID
   86: DebugPrintMarker (
   87    IN  UINTN        ErrorLevel,

MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:
   97  VOID
   98: DebugPrintMarker (
   99    IN  UINTN        ErrorLevel,

MdePkg\Library\DxeRuntimeDebugLibSerialPort\DebugLib.c:
  156  VOID
  157: DebugPrintMarker (
  158    IN  UINTN        ErrorLevel,

MdePkg\Library\UefiDebugLibConOut\DebugLib.c:
   79  VOID
   80: DebugPrintMarker (
   81    IN  UINTN        ErrorLevel,

MdePkg\Library\UefiDebugLibDebugPortProtocol\DebugLib.c:
  137  VOID
  138: DebugPrintMarker (
  139    IN  UINTN        ErrorLevel,

MdePkg\Library\UefiDebugLibStdErr\DebugLib.c:
   79  VOID
   80: DebugPrintMarker (
   81    IN  UINTN        ErrorLevel,

OvmfPkg\Library\PlatformDebugLibIoPort\DebugLib.c:
   79  VOID
   80: DebugPrintMarker (
   81    IN  UINTN        ErrorLevel,

Also, the ErrorLevel parameter is a bitmask.  It cannot be used in
a switch/case statement for only 1 bit being set.  To test for a 
debug message of type ERROR or WARN, a check must be done for that
one bit being set in ErrorLevel.

The DebugLib also provides support for ASSERT() macros.  Should 
ASSERT() messages have the same color as an ERROR message?  Or
its own color?

The logic below changes the color for the message based on message
type.  But is does not save/restore the current ANSI color setting.
It see it using the END_ESC_SEQ.  Does that put the color settings
back to the previous setting?

Thanks,

Mike

> -----Original Message-----
> From: Rebecca Cran <rebecca@quicinc.com>
> Sent: Thursday, October 27, 2022 11:51 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 v4 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/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf |  2 +-
>  MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf         |  2 +-
>  MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c                 | 64 ++++++++++++++++++++
>  MdePkg/Library/UefiDebugLibConOut/DebugLib.c                     | 64 ++++++++++++++++++++
>  5 files changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index 4c81cbd75ab2..8ddc46b62e7d 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>
> @@ -1977,6 +1978,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/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/Library/BaseDebugLibSerialPort/DebugLib.c b/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
> index bd5686947712..df31bd1ffb9f 100644
> --- a/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
> +++ b/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
> @@ -26,6 +26,10 @@
>  //
>  #define MAX_DEBUG_MESSAGE_LENGTH  0x100
> 
> +#define RED_ESC_SEQ     "\033[31m"
> +#define YELLOW_ESC_SEQ  "\033[33m"
> +#define END_ESC_SEQ     "\033[0m"
> +
>  //
>  // VA_LIST can not initialize to NULL for all compiler, so we use this to
>  // indicate a null VA_LIST
> @@ -77,6 +81,62 @@ DebugPrint (
>    VA_END (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.
> +
> +**/
> +STATIC
> +RETURN_STATUS
> +AsciiDebugGetColorString (
> +  IN OUT CHAR8  *String,
> +  IN UINTN      StringLen,
> +  IN UINTN      ErrorLevel
> +  )
> +{
> +  CHAR8  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> +  UINTN  ReqBufferLen;
> +
> +  ReqBufferLen = AsciiStrLen (String) +
> +                 AsciiStrLen (RED_ESC_SEQ) +
> +                 AsciiStrLen (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, YELLOW_ESC_SEQ);
> +      break;
> +    case DEBUG_ERROR:
> +      AsciiStrCpyS (Buffer, MAX_DEBUG_MESSAGE_LENGTH, 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, END_ESC_SEQ);
> +      break;
> +  }
> +
> +  AsciiStrCpyS (String, StringLen, Buffer);
> +
> +  return RETURN_SUCCESS;
> +}
> +
>  /**
>    Prints a debug message to the debug output device if the specified
>    error level is enabled base on Null-terminated format string and a
> @@ -125,6 +185,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/UefiDebugLibConOut/DebugLib.c b/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
> index 65c8dc2b4654..521298a7c8a7 100644
> --- a/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
> +++ b/MdePkg/Library/UefiDebugLibConOut/DebugLib.c
> @@ -20,6 +20,10 @@
>  //
>  #define MAX_DEBUG_MESSAGE_LENGTH  0x100
> 
> +#define RED_ESC_SEQ     L"\033[31m"
> +#define YELLOW_ESC_SEQ  L"\033[33m"
> +#define END_ESC_SEQ     L"\033[0m"
> +
>  //
>  // VA_LIST can not initialize to NULL for all compiler, so we use this to
>  // indicate a null VA_LIST
> @@ -59,6 +63,62 @@ DebugPrint (
>    VA_END (Marker);
>  }
> 
> +/**
> +  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.
> +
> +**/
> +STATIC
> +RETURN_STATUS
> +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;
> +}
> +
>  /**
>    Prints a debug message to the debug output device if the specified
>    error level is enabled base on Null-terminated format string and a
> @@ -108,6 +168,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