[GIT PULL] more printk for 6.15

[GIT PULL] more printk for 6.15

[Petr Mladek]

Hi Linus,

please pull few more printk-related changes from

  git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git tags/printk-for-6.15-2

=====================================

- Silence warnings about candidates for ‘gnu_print’ format attribute.

----------------------------------------------------------------
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: Drop unused const char fmt * in va_format()
      vsnprintf: Silence false positive GCC warning for va_format()

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

Re: [GIT PULL] more printk for 6.15

[Linus Torvalds]

On Wed, 2 Apr 2025 at 05:58, Petr Mladek <pmladek@suse.com> wrote:
>
> please pull few more printk-related changes from

Pulled. However, I reacted to this mess:

  +#pragma GCC diagnostic push
  +#ifndef __clang__
  +#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
  +#endif

do we really need a "#ifndef __clang__" there? It's "#pragma GCC"
after all, and the diagnostic push/pop are done unconditionally.

I can well imagine that yes, we need it for some strange and stupid
reason, but it looks wrong, and the commit message doesn't explain why
we'd need it.

And once again the "Link" is completely useless and doesn't point to
any explanation, only points to the submission that has all the same
info.

I hate those things. The disappointment is real: "Oh, an explanation"
followed by "No, just useless noise, doing a google search would
almost certainly have been more productive".

              Linus

Re: [GIT PULL] more printk for 6.15

[pr-tracker-bot]

The pull request you sent on Wed, 2 Apr 2025 14:58:24 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git tags/printk-for-6.15-2

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/af54a3a151691a969b04396cff15afe70d4da824

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] more printk for 6.15

[Andy Shevchenko]

Wed, Apr 02, 2025 at 10:12:27AM -0700, Linus Torvalds kirjoitti:
> On Wed, 2 Apr 2025 at 05:58, Petr Mladek <pmladek@suse.com> wrote:
> >
> > please pull few more printk-related changes from
> 
> Pulled. However, I reacted to this mess:

Yeah, this is the most plausible solution (rather workaround) proposed by
Rasmus. He thinks that GCC fails to recognize that va_format() is not what it
thinks it is.

>   +#pragma GCC diagnostic push
>   +#ifndef __clang__
>   +#pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
>   +#endif
> 
> do we really need a "#ifndef __clang__" there? It's "#pragma GCC"
> after all, and the diagnostic push/pop are done unconditionally.

Yes. Clang complains on unknown pragma.

> I can well imagine that yes, we need it for some strange and stupid
> reason, but it looks wrong, and the commit message doesn't explain why
> we'd need it.

Ah, sorry, since Rasmus said something like "and add necessary magic to make
clang work" I mistakenly assumed that's kinda obvious that this is GCC only
stuff.

> And once again the "Link" is completely useless and doesn't point to
> any explanation, only points to the submission that has all the same
> info.

The problem with (automatic) Link tag is that it points to the latest version
of the patch where all of the discussion have been settled down. And more (but
maybe not full) information is available on the previous versions. The fix
would be to have some kind of version tracking system for the series (oh,
sounds like Gerrit :).

> I hate those things. The disappointment is real: "Oh, an explanation"
> followed by "No, just useless noise, doing a google search would
> almost certainly have been more productive".

-- 
With Best Regards,
Andy Shevchenko

Re: [GIT PULL] more printk for 6.15

[Linus Torvalds]

On Wed, 2 Apr 2025 at 11:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> Yes. Clang complains on unknown pragma.

What a crock.

It says GCC, for chrissake!

And clang clearly doesn't complain about

>   +#pragma GCC diagnostic push
>   +#pragma GCC diagnostic pop

which are *not* protected by that #ifndef __clang__ thing.

So this smells like a clang bug to me.

Can we please use wrapper defines instead so that we don't have that
#ifndef in the middle of code? And since those don't work with
'#pragma', they need to use the _Pragma() operator instead.

Something like

   #define GCC_PRAGMA(x) _Pragma(#x)

in compiler-gcc.h, and then add a

  #ifndef GCC_PRAGMA
    #define GCC_PRAGMA(x) /* Nothing */
  #endif

and then you can just do

   GCC_PRAGMA(Wsuggest-attribute=format)

in places like this?

(Entirely untested: I *despise* pragma in general).

Or hey, how about we just add "-Wno-suggest-attribute=format" to the
compiler command line? Like we do for all the other garbage warnings
that we don't want to see.

                Linus

