[PATCH v1 0/6] vsprintf: Add __printf attribute to where it's required

[PATCH v1 0/6] vsprintf: Add __printf attribute to where it's required

[Andy Shevchenko]

This whole series started from a simple fix (see the last patch)
to make GCC (Debian 14.2.0-17) happy when compiling with `make W=1`
(note, that CONFIG_WERROR=y and all warnings break the build!)
down to a rabbit hole.

The series is organised in a strict order and can't be split or
reshuffled, otherwise see above.

Also note the last patch has a bit of a hackish approach and
I have no idea how to fix it differently, I tried a few different,
all failed. So, if you think there is a better one, please advise!

I believe the best route for the series is printk tree with immutable
tag or branch for the others.

Andy Shevchenko (6):
  seq_buf: Mark binary printing functions with __printf() attribute
  seq_file: Mark binary printing functions with __printf() attribute
  tracing: Mark binary printing functions with __printf() attribute
  vsnprintf: Mark binary printing functions with __printf() attribute
  vsnprintf: Mark pointer() with __printf() attribute
  vsnprintf: Mark va_format() with __printf() attribute

 include/linux/printk.h    |  5 ++++-
 include/linux/seq_buf.h   |  4 ++--
 include/linux/seq_file.h  |  1 +
 include/linux/string.h    |  4 ++--
 include/linux/trace_seq.h |  7 ++++---
 kernel/trace/trace.c      |  3 ---
 kernel/trace/trace.h      | 16 +++++++++-------
 lib/vsprintf.c            |  9 +++++----
 8 files changed, 27 insertions(+), 22 deletions(-)

-- 
2.47.2

[PATCH v1 1/6] seq_buf: Mark binary printing functions with __printf() attribute

[Andy Shevchenko]

Binary printing functions are using printf() type of format, and compiler
is not happy about them as is:

lib/seq_buf.c:162:17: error: function ‘seq_buf_bprintf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]

Fix the compilation errors by adding __printf() attribute.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/seq_buf.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index fe41da005970..52791e070506 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -167,8 +167,8 @@ extern int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str,
 			    const void *buf, size_t len, bool ascii);
 
 #ifdef CONFIG_BINARY_PRINTF
-extern int
-seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
+__printf(2, 0)
+int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
 #endif
 
 void seq_buf_do_printk(struct seq_buf *s, const char *lvl);
-- 
2.47.2

Re: [PATCH v1 1/6] seq_buf: Mark binary printing functions with __printf() attribute

[Steven Rostedt]

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

> Binary printing functions are using printf() type of format, and compiler
> is not happy about them as is:
> 
> lib/seq_buf.c:162:17: error: function ‘seq_buf_bprintf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
> 
> Fix the compilation errors by adding __printf() attribute.
> 

Should also note the removal of "extern"

-- Steve

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/seq_buf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index fe41da005970..52791e070506 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -167,8 +167,8 @@ extern int seq_buf_hex_dump(struct seq_buf *s, const char *prefix_str,
>  			    const void *buf, size_t len, bool ascii);
>  
>  #ifdef CONFIG_BINARY_PRINTF
> -extern int
> -seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
> +__printf(2, 0)
> +int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary);
>  #endif
>  
>  void seq_buf_do_printk(struct seq_buf *s, const char *lvl);

Re: [PATCH v1 1/6] seq_buf: Mark binary printing functions with __printf() attribute

[Andy Shevchenko]

On Mon, Mar 24, 2025 at 6:03 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 20 Mar 2025 20:04:22 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > Binary printing functions are using printf() type of format, and compiler
> > is not happy about them as is:
> >
> > lib/seq_buf.c:162:17: error: function ‘seq_buf_bprintf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
> >
> > Fix the compilation errors by adding __printf() attribute.
>
> Should also note the removal of "extern"

If it is worth it, why not? The idea is to make it slightly more
consistent with code around (file seems to have both approaches).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/6] seq_buf: Mark binary printing functions with __printf() attribute

[Andy Shevchenko]

On Mon, Mar 24, 2025 at 12:04:30PM -0400, Steven Rostedt wrote:
> On Thu, 20 Mar 2025 20:04:22 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > Binary printing functions are using printf() type of format, and compiler
> > is not happy about them as is:
> > 
> > lib/seq_buf.c:162:17: error: function ‘seq_buf_bprintf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
> > 
> > Fix the compilation errors by adding __printf() attribute.
> > 
> 
> Should also note the removal of "extern"

