Re: [musl] [PATCH] vfprintf: support C2x %b and %B conversion specifiers

[Rich Felker <dalias@libc.org>]

On Thu, Sep 08, 2022 at 06:36:49PM +0200, Gabriel Ravier wrote:
> These specifiers allow for formatted input/output of binary integers,
> and have been added to C2x with N2630. The uppercase B specifier is
> not formally required by C2x, as only lowercase specifiers were
> reserved by C, and thus an implementation could have been using
> uppercase B for an extension of their own, but C2x still has a note
> saying that it is recommended practice to implement it as the logical
> counterpart to b.
> 
> I have tested this on:
> - glibc's tests for %b and %B
> - The libc testsuite I'm developing over at https://github.com/GabrielRavier/yalibct
> - musl's libc-test
> - musl's libc-testsuite
> 
> and observed no regressions.
> ---
>  src/stdio/vfprintf.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c
> index 9b961e7f..89426b72 100644
> --- a/src/stdio/vfprintf.c
> +++ b/src/stdio/vfprintf.c
> @@ -49,7 +49,7 @@ enum {
>  static const unsigned char states[]['z'-'A'+1] = {
>  	{ /* 0: bare types */
>  		S('d') = INT, S('i') = INT,
> -		S('o') = UINT, S('u') = UINT, S('x') = UINT, S('X') = UINT,
> +		S('o') = UINT, S('u') = UINT, S('x') = UINT, S('X') = UINT, S('b') = UINT, S('B') = UINT,
>  		S('e') = DBL, S('f') = DBL, S('g') = DBL, S('a') = DBL,
>  		S('E') = DBL, S('F') = DBL, S('G') = DBL, S('A') = DBL,
>  		S('c') = CHAR, S('C') = INT,
> @@ -59,7 +59,7 @@ static const unsigned char states[]['z'-'A'+1] = {
>  		S('z') = ZTPRE, S('j') = JPRE, S('t') = ZTPRE,
>  	}, { /* 1: l-prefixed */
>  		S('d') = LONG, S('i') = LONG,
> -		S('o') = ULONG, S('u') = ULONG, S('x') = ULONG, S('X') = ULONG,
> +		S('o') = ULONG, S('u') = ULONG, S('x') = ULONG, S('X') = ULONG, S('b') = ULONG, S('B') = ULONG,
>  		S('e') = DBL, S('f') = DBL, S('g') = DBL, S('a') = DBL,
>  		S('E') = DBL, S('F') = DBL, S('G') = DBL, S('A') = DBL,
>  		S('c') = INT, S('s') = PTR, S('n') = PTR,
> @@ -68,17 +68,20 @@ static const unsigned char states[]['z'-'A'+1] = {
>  		S('d') = LLONG, S('i') = LLONG,
>  		S('o') = ULLONG, S('u') = ULLONG,
>  		S('x') = ULLONG, S('X') = ULLONG,
> +		S('b') = ULLONG, S('B') = ULLONG,
>  		S('n') = PTR,
>  	}, { /* 3: h-prefixed */
>  		S('d') = SHORT, S('i') = SHORT,
>  		S('o') = USHORT, S('u') = USHORT,
>  		S('x') = USHORT, S('X') = USHORT,
> +		S('b') = USHORT, S('B') = USHORT,
>  		S('n') = PTR,
>  		S('h') = HHPRE,
>  	}, { /* 4: hh-prefixed */
>  		S('d') = CHAR, S('i') = CHAR,
>  		S('o') = UCHAR, S('u') = UCHAR,
>  		S('x') = UCHAR, S('X') = UCHAR,
> +		S('b') = UCHAR, S('B') = UCHAR,
>  		S('n') = PTR,
>  	}, { /* 5: L-prefixed */
>  		S('e') = LDBL, S('f') = LDBL, S('g') = LDBL, S('a') = LDBL,
> @@ -88,11 +91,13 @@ static const unsigned char states[]['z'-'A'+1] = {
>  		S('d') = PDIFF, S('i') = PDIFF,
>  		S('o') = SIZET, S('u') = SIZET,
>  		S('x') = SIZET, S('X') = SIZET,
> +		S('b') = SIZET, S('B') = SIZET,
>  		S('n') = PTR,
>  	}, { /* 7: j-prefixed */
>  		S('d') = IMAX, S('i') = IMAX,
>  		S('o') = UMAX, S('u') = UMAX,
>  		S('x') = UMAX, S('X') = UMAX,
> +		S('b') = UMAX, S('B') = UMAX,
>  		S('n') = PTR,
>  	}
>  };
> @@ -162,6 +167,12 @@ static char *fmt_o(uintmax_t x, char *s)
>  	return s;
>  }
>  
> +static char *fmt_b(uintmax_t x, char *s)
> +{
> +	for (; x; x>>=1) *--s = '0' + (x&1);
> +	return s;
> +}
> +
>  static char *fmt_u(uintmax_t x, char *s)
>  {
>  	unsigned long y;
> @@ -529,7 +540,7 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
>  		if (!f) continue;
>  
>  		z = buf + sizeof(buf);
> -		prefix = "-+   0X0x";
> +		prefix = "-+   0X0x0B0b";
>  		pl = 0;
>  		t = s[-1];
>  
> @@ -559,6 +570,10 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
>  			a = fmt_x(arg.i, z, t&32);
>  			if (arg.i && (fl & ALT_FORM)) prefix+=(t>>4), pl=2;
>  			if (0) {
> +		case 'b': case 'B':
> +			a = fmt_b(arg.i, z);
> +			if (arg.i && (fl & ALT_FORM)) prefix += 9 + ((t == 'b') << 1), pl=2;
> +			} if (0) {
>  		case 'o':
>  			a = fmt_o(arg.i, z);
>  			if ((fl&ALT_FORM) && p<z-a+1) p=z-a+1;
> -- 
> 2.37.3

I'm not sure what the schedule on taking this or other C2x changes
relative to the standard becoming official should be, so we should
probably discuss that at some point. The only real hard concern is
that we need to be careful not to take anything where the standard
mandated behavior might change before it's final.

For the above patch specifically:

1. There's a buffer overflow because you did not adjust the size
   formula for buf[]. Presently it's only 40-55 bytes (due to the
   inclusion of LDBL_MANT_DIG/4 in there, which is almost surely cruft
   predating the initial release) while a binary-form string requires
   at least 64 bytes.

2. Presumably the wide printf forms need to accept %b too. Does scanf
   need to accept them too? I think those would all be easy changes,
   since scanf already has the strto* core with arbitrary base
   available to it, and wide printf just calls back to the narrow one.

I don't see anything else immediately wrong. No rush to submit an
updated patch. Let's first figure out the timeline for C2x features,
and I'll try to figure out what's going on with the weird buf[] size
expression and clean it up separately so that your patch is just
adding the feature and not also fighting with historical cruft.

Rich