On Wed, Jul 6, 2022 at 7:22 PM Kinney, Michael D <michael.d.kin...@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 <devel@edk2.groups.io> On Behalf Of Pedro > Falcato > > Sent: Monday, July 4, 2022 6:16 PM > > To: devel@edk2.groups.io > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming < > gaolim...@byosoft.com.cn>; Liu, Zhiguang > > <zhiguang....@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.kin...@intel.com> > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > Cc: Zhiguang Liu <zhiguang....@intel.com> > > Signed-off-by: Pedro Falcato <pedro.falc...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#91123): https://edk2.groups.io/g/devel/message/91123 Mute This Topic: https://groups.io/mt/92177290/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