Ah, just noticed that you are looking at v1, there is also v2 available:
20250321144822.324050-1-andriy.shevchenko@linux.intel.com

Thank you for the review, nevertheless!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/6] seq_file: Mark binary printing functions with __printf() attribute

[Andy Shevchenko]

Binary printing functions are using printf() type of format, and compiler
is not happy about them as is:

fs/seq_file.c:418:35: error: function ‘seq_bprintf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]

Fix the compilation errors by adding __printf() attribute.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/seq_file.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 2fb266ea69fa..d6ebf0596510 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -181,6 +181,7 @@ int seq_open_private(struct file *, const struct seq_operations *, int);
 int seq_release_private(struct inode *, struct file *);
 
 #ifdef CONFIG_BINARY_PRINTF
+__printf(2, 0)
 void seq_bprintf(struct seq_file *m, const char *f, const u32 *binary);
 #endif
 
-- 
2.47.2

[PATCH v1 3/6] tracing: Mark binary printing functions with __printf() attribute

[Andy Shevchenko]

Binary printing functions are using printf() type of format, and compiler
is not happy about them as is:

kernel/trace/trace.c:3292:9: error: function ‘trace_vbprintk’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
kernel/trace/trace_seq.c:182:9: error: function ‘trace_seq_bprintf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]

Fix the compilation errors by adding __printf() attribute.

While at it, move existing __printf() attributes from the implementations
to the declarations.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/trace_seq.h |  7 ++++---
 kernel/trace/trace.c      |  3 ---
 kernel/trace/trace.h      | 16 +++++++++-------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
index 1ef95c0287f0..7e69628092e4 100644
--- a/include/linux/trace_seq.h
+++ b/include/linux/trace_seq.h
@@ -88,8 +88,8 @@ extern __printf(2, 3)
 void trace_seq_printf(struct trace_seq *s, const char *fmt, ...);
 extern __printf(2, 0)
 void trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args);
-extern void
-trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary);
+extern __printf(2, 0)
+void trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary);
 extern int trace_print_seq(struct seq_file *m, struct trace_seq *s);
 extern int trace_seq_to_user(struct trace_seq *s, char __user *ubuf,
 			     int cnt);
@@ -113,7 +113,8 @@ static inline __printf(2, 3)
 void trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
 {
 }
-static inline void
+static inline __printf(2, 0)
+void
 trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
 {
 }
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 0e6d517e74e0..6a29218ca210 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3375,7 +3375,6 @@ __trace_array_vprintk(struct trace_buffer *buffer,
 	return len;
 }
 
-__printf(3, 0)
 int trace_array_vprintk(struct trace_array *tr,
 			unsigned long ip, const char *fmt, va_list args)
 {
@@ -3450,7 +3449,6 @@ int trace_array_init_printk(struct trace_array *tr)
 }
 EXPORT_SYMBOL_GPL(trace_array_init_printk);
 
