include/linux/compiler-gcc.h | 2 ++ lib/vsprintf.c | 9 ++++----- 2 files changed, 6 insertions(+), 5 deletions(-)
Hi all,
This is a follow up to the complaint that Linus made at [1] about how
the #pragma and #ifdef to disable -Wsuggest-attribute=format is
currently ugly. Convert the #pragma and #ifdef to the existing __diag()
infrastructure in the kernel to hide some of the ugliness.
I am sending it to both the vsprintf maintainers/reviewers and Linus, in
case he wants to apply it himself (since it is pretty simple).
[1]: https://lore.kernel.org/CAHk-=wgfX9nBGE0Ap9GjhOy7Mn=RSy=rx0MvqfYFFDx31KJXqQ@mail.gmail.com/
---
Nathan Chancellor (2):
compiler-gcc.h: Introduce __diag_GCC_all
vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'
include/linux/compiler-gcc.h | 2 ++
lib/vsprintf.c | 9 ++++-----
2 files changed, 6 insertions(+), 5 deletions(-)
---
base-commit: 9554264e302cccf4c2a1e9972f2e707b09ef74fd
change-id: 20250404-vsprintf-convert-pragmas-to-__diag-df7a84851853
Best regards,
--
Nathan Chancellor <nathan@kernel.org>
Fri, Apr 04, 2025 at 03:10:01PM -0700, Nathan Chancellor kirjoitti: > Hi all, > > This is a follow up to the complaint that Linus made at [1] about how > the #pragma and #ifdef to disable -Wsuggest-attribute=format is > currently ugly. Convert the #pragma and #ifdef to the existing __diag() > infrastructure in the kernel to hide some of the ugliness. > > I am sending it to both the vsprintf maintainers/reviewers and Linus, in > case he wants to apply it himself (since it is pretty simple). Hmm... You haven't put my tag, nor added me to Cc list... > [1]: https://lore.kernel.org/CAHk-=wgfX9nBGE0Ap9GjhOy7Mn=RSy=rx0MvqfYFFDx31KJXqQ@mail.gmail.com/ -- With Best Regards, Andy Shevchenko
Sat, Apr 05, 2025 at 07:51:32PM +0300, Andy Shevchenko kirjoitti: > Fri, Apr 04, 2025 at 03:10:01PM -0700, Nathan Chancellor kirjoitti: > > Hi all, > > > > This is a follow up to the complaint that Linus made at [1] about how > > the #pragma and #ifdef to disable -Wsuggest-attribute=format is > > currently ugly. Convert the #pragma and #ifdef to the existing __diag() > > infrastructure in the kernel to hide some of the ugliness. > > > > I am sending it to both the vsprintf maintainers/reviewers and Linus, in > > case he wants to apply it himself (since it is pretty simple). > > Hmm... You haven't put my tag, nor added me to Cc list... Ah, now I see it, it was send to the @linux.intel.com address. What is strange is that lore web UI doesn't show it for me: https://lore.kernel.org/all/?q=%22Andy+Shevchenko%22+-f%3A%22Andy+Shevchenko%22 > > [1]: https://lore.kernel.org/CAHk-=wgfX9nBGE0Ap9GjhOy7Mn=RSy=rx0MvqfYFFDx31KJXqQ@mail.gmail.com/ -- With Best Regards, Andy Shevchenko
On Fri, 04 Apr 2025 15:10:01 -0700 Nathan Chancellor <nathan@kernel.org> wrote: > Hi all, > > This is a follow up to the complaint that Linus made at [1] about how > the #pragma and #ifdef to disable -Wsuggest-attribute=format is > currently ugly. Convert the #pragma and #ifdef to the existing __diag() > infrastructure in the kernel to hide some of the ugliness. It's still horribly ugly. Perhaps the compilers ought to support __attribute__((format(none))) to disable the warning. And then disable it for older compilers. David
On Sat, 5 Apr 2025 at 02:11, David Laight <david.laight.linux@gmail.com> wrote:
>
> Perhaps the compilers ought to support __attribute__((format(none)))
> to disable the warning.
D'oh, that's a good idea.
And gcc already supports it, even if we have to hack it up.
So let's remove this whole horrible garbage entirely, and replace it
with __printf(1,0) which should do exactly that.
The 1 is for the format string argument number, and we're just *lying*
about it. But there is not format string argument, and gcc just checks
for 'is it a char pointer).
The real format string argument is va_fmt->fmt, but there's no way to
tell gcc that.
And the 0 is is to tell gcc that there's nothing to verify.
Then, if you do that, gcc will say "oh, maybe you need to do the same
for the 'pointer()' function". That one has a real 'fmt' thing, but
again nothing to be checked, so we do the same '__printf(1,0)' there
too.
There it makes more sense, because argument 1 _is_ actually a format
string, so we're not lying about it.
IOW, something like this:
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1700,9 +1700,10 @@ char *escaped_string(...
}
-#pragma GCC diagnostic push
-#ifndef __clang__
-#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
-#endif
-static char *va_format(char *buf, char *end, struct va_format *va_fmt,
+/*
+ * The '__printf(1,0)' thing is a hack make gcc not ask us to use a
+ * a format attribute. 'buf' is *not* the format, 'va_fmt->fmt' is.
+ */
+static __printf(1,0)
+char *va_format(char *buf, char *end, struct va_format *va_fmt,
struct printf_spec spec)
{
@@ -1718,5 +1719,4 @@ static char *va_format(...
return buf;
}
-#pragma GCC diagnostic pop
static noinline_for_stack
@@ -2429,5 +2429,5 @@ early_param(...
* 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)
Does that work for people who see this warning?
Linus
On Sat, Apr 05 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sat, 5 Apr 2025 at 02:11, David Laight <david.laight.linux@gmail.com> wrote:
>>
>> Perhaps the compilers ought to support __attribute__((format(none)))
>> to disable the warning.
>
> D'oh, that's a good idea.
>
> And gcc already supports it, even if we have to hack it up.
>
> So let's remove this whole horrible garbage entirely, and replace it
> with __printf(1,0) which should do exactly that.
>
> The 1 is for the format string argument number, and we're just *lying*
> about it. But there is not format string argument, and gcc just checks
> for 'is it a char pointer).
>
> The real format string argument is va_fmt->fmt, but there's no way to
> tell gcc that.
>
> And the 0 is is to tell gcc that there's nothing to verify.
>
> Then, if you do that, gcc will say "oh, maybe you need to do the same
> for the 'pointer()' function". That one has a real 'fmt' thing, but
> again nothing to be checked, so we do the same '__printf(1,0)' there
> too.
>
> There it makes more sense, because argument 1 _is_ actually a format
> string, so we're not lying about it.
>
> IOW, something like this:
>
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1700,9 +1700,10 @@ char *escaped_string(...
> }
>
> -#pragma GCC diagnostic push
> -#ifndef __clang__
> -#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> -#endif
> -static char *va_format(char *buf, char *end, struct va_format *va_fmt,
> +/*
> + * The '__printf(1,0)' thing is a hack make gcc not ask us to use a
> + * a format attribute. 'buf' is *not* the format, 'va_fmt->fmt' is.
> + */
> +static __printf(1,0)
> +char *va_format(char *buf, char *end, struct va_format *va_fmt,
> struct printf_spec spec)
> {
> @@ -1718,5 +1719,4 @@ static char *va_format(...
> return buf;
> }
> -#pragma GCC diagnostic pop
>
> static noinline_for_stack
> @@ -2429,5 +2429,5 @@ early_param(...
> * 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)
>
> Does that work for people who see this warning?
IMHO, this is much worse.
Yes, as I also said in the previous thread, I consider the
warning/suggestion here a gcc bug, as it shouldn't make that suggestion
when one doesn't pass any of the function's arguments as the fmt
argument to another __format__(()) annotated-function.
But we have this __diag infrastructure exactly to silence special cases
(and sorry I forgot about that when suggesting the #pragma approach to
Andy), and this is very much a special case: It's the only place in the
whole codebase that has any reason to dereference that va_fmt, and any
other function anywhere calling a vsprintf()-like really should have
gotten the format string that goes along with the varargs from its
caller.
As this is apparently some newer gcc that has started doing this, you
just risk the next version turning the wrongness to 11 and complaining
that "buf" or "fmt" is not passed to a vsprintf-like function. Let's not
do "a hack make gcc not ask us to use a format attribute" when we have
a proper way to selectively silence such false-positives. If this was
something happening all over, we'd do -Wno-suggest-attribute=format, not
spread these annotations. But this really is a special case in the guts
of our printf implementation.
So, FWIW, ack on Nathan's fixups, nak on this one.
Rasmus
On Mon 2025-04-07 09:31:28, Rasmus Villemoes wrote:
> On Sat, Apr 05 2025, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Sat, 5 Apr 2025 at 02:11, David Laight <david.laight.linux@gmail.com> wrote:
> >>
> >> Perhaps the compilers ought to support __attribute__((format(none)))
> >> to disable the warning.
> >
> > D'oh, that's a good idea.
> >
> > And gcc already supports it, even if we have to hack it up.
> >
> > So let's remove this whole horrible garbage entirely, and replace it
> > with __printf(1,0) which should do exactly that.
> >
> > The 1 is for the format string argument number, and we're just *lying*
> > about it. But there is not format string argument, and gcc just checks
> > for 'is it a char pointer).
> >
> > The real format string argument is va_fmt->fmt, but there's no way to
> > tell gcc that.
> >
> > And the 0 is is to tell gcc that there's nothing to verify.
> >
> > Then, if you do that, gcc will say "oh, maybe you need to do the same
> > for the 'pointer()' function". That one has a real 'fmt' thing, but
> > again nothing to be checked, so we do the same '__printf(1,0)' there
> > too.
> >
> > There it makes more sense, because argument 1 _is_ actually a format
> > string, so we're not lying about it.
> >
> > IOW, something like this:
> >
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1700,9 +1700,10 @@ char *escaped_string(...
> > }
> >
> > -#pragma GCC diagnostic push
> > -#ifndef __clang__
> > -#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> > -#endif
> > -static char *va_format(char *buf, char *end, struct va_format *va_fmt,
> > +/*
> > + * The '__printf(1,0)' thing is a hack make gcc not ask us to use a
> > + * a format attribute. 'buf' is *not* the format, 'va_fmt->fmt' is.
> > + */
> > +static __printf(1,0)
> > +char *va_format(char *buf, char *end, struct va_format *va_fmt,
> > struct printf_spec spec)
> > {
> > @@ -1718,5 +1719,4 @@ static char *va_format(...
> > return buf;
> > }
> > -#pragma GCC diagnostic pop
> >
> > static noinline_for_stack
> > @@ -2429,5 +2429,5 @@ early_param(...
> > * 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)
> >
> > Does that work for people who see this warning?
>
> IMHO, this is much worse.
>
> Yes, as I also said in the previous thread, I consider the
> warning/suggestion here a gcc bug, as it shouldn't make that suggestion
> when one doesn't pass any of the function's arguments as the fmt
> argument to another __format__(()) annotated-function.
>
> But we have this __diag infrastructure exactly to silence special cases
> (and sorry I forgot about that when suggesting the #pragma approach to
> Andy), and this is very much a special case: It's the only place in the
> whole codebase that has any reason to dereference that va_fmt, and any
> other function anywhere calling a vsprintf()-like really should have
> gotten the format string that goes along with the varargs from its
> caller.
>
> As this is apparently some newer gcc that has started doing this, you
> just risk the next version turning the wrongness to 11 and complaining
> that "buf" or "fmt" is not passed to a vsprintf-like function. Let's not
> do "a hack make gcc not ask us to use a format attribute" when we have
> a proper way to selectively silence such false-positives. If this was
> something happening all over, we'd do -Wno-suggest-attribute=format, not
> spread these annotations. But this really is a special case in the guts
> of our printf implementation.
>
> So, FWIW, ack on Nathan's fixups, nak on this one.
I think that we all agree that this patchset is better than the
current state.
I have added Andy's Tested-by from
https://lore.kernel.org/r/Z-557YrwVr8bONq4@smile.fi.intel.com
Link to the previous thread, see
https://lore.kernel.org/r/CAHk-=wgfX9nBGE0Ap9GjhOy7Mn=RSy=rx0MvqfYFFDx31KJXqQ@mail.gmail.com
and pushed this into printk/linux.git, branch for-6.15-printf-attribute.
It was the branch with the already pulled code, see
https://web.git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git/log/?h=for-6.15-printf-attribute
I am going to give it few days in linux-next and create another
pull request to have this sorted in 6.15 where it stated.
Best Regards,
Petr
On Sat, Apr 5, 2025 at 8:27 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, 5 Apr 2025 at 02:11, David Laight <david.laight.linux@gmail.com> wrote:
> >
> > Perhaps the compilers ought to support __attribute__((format(none)))
> > to disable the warning.
>
> D'oh, that's a good idea.
>
> And gcc already supports it, even if we have to hack it up.
>
> So let's remove this whole horrible garbage entirely, and replace it
> with __printf(1,0) which should do exactly that.
>
> The 1 is for the format string argument number, and we're just *lying*
> about it. But there is not format string argument, and gcc just checks
> for 'is it a char pointer).
>
> The real format string argument is va_fmt->fmt, but there's no way to
> tell gcc that.
>
> And the 0 is is to tell gcc that there's nothing to verify.
>
> Then, if you do that, gcc will say "oh, maybe you need to do the same
> for the 'pointer()' function". That one has a real 'fmt' thing, but
> again nothing to be checked, so we do the same '__printf(1,0)' there
> too.
>
> There it makes more sense, because argument 1 _is_ actually a format
> string, so we're not lying about it.
>
> IOW, something like this:
>
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1700,9 +1700,10 @@ char *escaped_string(...
> }
>
> -#pragma GCC diagnostic push
> -#ifndef __clang__
> -#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> -#endif
> -static char *va_format(char *buf, char *end, struct va_format *va_fmt,
> +/*
> + * The '__printf(1,0)' thing is a hack make gcc not ask us to use a
> + * a format attribute. 'buf' is *not* the format, 'va_fmt->fmt' is.
> + */
> +static __printf(1,0)
> +char *va_format(char *buf, char *end, struct va_format *va_fmt,
> struct printf_spec spec)
> {
> @@ -1718,5 +1719,4 @@ static char *va_format(...
> return buf;
> }
> -#pragma GCC diagnostic pop
>
> static noinline_for_stack
> @@ -2429,5 +2429,5 @@ early_param(...
> * 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)
>
> Does that work for people who see this warning?
This is quite similar to my initial approach [1] which Rasmus was
against (okay, I did the nasty castings on top of the printf() there,
but still). TL;DR: I assume it will work, but let others comment on
this.
[1]: https://lore.kernel.org/lkml/20250320180926.4002817-7-andriy.shevchenko@linux.intel.com/
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.