[PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[Nathan Chancellor]

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>

[PATCH 1/2] compiler-gcc.h: Introduce __diag_GCC_all

[Nathan Chancellor]

It is not possible disabling a diagnostic for all versions of GCC
without hard coding the minimum supported version at the site, as the
GCC specific macros require a minimum version to disable the warning
for:

    __diag_ignore(GCC, 5, ...);

__diag_ignore_all() does not solve this issue because it disables a
diagnostic for all versions of both GCC and clang, not just one or the
other.

Introduce __diag_GCC_all so that developers can write

    __diag_ignore(GCC, all, ...);

to disable a particular diagnostic for all versions of GCC, while not
affecting clang.

Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 include/linux/compiler-gcc.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index c9b58188ec61..c75a222880f9 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -127,6 +127,8 @@
 #define __diag_GCC_8(s)
 #endif
 
+#define __diag_GCC_all(s)	__diag(s)
+
 #define __diag_ignore_all(option, comment) \
 	__diag(__diag_GCC_ignore option)
 

-- 
2.49.0

[PATCH 2/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[Nathan Chancellor]

The GCC specific warning '-Wsuggest-attribute=format' is disabled around
va_format() using raw #pragma statements, which includes an
'#ifndef __clang__' to avoid a warning about an unknown warning option
from clang (which recognizes '#pragma GCC' for compatibility reasons):

  lib/vsprintf.c:1703:32: error: unknown warning group '-Wsuggest-attribute=format', ignored [-Werror,-Wunknown-warning-option]
   1703 | #pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
        |                                ^

While the current solution works, it is not visually appealing. The
kernel already has some infrastructure that wraps these #pragma
statements to give more specific control over diagnostics without
needing #ifdef blocks for different compilers. Convert the existing
statements over to the __diag macros.

Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 lib/vsprintf.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a2195bc81723..8a6cdee0d4ad 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1699,10 +1699,9 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	return buf;
 }
 
-#pragma GCC diagnostic push
-#ifndef __clang__
-#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
-#endif
+__diag_push();
+__diag_ignore(GCC, all, "-Wsuggest-attribute=format",
+	      "Not a valid __printf() conversion candidate.");
 static char *va_format(char *buf, char *end, struct va_format *va_fmt,
 		       struct printf_spec spec)
 {
@@ -1717,7 +1716,7 @@ static char *va_format(char *buf, char *end, struct va_format *va_fmt,
 
 	return buf;
 }
-#pragma GCC diagnostic pop
+__diag_pop();
 
 static noinline_for_stack
 char *uuid_string(char *buf, char *end, const u8 *addr,

-- 
2.49.0

Re: [PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[David Laight]

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

Re: [PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[Andy Shevchenko]

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

Re: [PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[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

Re: [PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[Linus Torvalds]

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

Re: [PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[Andy Shevchenko]

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

Re: [PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[Rasmus Villemoes]

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

Re: [PATCH 0/2] vsprintf: Use __diag macros to disable '-Wsuggest-attribute=format'

[Petr Mladek]

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