-__printf(3, 4)
 int trace_array_printk_buf(struct trace_buffer *buffer,
 			   unsigned long ip, const char *fmt, ...)
 {
@@ -3466,7 +3464,6 @@ int trace_array_printk_buf(struct trace_buffer *buffer,
 	return ret;
 }
 
-__printf(2, 0)
 int trace_vprintk(unsigned long ip, const char *fmt, va_list args)
 {
 	return trace_array_vprintk(printk_trace, ip, fmt, args);
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 9c21ba45b7af..447d4f2a7fd2 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -836,13 +836,15 @@ static inline void __init disable_tracing_selftest(const char *reason)
 
 extern void *head_page(struct trace_array_cpu *data);
 extern unsigned long long ns2usecs(u64 nsec);
-extern int
-trace_vbprintk(unsigned long ip, const char *fmt, va_list args);
-extern int
-trace_vprintk(unsigned long ip, const char *fmt, va_list args);
-extern int
-trace_array_vprintk(struct trace_array *tr,
-		    unsigned long ip, const char *fmt, va_list args);
+
+__printf(2, 0)
+int trace_vbprintk(unsigned long ip, const char *fmt, va_list args);
+__printf(2, 0)
+int trace_vprintk(unsigned long ip, const char *fmt, va_list args);
+__printf(3, 0)
+int trace_array_vprintk(struct trace_array *tr,
+			unsigned long ip, const char *fmt, va_list args);
+__printf(3, 4)
 int trace_array_printk_buf(struct trace_buffer *buffer,
 			   unsigned long ip, const char *fmt, ...);
 void trace_printk_seq(struct trace_seq *s);
-- 
2.47.2

Re: [PATCH v1 3/6] tracing: Mark binary printing functions with __printf() attribute

[Andy Shevchenko]

On Thu, Mar 20, 2025 at 08:04:24PM +0200, Andy Shevchenko wrote:
> Binary printing functions are using printf() type of format, and compiler
> is not happy about them as is:
> 
> kernel/trace/trace.c:3292:9: error: function ‘trace_vbprintk’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
> kernel/trace/trace_seq.c:182:9: error: function ‘trace_seq_bprintf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
> 
> Fix the compilation errors by adding __printf() attribute.
> 
> While at it, move existing __printf() attributes from the implementations
> to the declarations.

This also missed removal of one new line and __printf() in the C file.
Will be improved in v2.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/6] tracing: Mark binary printing functions with __printf() attribute

[Steven Rostedt]

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

> @@ -113,7 +113,8 @@ static inline __printf(2, 3)
>  void trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
>  {
>  }
> -static inline void
> +static inline __printf(2, 0)
> +void
>  trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
>  {
>  }

Do we need to split the line after the __printf()? Can't the above be:

static inline __printf(2, 0) void
trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)

Or even:

__printf(2, 0)
static inline void
trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)

I rather not split the prefix elements of a function over two lines. I
rather not even split them from the function itself, but tend to do that if
space is needed.

-- Steve

Re: [PATCH v1 3/6] tracing: Mark binary printing functions with __printf() attribute

[Andy Shevchenko]

On Mon, Mar 24, 2025 at 6:02 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 20 Mar 2025 20:04:24 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> > -static inline void
> > +static inline __printf(2, 0)
> > +void
> >  trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
> >  {
> >  }
>
> Do we need to split the line after the __printf()? Can't the above be:
>
> static inline __printf(2, 0) void
> trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)
>
> Or even:
>
> __printf(2, 0)
> static inline void
> trace_seq_bprintf(struct trace_seq *s, const char *fmt, const u32 *binary)

I really tried hard to be consistent with something. tracing code is
most inconsistent in the regard to attributes and other things (at
least in this patch series).

> I rather not split the prefix elements of a function over two lines. I
> rather not even split them from the function itself, but tend to do that if
> space is needed.

I also looked at the approaches that are used in include/linux and
tried to follow that when in doubt. And I think the way to leave
static inline leading the stub is most used, followed by attribute.
Then for the declaration is all the same, leading attribute followed
by the returning type and so on...



-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/6] vsnprintf: Mark binary printing functions with __printf() attribute

[Andy Shevchenko]

Binary printf() functions are using printf() type of format, and compiler
is not happy about them as is:

lib/vsprintf.c:3130:47: error: function ‘vbin_printf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
lib/vsprintf.c:3298:33: error: function ‘bstr_printf’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]

Fix the compilation errors by adding __printf() attribute.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/string.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index f8e21e80942f..f15696e8e4d4 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -336,8 +336,8 @@ int __sysfs_match_string(const char * const *array, size_t n, const char *s);
 #define sysfs_match_string(_a, _s) __sysfs_match_string(_a, ARRAY_SIZE(_a), _s)
 
 #ifdef CONFIG_BINARY_PRINTF
-int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
-int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf);
+__printf(3, 0) int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
+__printf(3, 0) int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf);
 #endif
 
 extern ssize_t memory_read_from_buffer(void *to, size_t count, loff_t *ppos,
-- 
2.47.2

[PATCH v1 5/6] vsnprintf: Mark pointer() with __printf() attribute

[Andy Shevchenko]

pointer() is using printf() type of format, and GCC compiler
(Debian 14.2.0-17) is not happy about this:

lib/vsprintf.c:2466:17: error: function ‘pointer’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]

Fix the compilation errors (`make W=1` when CONFIG_WERROR=y, which is default)
by adding __printf() attribute.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 56fe96319292..8ebb5f866b08 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2419,7 +2419,7 @@ char *rust_fmt_argument(char *buf, char *end, void *ptr);
  * from Rust code to format core::fmt::Arguments. Do *not* use it from C.
  * See rust/kernel/print.rs for details.
  */
