| Message ID | 20250320180926.4002817-4-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 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.
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
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...
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);
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(-)