Re: [GIT PULL] more printk for 6.15

[Linus Torvalds]

On Wed, 2 Apr 2025 at 11:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> The problem with (automatic) Link tag is that it points to the latest version
> of the patch where all of the discussion have been settled down. And more (but
> maybe not full) information is available on the previous versions. The fix
> would be to have some kind of version tracking system for the series (oh,
> sounds like Gerrit :).

No, the fix is to admit that noise is noise, and that search engines
are better at context than some silly commit-time noise.

The whole "link to original submission" is garbage.

               Linus

Re: [GIT PULL] more printk for 6.15

[Linus Torvalds]

On Wed, 2 Apr 2025 at 12:07, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The whole "link to original submission" is garbage.

Just to clarify: people should link to the *problem* report. Or to the
*debugging* thread.

But linking to the final result is pointless. That's what in the tree,
and any subsequent discussion about it is stale and late.

People sometimes argue that it's good for belated Ack's etc. But
Christ - they are *belated*.

        Linus

Re: [GIT PULL] more printk for 6.15

[Andy Shevchenko]

+Cc: Kees and Nathan (I believe this discussion has some material for
you, folks, to think of / comment on / etc)

On Wed, Apr 2, 2025 at 10:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, 2 Apr 2025 at 11:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > Yes. Clang complains on unknown pragma.
>
> What a crock.
>
> It says GCC, for chrissake!
>
> And clang clearly doesn't complain about
>
> >   +#pragma GCC diagnostic push
> >   +#pragma GCC diagnostic pop
>
> which are *not* protected by that #ifndef __clang__ thing.
>
> So this smells like a clang bug to me.

I Cc'ed Nathan, perhaps he can comment on this.

> Can we please use wrapper defines instead so that we don't have that
> #ifndef in the middle of code? And since those don't work with
> '#pragma', they need to use the _Pragma() operator instead.
>
> Something like
>
>    #define GCC_PRAGMA(x) _Pragma(#x)
>
> in compiler-gcc.h, and then add a
>
>   #ifndef GCC_PRAGMA
>     #define GCC_PRAGMA(x) /* Nothing */
>   #endif
>
> and then you can just do
>
>    GCC_PRAGMA(Wsuggest-attribute=format)
>
> in places like this?
>
> (Entirely untested: I *despise* pragma in general).

Maybe. Tomorrow I can look at this.

> Or hey, how about we just add "-Wno-suggest-attribute=format" to the
> compiler command line? Like we do for all the other garbage warnings
> that we don't want to see.

I actually don't know what the benefit of __printf() attribute from
security (?) point of view is. I may speculate that this helps to
validate the format string and arguments (when provided as ...) and
helps with potential wrong argument sizes, etc. Kees, what do you
think about Linus' proposal?

-- 
With Best Regards,
Andy Shevchenko

Re: [GIT PULL] more printk for 6.15

[Steven Rostedt]

On Wed, 2 Apr 2025 12:10:08 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> People sometimes argue that it's good for belated Ack's etc. But
> Christ - they are *belated*.

I would also argue that it's good for the actual ack, because it gives you
a link back to email were it was likely acked, in case you want to confirm
it was acked.

Also, it's a case to prove Cc's, as there's been times that I've seen
someone add a "Cc:" to the commit log but not actually Cc the person in the
email!

My scripts do add the link back to the submission, but I also make it a
point to separate out Link: tags that hold actual discussions that happened
outside the patch submission.

For example, my last patch post had:

    ring-buffer: Use flush_kernel_vmap_range() over flush_dcache_folio()
    
    Some architectures do not have data cache coherency between user and
    kernel space. For these architectures, the cache needs to be flushed on
    both the kernel and user addresses so that user space can see the updates
    the kernel has made.
    
    Instead of using flush_dcache_folio() and playing with virt_to_folio()
    within the call to that function, use flush_kernel_vmap_range() which
    takes the virtual address and does the work for those architectures that
    need it.
    
    This also fixes a bug where the flush of the reader page only flushed one
    page. If the sub-buffer order is 1 or more, where the sub-buffer size
    would be greater than a page, it would miss the rest of the sub-buffer
    content, as the "reader page" is not just a page, but the size of a
    sub-buffer.
    
    Link: https://lore.kernel.org/all/CAG48ez3w0my4Rwttbc5tEbNsme6tc0mrSN95thjXUFaJ3aQ6SA@mail.gmail.com/
    
    Cc: stable@vger.kernel.org
