Re: [PATCH V3 13/17] MdeModulePkg/PeiDxeDebugLibReportStatusCode: Add new APIs

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Gao, Liming
> Sent: Thursday, March 21, 2019 2:58 PM
> To: Gao, Zhichao; edk2-devel@lists.01.org
> Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Sean Brogan; Michael Turner;
> Bret Barkelew
> Subject: RE: [PATCH V3 13/17]
> MdeModulePkg/PeiDxeDebugLibReportStatusCode: Add new APIs
> 
> Zhichao:
>   Why do below change? Seemly, this change is not related to add new APIs. In
> fact, current log is added as the purpose by commit
> 137ed15511e2045a7333e33ae7f1e873ce1961dd.

I would suggest to add another separate commit to refine the below
comments to state the message truncating behavior here:

  //
  // If the TotalSize is larger than the maximum record size, then return
                                                              ^^^^^^^^^^^
  //
  if (TotalSize > sizeof (Buffer)) {
    TotalSize = sizeof (Buffer);
  }

Best Regards,
Hao Wu

> 
>   if (TotalSize > sizeof (Buffer)) {
>     TotalSize = sizeof (Buffer);  ==> return;
>   }
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: Gao, Zhichao
> >Sent: Tuesday, March 19, 2019 11:26 PM
> >To: edk2-devel@lists.01.org
> >Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> >Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> ><liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> >Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> ><Bret.Barkelew@microsoft.com>
> >Subject: [PATCH V3 13/17]
> >MdeModulePkg/PeiDxeDebugLibReportStatusCode: Add new APIs
> >
> >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: Jian J Wang <jian.j.wang@intel.com>
> >Cc: Hao Wu <hao.a.wu@intel.com>
> >Cc: Ray Ni <ray.ni@intel.com>
> >Cc: Star Zeng <star.zeng@intel.com>
> >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>
> >---
> > .../PeiDxeDebugLibReportStatusCode/DebugLib.c      | 144
> >++++++++++++++++++---
> > 1 file changed, 128 insertions(+), 16 deletions(-)
> >
> >diff --git
> >a/MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/DebugLib.c
> >b/MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/DebugLib.c
> >index 6f0f416273..f1d31cb619 100644
> >--- a/MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/DebugLib.c
> >+++
> >b/MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/DebugLib.c
> >@@ -4,7 +4,7 @@
> >   Note that if the debug message length is larger than the maximum
> allowable
> >   record length, then the debug message will be ignored directly.
> >
> >-  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> >+  Copyright (c) 2006 - 2019, 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
> >   which accompanies this distribution.  The full text of the license may be
> >found at
> >@@ -27,6 +27,12 @@
> > #include <Library/PcdLib.h>
> > #include <Library/DebugPrintErrorLevelLib.h>
> >
> >+//
> >+// VA_LIST can not initialize to NULL for all compiler, so we use this to
> >+// indicate a null VA_LIST
> >+//
> >+VA_LIST     mVaListNull;
> >+
> > /**
> >   Prints a debug message to the debug output device if the specified error
> >level is enabled.
> >
> >@@ -52,12 +58,43 @@ 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
> >+  )
> > {
> >   UINT64          Buffer[(EFI_STATUS_CODE_DATA_MAX_SIZE / sizeof (UINT64))
> >+ 1];
> >   EFI_DEBUG_INFO  *DebugInfo;
> >   UINTN           TotalSize;
> >-  VA_LIST         VaListMarker;
> >-  BASE_LIST       BaseListMarker;
> >+  BASE_LIST       BaseListMarkerPointer;
> >   CHAR8           *FormatString;
> >   BOOLEAN         Long;
> >
> >@@ -96,7 +133,7 @@ DebugPrint (
> >   // If the TotalSize is larger than the maximum record size, then return
> >   //
> >   if (TotalSize > sizeof (Buffer)) {
> >-    TotalSize = sizeof (Buffer);
> >+    return;
> >   }
> >
> >   //
> >@@ -109,7 +146,7 @@ DebugPrint (
> >   //
> >   DebugInfo             = (EFI_DEBUG_INFO *)(Buffer) + 1;
> >   DebugInfo->ErrorLevel = (UINT32)ErrorLevel;
> >-  BaseListMarker        = (BASE_LIST)(DebugInfo + 1);
> >+  BaseListMarkerPointer = (BASE_LIST)(DebugInfo + 1);
> >   FormatString          = (CHAR8 *)((UINT64 *)(DebugInfo + 1) + 12);
> >
> >   //
> >@@ -125,7 +162,6 @@ DebugPrint (
> >   // of format in DEBUG string, which is followed by the DEBUG format string.
> >   // Here we will process the variable arguments and pack them in this area.
> >   //
> >-  VA_START (VaListMarker, Format);
> >
> >   //
> >   // Use the actual format string.
> >@@ -167,7 +203,11 @@ DebugPrint (
> >         // '*' in format field means the precision of the field is specified by
> >         // a UINTN argument in the argument list.
> >         //
> >-        BASE_ARG (BaseListMarker, UINTN) = VA_ARG (VaListMarker, UINTN);
> >+        if (BaseListMarker == NULL) {
> >+          BASE_ARG (BaseListMarkerPointer, UINTN) = VA_ARG (VaListMarker,
> >UINTN);
> >+        } else {
> >+          BASE_ARG (BaseListMarkerPointer, UINTN) = BASE_ARG
> >(BaseListMarker, UINTN);
> >+        }
> >         continue;
> >       }
> >       if (*Format == '\0') {
> >@@ -192,16 +232,36 @@ DebugPrint (
> >     }
> >     if (*Format == 'p' || *Format == 'X' || *Format == 'x' || *Format == 'd' ||
> >*Format == 'u') {
> >       if (Long) {
> >-        BASE_ARG (BaseListMarker, INT64) = VA_ARG (VaListMarker, INT64);
> >+        if (BaseListMarker == NULL) {
> >+          BASE_ARG (BaseListMarkerPointer, INT64) = VA_ARG (VaListMarker,
> >INT64);
> >+        } else {
> >+          BASE_ARG (BaseListMarkerPointer, INT64) = BASE_ARG
> >(BaseListMarker, INT64);
> >+        }
> >       } else {
> >-        BASE_ARG (BaseListMarker, int) = VA_ARG (VaListMarker, int);
> >+        if (BaseListMarker == NULL) {
> >+          BASE_ARG (BaseListMarkerPointer, int) = VA_ARG (VaListMarker, int);
> >+        } else {
> >+          BASE_ARG (BaseListMarkerPointer, int) = BASE_ARG (BaseListMarker,
> >int);
> >+        }
> >       }
> >     } else if (*Format == 's' || *Format == 'S' || *Format == 'a' || *Format ==
> 'g'
> >|| *Format == 't') {
> >-      BASE_ARG (BaseListMarker, VOID *) = VA_ARG (VaListMarker, VOID *);
> >+      if (BaseListMarker == NULL) {
> >+        BASE_ARG (BaseListMarkerPointer, VOID *) = VA_ARG (VaListMarker,
> >VOID *);
> >+      } else {
> >+        BASE_ARG (BaseListMarkerPointer, VOID *) = BASE_ARG
> >(BaseListMarker, VOID *);
> >+      }
> >     } else if (*Format == 'c') {
> >-      BASE_ARG (BaseListMarker, UINTN) = VA_ARG (VaListMarker, UINTN);
> >+      if (BaseListMarker == NULL) {
> >+        BASE_ARG (BaseListMarkerPointer, UINTN) = VA_ARG (VaListMarker,
> >UINTN);
> >+      } else {
> >+        BASE_ARG (BaseListMarkerPointer, UINTN) = BASE_ARG
> >(BaseListMarker, UINTN);
> >+      }
> >     } else if (*Format == 'r') {
> >-      BASE_ARG (BaseListMarker, RETURN_STATUS) = VA_ARG (VaListMarker,
> >RETURN_STATUS);
> >+      if (BaseListMarker == NULL) {
> >+        BASE_ARG (BaseListMarkerPointer, RETURN_STATUS) = VA_ARG
> >(VaListMarker, RETURN_STATUS);
> >+      } else {
> >+        BASE_ARG (BaseListMarkerPointer, RETURN_STATUS) = BASE_ARG
> >(BaseListMarker, RETURN_STATUS);
> >+      }
> >     }
> >
> >     //
> >@@ -209,17 +269,15 @@ DebugPrint (
> >     // This indicates that the DEBUG() macro is passing in more argument than
> >can be handled by
> >     // the EFI_DEBUG_INFO record
> >     //
> >-    ASSERT ((CHAR8 *)BaseListMarker <= FormatString);
> >+    ASSERT ((CHAR8 *)BaseListMarkerPointer <= FormatString);
> >
> >     //
> >     // If the converted BASE_LIST is larger than the 12 * sizeof (UINT64)
> >allocated bytes, then return
> >     //
> >-    if ((CHAR8 *)BaseListMarker > FormatString) {
> >-      VA_END (VaListMarker);
> >+    if ((CHAR8 *)BaseListMarkerPointer > FormatString) {
> >       return;
> >     }
> >   }
> >-  VA_END (VaListMarker);
> >
> >   //
> >   // Send the DebugInfo record
> >@@ -235,6 +293,60 @@ DebugPrint (
> >     );
> > }
> >
> >+/**
> >+  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.
> >   This may be followed by a breakpoint or a dead loop.
> >--
> >2.16.2.windows.1