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

[Willy Tarreau <w@1wt.eu>]

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@weissschuh.net> wrote:
> 
> > Hi David,
> > 
> > thanks again for your patches!
> > 
> > On 2026-03-08 11:37:25+0000, david.laight.linux@gmail.com wrote:
> > > From: David Laight <david.laight.linux@gmail.com>
> > > 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. 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 ?

Willy