Re: [PATCH v1 6/6] vsnprintf: Mark va_format() with __printf() attribute

[Rasmus Villemoes <ravi@prevas.dk>]

On Thu, Mar 20 2025, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> va_format() is using printf() type of format, and GCC compiler
> (Debian 14.2.0-17) is not happy about this:
>
> lib/vsprintf.c:1704:9: error: function ‘va_format’ might be a candidate for ‘gnu_print ’ format attribute [-Werror=suggest-attribute=format]
>
> Fix the compilation errors (`make W=1` when CONFIG_WERROR=y, which is default)                                   by adding __printf() attribute. This, unfortunately, requires to reconsider
> the type of the parameter used for that. That's why I added static_assert()
> and used explicit casting. Any other solution I tried failed with the similar
> or other error.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/printk.h | 5 ++++-
>  lib/vsprintf.c         | 7 ++++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 4217a9f412b2..182d48b4930f 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -2,12 +2,13 @@
>  #ifndef __KERNEL_PRINTK__
>  #define __KERNEL_PRINTK__
>  
> -#include <linux/stdarg.h>
>  #include <linux/init.h>
>  #include <linux/kern_levels.h>
>  #include <linux/linkage.h>
>  #include <linux/ratelimit_types.h>
>  #include <linux/once_lite.h>
> +#include <linux/stdarg.h>
> +#include <linux/stddef.h>
>  
>  struct console;
>  
> @@ -87,6 +88,8 @@ struct va_format {
>  	va_list *va;
>  };
>  
> +static_assert(offsetof(struct va_format, fmt) == 0);
> +
>  /*
>   * FW_BUG
>   * Add this to a message where you are sure the firmware is buggy or behaves
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 8ebb5f866b08..ebb3c563a7ee 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1692,9 +1692,10 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>  	return buf;
>  }
>  
> -static char *va_format(char *buf, char *end, struct va_format *va_fmt,
> -		       struct printf_spec spec, const char *fmt)
> +static __printf(3, 0)
> +char *va_format(char *buf, char *end, const char *fmt, struct printf_spec spec)
>  {
> +	struct va_format *va_fmt = (struct va_format *)fmt;
>  	va_list va;
>  
>  	if (check_pointer(&buf, end, va_fmt, spec))
> @@ -2462,7 +2463,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	case 'U':
>  		return uuid_string(buf, end, ptr, spec, fmt);
>  	case 'V':
> -		return va_format(buf, end, ptr, spec, fmt);
> +		return va_format(buf, end, ptr, spec);
>  	case 'K':
>  		return restricted_pointer(buf, end, ptr, spec);
>  	case 'N':

I'm sorry, but this is broken in so many ways I don't know where to
start.

The format string that va_format actually passes on is va_fmt->fmt, and
that is _not_ at all the same thing as va_fmt cast to (const char*),
even if ->fmt is the first member, so the static_assert doesn't do what
you think it does. So of course the ptr variable (which is (void*)) can
be passed as a (const char*) argument just as well as it can be passed
as a (struct va_format *) argument, and sure, the callee can take that
arbitrary pointer and cast to the real type of that argument. But it
seems you did that only to have _some_ random const char* parameter to
attach that __printf attribute to.

So, since the format string that va_format() passes to vsnprintf() is
not one of va_format()'s own parameters, there is no parameter to attach
that __printf() attribute to. So I actually consider this somewhat of a
gcc bug. But can't we just silence that false positive with the tool
that gcc provides for this very purpose:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
static char *va_format(char *buf, char *end, struct va_format *va_fmt,
...
}
#pragma GCC diagnostic pop

with whatever added sauce to also make it work for clang.

Then you don't need to annotate pointer(), and then you don't need to
annotate the BINARY_PRINTF users of pointer(), though I think those two
do make sense.

Rasmus