Re: [PATCH v2 next 06/11] tools/nolibc/printf: Use bit-masks to hold requested flag, length and conversion chars

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

On Wed, 18 Feb 2026 18:36:28 +0100
Thomas Weißschuh <linux@weissschuh.net> wrote:

...
> > > With signed chars I get:
> > > 
> > > sysroot/i386/include/stdio.h:321:37: error: comparison is always false due to limited range of data type [-Werror=type-limits]
> > >   321 |         (ch < (cmp_1 & ~0x1f) || ch > (cmp_1 | 0x1f) ? 0 : \  
> > 
> > Stupid type-limits warning.
> > It is almost impossible to get rid of without making the code unreadable.
> >   
> > >       |                                     ^
> > > sysroot/i386/include/stdio.h:389:35: note: in expansion of macro '_NOLIBC_PF_CHAR_IS_ONE_OF'
> > >   389 |                         ch_flag = _NOLIBC_PF_CHAR_IS_ONE_OF(ch, 'c', 'd', 'i', 'u', 'x', 'p');
> > >       |
> > > 
> > > This can be fixed by switching 'ch' to be always unsigned.  
> > 
> > That's likely to provoke another error elsewhere.
> > In any case optimising that test away makes the code smaller!  
> 
> We will need to get rid of this warning in some way.
> Using pragmas in nolibc proper is something we don't want to do.

I think I've a version that won't generate the warning.
Not that I get it from the version of gcc I'm using.
Just changing the ch to +ch might be enough, and the optimiser
will still optimise to a single signed compare.

> 
> > > You can run the the builtin test suite like this:
> > > -p triggers the download of the toolchains
> > > -l uses LLVM/clang instead of the downloaded toolchain
> > > 
> > > $ cd tools/testing/selftests/nolibc
> > > $ ./run-tests.sh -m user -p
> > > $ ./run-tests.sh -m user -l  
> > 
> > I've just been running:
> > 	rm nolibc-test; make -O /path/.../dir && ./nolibc-test printf  
> 
> This will miss weird things going on between different architectures.
> Probably not directly relevant for changes to printf, but still useful.
> And it shows the type limits warning.
> 
> > > > +
> > > >  typedef int (*__nolibc_printf_cb)(void *state, const char *buf, size_t size);
> > > >  
> > > >  static __attribute__((unused, format(printf, 3, 0)))
> > > >  int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list args)
> > > >  {
> > > > -	char lpref, ch;
> > > > -	unsigned long long v;
> > > > +	char ch;
> > > >  	unsigned int written, width;
> > > > +	unsigned int flags, ch_flag;
> > > >  	size_t len;
> > > >  	char tmpbuf[21];
> > > >  	const char *outstr;
> > > > @@ -265,6 +290,7 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> > > >  			break;
> > > >  
> > > >  		width = 0;
> > > > +		flags = 0;
> > > >  		if (ch != '%') {
> > > >  			while (*fmt && *fmt != '%')
> > > >  				fmt++;
> > > > @@ -274,6 +300,14 @@ int __nolibc_printf(__nolibc_printf_cb cb, void *state, const char *fmt, va_list
> > > >  
> > > >  			ch = *fmt++;
> > > >  
> > > > +			/* Conversion flag characters */
> > > > +			for (;; ch = *fmt++) {
> > > > +				ch_flag = _NOLIBC_PF_CHAR_IS_ONE_OF(ch, ' ', '#', '+', '-', '0');
> > > > +				if (!ch_flag)
> > > > +					break;
> > > > +				flags |= ch_flag;
> > > > +			}    
> > > 
> > > What is the advantage of this over:
> > > 
> > > while (1) {
> > > 	/* ... */
> > > 
> > > 	ch = *fmt++;
> > > }  
> > 
> > One line shorter :-)  
> 
> Meh. Let's not optimize for that.

Actually I looked at that code again.
The outer 'ch = *fmt++' can be put inside the loop.
It was outside the 'width' code and I inserted that bit in the gap.

...
> > > Why separate input and ouput arguments instead of one combined one ('+r')?
> > > I have been wondering the same about the kernel definition, too.  
> > 
> > I'm not sure either.
> > Certainly "+r" is more modern - which is why it isn't used in a lot of places.
> > They may be identical (indeed "+r" might get converted to the "0" form),
> > but maybe it gives better separation - I just copied the same version.  
> 
> Okay. We use '+r' in the syscall wrappers, so availability should not be
> an issue. But I don't really care on way or another.

"+r" seems to work - I will use it.

	David

> 
> 
> Thomas