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

[David Laight <david.laight.linux@gmail.com>]

On Sun, 8 Feb 2026 16:47:23 +0100
Willy Tarreau <w@1wt.eu> wrote:

> On Fri, Feb 06, 2026 at 07:11:17PM +0000, david.laight.linux@gmail.com 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 ?

The could just be 2 here, but the next patch needs extra space for
all the '0' bytes.
I do like to make char[] arrays multiple of 4 bytes as well.
That is one reason for rounding the 21 up to 24.
It does also mean the line doesn't need changing to support octal.

> 
> >  	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 ?

Sign and zero pad (next patch).
Ok, make a comment...

> 
> > +				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.

Wrong reason.
The next patch changes it to:
					len = strnlen(outstr, precision);
and I didn't want to change the name of the label as well as this line.

> 
> (...)
> > -			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;

If you look closely strerror() gets inlined here (at least with the makefile
that builds the test program).

I think you'll get smaller code by marking some of the functions noinline.

There is an 'interesting' option for 64bit.
Put "errno=" into 'sign', set 'v = errno', clear '.' (or set precision to 1)
and jump to the 'output in decimal' code.

strerror() could then be implemented using snprintf(buf, n, "%m");

Related is inlining the 'divide by reciprocal' u64toa() code into sprintf()
- that might save it spilling to stack and be smaller.
Then implement atoi() (etc) using snprintf().
Might generate smaller code overall - but it isn't a serious suggestion.

	David

> 
> Overall OK to me.
	Ta
> 
> Willy