On Fri, May 14, 2021 at 4:25 AM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote: > > On Thu, May 13, 2021 at 1:03 PM 'David Gow' via KUnit Development > <kunit-dev@xxxxxxxxxxxxxxxx> wrote: > > > > Some KUnit functions use variable arguments to implement a printf-like > > format string. Use the __printf() attribute to let the compiler warn if > > invalid format strings are passed in. > > > > If the kernel is build with W=1, it complained about the lack of these > > specifiers, e.g.: > > ../lib/kunit/test.c:72:2: warning: function ‘kunit_log_append’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format] > > > > Signed-off-by: David Gow <davidgow@xxxxxxxxxx> > > Reviewed-by: Daniel Latypov <dlatypov@xxxxxxxxxx> > > As noted below, these additions don't really do anything. > Unfortunately, they just make compiler warnings noisier in the case of > kunit_log_append(). > > But if this silences a W=1 warning, then we should probably add them in. > I guess it also serves as documentation that we're using the same > standard format specifiers and not something custom, which is nice. > Yeah: I did this to get rid of the W=1 warnings. I don't know if there's a way of doing this which would be less verbose: I do think that the format checking is worthwhile in general, even if we're hitting a few nasty cases where things are nested in macros. > > --- > > include/kunit/test.h | 2 +- > > lib/kunit/string-stream.h | 6 +++--- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index 49601c4b98b8..af2e386b867c 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h > > @@ -610,7 +610,7 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp) > > > > void kunit_cleanup(struct kunit *test); > > > > -void kunit_log_append(char *log, const char *fmt, ...); > > +void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...); > > Before this patch: > ../lib/kunit/kunit-example-test.c: In function ‘example_simple_test’: > ../include/linux/kern_levels.h:5:18: warning: format ‘%s’ expects > argument of type ‘char *’, but argument 3 has type ‘int’ [-Wformat=] > 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ > | ^~~~~~ > ../include/kunit/test.h:622:10: note: in definition of macro ‘kunit_log’ > 622 | printk(lvl fmt, ##__VA_ARGS__); \ > | ^~~ > ../include/kunit/test.h:641:2: note: in expansion of macro ‘kunit_printk’ > 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) > | ^~~~~~~~~~~~ > ../include/linux/kern_levels.h:14:19: note: in expansion of macro ‘KERN_SOH’ > 14 | #define KERN_INFO KERN_SOH "6" /* informational */ > | ^~~~~~~~ > ../include/kunit/test.h:641:15: note: in expansion of macro ‘KERN_INFO’ > 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) > | ^~~~~~~~~ > ../lib/kunit/kunit-example-test.c:23:2: note: in expansion of macro ‘kunit_info’ > 23 | kunit_info(test, "invalid: %s", 42); > > After this patch, it gets noisier: > In file included from ../lib/kunit/kunit-example-test.c:9: > ../lib/kunit/kunit-example-test.c: In function ‘example_simple_test’: > ../include/linux/kern_levels.h:5:18: warning: format ‘%s’ expects > argument of type ‘char *’, but argument 3 has type ‘int’ [-Wformat=] > 5 | #define KERN_SOH "\001" /* ASCII Start Of Header */ > | ^~~~~~ > ../include/kunit/test.h:622:10: note: in definition of macro ‘kunit_log’ > 622 | printk(lvl fmt, ##__VA_ARGS__); \ > | ^~~ > ../include/kunit/test.h:641:2: note: in expansion of macro ‘kunit_printk’ > 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) > | ^~~~~~~~~~~~ > ../include/linux/kern_levels.h:14:19: note: in expansion of macro ‘KERN_SOH’ > 14 | #define KERN_INFO KERN_SOH "6" /* informational */ > | ^~~~~~~~ > ../include/kunit/test.h:641:15: note: in expansion of macro ‘KERN_INFO’ > 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) > | ^~~~~~~~~ > ../lib/kunit/kunit-example-test.c:23:2: note: in expansion of macro ‘kunit_info’ > 23 | kunit_info(test, "invalid: %s", 42); > | ^~~~~~~~~~ > ../include/kunit/test.h:105:31: warning: format ‘%s’ expects argument > of type ‘char *’, but argument 4 has type ‘int’ [-Wformat=] > 105 | #define KUNIT_SUBTEST_INDENT " " > | ^~~~~~ > ../include/kunit/test.h:623:42: note: in definition of macro ‘kunit_log’ > 623 | kunit_log_append((test_or_suite)->log, fmt "\n", \ > | ^~~ > ../include/kunit/test.h:628:23: note: in expansion of macro > ‘KUNIT_SUBTEST_INDENT’ > 628 | kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt, \ > | ^~~~~~~~~~~~~~~~~~~~ > ../include/kunit/test.h:641:2: note: in expansion of macro ‘kunit_printk’ > 641 | kunit_printk(KERN_INFO, test, fmt, ##__VA_ARGS__) > | ^~~~~~~~~~~~ > ../lib/kunit/kunit-example-test.c:23:2: note: in expansion of macro ‘kunit_info’ > 23 | kunit_info(test, "invalid: %s", 42); > | ^~~~~~~~~~ > > Yeah: that is pretty ugly. TBH, it was pretty ugly beforehand, and this does make it worse. I guess that's the price we pay for having so many nested macros, as well. Personally, I suspect this is still worth it to get rid of the compiler warnings, but only just. > > > > /* > > * printk and log to per-test or per-suite log buffer. Logging only done > > diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h > > index fe98a00b75a9..5e94b623454f 100644 > > --- a/lib/kunit/string-stream.h > > +++ b/lib/kunit/string-stream.h > > @@ -35,9 +35,9 @@ struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp); > > int __printf(2, 3) string_stream_add(struct string_stream *stream, > > const char *fmt, ...); > > > > -int string_stream_vadd(struct string_stream *stream, > > - const char *fmt, > > - va_list args); > > +int __printf(2, 0) string_stream_vadd(struct string_stream *stream, > > + const char *fmt, > > + va_list args); > > This is never called with a literal `fmt` string. > It's currently only ever called through the _add variant, which does > have __printf(2,3). > > So this can't catch any mistakes currently. > And I think it's hard to imagine we'd ever pass in a literal format > string w/ a va_list. > Yeah: I was tempted to leave this one out, but it was triggering warnings with the "you should use __printf()" heuristic. In fact, it had two warnings. The __printf() specifier documentation does specifically call out cases where a va_list is passed in as a case to use '0' for the positional argument, but only the format string is checked for validity: there's no typechecking. > > > > char *string_stream_get_string(struct string_stream *stream); > > > > -- > > 2.31.1.751.gd2f1c929bd-goog > > > > -- > > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@xxxxxxxxxxxxxxxx. > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210513200350.854429-1-davidgow%40google.com.
