Message-ID: <20260207232825.56b77d38@pumpkin>
Date: Sat, 7 Feb 2026 23:28:25 +0000
From: David Laight <david.laight.linux@...il.com>
To: Willy Tarreau <w@....eu>
Cc: Thomas Weißschuh <linux@...ssschuh.net>,
 linux-kernel@...r.kernel.org, Cheng Li <lechain@...il.com>
Subject: Re: [PATCH v2 next 02/11] tools/nolibc/printf: Move snprintf length
 check to callback

On Sat, 7 Feb 2026 20:12:30 +0100
Willy Tarreau <w@....eu> wrote:

> On Fri, Feb 06, 2026 at 07:11:12PM +0000, david.laight.linux@...il.com wrote:
> > From: David Laight <david.laight.linux@...il.com>
> > 
> > Move output truncation to the snprintf() callback.
> > This simplifies the main code and ensures the truncation will be
> > correct when left-alignment is added.
> > 
> > Add a zero length callback to 'finalise' the buffer rather than
> > doing it in snprintf() itself.
> > 
> > Signed-off-by: David Laight <david.laight.linux@...il.com>
> > ---
> > 
> > Changes for v2:
> > - Formally patch 1
> > - Add comments about the final callback.
> > 
> >  tools/include/nolibc/stdio.h | 68 ++++++++++++++++++++++--------------
> >  1 file changed, 41 insertions(+), 27 deletions(-)
> > 
> > diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> > index f162cc697a73..36733ecd4261 100644
> > --- a/tools/include/nolibc/stdio.h
> > +++ b/tools/include/nolibc/stdio.h
> > @@ -245,15 +245,15 @@ char *fgets(char *s, int size, FILE *stream)
> >   *  - %s
> >   *  - unknown modifiers are ignored.
> >   */
> > -typedef int (*__nolibc_printf_cb)(intptr_t state, const char *buf, size_t size);
> > +typedef int (*__nolibc_printf_cb)(void *state, const char *buf, size_t size);
> >  
> > -static __attribute__((unused, format(printf, 4, 0)))
> > -int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char *fmt, va_list args)
> > +static __attribute__((unused, format(printf, 3, 0)))
> > +int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list args)
> >  {
> >  	char escape, lpref, ch;
> >  	unsigned long long v;
> >  	unsigned int written, width;
> > -	size_t len, ofs, w;
> > +	size_t len, ofs;
> >  	char tmpbuf[21];
> >  	const char *outstr;
> >  
> > @@ -355,17 +355,13 @@ int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char
> >  			outstr = fmt;
> >  			len = ofs - 1;
> >  		flush_str:
> > -			if (n) {
> > -				w = len < n ? len : n;
> > -				n -= w;
> > -				while (width-- > w) {
> > -					if (cb(state, " ", 1) != 0)
> > -						return -1;
> > -					written += 1;
> > -				}
> > -				if (cb(state, outstr, w) != 0)
> > +			while (width-- > len) {
> > +				if (cb(state, " ", 1) != 0)
> >  					return -1;
> > +				written += 1;
> >  			}
> > +			if (cb(state, outstr, len) != 0)
> > +				return -1;
> >  
> >  			written += len;
> >  		do_escape:
> > @@ -378,18 +374,23 @@ int __nolibc_printf(__nolibc_printf_cb cb, intptr_t state, size_t n, const char
> >  
> >  		/* literal char, just queue it */
> >  	}
> > +
> > +	/* Flush/terminate any buffer. */
> > +	if (cb(state, NULL, 0) != 0)
> > +		return -1;
> > +  
> 
> I suspect this hunk is in fact part of the next patch which adds
> buffering, because I don't see what there is to flush here without
> any buffer.

Ok, it could be 'terminate' here, and I could change the line again
in the next patch.
If the next patch goes in (it can be missed out) then maybe a different
comment would be better.
Or something more 'wooly' line 'finalise the output'.

> If really needed, then I think that it's about time to
> start adding a comment about the __nolibc_printf() function to explain
> how it's supposed to work, because callback-driven code is unreadable,
> there are hidden expectations everywhere that are super hard to guess
> or verify.

Something has to add the terminating '\0'.
The callback function has the rest of the logic for writing to the buffer.
So it saves replicating the length test (as was done before).
It also lets vsnprintf() directly return the value returned from
__nolibc_fprintf_cb().
The '\0' could be added after every memcpy(), but that has hard code paths
when the buffer length is 0 or 1.

It also helps if the compiler decides to inline vsnprintf.

But yes, some of the motivation does come from the following patch.
There you really do want all the write system calls to be in one function.

> 
> >  	return written;
> >  }
> >  
> > -static int __nolibc_fprintf_cb(intptr_t state, const char *buf, size_t size)
> > +static int __nolibc_fprintf_cb(void *stream, const char *buf, size_t size)  
> 
> I must confess I'm not a big fan of the void* here. I've seen that you're
> having one state for snprintf() and another one for fprintf(), maybe they
> could be efficiently merged into a common printf_state ? Note that I'm not
> vetoing this, I just want to be convinced that it's the best choice, and
> neither the code, comments nor commit messages for now suggest so.

'void *' has to be better than intptr_t.
A common state wouldn't work - all the fields are different
(before and after the next patch).
At least here all the functions are together and it is a straight callback.
The usual problem with registering callback for later is the extreme
difficulty in safely de-registering them.

> 
> >  {
> > -	return _fwrite(buf, size, (FILE *)state);
> > +	return size ? _fwrite(buf, size, stream) : 0;
> >  }
> >  
> >  static __attribute__((unused, format(printf, 2, 0)))
> >  int vfprintf(FILE *stream, const char *fmt, va_list args)
> >  {
> > -	return __nolibc_printf(__nolibc_fprintf_cb, (intptr_t)stream, SIZE_MAX, fmt, args);
> > +	return __nolibc_printf(__nolibc_fprintf_cb, stream, fmt, args);
> >  }
> >  
> >  static __attribute__((unused, format(printf, 1, 0)))
> > @@ -447,26 +448,39 @@ int dprintf(int fd, const char *fmt, ...)
> >  	return ret;
> >  }
> >  
> > -static int __nolibc_sprintf_cb(intptr_t _state, const char *buf, size_t size)
> > +struct __nolibc_sprintf_cb_state {
> > +	char *buf;
> > +	size_t size;
> > +};
> > +
> > +static int __nolibc_sprintf_cb(void *v_state, const char *buf, size_t size)
> >  {
> > -	char **state = (char **)_state;
> > +	struct __nolibc_sprintf_cb_state *state = v_state;
> > +	char *tgt;
> >  
> > -	memcpy(*state, buf, size);
> > -	*state += size;
> > +	if (size >= state->size) {
> > +		if (state->size <= 1)
> > +			return 0;  
> 
> I failed to understand that one. Don't we want to at least write the
> trailing zero when there's one byte left ? A short comment explaining
> that case would help.

state->size is normally greater than zero.
So when size if zero the first condition is false, and the *tgt = 0
line is executed later.
However it is valid to call vsnprintf(NULL, 0, fmt, args).
That is the only way state->size can be zero - in that case the code
needs to return without writing to the buffer.

> 
> > +		size = state->size - 1;
> > +	}
> > +	tgt = state->buf;
> > +	if (size) {
> > +		state->size -= size;
> > +		state->buf = tgt + size;
> > +		memcpy(tgt, buf, size);
> > +	} else {
> > +		/* In particular from cb(NULL, 0) at the end of __nolibc_printf(). */
> > +		*tgt = '\0';
> > +	}  
> 
> Usually, "if/else" constructs result in larger code due to jumps.

Size doesn't usually matter, what makes a big difference is the branches
being mispredicted - I think that is ~20 clocks on my zen-5 (getting close
the the P4-netburst values).

I think the nolibc is optimised for size, but only within reason.
That if/else does help document what is going on.

> Here we certainly can unconditionally write the trailing zero.

Writing the zero then overwriting it with 'no bytes' is too subtle (even for me).
I'm sure you'd have wanted a big comment :-)

