| Message ID | 20240221092728.1281499-1-davidgow@google.com |
|---|---|
| Headers | show |
| Series | kunit: Fix printf format specifier issues in KUnit assertions | expand |
On 2/21/24 02:27, David Gow wrote: > KUnit has several macros which accept a log message, which can contain > printf format specifiers. Some of these (the explicit log macros) > already use the __printf() gcc attribute to ensure the format specifiers > are valid, but those which could fail the test, and hence used > __kunit_do_failed_assertion() behind the scenes, did not. > > These include: > - KUNIT_EXPECT_*_MSG() > - KUNIT_ASSERT_*_MSG() > - KUNIT_FAIL() > > This series adds the __printf() attribute, and fixes all of the issues > uncovered. (Or, at least, all of those I could find with an x86_64 > allyesconfig, and the default KUnit config on a number of other > architectures. Please test!) > > The issues in question basically take the following forms: > - int / long / long long confusion: typically a type being updated, but > the format string not. > - Use of integer format specifiers (%d/%u/%li/etc) for types like size_t > or pointer differences (technically ptrdiff_t), which would only work > on some architectures. > - Use of integer format specifiers in combination with PTR_ERR(), where > %pe would make more sense. > - Use of empty messages which, whilst technically not incorrect, are not > useful and trigger a gcc warning. > > We'd like to get these (or equivalent) in for 6.9 if possible, so please > do take a look if possible. > > Thanks, > -- David > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Closes: https://lore.kernel.org/linux-kselftest/CAHk-=wgJMOquDO5f8ShH1f4rzZwzApNVCw643m5-Yj+BfsFstA@mail.gmail.com/ > > Thank you for a quick response David. I will apply the series to kunit next for Linux 6.9 as soon as the reviews are complete. thanks, -- Shuah
On Wed, Feb 21, 2024 at 4:28 AM David Gow <davidgow@google.com> wrote: > > KUnit's executor_test logs the filter string in KUNIT_ASSERT_EQ_MSG(), > but passed a random character from the filter, rather than the whole > string. > > This was found by annotating KUNIT_ASSERT_EQ_MSG() to let gcc validate > the format string. > > Fixes: 76066f93f1df ("kunit: add tests for filtering attributes") > Signed-off-by: David Gow <davidgow@google.com> Hello! This change looks good to me. Thanks for fixing this mistake. Thanks! -Rae Reviewed-by: Rae Moar <rmoar@google.com> > --- > lib/kunit/executor_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > index 22d4ee86dbed..3f7f967e3688 100644 > --- a/lib/kunit/executor_test.c > +++ b/lib/kunit/executor_test.c > @@ -129,7 +129,7 @@ static void parse_filter_attr_test(struct kunit *test) > GFP_KERNEL); > for (j = 0; j < filter_count; j++) { > parsed_filters[j] = kunit_next_attr_filter(&filter, &err); > - KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter '%s'", filters[j]); > + KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter from '%s'", filters); > } > > KUNIT_EXPECT_STREQ(test, kunit_attr_filter_name(parsed_filters[0]), "speed"); > -- > 2.44.0.rc0.258.g7320e95886-goog >
Hello, On 21/02/2024 17:27:18+0800, David Gow wrote: > 'days' is a s64 (from div_s64), and so should use a %lld specifier. > > This was found by extending KUnit's assertion macros to use gcc's > __printf attribute. > > Fixes: 1d1bb12a8b18 ("rtc: Improve performance of rtc_time64_to_tm(). Add tests.") > Signed-off-by: David Gow <davidgow@google.com> Who do you expect to take this patch? > --- > drivers/rtc/lib_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/rtc/lib_test.c b/drivers/rtc/lib_test.c > index d5caf36c56cd..225c859d6da5 100644 > --- a/drivers/rtc/lib_test.c > +++ b/drivers/rtc/lib_test.c > @@ -54,7 +54,7 @@ static void rtc_time64_to_tm_test_date_range(struct kunit *test) > > days = div_s64(secs, 86400); > > - #define FAIL_MSG "%d/%02d/%02d (%2d) : %ld", \ > + #define FAIL_MSG "%d/%02d/%02d (%2d) : %lld", \ > year, month, mday, yday, days > > KUNIT_ASSERT_EQ_MSG(test, year - 1900, result.tm_year, FAIL_MSG); > -- > 2.44.0.rc0.258.g7320e95886-goog > >
On 2/27/24 13:32, Alexandre Belloni wrote: > Hello, > > On 21/02/2024 17:27:18+0800, David Gow wrote: >> 'days' is a s64 (from div_s64), and so should use a %lld specifier. >> >> This was found by extending KUnit's assertion macros to use gcc's >> __printf attribute. >> >> Fixes: 1d1bb12a8b18 ("rtc: Improve performance of rtc_time64_to_tm(). Add tests.") >> Signed-off-by: David Gow <davidgow@google.com> > > Who do you expect to take this patch? > I am going to be applying this series to linux-kselftest kunit next in just a bit. Would you like to ack the pacth? thanks, -- Shuah
On 27/02/2024 14:23:29-0700, Shuah Khan wrote: > On 2/27/24 13:32, Alexandre Belloni wrote: > > Hello, > > > > On 21/02/2024 17:27:18+0800, David Gow wrote: > > > 'days' is a s64 (from div_s64), and so should use a %lld specifier. > > > > > > This was found by extending KUnit's assertion macros to use gcc's > > > __printf attribute. > > > > > > Fixes: 1d1bb12a8b18 ("rtc: Improve performance of rtc_time64_to_tm(). Add tests.") > > > Signed-off-by: David Gow <davidgow@google.com> > > > > Who do you expect to take this patch? > > > > I am going to be applying this series to linux-kselftest kunit next > in just a bit. Would you like to ack the pacth? Sure, Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > thanks, > -- Shuah > >
On 2/21/24 14:29, Justin Stitt wrote: > Hi, > > On Wed, Feb 21, 2024 at 05:27:20PM +0800, David Gow wrote: >> The drm_buddy_test's alloc_contiguous test used a u64 for the page size, >> which was then updated to be an 'unsigned long' to avoid 64-bit >> multiplication division helpers. >> >> However, the variable is logged by some KUNIT_ASSERT_EQ_MSG() using the >> '%d' or '%llu' format specifiers, the former of which is always wrong, >> and the latter is no longer correct now that ps is no longer a u64. Fix >> these to all use '%lu'. >> >> Also, drm_mm_test calls KUNIT_FAIL() with an empty string as the >> message. gcc warns if a printf format string is empty (apparently), so > > clang does too; under -Wformat-zero-length > >> give these some more detailed error messages, which should be more >> useful anyway. >> >> Fixes: a64056bb5a32 ("drm/tests/drm_buddy: add alloc_contiguous test") >> Fixes: fca7526b7d89 ("drm/tests/drm_buddy: fix build failure on 32-bit targets") >> Fixes: fc8d29e298cf ("drm: selftest: convert drm_mm selftest to KUnit") >> Signed-off-by: David Gow <davidgow@google.com> > > Reviewed-by: Justin Stitt <justinstitt@google.com> David, Please send this on top of Linux 6.9-rc6 - this one doesn't apply as is due to conflict between this one and fca7526b7d89 I think if we can fix this here - we won't problems during pull request merge. thanks, -- Shuah
On 2/21/24 02:27, David Gow wrote: > KUnit has several macros which accept a log message, which can contain > printf format specifiers. Some of these (the explicit log macros) > already use the __printf() gcc attribute to ensure the format specifiers > are valid, but those which could fail the test, and hence used > __kunit_do_failed_assertion() behind the scenes, did not. > > These include: > - KUNIT_EXPECT_*_MSG() > - KUNIT_ASSERT_*_MSG() > - KUNIT_FAIL() > > This series adds the __printf() attribute, and fixes all of the issues > uncovered. (Or, at least, all of those I could find with an x86_64 > allyesconfig, and the default KUnit config on a number of other > architectures. Please test!) > > The issues in question basically take the following forms: > - int / long / long long confusion: typically a type being updated, but > the format string not. > - Use of integer format specifiers (%d/%u/%li/etc) for types like size_t > or pointer differences (technically ptrdiff_t), which would only work > on some architectures. > - Use of integer format specifiers in combination with PTR_ERR(), where > %pe would make more sense. > - Use of empty messages which, whilst technically not incorrect, are not > useful and trigger a gcc warning. > > We'd like to get these (or equivalent) in for 6.9 if possible, so please > do take a look if possible. > > Thanks, > -- David > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Closes: https://lore.kernel.org/linux-kselftest/CAHk-=wgJMOquDO5f8ShH1f4rzZwzApNVCw643m5-Yj+BfsFstA@mail.gmail.com/ > > David Gow (9): > kunit: test: Log the correct filter string in executor_test > lib/cmdline: Fix an invalid format specifier in an assertion msg > lib: memcpy_kunit: Fix an invalid format specifier in an assertion msg > time: test: Fix incorrect format specifier > rtc: test: Fix invalid format specifier. > net: test: Fix printf format specifier in skb_segment kunit test > drm: tests: Fix invalid printf format specifiers in KUnit tests > drm/xe/tests: Fix printf format specifiers in xe_migrate test > kunit: Annotate _MSG assertion variants with gnu printf specifiers > Applied all patches in this series except to linux-ksefltest kunit for linux 6.9-rc1 drm: tests: Fix invalid printf format specifiers in KUnit tests David, as requtested in 7/9 thread, if you can send me patch on top pf 6.8-rc6, will apply it 7-9 drm: tests: Fix invalid printf format specifiers in KUnit tests thanks, -- Shuah