>   Cc: Linus Torvalds <torvalds@linux-foundation.org>
>   Cc: Masami Hiramatsu <mhiramat@kernel.org>
>   Cc: Mark Rutland <mark.rutland@arm.com>
>   Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>   Cc: Andrew Morton <akpm@linux-foundation.org>
>   Cc: Vincent Donnefort <vdonnefort@google.com>
>   Cc: Vlastimil Babka <vbabka@suse.cz>
>   Cc: Mike Rapoport <rppt@kernel.org>
>   Link: https://lore.kernel.org/20250402144953.920792197@goodmis.org
    Fixes: 117c39200d9d7 ("ring-buffer: Introducing ring-buffer mapping functions");
    Suggested-by: Jann Horn <jannh@google.com>
    Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

The above ">" lines are added by my scripts. The top Link: is the
discussion (which I keep separate as it is easy to see and is used for
people wanting to see the discussion that was not part of the submission)
where as the bottom Link points to the patch submission which sometimes
also includes the discussion. That is, the Link that is above the other
tags is to the discussion that was likely what was the reason for this
patch to be created.

One case where this is also useful for my work flow, is that when I do a
new version, I will use that bottom link to cut and paste it in my
submission of the second version, by changing it to:

  Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
  ---
  Change since v5: https://lore.kernel.org/linux-trace-kernel/20250401225842.261475465@goodmis.org/

  - Use %pa instead of %lx for start and size sizes (Mike Rapoport)


Where I moved the bottom Link from patch v5 below the "---" and added the
Changes since v5: with that link to v5. Now, if you go from the git Link to
the submission, you also get a link to the previous version, which probably
has a discussion on why it was changed. And that link has a link to the
previous submission from that.

Hence, the Link tags hold the chain to get you to every version of the
patch. In a lot of the cases, the earlier versions have the discussions.

Note, for patch series, I sometimes do not have the individual patches
have the link back to the previous version, but I make sure the cover
letter does have the link back to the previous version.

In practice, I have found this very useful and has given me the history of
the change that landed in git.

Automating the Link tags and making sure every new version has a link to
the previous version will likely take you to the discussions to why the
patch was written the way it was written.

-- Steve

Re: [GIT PULL] more printk for 6.15

[Linus Torvalds]

On Wed, 2 Apr 2025 at 12:43, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> I would also argue that it's good for the actual ack, because it gives you
> a link back to email were it was likely acked, in case you want to confirm
> it was acked.

Let's make the rule that you can have your useless Link: tags for the
pointless patch source if you want.

IF YOU ALSO PUT THE ACTUALLY USEFUL LINKS IN THERE!

In other words - don't make me go look at the patch submission and be
disappointed.

Make the *first* link be something useful, like the *reason* for the
patch in the first place.

Then you can add your pointless noise afterwards.

Because no, "it has an actual ack" is not a good reason. Nobody cares
about the ack. The *reason* people look at the link is because
something went wrong, and you want some serious explanation for why
the patch exists.

Seeing extra "Acks" is not a reason.

           Linus

             Linus

Re: [GIT PULL] more printk for 6.15

[Sean Christopherson]

On Wed, Apr 02, 2025, Linus Torvalds wrote:
> On Wed, 2 Apr 2025 at 12:07, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > The whole "link to original submission" is garbage.
> 
> Just to clarify: people should link to the *problem* report. Or to the
> *debugging* thread.
> 
> But linking to the final result is pointless. That's what in the tree,
> and any subsequent discussion about it is stale and late.

It's not pointless noise to everyone.

For people that come across the commit after the fact, which may be years down
the road, e.g. when doing git archaeology, having an explicit link to *something*
is extremely valuable.  The final submission may not have the full context, but
more often than not there are links and references to threads that do provide
additional context.  At the very least, it's a starting point for the hunt.

"just Google it" does work most of the time, but search engines won't help all
that much if the maintainer massaged the shortlog and/or changelog, especially
if the surgery done when applying is significant.  And if the source patch was
never posted to a public list, lack of a source Link is a hint that trying to
find the source/context may be futile.

Linking to source of the commit also provides a paper trail that can be used to
audit the final result.  As a maintainer, I've used the link to verify that I
actually applied the version I intended to apply.

