On Fri, May 14, 2021 at 4:25 AM Daniel Latypov dlatypov@google.com wrote:
On Thu, May 13, 2021 at 1:03 PM 'David Gow' via KUnit Development kunit-dev@googlegroups.com 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@google.com
Reviewed-by: Daniel Latypov dlatypov@google.com
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@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210513200350.854429-1-davidgow....