-static noinline_for_stack
+static noinline_for_stack __printf(1, 0)
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
-- 
2.47.2

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

[Rasmus Villemoes]

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

> pointer() is using printf() type of format, and GCC compiler
> (Debian 14.2.0-17) is not happy about this:
>
> lib/vsprintf.c:2466:17: error: function ‘pointer’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
>
> Fix the compilation errors (`make W=1` when CONFIG_WERROR=y, which is default)
> by adding __printf() attribute.
>

I had quite a bit of trouble reproducing, until I realized I had to
apply your patches in reverse order, because adding the attribute to one
function will then "taint" its callers.

So this one seems to be self-inflicted pain by the annotation of
va_format (which is completely broken, I'll reply separately to that
one). This doesn't solve the false warning for va_format(), but how
about we at least do

 static char *va_format(char *buf, char *end, struct va_format *va_fmt,
-                      struct printf_spec spec, const char *fmt)
+                      struct printf_spec spec)
 {

        case 'V':
-               return va_format(buf, end, ptr, spec, fmt);
+               return va_format(buf, end, ptr, spec);
        case 'K':

because va_format() doesn't use that fmt argument at all.

Rasmus

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

[Andy Shevchenko]

On Fri, Mar 21, 2025 at 02:43:18PM +0100, Rasmus Villemoes wrote:
> On Thu, Mar 20 2025, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > pointer() is using printf() type of format, and GCC compiler
> > (Debian 14.2.0-17) is not happy about this:
> >
> > lib/vsprintf.c:2466:17: error: function ‘pointer’ might be a candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
> >
> > Fix the compilation errors (`make W=1` when CONFIG_WERROR=y, which is default)
> > by adding __printf() attribute.
> >
> 
> I had quite a bit of trouble reproducing, until I realized I had to
> apply your patches in reverse order, because adding the attribute to one
> function will then "taint" its callers.

Exactly, that's why cover letter has "strict order" mention.

> So this one seems to be self-inflicted pain by the annotation of
> va_format (which is completely broken, I'll reply separately to that
> one). This doesn't solve the false warning for va_format(), but how
> about we at least do
> 
>  static char *va_format(char *buf, char *end, struct va_format *va_fmt,
> -                      struct printf_spec spec, const char *fmt)
> +                      struct printf_spec spec)
>  {
> 
>         case 'V':
> -               return va_format(buf, end, ptr, spec, fmt);
> +               return va_format(buf, end, ptr, spec);
>         case 'K':
> 
> because va_format() doesn't use that fmt argument at all.

Yes, I was thinking about this. I'll do it in a separate patch in v2.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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':
-- 
2.47.2

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

[Rasmus Villemoes]

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

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

[Andy Shevchenko]

On Fri, Mar 21, 2025 at 03:09:20PM +0100, Rasmus Villemoes wrote:
> On Thu, Mar 20 2025, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

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

You shouldn't be sorry, my guts feeling was on the same page, I was sending it
with the expectation that someone will direct me, so thank you!

(And that's why there is a paragraph about this rather hackish patch)

> 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.

True, and again, I felt that what I'm doing here is all not right.

> 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.

clang doesn't produce this warning to me. But I will check again.

> Then you don't need to annotate pointer(),

Sure, I also felt that that one is hack to satisfy a broken tool.

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

I prefer to have them marked as they really like printf():s.

Thanks for the suggestion, I will experiment and send the result in v2.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/6] vsprintf: Add __printf attribute to where it's required

[Andy Shevchenko]

On Thu, Mar 20, 2025 at 08:04:21PM +0200, Andy Shevchenko wrote:
> This whole series started from a simple fix (see the last patch)
> to make GCC (Debian 14.2.0-17) happy when compiling with `make W=1`
> (note, that CONFIG_WERROR=y and all warnings break the build!)
> down to a rabbit hole.
> 
> The series is organised in a strict order and can't be split or
> reshuffled, otherwise see above.
> 
> Also note the last patch has a bit of a hackish approach and
> I have no idea how to fix it differently, I tried a few different,
> all failed. So, if you think there is a better one, please advise!
> 
> I believe the best route for the series is printk tree with immutable
> tag or branch for the others.

Alternatively first 4 can be done first as they are pretty much
straightforward. They also can be squashed to one (as the same
topic behind), but it all is up to the respective maintainers.

-- 
With Best Regards,
Andy Shevchenko