E.g. with zero a priori knowledge of the situation, it was trivially easy for me
to verify that Thomas' goof[*] with the irq/msi series was due to v2 getting
applied instead of v4, thanks to the Links in the buggy commits.

I can appreciate that in your role, a Link to the source patch is a false positive
more often than not.  But isn't that a problem with the tag being ambiguous?  I
assume it's not the mere presense of the line in the changelog that's most
frustrating, it's the wasted time and crushing disappointment of following the
link and finding nothing useful.

So rather than trash the convention entirely and risk throwing out the baby with
the bath water, what if we tweak the convention to use a dedicated tag, a la
Closes?  E.g. Source or something.  If b4 implements the new tag, it shouldn't be
too hard to get maintainers to follow suit.  Then you can quickly gloss over the
tag and mutter about its uselessness as you do so, and us plebs can continue
reveling in the joy of our source links.

[*] https://lkml.kernel.org/r/20250319104921.201434198%40linutronix.de

Re: [GIT PULL] more printk for 6.15

[Steven Rostedt]

On Wed, 2 Apr 2025 13:00:20 -0700
Sean Christopherson <seanjc@google.com> wrote:

> "just Google it" does work most of the time, but search engines won't help all
> that much if the maintainer massaged the shortlog and/or changelog, especially
> if the surgery done when applying is significant. 

Actually that's a good point. There's several times I get a patch with an
incoherent subject line, and I will change it without requesting for an
update. And usually in these cases I tend to rewrite the change log as the
submitter isn't a native English speaker and I reword it to make it have
proper grammar and such.

The Link: tag in this case is the only evidence that the commit came from
that email. The patch itself can sometimes be modified if there's a small
conflict with the current code. If it's anything more than "oh there's an
added line just before the code that this patch touches" I'll ask the
submitter to rebase, but for the case the code around the patch changed,
that's enough to make the commit not 100% what was submitted.

-- Steve

Re: [GIT PULL] more printk for 6.15

[Andy Shevchenko]

On Wed, Apr 2, 2025 at 10:43 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 2 Apr 2025 12:10:08 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:

...

> Also, it's a case to prove Cc's, as there's been times that I've seen
> someone add a "Cc:" to the commit log but not actually Cc the person in the
> email!

Side note about my whining [1] of addition Cc to the commit messages in general.

[1]: https://lore.kernel.org/linux-doc/Zikc4fDNDer6hSzJ@smile.fi.intel.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [GIT PULL] more printk for 6.15

[Nathan Chancellor]

On Wed, Apr 02, 2025 at 10:25:46PM +0300, Andy Shevchenko wrote:
> +Cc: Kees and Nathan (I believe this discussion has some material for
> you, folks, to think of / comment on / etc)

Thanks, I have commented on the part of the message that seem relevant
for me.

> On Wed, Apr 2, 2025 at 10:06 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Wed, 2 Apr 2025 at 11:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >
> > > Yes. Clang complains on unknown pragma.
> >
> > What a crock.
> >
> > It says GCC, for chrissake!
> >
> > And clang clearly doesn't complain about
> >
> > >   +#pragma GCC diagnostic push
> > >   +#pragma GCC diagnostic pop
> >
> > which are *not* protected by that #ifndef __clang__ thing.
> >
> > So this smells like a clang bug to me.

Yes, clang implements support for '#pragma GCC' for compatability with
existing source code:

https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas

Otherwise, the pragma would need to be duplicated if the warning was
shared between the compilers (as many are nowadays).

It complains specifically about an unknown warning being passed to
'diagnostic ignored':

  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"
        |                                ^
  1 error generated.

Which I suppose you could argue is a bug since it is a GCC pragma,
although warning on an unknown option to the ignored diagnostic pragma
is what GCC does as well (it just ignores '#pragma clang' altogether):

  $ echo '#pragma GCC diagnostic ignored "-Wfoo"' | gcc -fsyntax-only -x c -
  <stdin>:1:32: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]

I can look into filing a report upstream about this, however...

> > Can we please use wrapper defines instead so that we don't have that
> > #ifndef in the middle of code? And since those don't work with
> > '#pragma', they need to use the _Pragma() operator instead.
> >
> > Something like
> >
> >    #define GCC_PRAGMA(x) _Pragma(#x)
> >
> > in compiler-gcc.h, and then add a
> >
> >   #ifndef GCC_PRAGMA
> >     #define GCC_PRAGMA(x) /* Nothing */
> >   #endif
> >
> > and then you can just do
> >
> >    GCC_PRAGMA(Wsuggest-attribute=format)
> >
> > in places like this?
> >
> > (Entirely untested: I *despise* pragma in general).

