Re: [PATCH V4 08/17] ArmPkg/SemiHostingDebugLib: Add new APIs

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

Hi Zhichao,

Apologies for delay in responding, due to holiday and stuff.

On the whole this looks fine (one comment below), but I don't actually
have any platform on which to test this.

Ard: maybe it's time to retire this component?

On Thu, Mar 21, 2019 at 10:04:50PM +0800, Zhichao Gao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1395
> 
> Add new APIs' implementation (DebugVPrint, DebugBPrint)
> in the DebugLib instance. These APIs would expose print
> routines with VaList parameter and BaseList parameter.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> ---
>  ArmPkg/Library/SemiHostingDebugLib/DebugLib.c | 106 ++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmPkg/Library/SemiHostingDebugLib/DebugLib.c b/ArmPkg/Library/SemiHostingDebugLib/DebugLib.c
> index ec03edb774..a368dd43b8 100644
> --- a/ArmPkg/Library/SemiHostingDebugLib/DebugLib.c
> +++ b/ArmPkg/Library/SemiHostingDebugLib/DebugLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    UEFI Debug Library that uses PrintLib to send messages to STDERR.
>  
> -  Copyright (c) 2006 - 2007, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>    Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
> @@ -27,6 +27,12 @@
>  //
>  #define MAX_DEBUG_MESSAGE_LENGTH  0x100
>  
> +//
> +// VA_LIST can not initialize to NULL for all compiler, so we use this to
> +// indicate a null VA_LIST
> +//
> +VA_LIST     mVaListNull;

I would prefer if this was marked STATIC.

If you feel strongly otherwise, leave it as is.
Either way:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> +
>  /**
>  
>    Prints a debug message to the debug output device if the specified error level is enabled.
> @@ -48,9 +54,41 @@ DebugPrint (
>    IN  CONST CHAR8  *Format,
>    ...
>    )
> +{
> +  VA_LIST         Marker;
> +
> +  VA_START (Marker, Format);
> +  DebugVPrint (ErrorLevel, Format, Marker);
> +  VA_END (Marker);
> +}
> +
> +
> +/**
> +  Prints a debug message to the debug output device if the specified
> +  error level is enabled base on Null-terminated format string and a
> +  VA_LIST argument list or a BASE_LIST argument list.
> +
> +  If any bit in ErrorLevel is also set in DebugPrintErrorLevelLib function
> +  GetDebugPrintErrorLevel (), then print the message specified by Format and
> +  the associated variable argument list to the debug output device.
> +
> +  If Format is NULL, then ASSERT().
> +
> +  @param  ErrorLevel      The error level of the debug message.
> +  @param  Format          Format string for the debug message to print.
> +  @param  VaListMarker    VA_LIST marker for the variable argument list.
> +  @param  BaseListMarker  BASE_LIST marker for the variable argument list.
> +
> +**/
> +VOID
> +DebugPrintMarker (
> +  IN  UINTN         ErrorLevel,
> +  IN  CONST CHAR8   *Format,
> +  IN  VA_LIST       VaListMarker,
> +  IN  BASE_LIST     BaseListMarker
> +  )
>  {
>    CHAR8    AsciiBuffer[MAX_DEBUG_MESSAGE_LENGTH];
> -  VA_LIST  Marker;
>  
>    //
>    // If Format is NULL, then ASSERT().
> @@ -67,14 +105,72 @@ DebugPrint (
>    //
>    // Convert the DEBUG() message to a Unicode String
>    //
> -  VA_START (Marker, Format);
> -  AsciiVSPrint (AsciiBuffer, sizeof (AsciiBuffer), Format, Marker);
> -  VA_END (Marker);
> +  if (BaseListMarker == NULL) {
> +    AsciiVSPrint (AsciiBuffer, sizeof (AsciiBuffer), Format, VaListMarker);
> +  } else {
> +    AsciiBSPrint (AsciiBuffer, sizeof (AsciiBuffer), Format, BaseListMarker);
> +  }
>  
>    SemihostWriteString (AsciiBuffer);
>  }
>  
>  
> +/**
> +  Prints a debug message to the debug output device if the specified
> +  error level is enabled.
> +
> +  If any bit in ErrorLevel is also set in DebugPrintErrorLevelLib function
> +  GetDebugPrintErrorLevel (), then print the message specified by Format and
> +  the associated variable argument list to the debug output device.
> +
> +  If Format is NULL, then ASSERT().
> +
> +  @param  ErrorLevel    The error level of the debug message.
> +  @param  Format        Format string for the debug message to print.
> +  @param  VaListMarker  VA_LIST marker for the variable argument list.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugVPrint (
> +  IN  UINTN         ErrorLevel,
> +  IN  CONST CHAR8   *Format,
> +  IN  VA_LIST       VaListMarker
> +  )
> +{
> +  DebugPrintMarker (ErrorLevel, Format, VaListMarker, NULL);
> +}
> +
> +
> +/**
> +  Prints a debug message to the debug output device if the specified
> +  error level is enabled.
> +  This function use BASE_LIST which would provide a more compatible
> +  service than VA_LIST.
> +
> +  If any bit in ErrorLevel is also set in DebugPrintErrorLevelLib function
> +  GetDebugPrintErrorLevel (), then print the message specified by Format and
> +  the associated variable argument list to the debug output device.
> +
> +  If Format is NULL, then ASSERT().
> +
> +  @param  ErrorLevel      The error level of the debug message.
> +  @param  Format          Format string for the debug message to print.
> +  @param  BaseListMarker  BASE_LIST marker for the variable argument list.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugBPrint (
> +  IN  UINTN         ErrorLevel,
> +  IN  CONST CHAR8   *Format,
> +  IN  BASE_LIST     BaseListMarker
> +  )
> +{
> +  DebugPrintMarker (ErrorLevel, Format, mVaListNull, BaseListMarker);
> +}
> +
> +
>  /**
>  
>    Prints an assert message containing a filename, line number, and description.
> -- 
> 2.16.2.windows.1
>