[edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

[edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

[Jeshua Smith via groups.io]

While using the ConfigurationManagerObjectParser to dump objects and debug
adding new objects, I noticed some bugs and deficiencies. This series is
intended to address those.

Jeshua Smith (2):
  DynamicTablesPkg/TableHelperLib: Fix and improve text handling
  DynamicTablesPkg/TableHelperLib: Enhance error handling

 .../ConfigurationManagerObjectParser.c        | 223 ++++++++++++++----
 1 file changed, 176 insertions(+), 47 deletions(-)

-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109363): https://edk2.groups.io/g/devel/message/109363
Mute This Topic: https://groups.io/mt/101801382/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling

[Jeshua Smith via groups.io]

This fixes two bugs and adds some enhancements to the handling of
characters and strings in objects being printed by the CM ObjectParser.

Bug fixes:
1. PrintOemID() currently attempts to print characters with "%C",
   but the correct syntax is (lowercase) "%c". This bug results in
   "CCCCCC" being printed instead of the actual ASCII characters.
2. PrintString() is being passed a pointer to data in objects, but in
   some cases this data is the actual string to print and other cases
   it is a pointer to the string to print. This adds a PrintStringPtr
   function and uses the correct functions depending on the situation.

Enhancements:
1. Some objects contain ASCII characters, which are currently printed
   as their hex values. This adds functions to print out ASCII
   character fields as text rather than hex, and uses those functions in
   several cases where the object data is defined to be ASCII.
2. The PrintOemID() function is replaced with the new identical but more
   generecically-named PrintChar6() function.

Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
---
 .../ConfigurationManagerObjectParser.c        | 176 ++++++++++++++----
 1 file changed, 143 insertions(+), 33 deletions(-)

diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
index 99d6032510..92df1efee8 100644
--- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
@@ -14,7 +14,7 @@
 STATIC
 VOID
 EFIAPI
-PrintOemId (
+PrintString (
   CONST CHAR8  *Format,
   UINT8        *Ptr
   );
@@ -22,7 +22,31 @@ PrintOemId (
 STATIC
 VOID
 EFIAPI
-PrintString (
+PrintStringPtr (
+  CONST CHAR8  *Format,
+  UINT8        *Ptr
+  );
+
+STATIC
+VOID
+EFIAPI
+PrintChar4 (
+  CONST CHAR8  *Format,
+  UINT8        *Ptr
+  );
+
+STATIC
+VOID
+EFIAPI
+PrintChar6 (
+  CONST CHAR8  *Format,
+  UINT8        *Ptr
+  );
+
+STATIC
+VOID
+EFIAPI
+PrintChar8 (
   CONST CHAR8  *Format,
   UINT8        *Ptr
   );
@@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER  CmArmItsGroupNodeParser[] = {
 /** A parser for EArmObjNamedComponent.
 */
 STATIC CONST CM_OBJ_PARSER  CmArmNamedComponentNodeParser[] = {
-  { "Token",             sizeof (CM_OBJECT_TOKEN), "0x%p", NULL        },
-  { "IdMappingCount",    4,                        "0x%x", NULL        },
-  { "IdMappingToken",    sizeof (CM_OBJECT_TOKEN), "0x%p", NULL        },
-  { "Flags",             4,                        "0x%x", NULL        },
-  { "CacheCoherent",     4,                        "0x%x", NULL        },
-  { "AllocationHints",   1,                        "0x%x", NULL        },
-  { "MemoryAccessFlags", 1,                        "0x%x", NULL        },
-  { "AddressSizeLimit",  1,                        "0x%x", NULL        },
-  { "ObjectName",        sizeof (CHAR8 *),         NULL,   PrintString },
-  { "Identifier",        4,                        "0x%x", NULL        },
+  { "Token",             sizeof (CM_OBJECT_TOKEN), "0x%p", NULL           },
+  { "IdMappingCount",    4,                        "0x%x", NULL           },
+  { "IdMappingToken",    sizeof (CM_OBJECT_TOKEN), "0x%p", NULL           },
+  { "Flags",             4,                        "0x%x", NULL           },
+  { "CacheCoherent",     4,                        "0x%x", NULL           },
+  { "AllocationHints",   1,                        "0x%x", NULL           },
+  { "MemoryAccessFlags", 1,                        "0x%x", NULL           },
+  { "AddressSizeLimit",  1,                        "0x%x", NULL           },
+  { "ObjectName",        sizeof (CHAR8 *),         NULL,   PrintStringPtr },
+  { "Identifier",        4,                        "0x%x", NULL           },
 };
 
 /** A parser for EArmObjRootComplex.
@@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  ArmNamespaceObjectParser[] = {
 */
 STATIC CONST CM_OBJ_PARSER  StdObjCfgMgrInfoParser[] = {
   { "Revision", 4, "0x%x",         NULL       },
-  { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId }
+  { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 }
 };
 
 /** A parser for EStdObjAcpiTableList.
 */
 STATIC CONST CM_OBJ_PARSER  StdObjAcpiTableInfoParser[] = {
-  { "AcpiTableSignature", 4,                                      "0x%x",   NULL },
-  { "AcpiTableRevision",  1,                                      "%d",     NULL },
-  { "TableGeneratorId",   sizeof (ACPI_TABLE_GENERATOR_ID),       "0x%x",   NULL },
-  { "AcpiTableData",      sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p",   NULL },
-  { "OemTableId",         8,                                      "0x%LLX", NULL },
-  { "OemRevision",        4,                                      "0x%x",   NULL },
-  { "MinorRevision",      1,                                      "0x%x",   NULL },
+  { "AcpiTableSignature", 4,                                      "%c%c%c%c",         PrintChar4 },
+  { "AcpiTableRevision",  1,                                      "%d",               NULL       },
+  { "TableGeneratorId",   sizeof (ACPI_TABLE_GENERATOR_ID),       "0x%x",             NULL       },
+  { "AcpiTableData",      sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p",             NULL       },
+  { "OemTableId",         8,                                      "%c%c%c%c%c%c%c%c", PrintChar8 },
+  { "OemRevision",        4,                                      "0x%x",             NULL       },
+  { "MinorRevision",      1,                                      "0x%x",             NULL       },
 };
 
 /** A parser for EStdObjSmbiosTableList.
@@ -773,22 +797,99 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
     ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
 };
 
-/** Print OEM Id.
+/** Print string data.
+
+  The string must be NULL terminated.
+
+  @param [in]  Format  Format to print the Ptr.
+  @param [in]  Ptr     Pointer to the string.
+**/
+STATIC
+VOID
+EFIAPI
+PrintString (
+  IN CONST CHAR8  *Format,
+  IN UINT8        *Ptr
+  )
+{
+  if (Ptr == NULL) {
+    ASSERT (0);
+    return;
+  }
+
+  DEBUG ((DEBUG_ERROR, "%a", Ptr));
+}
+
+/** Print string from pointer.
+
+  The string must be NULL terminated.
+
+  @param [in]  Format      Format to print the string.
+  @param [in]  Ptr         Pointer to the string pointer.
+**/
+STATIC
+VOID
+EFIAPI
+PrintStringPtr (
+  IN CONST CHAR8  *Format,
+  IN UINT8        *Ptr
+  )
+{
+  UINT8  *String;
+
+  if (Ptr == NULL) {
+    ASSERT (0);
+    return;
+  }
+
+  String = *(UINT8 **)Ptr;
+
+  if (String == NULL) {
+    String = (UINT8 *)"(NULLPTR)";
+  }
+
+  PrintString (Format, String);
+}
+
+/** Print 4 characters.
 
   @param [in]  Format  Format to print the Ptr.
-  @param [in]  Ptr     Pointer to the OEM Id.
+  @param [in]  Ptr     Pointer to the characters.
 **/
 STATIC
 VOID
 EFIAPI
-PrintOemId (
+PrintChar4 (
   IN  CONST CHAR8  *Format,
   IN  UINT8        *Ptr
   )
 {
   DEBUG ((
-    DEBUG_INFO,
-    (Format != NULL) ? Format : "%C%C%C%C%C%C",
+    DEBUG_ERROR,
+    (Format != NULL) ? Format : "%c%c%c%c",
+    Ptr[0],
+    Ptr[1],
+    Ptr[2],
+    Ptr[3]
+    ));
+}
+
+/** Print 6 characters.
+
+  @param [in]  Format  Format to print the Ptr.
+  @param [in]  Ptr     Pointer to the characters.
+**/
+STATIC
+VOID
+EFIAPI
+PrintChar6 (
+  IN  CONST CHAR8  *Format,
+  IN  UINT8        *Ptr
+  )
+{
+  DEBUG ((
+    DEBUG_ERROR,
+    (Format != NULL) ? Format : "%c%c%c%c%c%c",
     Ptr[0],
     Ptr[1],
     Ptr[2],
@@ -798,22 +899,31 @@ PrintOemId (
     ));
 }
 
-/** Print string.
-
-  The string must be NULL terminated.
+/** Print 8 characters.
 
   @param [in]  Format  Format to print the Ptr.
-  @param [in]  Ptr     Pointer to the string.
+  @param [in]  Ptr     Pointer to the characters.
 **/
 STATIC
 VOID
 EFIAPI
-PrintString (
-  CONST CHAR8  *Format,
-  UINT8        *Ptr
+PrintChar8 (
+  IN  CONST CHAR8  *Format,
+  IN  UINT8        *Ptr
   )
 {
-  DEBUG ((DEBUG_ERROR, "%a", Ptr));
+  DEBUG ((
+    DEBUG_ERROR,
+    (Format != NULL) ? Format : "%c%c%c%c%c%c%c%c",
+    Ptr[0],
+    Ptr[1],
+    Ptr[2],
+    Ptr[3],
+    Ptr[4],
+    Ptr[5],
+    Ptr[6],
+    Ptr[7]
+    ));
 }
 
 /** Print fields of the objects.
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109365): https://edk2.groups.io/g/devel/message/109365
Mute This Topic: https://groups.io/mt/101801390/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

[Jeshua Smith via groups.io]

This patch enhances error handling and reporting in the CM ObjectParser.
Specifically:
1. ObjectIDs used as array indexes are checked for being out of bounds,
   and if so an error message is printed before the assert.
2. An error message is printed for unsupported NameSpaceIDs.
3. Adds support for unimplemented parsers by allowing IDs to list a
   NULL parser, resulting in an unimplemented message being printed.

Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
---
 .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
index 92df1efee8..22b8fdb906 100644
--- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
@@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
     ARRAY_SIZE (StdObjAcpiTableInfoParser) },
   { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
     ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
+  { "EStdObjMax",             NULL,                       0}
 };
 
 /** Print string data.
@@ -1066,6 +1067,12 @@ ParseCmObjDesc (
         return;
       }
 
+      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
+        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
+        ASSERT (0);
+        return;
+      }
+
       ParserArray = &StdNamespaceObjectParser[ObjId];
       break;
     case EObjNameSpaceArm:
@@ -1074,10 +1081,17 @@ ParseCmObjDesc (
         return;
       }
 
+      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
+        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
+        ASSERT (0);
+        return;
+      }
+
       ParserArray = &ArmNamespaceObjectParser[ObjId];
       break;
     default:
       // Not supported
+      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not supported by the parser\n", NameSpaceId, ObjId));
       ASSERT (0);
       return;
   } // switch
@@ -1095,21 +1109,26 @@ ParseCmObjDesc (
       ObjIndex + 1,
       ObjectCount
       ));
-    PrintCmObjDesc (
-      (VOID *)((UINTN)CmObjDesc->Data + Offset),
-      ParserArray->Parser,
-      ParserArray->ItemCount,
-      &RemainingSize,
-      1
-      );
-    if ((RemainingSize > CmObjDesc->Size) ||
-        (RemainingSize < 0))
-    {
-      ASSERT (0);
-      return;
-    }
+    if (ParserArray->Parser == NULL) {
+      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
+      RemainingSize = 0;
+    } else {
+      PrintCmObjDesc (
+        (VOID *)((UINTN)CmObjDesc->Data + Offset),
+        ParserArray->Parser,
+        ParserArray->ItemCount,
+        &RemainingSize,
+        1
+        );
+      if ((RemainingSize > CmObjDesc->Size) ||
+          (RemainingSize < 0))
+      {
+        ASSERT (0);
+        return;
+      }
 
-    Offset = CmObjDesc->Size - RemainingSize;
+      Offset = CmObjDesc->Size - RemainingSize;
+    }
   } // for
 
   ASSERT (RemainingSize == 0);
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109364): https://edk2.groups.io/g/devel/message/109364
Mute This Topic: https://groups.io/mt/101801385/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

[PierreGondois]

Hello Jeshua,

On 10/6/23 18:28, Jeshua Smith wrote:
> This patch enhances error handling and reporting in the CM ObjectParser.
> Specifically:
> 1. ObjectIDs used as array indexes are checked for being out of bounds,
>     and if so an error message is printed before the assert.
> 2. An error message is printed for unsupported NameSpaceIDs.
> 3. Adds support for unimplemented parsers by allowing IDs to list a
>     NULL parser, resulting in an unimplemented message being printed.

I am not sure I see in which context 3. would be used/necessary. Is it possible
to detail ?

(Code-wise everything looks good to me)

Regards,
Pierre


> 
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> ---
>   .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
>   1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> index 92df1efee8..22b8fdb906 100644
> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>       ARRAY_SIZE (StdObjAcpiTableInfoParser) },
>     { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
>       ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
> +  { "EStdObjMax",             NULL,                       0}
>   };
>   
>   /** Print string data.
> @@ -1066,6 +1067,12 @@ ParseCmObjDesc (
>           return;
>         }
>   
> +      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &StdNamespaceObjectParser[ObjId];
>         break;
>       case EObjNameSpaceArm:
> @@ -1074,10 +1081,17 @@ ParseCmObjDesc (
>           return;
>         }
>   
> +      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &ArmNamespaceObjectParser[ObjId];
>         break;
>       default:
>         // Not supported
> +      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not supported by the parser\n", NameSpaceId, ObjId));
>         ASSERT (0);
>         return;
>     } // switch
> @@ -1095,21 +1109,26 @@ ParseCmObjDesc (
>         ObjIndex + 1,
>         ObjectCount
>         ));
> -    PrintCmObjDesc (
> -      (VOID *)((UINTN)CmObjDesc->Data + Offset),
> -      ParserArray->Parser,
> -      ParserArray->ItemCount,
> -      &RemainingSize,
> -      1
> -      );
> -    if ((RemainingSize > CmObjDesc->Size) ||
> -        (RemainingSize < 0))
> -    {
> -      ASSERT (0);
> -      return;
> -    }
> +    if (ParserArray->Parser == NULL) {
> +      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
> +      RemainingSize = 0;
> +    } else {
> +      PrintCmObjDesc (
> +        (VOID *)((UINTN)CmObjDesc->Data + Offset),
> +        ParserArray->Parser,
> +        ParserArray->ItemCount,
> +        &RemainingSize,
> +        1
> +        );
> +      if ((RemainingSize > CmObjDesc->Size) ||
> +          (RemainingSize < 0))
> +      {
> +        ASSERT (0);
> +        return;
> +      }
>   
> -    Offset = CmObjDesc->Size - RemainingSize;
> +      Offset = CmObjDesc->Size - RemainingSize;
> +    }
>     } // for
>   
>     ASSERT (RemainingSize == 0);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109486): https://edk2.groups.io/g/devel/message/109486
Mute This Topic: https://groups.io/mt/101801385/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling

[PierreGondois]

Hello Jeshua,
Just a small remark, but this should be equivalent, so:

Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

On 10/6/23 18:28, Jeshua Smith wrote:
> This fixes two bugs and adds some enhancements to the handling of
> characters and strings in objects being printed by the CM ObjectParser.
> 
> Bug fixes:
> 1. PrintOemID() currently attempts to print characters with "%C",
>     but the correct syntax is (lowercase) "%c". This bug results in
>     "CCCCCC" being printed instead of the actual ASCII characters.
> 2. PrintString() is being passed a pointer to data in objects, but in
>     some cases this data is the actual string to print and other cases
>     it is a pointer to the string to print. This adds a PrintStringPtr
>     function and uses the correct functions depending on the situation.
> 
> Enhancements:
> 1. Some objects contain ASCII characters, which are currently printed
>     as their hex values. This adds functions to print out ASCII
>     character fields as text rather than hex, and uses those functions in
>     several cases where the object data is defined to be ASCII.
> 2. The PrintOemID() function is replaced with the new identical but more
>     generecically-named PrintChar6() function.
> 
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> ---
>   .../ConfigurationManagerObjectParser.c        | 176 ++++++++++++++----
>   1 file changed, 143 insertions(+), 33 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> index 99d6032510..92df1efee8 100644
> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> @@ -14,7 +14,7 @@
>   STATIC
>   VOID
>   EFIAPI
> -PrintOemId (
> +PrintString (
>     CONST CHAR8  *Format,
>     UINT8        *Ptr
>     );
> @@ -22,7 +22,31 @@ PrintOemId (
>   STATIC
>   VOID
>   EFIAPI
> -PrintString (
> +PrintStringPtr (
> +  CONST CHAR8  *Format,
> +  UINT8        *Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar4 (
> +  CONST CHAR8  *Format,
> +  UINT8        *Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar6 (
> +  CONST CHAR8  *Format,
> +  UINT8        *Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar8 (
>     CONST CHAR8  *Format,
>     UINT8        *Ptr
>     );
> @@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER  CmArmItsGroupNodeParser[] = {
>   /** A parser for EArmObjNamedComponent.
>   */
>   STATIC CONST CM_OBJ_PARSER  CmArmNamedComponentNodeParser[] = {
> -  { "Token",             sizeof (CM_OBJECT_TOKEN), "0x%p", NULL        },
> -  { "IdMappingCount",    4,                        "0x%x", NULL        },
> -  { "IdMappingToken",    sizeof (CM_OBJECT_TOKEN), "0x%p", NULL        },
> -  { "Flags",             4,                        "0x%x", NULL        },
> -  { "CacheCoherent",     4,                        "0x%x", NULL        },
> -  { "AllocationHints",   1,                        "0x%x", NULL        },
> -  { "MemoryAccessFlags", 1,                        "0x%x", NULL        },
> -  { "AddressSizeLimit",  1,                        "0x%x", NULL        },
> -  { "ObjectName",        sizeof (CHAR8 *),         NULL,   PrintString },
> -  { "Identifier",        4,                        "0x%x", NULL        },
> +  { "Token",             sizeof (CM_OBJECT_TOKEN), "0x%p", NULL           },
> +  { "IdMappingCount",    4,                        "0x%x", NULL           },
> +  { "IdMappingToken",    sizeof (CM_OBJECT_TOKEN), "0x%p", NULL           },
> +  { "Flags",             4,                        "0x%x", NULL           },
> +  { "CacheCoherent",     4,                        "0x%x", NULL           },
> +  { "AllocationHints",   1,                        "0x%x", NULL           },
> +  { "MemoryAccessFlags", 1,                        "0x%x", NULL           },
> +  { "AddressSizeLimit",  1,                        "0x%x", NULL           },
> +  { "ObjectName",        sizeof (CHAR8 *),         NULL,   PrintStringPtr },
> +  { "Identifier",        4,                        "0x%x", NULL           },
>   };
>   
>   /** A parser for EArmObjRootComplex.
> @@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  ArmNamespaceObjectParser[] = {
>   */
>   STATIC CONST CM_OBJ_PARSER  StdObjCfgMgrInfoParser[] = {
>     { "Revision", 4, "0x%x",         NULL       },
> -  { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId }
> +  { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 }

NIT:
Is it necessary, since "%c%c%c%c%c%c" is the default format of PrintChar6() ?

>   };
>   
>   /** A parser for EStdObjAcpiTableList.
>   */
>   STATIC CONST CM_OBJ_PARSER  StdObjAcpiTableInfoParser[] = {
> -  { "AcpiTableSignature", 4,                                      "0x%x",   NULL },
> -  { "AcpiTableRevision",  1,                                      "%d",     NULL },
> -  { "TableGeneratorId",   sizeof (ACPI_TABLE_GENERATOR_ID),       "0x%x",   NULL },
> -  { "AcpiTableData",      sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p",   NULL },
> -  { "OemTableId",         8,                                      "0x%LLX", NULL },
> -  { "OemRevision",        4,                                      "0x%x",   NULL },
> -  { "MinorRevision",      1,                                      "0x%x",   NULL },
> +  { "AcpiTableSignature", 4,                                      "%c%c%c%c",         PrintChar4 },
> +  { "AcpiTableRevision",  1,                                      "%d",               NULL       },
> +  { "TableGeneratorId",   sizeof (ACPI_TABLE_GENERATOR_ID),       "0x%x",             NULL       },
> +  { "AcpiTableData",      sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p",             NULL       },
> +  { "OemTableId",         8,                                      "%c%c%c%c%c%c%c%c", PrintChar8 },
> +  { "OemRevision",        4,                                      "0x%x",             NULL       },
> +  { "MinorRevision",      1,                                      "0x%x",             NULL       },
>   };
>   
>   /** A parser for EStdObjSmbiosTableList.
> @@ -773,22 +797,99 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>       ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
>   };
>   
> -/** Print OEM Id.
> +/** Print string data.
> +
> +  The string must be NULL terminated.
> +
> +  @param [in]  Format  Format to print the Ptr.
> +  @param [in]  Ptr     Pointer to the string.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintString (
> +  IN CONST CHAR8  *Format,
> +  IN UINT8        *Ptr
> +  )
> +{
> +  if (Ptr == NULL) {
> +    ASSERT (0);
> +    return;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "%a", Ptr));
> +}
> +
> +/** Print string from pointer.
> +
> +  The string must be NULL terminated.
> +
> +  @param [in]  Format      Format to print the string.
> +  @param [in]  Ptr         Pointer to the string pointer.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintStringPtr (
> +  IN CONST CHAR8  *Format,
> +  IN UINT8        *Ptr
> +  )
> +{
> +  UINT8  *String;
> +
> +  if (Ptr == NULL) {
> +    ASSERT (0);
> +    return;
> +  }
> +
> +  String = *(UINT8 **)Ptr;
> +
> +  if (String == NULL) {
> +    String = (UINT8 *)"(NULLPTR)";
> +  }
> +
> +  PrintString (Format, String);
> +}
> +
> +/** Print 4 characters.
>   
>     @param [in]  Format  Format to print the Ptr.
> -  @param [in]  Ptr     Pointer to the OEM Id.
> +  @param [in]  Ptr     Pointer to the characters.
>   **/
>   STATIC
>   VOID
>   EFIAPI
> -PrintOemId (
> +PrintChar4 (
>     IN  CONST CHAR8  *Format,
>     IN  UINT8        *Ptr
>     )
>   {
>     DEBUG ((
> -    DEBUG_INFO,
> -    (Format != NULL) ? Format : "%C%C%C%C%C%C",
> +    DEBUG_ERROR,
> +    (Format != NULL) ? Format : "%c%c%c%c",
> +    Ptr[0],
> +    Ptr[1],
> +    Ptr[2],
> +    Ptr[3]
> +    ));
> +}
> +
> +/** Print 6 characters.
> +
> +  @param [in]  Format  Format to print the Ptr.
> +  @param [in]  Ptr     Pointer to the characters.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar6 (
> +  IN  CONST CHAR8  *Format,
> +  IN  UINT8        *Ptr
> +  )
> +{
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    (Format != NULL) ? Format : "%c%c%c%c%c%c",
>       Ptr[0],
>       Ptr[1],
>       Ptr[2],
> @@ -798,22 +899,31 @@ PrintOemId (
>       ));
>   }
>   
> -/** Print string.
> -
> -  The string must be NULL terminated.
> +/** Print 8 characters.
>   
>     @param [in]  Format  Format to print the Ptr.
> -  @param [in]  Ptr     Pointer to the string.
> +  @param [in]  Ptr     Pointer to the characters.
>   **/
>   STATIC
>   VOID
>   EFIAPI
> -PrintString (
> -  CONST CHAR8  *Format,
> -  UINT8        *Ptr
> +PrintChar8 (
> +  IN  CONST CHAR8  *Format,
> +  IN  UINT8        *Ptr
>     )
>   {
> -  DEBUG ((DEBUG_ERROR, "%a", Ptr));
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    (Format != NULL) ? Format : "%c%c%c%c%c%c%c%c",
> +    Ptr[0],
> +    Ptr[1],
> +    Ptr[2],
> +    Ptr[3],
> +    Ptr[4],
> +    Ptr[5],
> +    Ptr[6],
> +    Ptr[7]
> +    ));
>   }
>   
>   /** Print fields of the objects.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109487): https://edk2.groups.io/g/devel/message/109487
Mute This Topic: https://groups.io/mt/101801390/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling

[Jeshua Smith via groups.io]

Thanks for the review. Reply inline.

-----Original Message-----
From: Pierre Gondois <pierre.gondois@arm.com> 
Sent: Tuesday, October 10, 2023 4:14 AM
To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com
Subject: Re: [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling

External email: Use caution opening links or attachments


Hello Jeshua,
Just a small remark, but this should be equivalent, so:

Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

On 10/6/23 18:28, Jeshua Smith wrote:
> This fixes two bugs and adds some enhancements to the handling of 
> characters and strings in objects being printed by the CM ObjectParser.
>
> Bug fixes:
> 1. PrintOemID() currently attempts to print characters with "%C",
>     but the correct syntax is (lowercase) "%c". This bug results in
>     "CCCCCC" being printed instead of the actual ASCII characters.
> 2. PrintString() is being passed a pointer to data in objects, but in
>     some cases this data is the actual string to print and other cases
>     it is a pointer to the string to print. This adds a PrintStringPtr
>     function and uses the correct functions depending on the situation.
>
> Enhancements:
> 1. Some objects contain ASCII characters, which are currently printed
>     as their hex values. This adds functions to print out ASCII
>     character fields as text rather than hex, and uses those functions in
>     several cases where the object data is defined to be ASCII.
> 2. The PrintOemID() function is replaced with the new identical but more
>     generecically-named PrintChar6() function.
>
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> ---
>   .../ConfigurationManagerObjectParser.c        | 176 ++++++++++++++----
>   1 file changed, 143 insertions(+), 33 deletions(-)
>
> diff --git 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c 
> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> index 99d6032510..92df1efee8 100644
> --- 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana
> +++ gerObjectParser.c
> @@ -14,7 +14,7 @@
>   STATIC
>   VOID
>   EFIAPI
> -PrintOemId (
> +PrintString (
>     CONST CHAR8  *Format,
>     UINT8        *Ptr
>     );
> @@ -22,7 +22,31 @@ PrintOemId (
>   STATIC
>   VOID
>   EFIAPI
> -PrintString (
> +PrintStringPtr (
> +  CONST CHAR8  *Format,
> +  UINT8        *Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar4 (
> +  CONST CHAR8  *Format,
> +  UINT8        *Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar6 (
> +  CONST CHAR8  *Format,
> +  UINT8        *Ptr
> +  );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar8 (
>     CONST CHAR8  *Format,
>     UINT8        *Ptr
>     );
> @@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER  CmArmItsGroupNodeParser[] = {
>   /** A parser for EArmObjNamedComponent.
>   */
>   STATIC CONST CM_OBJ_PARSER  CmArmNamedComponentNodeParser[] = {
> -  { "Token",             sizeof (CM_OBJECT_TOKEN), "0x%p", NULL        },
> -  { "IdMappingCount",    4,                        "0x%x", NULL        },
> -  { "IdMappingToken",    sizeof (CM_OBJECT_TOKEN), "0x%p", NULL        },
> -  { "Flags",             4,                        "0x%x", NULL        },
> -  { "CacheCoherent",     4,                        "0x%x", NULL        },
> -  { "AllocationHints",   1,                        "0x%x", NULL        },
> -  { "MemoryAccessFlags", 1,                        "0x%x", NULL        },
> -  { "AddressSizeLimit",  1,                        "0x%x", NULL        },
> -  { "ObjectName",        sizeof (CHAR8 *),         NULL,   PrintString },
> -  { "Identifier",        4,                        "0x%x", NULL        },
> +  { "Token",             sizeof (CM_OBJECT_TOKEN), "0x%p", NULL           },
> +  { "IdMappingCount",    4,                        "0x%x", NULL           },
> +  { "IdMappingToken",    sizeof (CM_OBJECT_TOKEN), "0x%p", NULL           },
> +  { "Flags",             4,                        "0x%x", NULL           },
> +  { "CacheCoherent",     4,                        "0x%x", NULL           },
> +  { "AllocationHints",   1,                        "0x%x", NULL           },
> +  { "MemoryAccessFlags", 1,                        "0x%x", NULL           },
> +  { "AddressSizeLimit",  1,                        "0x%x", NULL           },
> +  { "ObjectName",        sizeof (CHAR8 *),         NULL,   PrintStringPtr },
> +  { "Identifier",        4,                        "0x%x", NULL           },
>   };
>
>   /** A parser for EArmObjRootComplex.
> @@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  ArmNamespaceObjectParser[] = {
>   */
>   STATIC CONST CM_OBJ_PARSER  StdObjCfgMgrInfoParser[] = {
>     { "Revision", 4, "0x%x",         NULL       },
> -  { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId }
> +  { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 }

NIT:
Is it necessary, since "%c%c%c%c%c%c" is the default format of PrintChar6() ?

[JDS]: I considered changing it as unnecessary, but in the end left it as is since the original code used the same pattern (i.e. passing in a format that was the same as the default format).

>   };
>
>   /** A parser for EStdObjAcpiTableList.
>   */
>   STATIC CONST CM_OBJ_PARSER  StdObjAcpiTableInfoParser[] = {
> -  { "AcpiTableSignature", 4,                                      "0x%x",   NULL },
> -  { "AcpiTableRevision",  1,                                      "%d",     NULL },
> -  { "TableGeneratorId",   sizeof (ACPI_TABLE_GENERATOR_ID),       "0x%x",   NULL },
> -  { "AcpiTableData",      sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p",   NULL },
> -  { "OemTableId",         8,                                      "0x%LLX", NULL },
> -  { "OemRevision",        4,                                      "0x%x",   NULL },
> -  { "MinorRevision",      1,                                      "0x%x",   NULL },
> +  { "AcpiTableSignature", 4,                                      "%c%c%c%c",         PrintChar4 },
> +  { "AcpiTableRevision",  1,                                      "%d",               NULL       },
> +  { "TableGeneratorId",   sizeof (ACPI_TABLE_GENERATOR_ID),       "0x%x",             NULL       },
> +  { "AcpiTableData",      sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p",             NULL       },
> +  { "OemTableId",         8,                                      "%c%c%c%c%c%c%c%c", PrintChar8 },
> +  { "OemRevision",        4,                                      "0x%x",             NULL       },
> +  { "MinorRevision",      1,                                      "0x%x",             NULL       },
>   };
>
>   /** A parser for EStdObjSmbiosTableList.
> @@ -773,22 +797,99 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>       ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
>   };
>
> -/** Print OEM Id.
> +/** Print string data.
> +
> +  The string must be NULL terminated.
> +
> +  @param [in]  Format  Format to print the Ptr.
> +  @param [in]  Ptr     Pointer to the string.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintString (
> +  IN CONST CHAR8  *Format,
> +  IN UINT8        *Ptr
> +  )
> +{
> +  if (Ptr == NULL) {
> +    ASSERT (0);
> +    return;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "%a", Ptr));
> +}
> +
> +/** Print string from pointer.
> +
> +  The string must be NULL terminated.
> +
> +  @param [in]  Format      Format to print the string.
> +  @param [in]  Ptr         Pointer to the string pointer.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintStringPtr (
> +  IN CONST CHAR8  *Format,
> +  IN UINT8        *Ptr
> +  )
> +{
> +  UINT8  *String;
> +
> +  if (Ptr == NULL) {
> +    ASSERT (0);
> +    return;
> +  }
> +
> +  String = *(UINT8 **)Ptr;
> +
> +  if (String == NULL) {
> +    String = (UINT8 *)"(NULLPTR)";
> +  }
> +
> +  PrintString (Format, String);
> +}
> +
> +/** Print 4 characters.
>
>     @param [in]  Format  Format to print the Ptr.
> -  @param [in]  Ptr     Pointer to the OEM Id.
> +  @param [in]  Ptr     Pointer to the characters.
>   **/
>   STATIC
>   VOID
>   EFIAPI
> -PrintOemId (
> +PrintChar4 (
>     IN  CONST CHAR8  *Format,
>     IN  UINT8        *Ptr
>     )
>   {
>     DEBUG ((
> -    DEBUG_INFO,
> -    (Format != NULL) ? Format : "%C%C%C%C%C%C",
> +    DEBUG_ERROR,
> +    (Format != NULL) ? Format : "%c%c%c%c",
> +    Ptr[0],
> +    Ptr[1],
> +    Ptr[2],
> +    Ptr[3]
> +    ));
> +}
> +
> +/** Print 6 characters.
> +
> +  @param [in]  Format  Format to print the Ptr.
> +  @param [in]  Ptr     Pointer to the characters.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar6 (
> +  IN  CONST CHAR8  *Format,
> +  IN  UINT8        *Ptr
> +  )
> +{
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    (Format != NULL) ? Format : "%c%c%c%c%c%c",
>       Ptr[0],
>       Ptr[1],
>       Ptr[2],
> @@ -798,22 +899,31 @@ PrintOemId (
>       ));
>   }
>
> -/** Print string.
> -
> -  The string must be NULL terminated.
> +/** Print 8 characters.
>
>     @param [in]  Format  Format to print the Ptr.
> -  @param [in]  Ptr     Pointer to the string.
> +  @param [in]  Ptr     Pointer to the characters.
>   **/
>   STATIC
>   VOID
>   EFIAPI
> -PrintString (
> -  CONST CHAR8  *Format,
> -  UINT8        *Ptr
> +PrintChar8 (
> +  IN  CONST CHAR8  *Format,
> +  IN  UINT8        *Ptr
>     )
>   {
> -  DEBUG ((DEBUG_ERROR, "%a", Ptr));
> +  DEBUG ((
> +    DEBUG_ERROR,
> +    (Format != NULL) ? Format : "%c%c%c%c%c%c%c%c",
> +    Ptr[0],
> +    Ptr[1],
> +    Ptr[2],
> +    Ptr[3],
> +    Ptr[4],
> +    Ptr[5],
> +    Ptr[6],
> +    Ptr[7]
> +    ));
>   }
>
>   /** Print fields of the objects.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109495): https://edk2.groups.io/g/devel/message/109495
Mute This Topic: https://groups.io/mt/101801390/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

[Jeshua Smith via groups.io]

#3 is not currently used by any published code. It is a development aid, which the ObjectParser itself seems to be.

Here's why I added it. Several people on our team have (not yet upstreamed) changes that resulted in additional ObjectIDs being added to the ObjectID enums, but without corresponding parsers being added to the ObjectParser. Dumping of the objects with the ObjectParser wasn't enabled by default, so this wasn't detected by them. Without #1 the ObjectParser code does out of bounds array accesses, sometimes leading to crashes. #1 will now detect and report that problem. When I enabled dumping of objects to debug my new code, I hit this bug leading me to write #1. For me to work around the issue of missing parsers in order to be able to continue debug with my work, the "easy" solution was for me to temporarily add the new ObjectIDs to the parser list with NULL parsers and then inform the responsible parties that they need to go and add parsers for their new ObjectIDs. Doing this required #3 (support for NULL parsers) to be added. Ideally any code that is upstreamed back to EDKII will have non-NULL parsers at the point it is sent upstream, but allowing in-development changes to temporarily use NULL parsers is helpful.

Hopefully that clarifies things.

-----Original Message-----
From: Pierre Gondois <pierre.gondois@arm.com> 
Sent: Tuesday, October 10, 2023 4:14 AM
To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com
Subject: Re: [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

External email: Use caution opening links or attachments


Hello Jeshua,

On 10/6/23 18:28, Jeshua Smith wrote:
> This patch enhances error handling and reporting in the CM ObjectParser.
> Specifically:
> 1. ObjectIDs used as array indexes are checked for being out of bounds,
>     and if so an error message is printed before the assert.
> 2. An error message is printed for unsupported NameSpaceIDs.
> 3. Adds support for unimplemented parsers by allowing IDs to list a
>     NULL parser, resulting in an unimplemented message being printed.

I am not sure I see in which context 3. would be used/necessary. Is it possible to detail ?

(Code-wise everything looks good to me)

Regards,
Pierre


>
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> ---
>   .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
>   1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c 
> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> index 92df1efee8..22b8fdb906 100644
> --- 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana
> +++ gerObjectParser.c
> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>       ARRAY_SIZE (StdObjAcpiTableInfoParser) },
>     { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
>       ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
> +  { "EStdObjMax",             NULL,                       0}
>   };
>
>   /** Print string data.
> @@ -1066,6 +1067,12 @@ ParseCmObjDesc (
>           return;
>         }
>
> +      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &StdNamespaceObjectParser[ObjId];
>         break;
>       case EObjNameSpaceArm:
> @@ -1074,10 +1081,17 @@ ParseCmObjDesc (
>           return;
>         }
>
> +      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &ArmNamespaceObjectParser[ObjId];
>         break;
>       default:
>         // Not supported
> +      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not 
> + supported by the parser\n", NameSpaceId, ObjId));
>         ASSERT (0);
>         return;
>     } // switch
> @@ -1095,21 +1109,26 @@ ParseCmObjDesc (
>         ObjIndex + 1,
>         ObjectCount
>         ));
> -    PrintCmObjDesc (
> -      (VOID *)((UINTN)CmObjDesc->Data + Offset),
> -      ParserArray->Parser,
> -      ParserArray->ItemCount,
> -      &RemainingSize,
> -      1
> -      );
> -    if ((RemainingSize > CmObjDesc->Size) ||
> -        (RemainingSize < 0))
> -    {
> -      ASSERT (0);
> -      return;
> -    }
> +    if (ParserArray->Parser == NULL) {
> +      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
> +      RemainingSize = 0;
> +    } else {
> +      PrintCmObjDesc (
> +        (VOID *)((UINTN)CmObjDesc->Data + Offset),
> +        ParserArray->Parser,
> +        ParserArray->ItemCount,
> +        &RemainingSize,
> +        1
> +        );
> +      if ((RemainingSize > CmObjDesc->Size) ||
> +          (RemainingSize < 0))
> +      {
> +        ASSERT (0);
> +        return;
> +      }
>
> -    Offset = CmObjDesc->Size - RemainingSize;
> +      Offset = CmObjDesc->Size - RemainingSize;
> +    }
>     } // for
>
>     ASSERT (RemainingSize == 0);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109499): https://edk2.groups.io/g/devel/message/109499
Mute This Topic: https://groups.io/mt/101801385/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

[PierreGondois]

Hi Jeshua,

On 10/10/23 17:41, Jeshua Smith wrote:
> #3 is not currently used by any published code. It is a development aid, which the ObjectParser itself seems to be.
> 
> Here's why I added it. Several people on our team have (not yet upstreamed) changes that resulted in additional ObjectIDs being added to the ObjectID enums, but without corresponding parsers being added to the ObjectParser. Dumping of the objects with the ObjectParser wasn't enabled by default, so this wasn't detected by them. Without #1 the ObjectParser code does out of bounds array accesses, sometimes leading to crashes. #1 will now detect and report that problem. When I enabled dumping of objects to debug my new code, I hit this bug leading me to write #1. For me to work around the issue of missing parsers in order to be able to continue debug with my work, the "easy" solution was for me to temporarily add the new ObjectIDs to the parser list with NULL parsers and then inform the responsible parties that they need to go and add parsers for their new ObjectIDs. Doing this required #3 (support for NULL parsers) to be added. Ideally any code that is upstreamed back to EDKII will have non-NULL parsers at the point it is sent upstream, but allowing in-development changes to temporarily use NULL parsers is helpful.
> 
> Hopefully that clarifies things.

Thanks for the detailed explanation,
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

> 
> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Tuesday, October 10, 2023 4:14 AM
> To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com
> Subject: Re: [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello Jeshua,
> 
> On 10/6/23 18:28, Jeshua Smith wrote:
>> This patch enhances error handling and reporting in the CM ObjectParser.
>> Specifically:
>> 1. ObjectIDs used as array indexes are checked for being out of bounds,
>>      and if so an error message is printed before the assert.
>> 2. An error message is printed for unsupported NameSpaceIDs.
>> 3. Adds support for unimplemented parsers by allowing IDs to list a
>>      NULL parser, resulting in an unimplemented message being printed.
> 
> I am not sure I see in which context 3. would be used/necessary. Is it possible to detail ?
> 
> (Code-wise everything looks good to me)
> 
> Regards,
> Pierre
> 
> 
>>
>> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
>> ---
>>    .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
>>    1 file changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git
>> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
>> bjectParser.c
>> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
>> bjectParser.c
>> index 92df1efee8..22b8fdb906 100644
>> ---
>> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
>> bjectParser.c
>> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana
>> +++ gerObjectParser.c
>> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>>        ARRAY_SIZE (StdObjAcpiTableInfoParser) },
>>      { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
>>        ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
>> +  { "EStdObjMax",             NULL,                       0}
>>    };
>>
>>    /** Print string data.
>> @@ -1066,6 +1067,12 @@ ParseCmObjDesc (
>>            return;
>>          }
>>
>> +      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
>> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
>> +        ASSERT (0);
>> +        return;
>> +      }
>> +
>>          ParserArray = &StdNamespaceObjectParser[ObjId];
>>          break;
>>        case EObjNameSpaceArm:
>> @@ -1074,10 +1081,17 @@ ParseCmObjDesc (
>>            return;
>>          }
>>
>> +      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
>> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
>> +        ASSERT (0);
>> +        return;
>> +      }
>> +
>>          ParserArray = &ArmNamespaceObjectParser[ObjId];
>>          break;
>>        default:
>>          // Not supported
>> +      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not
>> + supported by the parser\n", NameSpaceId, ObjId));
>>          ASSERT (0);
>>          return;
>>      } // switch
>> @@ -1095,21 +1109,26 @@ ParseCmObjDesc (
>>          ObjIndex + 1,
>>          ObjectCount
>>          ));
>> -    PrintCmObjDesc (
>> -      (VOID *)((UINTN)CmObjDesc->Data + Offset),
>> -      ParserArray->Parser,
>> -      ParserArray->ItemCount,
>> -      &RemainingSize,
>> -      1
>> -      );
>> -    if ((RemainingSize > CmObjDesc->Size) ||
>> -        (RemainingSize < 0))
>> -    {
>> -      ASSERT (0);
>> -      return;
>> -    }
>> +    if (ParserArray->Parser == NULL) {
>> +      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
>> +      RemainingSize = 0;
>> +    } else {
>> +      PrintCmObjDesc (
>> +        (VOID *)((UINTN)CmObjDesc->Data + Offset),
>> +        ParserArray->Parser,
>> +        ParserArray->ItemCount,
>> +        &RemainingSize,
>> +        1
>> +        );
>> +      if ((RemainingSize > CmObjDesc->Size) ||
>> +          (RemainingSize < 0))
>> +      {
>> +        ASSERT (0);
>> +        return;
>> +      }
>>
>> -    Offset = CmObjDesc->Size - RemainingSize;
>> +      Offset = CmObjDesc->Size - RemainingSize;
>> +    }
>>      } // for
>>
>>      ASSERT (RemainingSize == 0);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109524): https://edk2.groups.io/g/devel/message/109524
Mute This Topic: https://groups.io/mt/101801385/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

[Jeshua Smith via groups.io]

The patches in this series have both individually received:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

Can this be merged?

-----Original Message-----
From: Jeshua Smith <jeshuas@nvidia.com> 
Sent: Friday, October 6, 2023 10:28 AM
To: devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com; pierre.gondois@arm.com; Jeshua Smith <jeshuas@nvidia.com>
Subject: [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

While using the ConfigurationManagerObjectParser to dump objects and debug adding new objects, I noticed some bugs and deficiencies. This series is intended to address those.

Jeshua Smith (2):
  DynamicTablesPkg/TableHelperLib: Fix and improve text handling
  DynamicTablesPkg/TableHelperLib: Enhance error handling

 .../ConfigurationManagerObjectParser.c        | 223 ++++++++++++++----
 1 file changed, 176 insertions(+), 47 deletions(-)

--
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109930): https://edk2.groups.io/g/devel/message/109930
Mute This Topic: https://groups.io/mt/101801382/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

[Sami Mujawar]

Hi Jeshua,

Thank you for these fixes.

For this series,

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

I have now queued this up for merging.

Regards,

Sami Mujawar

On 06/10/2023 05:28 pm, Jeshua Smith wrote:
> While using the ConfigurationManagerObjectParser to dump objects and debug
> adding new objects, I noticed some bugs and deficiencies. This series is
> intended to address those.
>
> Jeshua Smith (2):
>    DynamicTablesPkg/TableHelperLib: Fix and improve text handling
>    DynamicTablesPkg/TableHelperLib: Enhance error handling
>
>   .../ConfigurationManagerObjectParser.c        | 223 ++++++++++++++----
>   1 file changed, 176 insertions(+), 47 deletions(-)
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109932): https://edk2.groups.io/g/devel/message/109932
Mute This Topic: https://groups.io/mt/101801382/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates

[Sami Mujawar]

Merged as c591395f4ab5..ec7f73436646

Thanks.

Regards,

Sami Mujawar


On 23/10/2023 05:55 pm, Sami Mujawar via groups.io wrote:
> Hi Jeshua,
>
> Thank you for these fixes.
>
> For this series,
>
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>
> I have now queued this up for merging.
>
> Regards,
>
> Sami Mujawar
>
> On 06/10/2023 05:28 pm, Jeshua Smith wrote:
>> While using the ConfigurationManagerObjectParser to dump objects and 
>> debug
>> adding new objects, I noticed some bugs and deficiencies. This series is
>> intended to address those.
>>
>> Jeshua Smith (2):
>>    DynamicTablesPkg/TableHelperLib: Fix and improve text handling
>>    DynamicTablesPkg/TableHelperLib: Enhance error handling
>>
>>   .../ConfigurationManagerObjectParser.c        | 223 ++++++++++++++----
>>   1 file changed, 176 insertions(+), 47 deletions(-)
>>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109933): https://edk2.groups.io/g/devel/message/109933
Mute This Topic: https://groups.io/mt/101801382/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-