Re: Libc-alpha Digest, Vol 20, Issue 81

Re: Libc-alpha Digest, Vol 20, Issue 81

[Alejandro Colomar (man-pages)]

Hi, Joseph!

On 10/13/21 2:37 AM, libc-alpha-request@sourceware.org wrote:
> 
> [PATCH v2] Support C2X printf %b, %B.eml
> 
> Subject:
> [PATCH v2] Support C2X printf %b, %B
> From:
> Joseph Myers <joseph@codesourcery.com>
> Date:
> 10/13/21, 2:37 AM
> 
> To:
> <libc-alpha@sourceware.org>
> 
> 
> C2X adds a printf %b format (see
> <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2630.pdf>, accepted
> for C2X), for outputting integers in binary.  It also has recommended
> practice for a corresponding %B format (like %b, but %#B starts the
> output with 0B instead of 0b).  Add support for these formats to
> glibc.
> 
> One existing test uses %b as an example of an unknown format, to test
> how glibc printf handles unknown formats; change that to %v.  Use of
> %b and %B as user-registered format specifiers continues to work (and
> we already have a test that covers that, tst-printfsz.c).
> 
> Note that C2X also has scanf %b support, plus support for binary
> constants starting 0b in strtol (base 0 and 2) and scanf %i (strtol
> base 0 and scanf %i coming from a previous paper that added binary
> integer literals).  I intend to implement those features in a separate
> patch or patches; as discussed in the thread starting at
> <https://sourceware.org/pipermail/libc-alpha/2020-December/120414.html>,
> they will be more complicated because they involve adding extra public
> symbols to ensure compatibility with existing code that might not
> expect 0b constants to be handled by strtol base 0 and 2 and scanf %i,
> whereas simply adding a new format specifier poses no such
> compatibility concerns.
> 
> Note that the actual conversion from integer to string uses existing
> code in _itoa.c.  That code has special cases for bases 8, 10 and 16,
> probably so that the compiler can optimize division by an integer
> constant in the code for those bases.  If desired such special cases
> could easily be added for base 2 as well, but that would be an
> optimization, not actually needed for these printf formats to work.
> 
> Tested for x86_64 and x86.  Also tested with build-many-glibcs.py for
> aarch64-linux-gnu with GCC mainline to make sure that the test does
> indeed build with GCC 12 (where format checking warnings are enabled
> for most of the test).
> 

Reviewed-by: Alejandro Colomar <alx.manpages@gmail.com>

Code-wise, I checked that it is almost identical to the patch I 
submitted in 2020, so LGTM.  However, this one doesn't optimize itoa and 
relatives, which I agree with you that may be better in a separate 
patch, if needed at all.

And you wrote the docs and many more tests than me :)

I just have an inline comment below.

Could you please ping (or CC) me when this (and related patches such as 
strtoX and scanf) is merged so that I add the necessary documentation to 
the man-pages?

Thanks!

Alex

> ---
> 
> The first patch version was
> <https://sourceware.org/pipermail/libc-alpha/2021-October/131764.html>.
> In this version, the warning disabling is updated to reflect that I've
> now added support for checking these formats to GCC 12.
> 
[...]
> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
> index 415f038141..b7e70035af 100644
> --- a/stdio-common/vfprintf-internal.c
> +++ b/stdio-common/vfprintf-internal.c
[...]
> @@ -706,6 +712,14 @@ static const uint8_t jump_table[] =
>       LABEL (form_hexa):							      \
>         /* Unsigned hexadecimal integer.  */				      \
>         base = 16;							      \
> +      goto LABEL (unsigned_number);					      \
> +      /* NOTREACHED */							      \
> +									      \
> +    LABEL (form_binary):						      \

I would have added form_binary just above form_octal, for symmetry (2, 
8, 16), but not very important.

> +      /* Unsigned binary integer.  */					      \
> +      base = 2;								      \
> +      goto LABEL (unsigned_number);					      \
> +      /* NOTREACHED */							      \
>   									      \
>       LABEL (unsigned_number):	  /* Unsigned number of base BASE.  */	      \
>   									      \

-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/