| Message ID | 20250320180926.4002817-7-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vsprintf: Add __printf attribute to where it's required | expand |
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
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.
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':
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(-)