> Bingo, we're
> saving 9 bytes on x86_64 by moving it above. And even 17 bytes by dropping
> the test on size and updating the state after the memcpy:
> 
> 	if (size >= state->size) {
> 		if (state->size <= 1)
> 			return 0;
> 		size = state->size - 1;
> 	}
> 	*state->buf = '\0';
> 	memcpy(state->buf, buf, size);
> 	state->buf += size;
> 	state->size -= size;

That starts relying on the compiler assuming that the memcpy() can't
be writing to state->buf and state->size.
I'm not certain it can assume that in the callback function
(With or without 'strict aliasing').
So the explicit local for tgt helps a lot - and does no harm.
In the past I've done:
	x->buf += size;
	memcpy(x->buf - size, src, size)
knowing that the compiler will just use the old value.


Note that something has to write the '\0' for:	
	snprintf(buf, 1, fmt, args);
and
	snprintf(buf, len, "");
but not for
	snprintf(buf, 0, fmt, args);
so just writing the the byte following the memcpy() isn't enough.

	David


> 
>   snprintf-patch1:000000000000003e t __nolibc_sprintf_cb
>   snprintf-patch1-alt1:0000000000000033 t __nolibc_sprintf_cb
>   snprintf-patch1-alt2:000000000000002d t __nolibc_sprintf_cb
> 
> (not tested but worth a try).
> 
> >  	return 0;
> >  }
> >  
> >  static __attribute__((unused, format(printf, 3, 0)))
> >  int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
> >  {
> > -	char *state = buf;
> > -	int ret;
> > +	struct __nolibc_sprintf_cb_state state = { .buf = buf, .size = size };
> >  
> > -	ret = __nolibc_printf(__nolibc_sprintf_cb, (intptr_t)&state, size, fmt, args);
> > -	if (ret < 0)
> > -		return ret;
> > -	buf[(size_t)ret < size ? (size_t)ret : size - 1] = '\0';
> > -	return ret;
> > +	return __nolibc_printf(__nolibc_sprintf_cb, &state, fmt, args);
> >  }
> >  
> >  static __attribute__((unused, format(printf, 3, 4)))  
> 
> Willy

Powered by blists - more mailing lists