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

[Thomas Weißschuh]

On 2026-02-06 19:11:16+0000, david.laight.linux@xxxxxxxxx wrote:
> From: David Laight <david.laight.linux@xxxxxxxxx>
> 
> Use flags bits (1u << (ch & 31)) for the flags, length modifiers, and
> conversion specifiers.
> This makes it easy to test for multiple values at once.
> 
> Detect the conversion flags " #+-0" although they are currently all ignored.
> 
> Add support for length modifiers 't' and 'z' (both long) and 'q' and 'L'
> (both long long).
> 
> Add support for "%i" (the same as %d").

Would it be much work to split the new functionality into its own patch?
> 
> Unconditionally generate the signed values (for %d) to remove a second
> set of checks for the size.
> 
> Signed-off-by: David Laight <david.laight.linux@xxxxxxxxx>
> ---
> 
> Changes for v2:
> - Use #defines to make the code a lot more readable.
> - Include the changes from the old patch 10 that used masks for the
>   conversion specifiers.
> - Detect all the valid flag characters even though they are not implemented.
> - Support for left justifying field is moved to patch 7.
> 
>  tools/include/nolibc/stdio.h | 151 ++++++++++++++++++++++++-----------
>  1 file changed, 103 insertions(+), 48 deletions(-)
> 
> diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h
> index bb54f488c228..b14cf8224403 100644
> --- a/tools/include/nolibc/stdio.h
> +++ b/tools/include/nolibc/stdio.h
> @@ -240,19 +240,44 @@ char *fgets(char *s, int size, FILE *stream)
>  }
>  
>  
> -/* minimal printf(). It supports the following formats:
> - *  - %[l*]{d,u,c,x,p}
> - *  - %s
> - *  - unknown modifiers are ignored.
> +/* 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
>   */
> +
> +/* This code uses 'flag' variables that are indexed by the low 6 bits
> + * of characters to optimise checks for multiple characters.
> + *
> + * _NOLIBC_PF_FLAGS_CONTAIN(flags, 'a', 'b'. ...)
> + * returns non-zero if the bit for any of the specified characters is set.
> + *
> + * _NOLIBC_PF_CHAR_IS_ONE_OF(ch, 'a', 'b'. ...)
> + * returns the flag bit for ch if it is one of the specified characters.
> + * All the characters must be in the same 32 character block (non-alphabetic,
> + * upper case, or lower case) of the ASCII character set.)
> + */
> +#define _NOLIBC_PF_FLAG(ch) (1u << ((ch) & 0x1f))
> +#define _NOLIBC_PF_FLAG_NZ(ch) ((ch) ? _NOLIBC_PF_FLAG(ch) : 0)
> +#define _NOLIBC_PF_FLAG8(cmp_1, cmp_2, cmp_3, cmp_4, cmp_5, cmp_6, cmp_7, cmp_8, ...) \
> +	(_NOLIBC_PF_FLAG_NZ(cmp_1) | _NOLIBC_PF_FLAG_NZ(cmp_2) | \
> +	 _NOLIBC_PF_FLAG_NZ(cmp_3) | _NOLIBC_PF_FLAG_NZ(cmp_4) | \
> +	 _NOLIBC_PF_FLAG_NZ(cmp_5) | _NOLIBC_PF_FLAG_NZ(cmp_6) | \
> +	 _NOLIBC_PF_FLAG_NZ(cmp_7) | _NOLIBC_PF_FLAG_NZ(cmp_8))
> +#define _NOLIBC_PF_FLAGS_CONTAIN(flags, ...) \
> +	((flags) & _NOLIBC_PF_FLAG8(__VA_ARGS__, 0, 0, 0, 0, 0, 0, 0))
> +#define _NOLIBC_PF_CHAR_IS_ONE_OF(ch, cmp_1, ...) \
> +	(ch < (cmp_1 & ~0x1f) || ch > (cmp_1 | 0x1f) ? 0 : \
> +		_NOLIBC_PF_FLAGS_CONTAIN(_NOLIBC_PF_FLAG(ch), cmp_1, __VA_ARGS__))

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 : \
      |                                     ^
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.

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

> +
>  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++;
}

Or combine it with the 'ch = *fmt++' from above and do:

while (1) {
	ch = *fmt++;

	/* ... */
}

These look much simpler to me.

> +
>  			/* width */
>  			while (ch >= '0' && ch <= '9') {
>  				width *= 10;

(...)

> +do_output:
>  		written += len;
>  
> +		/* An OPTIMIZER_HIDE_VAR() seems to stop gcc back-merging this
> +		 * code into one of the conditionals above.
> +		 */
> +		__asm__ volatile("" : "=r"(len) : "0"(len));

Can we make a definition in nolibc/compiler.h out of this?
Why the 'volatile'? Wo don't have that in the kernel.
Why separate input and ouput arguments instead of one combined one ('+r')?
I have been wondering the same about the kernel definition, too.

> +
>  		while (width > len) {
>  			unsigned int pad_len = ((width - len - 1) & 15) + 1;
>  			width -= pad_len;
> -- 
> 2.39.5
>