Re: [PATCH v2 next 07/11] tools/nolibc/printf: Add support for conversion flags "#- +" and format "%X"

[Willy Tarreau]

On Fri, Feb 06, 2026 at 07:11:17PM +0000, david.laight.linux@xxxxxxxxx wrote:
> -/* simple printf(). It supports the following formats:
> - *  - %[-][width][{l,t,z,ll,L,j,q}]{d,u,c,x,p,s,m,%}
> - *  - %%
> - *  - invalid formats are copied to the output buffer
> +/* printf(). Supports most of the normal integer and string formats.
> + *  - %[#-+ ][width][{l,t,z,ll,L,j,q}]{d,i,u,c,x,X,p,s,m,%}
> + *  - %% generates a single %
> + *  - %m outputs strerror(errno).
> + *  - # only affects %x and prepends 0x to non-zero values.
> + *  - %o (octal) isn't supported.
> + *  - %X outputs a..f the same as %x.
> + *  - No support for zero padding, precision or variable widths.
> + *  - No support for wide characters.
> + *  - invalid formats are copied to the output buffer.
>   */

Thanks for updating this one, it does help quite a bit.

>  /* This code uses 'flag' variables that are indexed by the low 6 bits
> @@ -279,7 +285,7 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
>  	unsigned int written, width;
>  	unsigned int flags, ch_flag;
>  	size_t len;
> -	char tmpbuf[21];
> +	char tmpbuf[32 + 24];

The previous buffer was sized to store a 64-bit int. I couldn't figure
what these 32 and 24 correspond to with the new supported specifiers.
Maybe please add a short comment on the line to hint about what they
correspond to ?

>  	const char *outstr;
>  
>  	written = 0;
> @@ -334,19 +340,32 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
>  
>  			/* Conversion specifiers. */
>  
> -			/* Numeric conversion specifiers. */
> -			ch_flag = _NOLIBC_PF_CHAR_IS_ONE_OF(ch, 'c', 'd', 'i', 'u', 'x', 'p');
> -			if (ch_flag != 0) {
> +			/* Numeric and pointer conversion specifiers.
> +			 *
> +			 * Use an explicit bound check (rather than _NOLIBC_PF_CHAR_IS_ONE_OF())
> +			 * so that 'X' can be allowed through.
> +			 * 'X' gets treated and 'x' because _NOLIBC_PF_FLAG() returns the same
> +			 * value for both.
> +			 */
> +			if ((ch < 'a' || ch > 'z') && ch != 'X')
> +				goto non_numeric_conversion;
> +
> +			/* We need to check for "%p" or "%#x" later, merging here gives better code.
> +			 * But '#' collides with 'c' so shift right.
> +			 */
> +			ch_flag = _NOLIBC_PF_FLAG(ch) | (flags & _NOLIBC_PF_FLAG('#')) >> 1;
> +			if (_NOLIBC_PF_FLAGS_CONTAIN(ch_flag, 'c', 'd', 'i', 'u', 'x', 'p', 's')) {
>  				unsigned long long v;
>  				long long signed_v;
> -				char *out = tmpbuf;
> +				char *out = tmpbuf + 32;

OK so you seem to be reserving a part of the buffer for certain uses ?

> +				int sign = 0;
>  
>  				/* 'long' is needed for pointer/string conversions and ltz lengths.
>  				 * A single test can be used provided 'p' (the same bit as '0')
>  				 * is masked from flags.
>  				 */
>  				if (_NOLIBC_PF_FLAGS_CONTAIN(ch_flag | (flags & ~_NOLIBC_PF_FLAG('p')),
> -							     'p', 'l', 't', 'z')) {
> +							     'p', 's', 'l', 't', 'z')) {
>  					v = va_arg(args, unsigned long);
>  					signed_v = (long)v;
>  				} else if (_NOLIBC_PF_FLAGS_CONTAIN(flags, 'j', 'q')) {
> @@ -365,40 +384,62 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
>  					goto do_output;
>  				}
>  
> +				if (_NOLIBC_PF_FLAGS_CONTAIN(ch_flag, 's')) {
> +					/* "%s" - character string. */
> +					if (!v) {
> +						outstr = "(null)";
> +						len = 6;
> +						goto do_output;
> +					}
> +					outstr = (void *)v;
> +do_strnlen_output:
> +					len = strnlen(outstr, INT_MAX);

I get why you turned strlen() to strnlen(INT_MAX)  (result being an int)
but this will not change anything IMHO in that the rest of a 2GB+ string
will be written in multiple passes and will overflow the output anyway.
Thus I think that sticking to strlen() remains simpler and less confusing.

(...)
> -			else if (ch == 'm') {
> +
> +non_numeric_conversion:
> +			if (ch == 'm') {
>  #ifdef NOLIBC_IGNORE_ERRNO
>  				outstr = "unknown error";
> +				len = __builtin_strlen(outstr);
>  #else
>  				outstr = strerror(errno);
> +				goto do_strnlen_output;
>  #endif /* NOLIBC_IGNORE_ERRNO */

It's simlper (and smaller) to use the common label for both here:

   #ifdef NOLIBC_IGNORE_ERRNO
   				outstr = "unknown error";
   #else
   				outstr = strerror(errno);
   #endif /* NOLIBC_IGNORE_ERRNO */
  +				goto do_strnlen_output;

Overall OK to me.

Willy