We have the __diag() infrastructure for this already. I think this issue
would be as simple as the following diff, which makes clang and GCC
happy without any obvious ifdeffery.

Cheers,
Nathan

diff --git a/include/linux/compiler-igcc.h b/include/linux/compiler-gcc.h
index 32048052c64a..5d07c469b571 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)
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 01699852f30c..6ff4d85e144e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1699,10 +1699,8 @@ 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", "<reason>");
 static char *va_format(char *buf, char *end, struct va_format *va_fmt,
 		       struct printf_spec spec)
 {
@@ -1717,7 +1715,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,

Re: [GIT PULL] more printk for 6.15

[Petr Mladek]

On Wed 2025-04-02 12:10:08, Linus Torvalds wrote:
> On Wed, 2 Apr 2025 at 12:07, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > The whole "link to original submission" is garbage.
> 
> Just to clarify: people should link to the *problem* report. Or to the
> *debugging* thread.
> 
> But linking to the final result is pointless. That's what in the tree,
> and any subsequent discussion about it is stale and late.

I agree that links to the threads where the discussion
happened might be more useful.

I personally use the link (added by b4 am -l) when doing
a code archeology. It often points to a whole patchset with
a cover leter. And it sometimes help to understand
the missing context.

Google is a good alternative. But the link is faster
and reliable.

Of course, the best situation is when the commit message
is good enough and the link is not needed. Which reminds
me that anything which triggered a useful discussion should
be explained in the commit message. I need to care more
about this.

Best Regards,
Petr

PS: The #pragma was discussed in v1 thread, see
    https://lore.kernel.org/r/87iko2ear3.fsf@prevas.dk
    

Re: [GIT PULL] more printk for 6.15

[Andy Shevchenko]

On Wed, Apr 02, 2025 at 01:34:22PM -0700, Nathan Chancellor wrote:
> On Wed, Apr 02, 2025 at 10:25:46PM +0300, Andy Shevchenko wrote:
> > +Cc: Kees and Nathan (I believe this discussion has some material for
> > you, folks, to think of / comment on / etc)
> 
> Thanks, I have commented on the part of the message that seem relevant
> for me.

Thank you!

> > On Wed, Apr 2, 2025 at 10:06 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > > On Wed, 2 Apr 2025 at 11:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > Yes. Clang complains on unknown pragma.
> > >
> > > What a crock.
> > >
> > > It says GCC, for chrissake!
> > >
> > > And clang clearly doesn't complain about
> > >
> > > >   +#pragma GCC diagnostic push
> > > >   +#pragma GCC diagnostic pop
> > >
> > > which are *not* protected by that #ifndef __clang__ thing.
> > >
> > > So this smells like a clang bug to me.
> 
> Yes, clang implements support for '#pragma GCC' for compatability with
> existing source code:
> 
> https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas
> 
> Otherwise, the pragma would need to be duplicated if the warning was
> shared between the compilers (as many are nowadays).
> 
> It complains specifically about an unknown warning being passed to
> 'diagnostic ignored':
> 
>   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"
>         |                                ^
>   1 error generated.
> 
> Which I suppose you could argue is a bug since it is a GCC pragma,
> although warning on an unknown option to the ignored diagnostic pragma
> is what GCC does as well (it just ignores '#pragma clang' altogether):
> 
>   $ echo '#pragma GCC diagnostic ignored "-Wfoo"' | gcc -fsyntax-only -x c -
>   <stdin>:1:32: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
> 
> I can look into filing a report upstream about this, however...
> 
> > > Can we please use wrapper defines instead so that we don't have that
> > > #ifndef in the middle of code? And since those don't work with
> > > '#pragma', they need to use the _Pragma() operator instead.
> > >
> > > Something like
> > >
> > >    #define GCC_PRAGMA(x) _Pragma(#x)
> > >
> > > in compiler-gcc.h, and then add a
> > >
> > >   #ifndef GCC_PRAGMA
> > >     #define GCC_PRAGMA(x) /* Nothing */
> > >   #endif
> > >
> > > and then you can just do
> > >
> > >    GCC_PRAGMA(Wsuggest-attribute=format)
> > >
> > > in places like this?
> > >
> > > (Entirely untested: I *despise* pragma in general).
> 
> We have the __diag() infrastructure for this already. I think this issue
> would be as simple as the following diff, which makes clang and GCC
> happy without any obvious ifdeffery.

FWIW, I have tested this in my case for both compilers and they are happy with it.

Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> diff --git a/include/linux/compiler-igcc.h b/include/linux/compiler-gcc.h
> index 32048052c64a..5d07c469b571 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)
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 01699852f30c..6ff4d85e144e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1699,10 +1699,8 @@ 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", "<reason>");
>  static char *va_format(char *buf, char *end, struct va_format *va_fmt,
>  		       struct printf_spec spec)
>  {
> @@ -1717,7 +1715,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,

-- 
With Best Regards,
Andy Shevchenko

Re: [GIT PULL] more printk for 6.15

[Kees Cook]

On Wed, Apr 02, 2025 at 10:25:46PM +0300, Andy Shevchenko wrote:
> I actually don't know what the benefit of __printf() attribute from
> security (?) point of view is. I may speculate that this helps to
> validate the format string and arguments (when provided as ...) and
> helps with potential wrong argument sizes, etc. Kees, what do you
> think about Linus' proposal?

It's a bit low on the severity list since we long ago removed %n, but
it's effectively a form of type-checking for arguments to printf. I look
at it more as a robustness/correctness checker. If we can make it work,
it's good to have. And it looks like Nathan's suggestion will make it
feasible.

-Kees

-- 
Kees Cook

Re: [GIT PULL] more printk for 6.15

[Petr Mladek]

On Thu 2025-04-03 15:07:09, Andy Shevchenko wrote:
> On Wed, Apr 02, 2025 at 01:34:22PM -0700, Nathan Chancellor wrote:
> > On Wed, Apr 02, 2025 at 10:25:46PM +0300, Andy Shevchenko wrote:
> > > +Cc: Kees and Nathan (I believe this discussion has some material for
> > > you, folks, to think of / comment on / etc)
> > 
> > Thanks, I have commented on the part of the message that seem relevant
> > for me.
> 
> Thank you!
> 
> > > On Wed, Apr 2, 2025 at 10:06 PM Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > > > On Wed, 2 Apr 2025 at 11:39, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > >
> > > > > Yes. Clang complains on unknown pragma.
> > > >
> > > > Can we please use wrapper defines instead so that we don't have that
> > > > #ifndef in the middle of code? And since those don't work with
> > > > '#pragma', they need to use the _Pragma() operator instead.
> > > >
> > > > Something like
> > > >
> > > >    #define GCC_PRAGMA(x) _Pragma(#x)
> > > >
> > > > in compiler-gcc.h, and then add a
> > > >
> > > >   #ifndef GCC_PRAGMA
> > > >     #define GCC_PRAGMA(x) /* Nothing */
> > > >   #endif
> > > >
> > > > and then you can just do
> > > >
> > > >    GCC_PRAGMA(Wsuggest-attribute=format)
> > > >
> > > > in places like this?
> > > >
> > > > (Entirely untested: I *despise* pragma in general).
> > 
> > We have the __diag() infrastructure for this already. I think this issue
> > would be as simple as the following diff, which makes clang and GCC
> > happy without any obvious ifdeffery.
> 
> FWIW, I have tested this in my case for both compilers and they are happy with it.
> 
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Great!

Nathan, would mind to send this as a proper patch, please?

> > diff --git a/include/linux/compiler-igcc.h b/include/linux/compiler-gcc.h
> > index 32048052c64a..5d07c469b571 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)
> >  
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 01699852f30c..6ff4d85e144e 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1699,10 +1699,8 @@ 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", "<reason>");
> >  static char *va_format(char *buf, char *end, struct va_format *va_fmt,
> >  		       struct printf_spec spec)
> >  {
> > @@ -1717,7 +1715,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,

Best Regards,
Petr

Re: [GIT PULL] more printk for 6.15

[Nathan Chancellor]

On Fri, Apr 04, 2025 at 10:19:02AM +0200, Petr Mladek wrote:
> On Thu 2025-04-03 15:07:09, Andy Shevchenko wrote:
> > FWIW, I have tested this in my case for both compilers and they are happy with it.
> > 
> > Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Great!
> 
> Nathan, would mind to send this as a proper patch, please?

Sure thing, I will send it along shortly.

Cheers,
Nathan