Re: [edk2-devel] [PATCH v2] MdePkg/BasePrintLib: Add %z specifier

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

[-- Attachment #1: Type: text/plain, Size: 10309 bytes --]

Hi Pedro,

There is a good brief description of size_t on this page:
https://en.wikipedia.org/wiki/C_data_types

sizeof() is built into the C compiler, but size_t is defined by the std C lib being used.  It can be as small as 16-bits.

We do not use the std C lib, so size_t is not defined when we do EDK II builds.  This means we do not know how large the value sizeof() returns is.

However, your request is to support printing values of type UINTN in the PrintLib.  I agree this feature is not present today.  Adding in a new format specifier would be required.

Using %z may be confusing because size_t defined by a C lib we are not using and UINTN defined in the UEFI Spec are not the same.

Do you have a suggestion for a different specifier that perhaps is not used in the ANSI C printf() format string?

If we do want a 100% ANSI C conformant version of a print library, then I agree this would require a new library class and we would need to use different API names so the caller knows which format string style to use.

There is a long history associated with the format string defined in PrintLib today.  Mostly related to size optimizations of the PrintLib when FLASH devices were much, much smaller.

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
Sent: Wednesday, July 6, 2022 3:27 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Andrew Fish (afish@apple.com) <afish@apple.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: Re: [edk2-devel] [PATCH v2] MdePkg/BasePrintLib: Add %z specifier

On Wed, Jul 6, 2022 at 7:22 PM Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:
Hi Pedro,

This is an interesting feature.

It is backwards compatible since you are adding a format specifier to the PrintLib class.

There is a 2nd lib instance that needs to be updated, and that is DxePrintLibPrint2Protocol in MdeModulePkg.

I think using ANSI C %z specifier syntax assumes that sizeof(size_t) == sizeof(UINTN) for all supported compilers/CPU archs.
The ANSI C 99 specification does not state that this is always true.  If may be true in practice for the compiler/CPU archs
currently supported by Tianocore.

It would be good to add a STATIC_ASSERT() to check that this is true and break the build if a compiler+CPU combination
is used where that assumption is false.  Not sure if this should go in Base.h with other STATIC_ASSERT()s for
types.  Or if it should go in PrintLib.h where this assumption is being made.


Hi Mike,
How can sizeof(size_t) != sizeof(UINTN)? It seems to be quite a stretch to interpret the standard in a way that the native word size will not be able to store "the maximum size of a theoretically possible object of any type". In any case, getting the size_t type is non-trivial and as far as I can tell would either depend on a C library or C extension trickery to try to get the type of sizeof(Type).

I would like to see some more background on the motivation for adding this feature.
I think the current workaround would be to typecast and integer of type size_t to UINT64 and use %Ld or
typecase to UINT32 and use %d.

Ok, some background: I essentially starting looking at DebugLib and PrintLib and I found myself very surprised by a good chunk of decisions. The API is very similar to standard printf() but it lacks support for a good portion of specifiers; some specifiers it implements don't have the C99 standard meaning, like the ones it implements for integers (%x and %lx for hex, for instance) which were non-obviously overridden to mean print UINT32 and print UINT64. Because of that, I feel that maybe reworking this library (creating PrintLib2 or something, since we can't break the existing uses) would be a good idea to have a "principle of least surprise" compliant API and would help smoothen the confusion curve between userspace code and EDK2 firmware.

Since reworking the whole of PrintLib is a non-trivial amount of work, I settled for this very tiny feature that doesn't break anyone's code and is very handy to avoid odd workarounds like casting everything to a UINT64 so you can print it with %lx. Obviously, if there's interest in getting a more standard environment[1] in EDK2 (and I personally, although possibly naiively, don't see the downside), going down a PrintLib2 in the long haul is a good idea. But this patch took me around 10 minutes to write up and is probably useful in its current form.

Thanks,

Pedro

[1] Why do I want to get a more standardized environment? Well, I simply believe it smoothens the learning curve between userspace -> kernel -> bootloader -> firmware. A good chunk of kernels and bootloaders already have relatively standard C APIs, but firmware, at least in our case, is still lacking. Also, makes you less prone to mistakes.

Thanks,

Mike


> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Pedro Falcato
> Sent: Monday, July 4, 2022 6:16 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang
> <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>
> Subject: [edk2-devel] [PATCH v2] MdePkg/BasePrintLib: Add %z specifier
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3977
>
> %z is used in standard C99 as the printf specifier for size_t types.
> Add support for it so we can portably print UINTN.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Cc: Liming Gao <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com<mailto:pedro.falcato@gmail.com>>
> ---
>  MdePkg/Include/Library/PrintLib.h              | 13 ++++++++-----
>  MdePkg/Library/BasePrintLib/PrintLibInternal.c |  9 +++++++++
>  MdePkg/Library/BasePrintLib/PrintLibInternal.h |  1 +
>  3 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/MdePkg/Include/Library/PrintLib.h b/MdePkg/Include/Library/PrintLib.h
> index 8d523cac52..0d67f62d3f 100644
> --- a/MdePkg/Include/Library/PrintLib.h
> +++ b/MdePkg/Include/Library/PrintLib.h
> @@ -42,6 +42,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>      - L, l
>        - The number being printed is size UINT64.  Only valid for types X, x, and d.
>          If this flag is not specified, then the number being printed is size int.
> +    - z
> +      - The number being printed is of size UINTN. Only valid for types X, x and d.
> +        If this flag is not specified, then the number being printed is size int.
>      - NOTE: All invalid flags are ignored.
>
>    [width]:
> @@ -73,18 +76,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>          using this type too by making sure bits 8..15 of the argument are set to 0.
>      - x
>        - The argument is an unsigned hexadecimal number.  The characters used are 0..9 and
> -        A..F.  If the flag 'L' is not specified, then the argument is assumed
> +        A..F.  If the flags 'L', 'z' are not specified, then the argument is assumed
>          to be size int.  This does not follow ANSI C.
>      - X
>        - The argument is an unsigned hexadecimal number and the number is padded with
> -        zeros.  This is equivalent to a format string of "0x". If the flag
> -        'L' is not specified, then the argument is assumed to be size int.
> +        zeros.  This is equivalent to a format string of "0x". If the flags
> +        'L', 'z' are not specified, then the argument is assumed to be size int.
>          This does not follow ANSI C.
>      - d
> -      - The argument is a signed decimal number.  If the flag 'L' is not specified,
> +      - The argument is a signed decimal number.  If the flags 'L', 'z' are not specified,
>          then the argument is assumed to be size int.
>      - u
> -      - The argument is a unsigned decimal number.  If the flag 'L' is not specified,
> +      - The argument is a unsigned decimal number.  If the flags 'L'. 'z' are not specified,
>          then the argument is assumed to be size int.
>      - p
>        - The argument is a pointer that is a (VOID *), and it is printed as an
> diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> index 42b598a432..1cd99b2213 100644
> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> @@ -720,6 +720,9 @@ BasePrintLibSPrintMarker (
>              case 'l':
>                Flags |= LONG_TYPE;
>                break;
> +            case 'z':
> +              Flags |= SIZET_TYPE;
> +              break;
>              case '*':
>                if ((Flags & PRECISION) == 0) {
>                  Flags |= PAD_TO_WIDTH;
> @@ -833,6 +836,12 @@ BasePrintLibSPrintMarker (
>                } else {
>                  Value = BASE_ARG (BaseListMarker, int);
>                }
> +            } else if ((Flags & SIZET_TYPE) != 0) {
> +              if (BaseListMarker == NULL) {
> +                Value = VA_ARG (VaListMarker, UINTN);
> +              } else {
> +                Value = BASE_ARG (BaseListMarker, UINTN);
> +              }
>              } else {
>                if (BaseListMarker == NULL) {
>                  Value = VA_ARG (VaListMarker, INT64);
> diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.h b/MdePkg/Library/BasePrintLib/PrintLibInternal.h
> index 34d591c6fc..9193e6192b 100644
> --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.h
> +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.h
> @@ -29,6 +29,7 @@
>  #define ARGUMENT_REVERSED    BIT12
>  #define COUNT_ONLY_NO_PRINT  BIT13
>  #define UNSIGNED_TYPE        BIT14
> +#define SIZET_TYPE           BIT15
>
>  //
>  // Record date and time information
> --
> 2.37.0
>
>
>
>
>


--
Pedro Falcato


[-- Attachment #2: Type: text/html, Size: 56255 bytes --]