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

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

I have mixed the section with IntelFrameworkModulePkg.
Seems the truncating behavior can work fine. And the comment of this section should be updated.

Thanks,
Zhichao

> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, March 21, 2019 3:06 PM
> To: Gao, Liming <liming.gao@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; edk2-devel@lists.01.org
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH V3 13/17]
> MdeModulePkg/PeiDxeDebugLibReportStatusCode: Add new APIs
> 
> > -----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