Re: [PATCH v5 next 00/17] Enhance printf()

[David Laight]

On Mon, 9 Mar 2026 07:55:30 +0100
Willy Tarreau <w@xxxxxx> wrote:

> Hi David,
> 
> On Sun, Mar 08, 2026 at 10:41:21PM +0000, David Laight wrote:
> > On Sun, 8 Mar 2026 22:01:19 +0100
> > Thomas Weißschuh <linux@xxxxxxxxxxxxxx> wrote:
> >   
> > > Hi David,
> > > 
> > > thanks again for your patches!
> > > 
> > > On 2026-03-08 11:37:25+0000, david.laight.linux@xxxxxxxxx wrote:  
> > > > From: David Laight <david.laight.linux@xxxxxxxxx>
> > > > David Laight (17):
> > > >   tools/nolibc: Add _NOLIBC_OPTIMIZER_HIDE_VAR() to compiler.h
> > > >   selftests/nolibc: Rename w to written in expect_vfprintf()
> > > >   tools/nolibc: Implement strerror() in terms of strerror_r()
> > > >   tools/nolibc: Rename the 'errnum' parameter to strerror()
> > > >   tools/nolibc/printf: Output pad characters in 16 byte chunks
> > > >   tools/nolibc/printf: Simplify __nolibc_printf()
> > > >   tools/nolibc/printf: Use goto and reduce indentation
> > > >   tools/nolibc/printf: Use bit-masks to hold requested flag, length and
> > > >     conversion chars
> > > >   tools/nolibc/printf: Add support for length modifiers tzqL and formats
> > > >     iX
> > > >   tools/nolibc/printf: Handle "%s" with the numeric formats
> > > >   tools/nolibc/printf: Prepend sign to converted number
> > > >   tools/nolibc/printf: Add support for conversion flags space and plus
> > > >   tools/nolibc/printf: Special case 0 and add support for %#x
> > > >   tools/nolibc/printf: Add support for left aligning fields
> > > >   tools/nolibc/printf: Add support for zero padding and field precision
> > > >   tools/nolibc/printf: Add support for octal output    
> > > 
> > > Beginning from here we have another sign-compare warning:
> > > 
> > > /home/t-8ch/.cache/crosstools/gcc-13.2.0-nolibc/i386-linux/bin/i386-linux-gcc -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 -W -Wall -Wextra -fno-stack-protector -Wmissing-prototypes  -fstack-protector-all -mstack-protector-guard=global -fsanitize=undefined -fsanitize-trap=all -m32 -Werror -Wl,--fatal-warnings  -o nolibc-test \
> > >   -nostdlib -nostdinc -static -Isysroot/i386/include nolibc-test.c nolibc-test-linkage.c -lgcc
> > > In file included from sysroot/i386/include/nolibc.h:123,
> > >                  from sysroot/i386/include/stdio.h:8,
> > >                  from nolibc-test.c:12:
> > > sysroot/i386/include/stdio.h: In function '__nolibc_printf':
> > > sysroot/i386/include/stdio.h:569:41: error: comparison of integer expressions of different signedness: 'unsigned int' and 'char' [-Werror=sign-compare]
> > >   569 |                         if (sign_prefix != *out) {
> > >       |                                         ^~
> > > cc1: all warnings being treated as errors
> > > 
> > > I have applied all of the patches *before*
> > > "tools/nolibc/printf: Prepend sign to converted number", which
> > > introduced the sign_prefix variable.
> > > 
> > > Could you fix this up and repost the remaining patches?  
> > 
> > I hate 'sign compare' ...
> > I dislike using casts to 'fix' it - random 'integer' casts have caused me
> > grief in the past.  
> 
> The cast here is unavoidable. One is a signed char, the other is an unsigned
> int, so the comparison can be done in 3 methods depending on the developer's
> original intent:
>   - 8 bits of the char against 8 lower bits of the unsigned int
>   - signed comparison where char is sign-extended and both are compared
>     as a 32-bit signed int
>   - unsigned comparison where char is zero-extended and both are compared
>     as a 32-bit unsigned int
> 
> > I don't want to make 'sign_prefix' signed - stops you adding 4 characters
> > (should you so desire); not to mention >> being either UB or implementation
> > defined on signed values (or maybe just negative ones).
> > 
> > The two obvious fixes are:
> > 	if (sign_prefix - *out)
> > or:
> > 	if (sign_prefix != *out + 0u) 
> > 
> > Your pick :-)
> > 
> > At least it is only the last couple of patches.  
> 
> It will not change much or will just even more hide the problem.

There is nothing to change, the 'problem' just need hiding so the
compiler doesn't complain.

> My understanding of that code is that it's neither of these cases. Based
> on the comment you apparently want in this test to only check for the
> first byte of sign_prefix against *out, right ? So that should be:
> 
>  	if ((char)sign_prefix != *out)
> 
> Did I get it right ?

Not really, the check can only succeed when both values are '0'
(which is the real condition being tested - as in the comment).

If you add that (char) cast you'll get an unnecessary '& 0xff' on
everything except x86 (where you might get a byte compare to memory
instead of a sign/zero extending memory read and register compare).

An alternative way of stopping the compiler complaining would be:
	if (sign_prefix != *(unsigned char *)out)

I'm waiting for the 'security' people to stop worrying about string
truncation and turn their attention to casts of integers :-)

	David

> 
> Willy