* [PATCH] kunit: Add gnu_printf specifiers @ 2021-05-13 20:03 David Gow 2021-05-13 20:25 ` Daniel Latypov 2021-06-15 21:01 ` Brendan Higgins 0 siblings, 2 replies; 5+ messages in thread From: David Gow @ 2021-05-13 20:03 UTC (permalink / raw) To: Brendan Higgins, Shuah Khan Cc: linux-kselftest, kunit-dev, linux-kernel, David Gow 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> --- 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, ...); /* * 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); char *string_stream_get_string(struct string_stream *stream); -- 2.31.1.751.gd2f1c929bd-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] kunit: Add gnu_printf specifiers 2021-05-13 20:03 [PATCH] kunit: Add gnu_printf specifiers David Gow @ 2021-05-13 20:25 ` Daniel Latypov 2021-05-13 20:48 ` David Gow 2021-06-15 21:01 ` Brendan Higgins 1 sibling, 1 reply; 5+ messages in thread From: Daniel Latypov @ 2021-05-13 20:25 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK, KUnit Development, Linux Kernel Mailing List 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. > --- > 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); | ^~~~~~~~~~ > > /* > * 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. > > 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%40google.com. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kunit: Add gnu_printf specifiers 2021-05-13 20:25 ` Daniel Latypov @ 2021-05-13 20:48 ` David Gow 2021-05-13 22:36 ` Daniel Latypov 0 siblings, 1 reply; 5+ messages in thread From: David Gow @ 2021-05-13 20:48 UTC (permalink / raw) To: Daniel Latypov Cc: Brendan Higgins, Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK, KUnit Development, Linux Kernel Mailing List 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%40google.com. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kunit: Add gnu_printf specifiers 2021-05-13 20:48 ` David Gow @ 2021-05-13 22:36 ` Daniel Latypov 0 siblings, 0 replies; 5+ messages in thread From: Daniel Latypov @ 2021-05-13 22:36 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK, KUnit Development, Linux Kernel Mailing List On Thu, May 13, 2021 at 1:48 PM David Gow <davidgow@google.com> wrote: > > 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. Yeah. In case it wasn't clear, I think both annotations are clearly worth having if they silence W=1 warnings. We're more likely to produce more noise w/ those warnings than the extra noise of someone making a typo or forgetting in a kunit_info() somewhere while writing a test. It's just a bit sad that doing what the compiler suggests doesn't really make life better :( > > > > > --- > > > 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%40google.com. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kunit: Add gnu_printf specifiers 2021-05-13 20:03 [PATCH] kunit: Add gnu_printf specifiers David Gow 2021-05-13 20:25 ` Daniel Latypov @ 2021-06-15 21:01 ` Brendan Higgins 1 sibling, 0 replies; 5+ messages in thread From: Brendan Higgins @ 2021-06-15 21:01 UTC (permalink / raw) To: David Gow Cc: Shuah Khan, open list:KERNEL SELFTEST FRAMEWORK, KUnit Development, Linux Kernel Mailing List On Thu, May 13, 2021 at 1:03 PM David Gow <davidgow@google.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> Acked-by: Brendan Higgins <brendanhiggins@google.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-15 21:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-13 20:03 [PATCH] kunit: Add gnu_printf specifiers David Gow 2021-05-13 20:25 ` Daniel Latypov 2021-05-13 20:48 ` David Gow 2021-05-13 22:36 ` Daniel Latypov 2021-06-15 21:01 ` Brendan Higgins
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.