2024-02-21 09:28:20

by David Gow

Subject: [PATCH 0/9] kunit: Fix printf format specifier issues in KUnit assertions

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 <[email protected]>
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

drivers/gpu/drm/tests/drm_buddy_test.c | 14 +++++++-------
drivers/gpu/drm/tests/drm_mm_test.c | 6 +++---
drivers/gpu/drm/xe/tests/xe_migrate.c | 8 ++++----
drivers/rtc/lib_test.c | 2 +-
include/kunit/test.h | 12 ++++++------
kernel/time/time_test.c | 2 +-
lib/cmdline_kunit.c | 2 +-
lib/kunit/executor_test.c | 2 +-
lib/memcpy_kunit.c | 4 ++--
net/core/gso_test.c | 2 +-
10 files changed, 27 insertions(+), 27 deletions(-)

--
2.44.0.rc0.258.g7320e95886-goog

2024-02-21 09:28:43

by David Gow

Subject: [PATCH 1/9] kunit: test: Log the correct filter string in executor_test

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 <[email protected]>
---
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

2024-02-21 09:31:55

by David Gow

Subject: [PATCH 7/9] drm: tests: Fix invalid printf format specifiers in KUnit tests

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
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 <[email protected]>
---
drivers/gpu/drm/tests/drm_buddy_test.c | 14 +++++++-------
drivers/gpu/drm/tests/drm_mm_test.c | 6 +++---
2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
index 8a464f7f4c61..3dbfa3078449 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -55,30 +55,30 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
KUNIT_ASSERT_FALSE_MSG(test,
drm_buddy_alloc_blocks(&mm, 0, mm_size,
ps, ps, list, 0),
- "buddy_alloc hit an error size=%d\n",
+ "buddy_alloc hit an error size=%lu\n",
ps);
} while (++i < n_pages);

KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
3 * ps, ps, &allocated,
DRM_BUDDY_CONTIGUOUS_ALLOCATION),
- "buddy_alloc didn't error size=%d\n", 3 * ps);
+ "buddy_alloc didn't error size=%lu\n", 3 * ps);

drm_buddy_free_list(&mm, &middle);
KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
3 * ps, ps, &allocated,
DRM_BUDDY_CONTIGUOUS_ALLOCATION),
- "buddy_alloc didn't error size=%llu\n", 3 * ps);
+ "buddy_alloc didn't error size=%lu\n", 3 * ps);
KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
2 * ps, ps, &allocated,
DRM_BUDDY_CONTIGUOUS_ALLOCATION),
- "buddy_alloc didn't error size=%llu\n", 2 * ps);
+ "buddy_alloc didn't error size=%lu\n", 2 * ps);

drm_buddy_free_list(&mm, &right);
KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
3 * ps, ps, &allocated,
DRM_BUDDY_CONTIGUOUS_ALLOCATION),
- "buddy_alloc didn't error size=%llu\n", 3 * ps);
+ "buddy_alloc didn't error size=%lu\n", 3 * ps);
/*
* At this point we should have enough contiguous space for 2 blocks,
* however they are never buddies (since we freed middle and right) so
@@ -87,13 +87,13 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
2 * ps, ps, &allocated,
DRM_BUDDY_CONTIGUOUS_ALLOCATION),
- "buddy_alloc hit an error size=%d\n", 2 * ps);
+ "buddy_alloc hit an error size=%lu\n", 2 * ps);

drm_buddy_free_list(&mm, &left);
KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
3 * ps, ps, &allocated,
DRM_BUDDY_CONTIGUOUS_ALLOCATION),
- "buddy_alloc hit an error size=%d\n", 3 * ps);
+ "buddy_alloc hit an error size=%lu\n", 3 * ps);

total = 0;
list_for_each_entry(block, &allocated, link)
diff --git a/drivers/gpu/drm/tests/drm_mm_test.c b/drivers/gpu/drm/tests/drm_mm_test.c
index 1eb0c304f960..f37c0d765865 100644
--- a/drivers/gpu/drm/tests/drm_mm_test.c
+++ b/drivers/gpu/drm/tests/drm_mm_test.c
@@ -157,7 +157,7 @@ static void drm_test_mm_init(struct kunit *test)

/* After creation, it should all be one massive hole */
if (!assert_one_hole(test, &mm, 0, size)) {
- KUNIT_FAIL(test, "");
+ KUNIT_FAIL(test, "mm not one hole on creation");
goto out;
}

@@ -171,14 +171,14 @@ static void drm_test_mm_init(struct kunit *test)

/* After filling the range entirely, there should be no holes */
if (!assert_no_holes(test, &mm)) {
- KUNIT_FAIL(test, "");
+ KUNIT_FAIL(test, "mm has holes when filled");
goto out;
}

/* And then after emptying it again, the massive hole should be back */
drm_mm_remove_node(&tmp);
if (!assert_one_hole(test, &mm, 0, size)) {
- KUNIT_FAIL(test, "");
+ KUNIT_FAIL(test, "mm does not have single hole after emptying");
goto out;
}

--
2.44.0.rc0.258.g7320e95886-goog

2024-02-21 09:32:22

by David Gow

Subject: [PATCH 8/9] drm/xe/tests: Fix printf format specifiers in xe_migrate test

KUNIT_FAIL() is used to fail the xe_migrate test when an error occurs.
However, there's a mismatch in the format specifier: '%li' is used to
log 'err', which is an 'int'.

Use '%i' instead of '%li', and for the case where we're printing an
error pointer, just use '%pe', instead of extracting the error code
manually with PTR_ERR(). (This also results in a nicer output when the
error code is known.)

Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
Signed-off-by: David Gow <[email protected]>
---
drivers/gpu/drm/xe/tests/xe_migrate.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c
index a6523df0f1d3..c347e2c29f81 100644
--- a/drivers/gpu/drm/xe/tests/xe_migrate.c
+++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
@@ -114,21 +114,21 @@ static void test_copy(struct xe_migrate *m, struct xe_bo *bo,
region |
XE_BO_NEEDS_CPU_ACCESS);
if (IS_ERR(remote)) {
- KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %li\n",
- str, PTR_ERR(remote));
+ KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %pe\n",
+ str, remote);
return;
}

err = xe_bo_validate(remote, NULL, false);
if (err) {
- KUNIT_FAIL(test, "Failed to validate system bo for %s: %li\n",
+ KUNIT_FAIL(test, "Failed to validate system bo for %s: %i\n",
str, err);
goto out_unlock;
}

err = xe_bo_vmap(remote);
if (err) {
- KUNIT_FAIL(test, "Failed to vmap system bo for %s: %li\n",
+ KUNIT_FAIL(test, "Failed to vmap system bo for %s: %i\n",
str, err);
goto out_unlock;
}
--
2.44.0.rc0.258.g7320e95886-goog

2024-02-21 09:32:28

by David Gow

Subject: [PATCH 2/9] lib/cmdline: Fix an invalid format specifier in an assertion msg

The correct format specifier for p - n (both p and n are pointers) is
%td, as the type should be ptrdiff_t.

This was discovered by annotating KUnit assertion macros with gcc's
printf specifier, but note that gcc incorrectly suggested a %d or %ld
specifier (depending on the pointer size of the architecture being
built).

Fixes: 0ea09083116d ("lib/cmdline: Allow get_options() to take 0 to validate the input")
Signed-off-by: David Gow <[email protected]>
---
lib/cmdline_kunit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
index d4572dbc9145..705b82736be0 100644
--- a/lib/cmdline_kunit.c
+++ b/lib/cmdline_kunit.c
@@ -124,7 +124,7 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in,
n, e[0], r[0]);

p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0]));
- KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %u out of bound", n, p - r);
+ KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %td out of bound", n, p - r);
}

static void cmdline_test_range(struct kunit *test)
--
2.44.0.rc0.258.g7320e95886-goog

2024-02-21 09:32:50

by David Gow

Subject: [PATCH 9/9] kunit: Annotate _MSG assertion variants with gnu printf specifiers

KUnit's assertion macros have variants which accept a printf format
string, to allow tests to specify a more detailed message on failure.
These (and the related KUNIT_FAIL() macro) ultimately wrap the
__kunit_do_failed_assertion() function, which accepted a printf format
specifier, but did not have the __printf attribute, so gcc couldn't warn
on incorrect agruments.

It turns out there were quite a few tests with such incorrect arguments.

Add the __printf() specifier now that we've fixed these errors, to
prevent them from recurring.

Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: David Gow <[email protected]>
---
include/kunit/test.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index fcb4a4940ace..61637ef32302 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -579,12 +579,12 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,

void __noreturn __kunit_abort(struct kunit *test);

-void __kunit_do_failed_assertion(struct kunit *test,
- const struct kunit_loc *loc,
- enum kunit_assert_type type,
- const struct kunit_assert *assert,
- assert_format_t assert_format,
- const char *fmt, ...);
+void __printf(6, 7) __kunit_do_failed_assertion(struct kunit *test,
+ const struct kunit_loc *loc,
+ enum kunit_assert_type type,
+ const struct kunit_assert *assert,
+ assert_format_t assert_format,
+ const char *fmt, ...);

#define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
--
2.44.0.rc0.258.g7320e95886-goog

2024-02-21 09:34:19

by David Gow

Subject: [PATCH 3/9] lib: memcpy_kunit: Fix an invalid format specifier in an assertion msg

The 'i' passed as an assertion message is a size_t, so should use '%zu',
not '%d'.

This was found by annotating the _MSG() variants of KUnit's assertions
to let gcc validate the format strings.

Fixes: bb95ebbe89a7 ("lib: Introduce CONFIG_MEMCPY_KUNIT_TEST")
Signed-off-by: David Gow <[email protected]>
---
lib/memcpy_kunit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
index 440aee705ccc..30e00ef0bf2e 100644
--- a/lib/memcpy_kunit.c
+++ b/lib/memcpy_kunit.c
@@ -32,7 +32,7 @@ struct some_bytes {
BUILD_BUG_ON(sizeof(instance.data) != 32); \
for (size_t i = 0; i < sizeof(instance.data); i++) { \
KUNIT_ASSERT_EQ_MSG(test, instance.data[i], v, \
- "line %d: '%s' not initialized to 0x%02x @ %d (saw 0x%02x)\n", \
+ "line %d: '%s' not initialized to 0x%02x @ %zu (saw 0x%02x)\n", \
__LINE__, #instance, v, i, instance.data[i]); \
} \
} while (0)
@@ -41,7 +41,7 @@ struct some_bytes {
BUILD_BUG_ON(sizeof(one) != sizeof(two)); \
for (size_t i = 0; i < sizeof(one); i++) { \
KUNIT_EXPECT_EQ_MSG(test, one.data[i], two.data[i], \
- "line %d: %s.data[%d] (0x%02x) != %s.data[%d] (0x%02x)\n", \
+ "line %d: %s.data[%zu] (0x%02x) != %s.data[%zu] (0x%02x)\n", \
__LINE__, #one, i, one.data[i], #two, i, two.data[i]); \
} \
kunit_info(test, "ok: " TEST_OP "() " name "\n"); \
--
2.44.0.rc0.258.g7320e95886-goog

2024-02-21 09:35:23

by David Gow

Subject: [PATCH 4/9] time: test: Fix incorrect format specifier

'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: 276010551664 ("time: Improve performance of time64_to_tm()")
Signed-off-by: David Gow <[email protected]>
---
kernel/time/time_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
index ca058c8af6ba..3e5d422dd15c 100644
--- a/kernel/time/time_test.c
+++ b/kernel/time/time_test.c
@@ -73,7 +73,7 @@ static void time64_to_tm_test_date_range(struct kunit *test)

days = div_s64(secs, 86400);

- #define FAIL_MSG "%05ld/%02d/%02d (%2d) : %ld", \
+ #define FAIL_MSG "%05ld/%02d/%02d (%2d) : %lld", \
year, month, mdday, yday, days

KUNIT_ASSERT_EQ_MSG(test, year - 1900, result.tm_year, FAIL_MSG);
--
2.44.0.rc0.258.g7320e95886-goog

2024-02-21 09:35:53

by David Gow

Subject: [PATCH 5/9] rtc: test: Fix invalid format specifier.

'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 <[email protected]>
---
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

2024-02-21 09:36:24

by David Gow

Subject: [PATCH 6/9] net: test: Fix printf format specifier in skb_segment kunit test

KUNIT_FAIL() accepts a printf-style format string, but previously did
not let gcc validate it with the __printf() attribute. The use of %lld
for the result of PTR_ERR() is not correct.

Instead, use %pe and pass the actual error pointer. printk() will format
it correctly (and give a symbolic name rather than a number if
available, which should make the output more readable, too).

Fixes: b3098d32ed6e ("net: add skb_segment kunit test")
Signed-off-by: David Gow <[email protected]>
---
net/core/gso_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/gso_test.c b/net/core/gso_test.c
index 4c2e77bd12f4..358c44680d91 100644
--- a/net/core/gso_test.c
+++ b/net/core/gso_test.c
@@ -225,7 +225,7 @@ static void gso_test_func(struct kunit *test)

segs = skb_segment(skb, features);
if (IS_ERR(segs)) {
- KUNIT_FAIL(test, "segs error %lld", PTR_ERR(segs));
+ KUNIT_FAIL(test, "segs error %pe", segs);
goto free_gso_skb;
} else if (!segs) {
KUNIT_FAIL(test, "no segments");
--
2.44.0.rc0.258.g7320e95886-goog

2024-02-21 10:22:10

by Matthew Auld

Subject: Re: [PATCH 7/9] drm: tests: Fix invalid printf format specifiers in KUnit tests

On 21/02/2024 09:27, 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
> 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 <[email protected]>
> ---
> drivers/gpu/drm/tests/drm_buddy_test.c | 14 +++++++-------
> drivers/gpu/drm/tests/drm_mm_test.c | 6 +++---
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
> index 8a464f7f4c61..3dbfa3078449 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -55,30 +55,30 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> KUNIT_ASSERT_FALSE_MSG(test,
> drm_buddy_alloc_blocks(&mm, 0, mm_size,
> ps, ps, list, 0),
> - "buddy_alloc hit an error size=%d\n",
> + "buddy_alloc hit an error size=%lu\n",
> ps);
> } while (++i < n_pages);
>
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%d\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
>
> drm_buddy_free_list(&mm, &middle);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 2 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 2 * ps);
> + "buddy_alloc didn't error size=%lu\n", 2 * ps);
>
> drm_buddy_free_list(&mm, &right);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
> /*
> * At this point we should have enough contiguous space for 2 blocks,
> * however they are never buddies (since we freed middle and right) so
> @@ -87,13 +87,13 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 2 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc hit an error size=%d\n", 2 * ps);
> + "buddy_alloc hit an error size=%lu\n", 2 * ps);
>
> drm_buddy_free_list(&mm, &left);
> KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc hit an error size=%d\n", 3 * ps);
> + "buddy_alloc hit an error size=%lu\n", 3 * ps);

There was also a fix for this in: 335126937753 ("drm/tests/drm_buddy:
fix 32b build"), but there everything was made u32.

Reviewed-by: Matthew Auld <[email protected]>

>
> total = 0;
> list_for_each_entry(block, &allocated, link)
> diff --git a/drivers/gpu/drm/tests/drm_mm_test.c b/drivers/gpu/drm/tests/drm_mm_test.c
> index 1eb0c304f960..f37c0d765865 100644
> --- a/drivers/gpu/drm/tests/drm_mm_test.c
> +++ b/drivers/gpu/drm/tests/drm_mm_test.c
> @@ -157,7 +157,7 @@ static void drm_test_mm_init(struct kunit *test)
>
> /* After creation, it should all be one massive hole */
> if (!assert_one_hole(test, &mm, 0, size)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm not one hole on creation");
> goto out;
> }
>
> @@ -171,14 +171,14 @@ static void drm_test_mm_init(struct kunit *test)
>
> /* After filling the range entirely, there should be no holes */
> if (!assert_no_holes(test, &mm)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm has holes when filled");
> goto out;
> }
>
> /* And then after emptying it again, the massive hole should be back */
> drm_mm_remove_node(&tmp);
> if (!assert_one_hole(test, &mm, 0, size)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm does not have single hole after emptying");
> goto out;
> }
>

2024-02-21 10:45:37

by Christian König

Subject: Re: [PATCH 7/9] drm: tests: Fix invalid printf format specifiers in KUnit tests

Am 21.02.24 um 10:27 schrieb David Gow:
> 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
> 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 <[email protected]>

Acked-by: Christian König <[email protected]>

Ping me if you want this patch pushed upstream through the DRM tree.

Christian.

> ---
> drivers/gpu/drm/tests/drm_buddy_test.c | 14 +++++++-------
> drivers/gpu/drm/tests/drm_mm_test.c | 6 +++---
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
> index 8a464f7f4c61..3dbfa3078449 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -55,30 +55,30 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> KUNIT_ASSERT_FALSE_MSG(test,
> drm_buddy_alloc_blocks(&mm, 0, mm_size,
> ps, ps, list, 0),
> - "buddy_alloc hit an error size=%d\n",
> + "buddy_alloc hit an error size=%lu\n",
> ps);
> } while (++i < n_pages);
>
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%d\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
>
> drm_buddy_free_list(&mm, &middle);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 2 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 2 * ps);
> + "buddy_alloc didn't error size=%lu\n", 2 * ps);
>
> drm_buddy_free_list(&mm, &right);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
> /*
> * At this point we should have enough contiguous space for 2 blocks,
> * however they are never buddies (since we freed middle and right) so
> @@ -87,13 +87,13 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 2 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc hit an error size=%d\n", 2 * ps);
> + "buddy_alloc hit an error size=%lu\n", 2 * ps);
>
> drm_buddy_free_list(&mm, &left);
> KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc hit an error size=%d\n", 3 * ps);
> + "buddy_alloc hit an error size=%lu\n", 3 * ps);
>
> total = 0;
> list_for_each_entry(block, &allocated, link)
> diff --git a/drivers/gpu/drm/tests/drm_mm_test.c b/drivers/gpu/drm/tests/drm_mm_test.c
> index 1eb0c304f960..f37c0d765865 100644
> --- a/drivers/gpu/drm/tests/drm_mm_test.c
> +++ b/drivers/gpu/drm/tests/drm_mm_test.c
> @@ -157,7 +157,7 @@ static void drm_test_mm_init(struct kunit *test)
>
> /* After creation, it should all be one massive hole */
> if (!assert_one_hole(test, &mm, 0, size)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm not one hole on creation");
> goto out;
> }
>
> @@ -171,14 +171,14 @@ static void drm_test_mm_init(struct kunit *test)
>
> /* After filling the range entirely, there should be no holes */
> if (!assert_no_holes(test, &mm)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm has holes when filled");
> goto out;
> }
>
> /* And then after emptying it again, the massive hole should be back */
> drm_mm_remove_node(&tmp);
> if (!assert_one_hole(test, &mm, 0, size)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm does not have single hole after emptying");
> goto out;
> }
>

2024-02-21 13:27:16

by Guenter Roeck

Subject: Re: [PATCH 1/9] kunit: test: Log the correct filter string in executor_test

On Wed, Feb 21, 2024 at 05:27:14PM +0800, David Gow 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 <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> 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
>

2024-02-21 13:27:39

by Guenter Roeck

Subject: Re: [PATCH 2/9] lib/cmdline: Fix an invalid format specifier in an assertion msg

On Wed, Feb 21, 2024 at 05:27:15PM +0800, David Gow wrote:
> The correct format specifier for p - n (both p and n are pointers) is
> %td, as the type should be ptrdiff_t.
>
> This was discovered by annotating KUnit assertion macros with gcc's
> printf specifier, but note that gcc incorrectly suggested a %d or %ld
> specifier (depending on the pointer size of the architecture being
> built).
>
> Fixes: 0ea09083116d ("lib/cmdline: Allow get_options() to take 0 to validate the input")
> Signed-off-by: David Gow <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> lib/cmdline_kunit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
> index d4572dbc9145..705b82736be0 100644
> --- a/lib/cmdline_kunit.c
> +++ b/lib/cmdline_kunit.c
> @@ -124,7 +124,7 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in,
> n, e[0], r[0]);
>
> p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0]));
> - KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %u out of bound", n, p - r);
> + KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %td out of bound", n, p - r);
> }
>
> static void cmdline_test_range(struct kunit *test)
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

2024-02-21 13:28:05

by Guenter Roeck

Subject: Re: [PATCH 3/9] lib: memcpy_kunit: Fix an invalid format specifier in an assertion msg

On Wed, Feb 21, 2024 at 05:27:16PM +0800, David Gow wrote:
> The 'i' passed as an assertion message is a size_t, so should use '%zu',
> not '%d'.
>
> This was found by annotating the _MSG() variants of KUnit's assertions
> to let gcc validate the format strings.
>
> Fixes: bb95ebbe89a7 ("lib: Introduce CONFIG_MEMCPY_KUNIT_TEST")
> Signed-off-by: David Gow <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> lib/memcpy_kunit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 440aee705ccc..30e00ef0bf2e 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -32,7 +32,7 @@ struct some_bytes {
> BUILD_BUG_ON(sizeof(instance.data) != 32); \
> for (size_t i = 0; i < sizeof(instance.data); i++) { \
> KUNIT_ASSERT_EQ_MSG(test, instance.data[i], v, \
> - "line %d: '%s' not initialized to 0x%02x @ %d (saw 0x%02x)\n", \
> + "line %d: '%s' not initialized to 0x%02x @ %zu (saw 0x%02x)\n", \
> __LINE__, #instance, v, i, instance.data[i]); \
> } \
> } while (0)
> @@ -41,7 +41,7 @@ struct some_bytes {
> BUILD_BUG_ON(sizeof(one) != sizeof(two)); \
> for (size_t i = 0; i < sizeof(one); i++) { \
> KUNIT_EXPECT_EQ_MSG(test, one.data[i], two.data[i], \
> - "line %d: %s.data[%d] (0x%02x) != %s.data[%d] (0x%02x)\n", \
> + "line %d: %s.data[%zu] (0x%02x) != %s.data[%zu] (0x%02x)\n", \
> __LINE__, #one, i, one.data[i], #two, i, two.data[i]); \
> } \
> kunit_info(test, "ok: " TEST_OP "() " name "\n"); \
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

2024-02-21 13:28:53

by Guenter Roeck

Subject: Re: [PATCH 5/9] rtc: test: Fix invalid format specifier.

On Wed, Feb 21, 2024 at 05:27:18PM +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 <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> 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
>

2024-02-21 13:29:18

by Guenter Roeck

Subject: Re: [PATCH 6/9] net: test: Fix printf format specifier in skb_segment kunit test

On Wed, Feb 21, 2024 at 05:27:19PM +0800, David Gow wrote:
> KUNIT_FAIL() accepts a printf-style format string, but previously did
> not let gcc validate it with the __printf() attribute. The use of %lld
> for the result of PTR_ERR() is not correct.
>
> Instead, use %pe and pass the actual error pointer. printk() will format
> it correctly (and give a symbolic name rather than a number if
> available, which should make the output more readable, too).
>
> Fixes: b3098d32ed6e ("net: add skb_segment kunit test")
> Signed-off-by: David Gow <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> net/core/gso_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/gso_test.c b/net/core/gso_test.c
> index 4c2e77bd12f4..358c44680d91 100644
> --- a/net/core/gso_test.c
> +++ b/net/core/gso_test.c
> @@ -225,7 +225,7 @@ static void gso_test_func(struct kunit *test)
>
> segs = skb_segment(skb, features);
> if (IS_ERR(segs)) {
> - KUNIT_FAIL(test, "segs error %lld", PTR_ERR(segs));
> + KUNIT_FAIL(test, "segs error %pe", segs);
> goto free_gso_skb;
> } else if (!segs) {
> KUNIT_FAIL(test, "no segments");
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

2024-02-21 13:32:35

by Guenter Roeck

Subject: Re: [PATCH 9/9] kunit: Annotate _MSG assertion variants with gnu printf specifiers

On Wed, Feb 21, 2024 at 05:27:22PM +0800, David Gow wrote:
> KUnit's assertion macros have variants which accept a printf format
> string, to allow tests to specify a more detailed message on failure.
> These (and the related KUNIT_FAIL() macro) ultimately wrap the
> __kunit_do_failed_assertion() function, which accepted a printf format
> specifier, but did not have the __printf attribute, so gcc couldn't warn
> on incorrect agruments.
>
> It turns out there were quite a few tests with such incorrect arguments.
>
> Add the __printf() specifier now that we've fixed these errors, to
> prevent them from recurring.
>
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: David Gow <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> include/kunit/test.h | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index fcb4a4940ace..61637ef32302 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -579,12 +579,12 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
>
> void __noreturn __kunit_abort(struct kunit *test);
>
> -void __kunit_do_failed_assertion(struct kunit *test,
> - const struct kunit_loc *loc,
> - enum kunit_assert_type type,
> - const struct kunit_assert *assert,
> - assert_format_t assert_format,
> - const char *fmt, ...);
> +void __printf(6, 7) __kunit_do_failed_assertion(struct kunit *test,
> + const struct kunit_loc *loc,
> + enum kunit_assert_type type,
> + const struct kunit_assert *assert,
> + assert_format_t assert_format,
> + const char *fmt, ...);
>
> #define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
> static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

2024-02-21 13:39:31

by Guenter Roeck

Subject: Re: [PATCH 4/9] time: test: Fix incorrect format specifier

On Wed, Feb 21, 2024 at 05:27:17PM +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: 276010551664 ("time: Improve performance of time64_to_tm()")
> Signed-off-by: David Gow <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> kernel/time/time_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
> index ca058c8af6ba..3e5d422dd15c 100644
> --- a/kernel/time/time_test.c
> +++ b/kernel/time/time_test.c
> @@ -73,7 +73,7 @@ static void time64_to_tm_test_date_range(struct kunit *test)
>
> days = div_s64(secs, 86400);
>
> - #define FAIL_MSG "%05ld/%02d/%02d (%2d) : %ld", \
> + #define FAIL_MSG "%05ld/%02d/%02d (%2d) : %lld", \
> year, month, mdday, yday, days
>
> KUNIT_ASSERT_EQ_MSG(test, year - 1900, result.tm_year, FAIL_MSG);
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

2024-02-21 13:41:58

by Guenter Roeck

Subject: Re: [PATCH 7/9] drm: tests: Fix invalid printf format specifiers in KUnit tests

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
> 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 <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> drivers/gpu/drm/tests/drm_buddy_test.c | 14 +++++++-------
> drivers/gpu/drm/tests/drm_mm_test.c | 6 +++---
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
> index 8a464f7f4c61..3dbfa3078449 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -55,30 +55,30 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> KUNIT_ASSERT_FALSE_MSG(test,
> drm_buddy_alloc_blocks(&mm, 0, mm_size,
> ps, ps, list, 0),
> - "buddy_alloc hit an error size=%d\n",
> + "buddy_alloc hit an error size=%lu\n",
> ps);
> } while (++i < n_pages);
>
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%d\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
>
> drm_buddy_free_list(&mm, &middle);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 2 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 2 * ps);
> + "buddy_alloc didn't error size=%lu\n", 2 * ps);
>
> drm_buddy_free_list(&mm, &right);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
> /*
> * At this point we should have enough contiguous space for 2 blocks,
> * however they are never buddies (since we freed middle and right) so
> @@ -87,13 +87,13 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 2 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc hit an error size=%d\n", 2 * ps);
> + "buddy_alloc hit an error size=%lu\n", 2 * ps);
>
> drm_buddy_free_list(&mm, &left);
> KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc hit an error size=%d\n", 3 * ps);
> + "buddy_alloc hit an error size=%lu\n", 3 * ps);
>
> total = 0;
> list_for_each_entry(block, &allocated, link)
> diff --git a/drivers/gpu/drm/tests/drm_mm_test.c b/drivers/gpu/drm/tests/drm_mm_test.c
> index 1eb0c304f960..f37c0d765865 100644
> --- a/drivers/gpu/drm/tests/drm_mm_test.c
> +++ b/drivers/gpu/drm/tests/drm_mm_test.c
> @@ -157,7 +157,7 @@ static void drm_test_mm_init(struct kunit *test)
>
> /* After creation, it should all be one massive hole */
> if (!assert_one_hole(test, &mm, 0, size)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm not one hole on creation");
> goto out;
> }
>
> @@ -171,14 +171,14 @@ static void drm_test_mm_init(struct kunit *test)
>
> /* After filling the range entirely, there should be no holes */
> if (!assert_no_holes(test, &mm)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm has holes when filled");
> goto out;
> }
>
> /* And then after emptying it again, the massive hole should be back */
> drm_mm_remove_node(&tmp);
> if (!assert_one_hole(test, &mm, 0, size)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm does not have single hole after emptying");
> goto out;
> }
>
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

2024-02-21 13:42:36

by Guenter Roeck

Subject: Re: [PATCH 8/9] drm/xe/tests: Fix printf format specifiers in xe_migrate test

On Wed, Feb 21, 2024 at 05:27:21PM +0800, David Gow wrote:
> KUNIT_FAIL() is used to fail the xe_migrate test when an error occurs.
> However, there's a mismatch in the format specifier: '%li' is used to
> log 'err', which is an 'int'.
>
> Use '%i' instead of '%li', and for the case where we're printing an
> error pointer, just use '%pe', instead of extracting the error code
> manually with PTR_ERR(). (This also results in a nicer output when the
> error code is known.)
>
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Signed-off-by: David Gow <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> drivers/gpu/drm/xe/tests/xe_migrate.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c
> index a6523df0f1d3..c347e2c29f81 100644
> --- a/drivers/gpu/drm/xe/tests/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
> @@ -114,21 +114,21 @@ static void test_copy(struct xe_migrate *m, struct xe_bo *bo,
> region |
> XE_BO_NEEDS_CPU_ACCESS);
> if (IS_ERR(remote)) {
> - KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %li\n",
> - str, PTR_ERR(remote));
> + KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %pe\n",
> + str, remote);
> return;
> }
>
> err = xe_bo_validate(remote, NULL, false);
> if (err) {
> - KUNIT_FAIL(test, "Failed to validate system bo for %s: %li\n",
> + KUNIT_FAIL(test, "Failed to validate system bo for %s: %i\n",
> str, err);
> goto out_unlock;
> }
>
> err = xe_bo_vmap(remote);
> if (err) {
> - KUNIT_FAIL(test, "Failed to vmap system bo for %s: %li\n",
> + KUNIT_FAIL(test, "Failed to vmap system bo for %s: %i\n",
> str, err);
> goto out_unlock;
> }
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

2024-02-21 18:21:41

by tip-bot2 for Alexey Makhalov

Subject: [tip: timers/core] time/kunit: Use correct format specifier

The following commit has been merged into the timers/core branch of tip:

Commit-ID: e0a1284b293bdf91a68a6d1a0479ad476d0d8ec2
Gitweb: https://git.kernel.org/tip/e0a1284b293bdf91a68a6d1a0479ad476d0d8ec2
Author: David Gow <[email protected]>
AuthorDate: Wed, 21 Feb 2024 17:27:17 +08:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 21 Feb 2024 12:00:42 +01:00

time/kunit: Use correct format specifier

'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: 276010551664 ("time: Improve performance of time64_to_tm()")
Signed-off-by: David Gow <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lore.kernel.org/r/[email protected]

---
kernel/time/time_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
index ca058c8..3e5d422 100644
--- a/kernel/time/time_test.c
+++ b/kernel/time/time_test.c
@@ -73,7 +73,7 @@ static void time64_to_tm_test_date_range(struct kunit *test)

days = div_s64(secs, 86400);

- #define FAIL_MSG "%05ld/%02d/%02d (%2d) : %ld", \
+ #define FAIL_MSG "%05ld/%02d/%02d (%2d) : %lld", \
year, month, mdday, yday, days

KUNIT_ASSERT_EQ_MSG(test, year - 1900, result.tm_year, FAIL_MSG);

2024-02-21 20:04:36

by Justin Stitt

Subject: Re: [PATCH 1/9] kunit: test: Log the correct filter string in executor_test

Hi,

On Wed, Feb 21, 2024 at 05:27:14PM +0800, David Gow 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 <[email protected]>

Reviewed-by: Justin Stitt <[email protected]>
> ---
> 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
>

Thanks
Justin

2024-02-21 20:10:36

by Justin Stitt

Subject: Re: [PATCH 2/9] lib/cmdline: Fix an invalid format specifier in an assertion msg

Hi,

On Wed, Feb 21, 2024 at 05:27:15PM +0800, David Gow wrote:
> The correct format specifier for p - n (both p and n are pointers) is
> %td, as the type should be ptrdiff_t.

I think %tu is better. d specifies a signed type. I don't doubt that the
warning is fixed but I think %tu represents the type semantics here.

>
> This was discovered by annotating KUnit assertion macros with gcc's
> printf specifier, but note that gcc incorrectly suggested a %d or %ld
> specifier (depending on the pointer size of the architecture being
> built).
>
> Fixes: 0ea09083116d ("lib/cmdline: Allow get_options() to take 0 to validate the input")
> Signed-off-by: David Gow <[email protected]>


> ---
> lib/cmdline_kunit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/cmdline_kunit.c b/lib/cmdline_kunit.c
> index d4572dbc9145..705b82736be0 100644
> --- a/lib/cmdline_kunit.c
> +++ b/lib/cmdline_kunit.c
> @@ -124,7 +124,7 @@ static void cmdline_do_one_range_test(struct kunit *test, const char *in,
> n, e[0], r[0]);
>
> p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0]));
> - KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %u out of bound", n, p - r);
> + KUNIT_EXPECT_PTR_EQ_MSG(test, p, NULL, "in test %u at %td out of bound", n, p - r);
> }
>
> static void cmdline_test_range(struct kunit *test)
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

Thanks
Justin

2024-02-21 20:27:35

by Justin Stitt

Subject: Re: [PATCH 9/9] kunit: Annotate _MSG assertion variants with gnu printf specifiers

Hi,

On Wed, Feb 21, 2024 at 05:27:22PM +0800, David Gow wrote:
> KUnit's assertion macros have variants which accept a printf format
> string, to allow tests to specify a more detailed message on failure.
> These (and the related KUNIT_FAIL() macro) ultimately wrap the
> __kunit_do_failed_assertion() function, which accepted a printf format
> specifier, but did not have the __printf attribute, so gcc couldn't warn
> on incorrect agruments.
>
> It turns out there were quite a few tests with such incorrect arguments.
>
> Add the __printf() specifier now that we've fixed these errors, to
> prevent them from recurring.
>
> Suggested-by: Linus Torvalds <[email protected]>
> Signed-off-by: David Gow <[email protected]>

Reviewed-by: Justin Stitt <[email protected]>
> ---
> include/kunit/test.h | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index fcb4a4940ace..61637ef32302 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -579,12 +579,12 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
>
> void __noreturn __kunit_abort(struct kunit *test);
>
> -void __kunit_do_failed_assertion(struct kunit *test,
> - const struct kunit_loc *loc,
> - enum kunit_assert_type type,
> - const struct kunit_assert *assert,
> - assert_format_t assert_format,
> - const char *fmt, ...);
> +void __printf(6, 7) __kunit_do_failed_assertion(struct kunit *test,
> + const struct kunit_loc *loc,
> + enum kunit_assert_type type,
> + const struct kunit_assert *assert,
> + assert_format_t assert_format,
> + const char *fmt, ...);
>
> #define _KUNIT_FAILED(test, assert_type, assert_class, assert_format, INITIALIZER, fmt, ...) do { \
> static const struct kunit_loc __loc = KUNIT_CURRENT_LOC; \
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

Thanks
Justin

2024-02-21 20:36:41

by Daniel Latypov

Subject: Re: [PATCH 1/9] kunit: test: Log the correct filter string in executor_test

On Wed, Feb 21, 2024 at 1:28 AM David Gow <[email protected]> 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.

Note: it's worse than that, afaict.

It's printing from a random bit of memory.
I was curious about this, so I found under UML, the string I got was
always "efault)" if I make it fail for j=0.

>
> 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 <[email protected]>

Reviewed-by: Daniel Latypov <[email protected]>

> ---
> 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);

note: if there is a v2, it might be nice to include `j` in the message.

2024-02-21 21:05:53

by Justin Stitt

Subject: Re: [PATCH 3/9] lib: memcpy_kunit: Fix an invalid format specifier in an assertion msg

Hi,

On Wed, Feb 21, 2024 at 05:27:16PM +0800, David Gow wrote:
> The 'i' passed as an assertion message is a size_t, so should use '%zu',
> not '%d'.
>
> This was found by annotating the _MSG() variants of KUnit's assertions
> to let gcc validate the format strings.
>
> Fixes: bb95ebbe89a7 ("lib: Introduce CONFIG_MEMCPY_KUNIT_TEST")
> Signed-off-by: David Gow <[email protected]>
> ---

Reviewed-by: Justin Stitt <[email protected]>
> lib/memcpy_kunit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 440aee705ccc..30e00ef0bf2e 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -32,7 +32,7 @@ struct some_bytes {
> BUILD_BUG_ON(sizeof(instance.data) != 32); \
> for (size_t i = 0; i < sizeof(instance.data); i++) { \
> KUNIT_ASSERT_EQ_MSG(test, instance.data[i], v, \
> - "line %d: '%s' not initialized to 0x%02x @ %d (saw 0x%02x)\n", \
> + "line %d: '%s' not initialized to 0x%02x @ %zu (saw 0x%02x)\n", \
> __LINE__, #instance, v, i, instance.data[i]); \
> } \
> } while (0)
> @@ -41,7 +41,7 @@ struct some_bytes {
> BUILD_BUG_ON(sizeof(one) != sizeof(two)); \
> for (size_t i = 0; i < sizeof(one); i++) { \
> KUNIT_EXPECT_EQ_MSG(test, one.data[i], two.data[i], \
> - "line %d: %s.data[%d] (0x%02x) != %s.data[%d] (0x%02x)\n", \
> + "line %d: %s.data[%zu] (0x%02x) != %s.data[%zu] (0x%02x)\n", \
> __LINE__, #one, i, one.data[i], #two, i, two.data[i]); \
> } \
> kunit_info(test, "ok: " TEST_OP "() " name "\n"); \
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

Thanks
Justin

2024-02-21 21:07:05

by Justin Stitt

Subject: Re: [PATCH 4/9] time: test: Fix incorrect format specifier

Hi,

On Wed, Feb 21, 2024 at 05:27:17PM +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: 276010551664 ("time: Improve performance of time64_to_tm()")
> Signed-off-by: David Gow <[email protected]>

Reviewed-by: Justin Stitt <[email protected]>
> ---
> kernel/time/time_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/time_test.c b/kernel/time/time_test.c
> index ca058c8af6ba..3e5d422dd15c 100644
> --- a/kernel/time/time_test.c
> +++ b/kernel/time/time_test.c
> @@ -73,7 +73,7 @@ static void time64_to_tm_test_date_range(struct kunit *test)
>
> days = div_s64(secs, 86400);
>
> - #define FAIL_MSG "%05ld/%02d/%02d (%2d) : %ld", \
> + #define FAIL_MSG "%05ld/%02d/%02d (%2d) : %lld", \
> year, month, mdday, yday, days
>
> KUNIT_ASSERT_EQ_MSG(test, year - 1900, result.tm_year, FAIL_MSG);
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

Thanks
Justin

2024-02-21 21:07:19

by Justin Stitt

Subject: Re: [PATCH 5/9] rtc: test: Fix invalid format specifier.

Hi,

On Wed, Feb 21, 2024 at 05:27:18PM +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 <[email protected]>

Reviewed-by: Justin Stitt <[email protected]>
> ---
> 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
>

Thanks
Justin

2024-02-21 21:46:12

by Justin Stitt

Subject: Re: [PATCH 7/9] drm: tests: Fix invalid printf format specifiers in KUnit tests

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 <[email protected]>

Reviewed-by: Justin Stitt <[email protected]>
> ---
> drivers/gpu/drm/tests/drm_buddy_test.c | 14 +++++++-------
> drivers/gpu/drm/tests/drm_mm_test.c | 6 +++---
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
> index 8a464f7f4c61..3dbfa3078449 100644
> --- a/drivers/gpu/drm/tests/drm_buddy_test.c
> +++ b/drivers/gpu/drm/tests/drm_buddy_test.c
> @@ -55,30 +55,30 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> KUNIT_ASSERT_FALSE_MSG(test,
> drm_buddy_alloc_blocks(&mm, 0, mm_size,
> ps, ps, list, 0),
> - "buddy_alloc hit an error size=%d\n",
> + "buddy_alloc hit an error size=%lu\n",
> ps);
> } while (++i < n_pages);
>
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%d\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
>
> drm_buddy_free_list(&mm, &middle);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 2 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 2 * ps);
> + "buddy_alloc didn't error size=%lu\n", 2 * ps);
>
> drm_buddy_free_list(&mm, &right);
> KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc didn't error size=%llu\n", 3 * ps);
> + "buddy_alloc didn't error size=%lu\n", 3 * ps);
> /*
> * At this point we should have enough contiguous space for 2 blocks,
> * however they are never buddies (since we freed middle and right) so
> @@ -87,13 +87,13 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
> KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 2 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc hit an error size=%d\n", 2 * ps);
> + "buddy_alloc hit an error size=%lu\n", 2 * ps);
>
> drm_buddy_free_list(&mm, &left);
> KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
> 3 * ps, ps, &allocated,
> DRM_BUDDY_CONTIGUOUS_ALLOCATION),
> - "buddy_alloc hit an error size=%d\n", 3 * ps);
> + "buddy_alloc hit an error size=%lu\n", 3 * ps);
>
> total = 0;
> list_for_each_entry(block, &allocated, link)
> diff --git a/drivers/gpu/drm/tests/drm_mm_test.c b/drivers/gpu/drm/tests/drm_mm_test.c
> index 1eb0c304f960..f37c0d765865 100644
> --- a/drivers/gpu/drm/tests/drm_mm_test.c
> +++ b/drivers/gpu/drm/tests/drm_mm_test.c
> @@ -157,7 +157,7 @@ static void drm_test_mm_init(struct kunit *test)
>
> /* After creation, it should all be one massive hole */
> if (!assert_one_hole(test, &mm, 0, size)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm not one hole on creation");
> goto out;
> }
>
> @@ -171,14 +171,14 @@ static void drm_test_mm_init(struct kunit *test)
>
> /* After filling the range entirely, there should be no holes */
> if (!assert_no_holes(test, &mm)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm has holes when filled");
> goto out;
> }
>
> /* And then after emptying it again, the massive hole should be back */
> drm_mm_remove_node(&tmp);
> if (!assert_one_hole(test, &mm, 0, size)) {
> - KUNIT_FAIL(test, "");
> + KUNIT_FAIL(test, "mm does not have single hole after emptying");
> goto out;
> }
>
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

Thanks
Justin

2024-02-21 21:46:35

by Justin Stitt

Subject: Re: [PATCH 6/9] net: test: Fix printf format specifier in skb_segment kunit test

Hi,

On Wed, Feb 21, 2024 at 05:27:19PM +0800, David Gow wrote:
> KUNIT_FAIL() accepts a printf-style format string, but previously did
> not let gcc validate it with the __printf() attribute. The use of %lld
> for the result of PTR_ERR() is not correct.
>
> Instead, use %pe and pass the actual error pointer. printk() will format
> it correctly (and give a symbolic name rather than a number if
> available, which should make the output more readable, too).
>
> Fixes: b3098d32ed6e ("net: add skb_segment kunit test")
> Signed-off-by: David Gow <[email protected]>

Looks good.

For those wondering, %pe has a special meaning in the kernel which can
be seen in lib/vsprintf.c.

Reviewed-by: Justin Stitt <[email protected]>
> ---
> net/core/gso_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/gso_test.c b/net/core/gso_test.c
> index 4c2e77bd12f4..358c44680d91 100644
> --- a/net/core/gso_test.c
> +++ b/net/core/gso_test.c
> @@ -225,7 +225,7 @@ static void gso_test_func(struct kunit *test)
>
> segs = skb_segment(skb, features);
> if (IS_ERR(segs)) {
> - KUNIT_FAIL(test, "segs error %lld", PTR_ERR(segs));
> + KUNIT_FAIL(test, "segs error %pe", segs);
> goto free_gso_skb;
> } else if (!segs) {
> KUNIT_FAIL(test, "no segments");
> --
> 2.44.0.rc0.258.g7320e95886-goog
>

Thanks
Justin

2024-02-22 05:05:39

by Lucas De Marchi

Subject: Re: [PATCH 8/9] drm/xe/tests: Fix printf format specifiers in xe_migrate test

On Wed, Feb 21, 2024 at 05:27:21PM +0800, David Gow wrote:
>KUNIT_FAIL() is used to fail the xe_migrate test when an error occurs.
>However, there's a mismatch in the format specifier: '%li' is used to
>log 'err', which is an 'int'.
>
>Use '%i' instead of '%li', and for the case where we're printing an
>error pointer, just use '%pe', instead of extracting the error code
>manually with PTR_ERR(). (This also results in a nicer output when the
>error code is known.)
>
>Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
>Signed-off-by: David Gow <[email protected]>


Reviewed-by: Lucas De Marchi <[email protected]>

this has a potential to cause conflicts with upcoming work, so I think
it's better to apply this through drm-xe-next. Let me know if you agree.

thanks
Lucas De Marchi

>---
> drivers/gpu/drm/xe/tests/xe_migrate.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c
>index a6523df0f1d3..c347e2c29f81 100644
>--- a/drivers/gpu/drm/xe/tests/xe_migrate.c
>+++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
>@@ -114,21 +114,21 @@ static void test_copy(struct xe_migrate *m, struct xe_bo *bo,
> region |
> XE_BO_NEEDS_CPU_ACCESS);
> if (IS_ERR(remote)) {
>- KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %li\n",
>- str, PTR_ERR(remote));
>+ KUNIT_FAIL(test, "Failed to allocate remote bo for %s: %pe\n",
>+ str, remote);
> return;
> }
>
> err = xe_bo_validate(remote, NULL, false);
> if (err) {
>- KUNIT_FAIL(test, "Failed to validate system bo for %s: %li\n",
>+ KUNIT_FAIL(test, "Failed to validate system bo for %s: %i\n",
> str, err);
> goto out_unlock;
> }
>
> err = xe_bo_vmap(remote);
> if (err) {
>- KUNIT_FAIL(test, "Failed to vmap system bo for %s: %li\n",
>+ KUNIT_FAIL(test, "Failed to vmap system bo for %s: %i\n",
> str, err);
> goto out_unlock;
> }
>--
>2.44.0.rc0.258.g7320e95886-goog
>

2024-02-22 06:00:37

by Linus Torvalds

Subject: Re: [PATCH 8/9] drm/xe/tests: Fix printf format specifiers in xe_migrate test

On Wed, 21 Feb 2024 at 21:05, Lucas De Marchi <[email protected]> wrote:
>
> this has a potential to cause conflicts with upcoming work, so I think
> it's better to apply this through drm-xe-next. Let me know if you agree.

I disagree. Violently.

For this to be fixed, we need to have the printf format checking enabled.

And we can't enable it until all the problems have been fixed.

Which means that we should *not* have to wait for [N] different trees
to fix their issues separately.

This should get fixed in the Kunit tree, so that the Kunit tree can
just send a pull request to me to enable format checking for the KUnit
tests, together with all the fixes. Trying to spread those fixes out
to different git branches will only result in pain and pointless
dependencies between different trees.

Honestly, the reason I noticed the problem in the first place was that
the drm tree had a separate bug, that had been apparently noted in
linux-next, and *despite* that it made it into a pull request to me
and caused new build failures in rc5.

So as things are, I am not IN THE LEAST interested in some kind of
"let us fix this in the drm tree separately" garbage. We're not
making things worse by trying to fix this in different trees.

We're fixing this in the Kunit tree, and I do not want to get *more*
problems from the drm side. I've had enough.

Linus

2024-02-22 06:22:44

by David Gow

Subject: Re: [PATCH 2/9] lib/cmdline: Fix an invalid format specifier in an assertion msg

On Thu, 22 Feb 2024 at 04:10, 'Justin Stitt' via KUnit Development
<[email protected]> wrote:
>
> Hi,
>
> On Wed, Feb 21, 2024 at 05:27:15PM +0800, David Gow wrote:
> > The correct format specifier for p - n (both p and n are pointers) is
> > %td, as the type should be ptrdiff_t.
>
> I think %tu is better. d specifies a signed type. I don't doubt that the
> warning is fixed but I think %tu represents the type semantics here.
>

While I agree that this should never be negative, I'd still lean on
this being a signed type, for two reasons:
- I think, if there's a bug in this code, it's easier to debug this if
a 'negative' value were to appear as such.
- While, as I understand it, the C spec does provide for a
ptrdiff_t-sized unsigned printf specifier in '%tu', the difference
between two pointers is always signed:

"When two pointers are subtracted, both shall point to elements of the
same array object,
or one past the last element of the array object; the result is the
difference of the
subscripts of the two array elements. The size of the result is
implementation-defined,
and its type (a signed integer type) is ptrdiff_t defined in the
<stddef.h> header"

(Technically, the kernel's ptrdiff_t type isn't defined in stddef.h,
so a bit of deviation from the spec is happening anyway, though.)

If there's a particularly good reason to make this unsigned in this
case, I'd be happy to change it, of course. But I'd otherwise prefer
to keep it as-is.

Cheers,
-- David

Attachments:

2024-02-22 09:52:58

by Thomas Hellström

Subject: Re: [PATCH 8/9] drm/xe/tests: Fix printf format specifiers in xe_migrate test

On Wed, 2024-02-21 at 17:27 +0800, David Gow wrote:
> KUNIT_FAIL() is used to fail the xe_migrate test when an error
> occurs.
> However, there's a mismatch in the format specifier: '%li' is used to
> log 'err', which is an 'int'.
>
> Use '%i' instead of '%li', and for the case where we're printing an
> error pointer, just use '%pe', instead of extracting the error code
> manually with PTR_ERR(). (This also results in a nicer output when
> the
> error code is known.)
>
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
> GPUs")
> Signed-off-by: David Gow <[email protected]>

Ack to merge through the Kunit tree.
Acked-by: Thomas Hellström <[email protected]>

2024-02-22 14:24:38

by Shuah Khan

Subject: Re: [PATCH 0/9] kunit: Fix printf format specifier issues in KUnit assertions

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 <[email protected]>
> 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

2024-02-22 17:44:00

by Daniel Latypov

Subject: Re: [PATCH 2/9] lib/cmdline: Fix an invalid format specifier in an assertion msg

On Wed, Feb 21, 2024 at 10:22 PM David Gow <[email protected]> wrote:
>
> On Thu, 22 Feb 2024 at 04:10, 'Justin Stitt' via KUnit Development
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On Wed, Feb 21, 2024 at 05:27:15PM +0800, David Gow wrote:
> > > The correct format specifier for p - n (both p and n are pointers) is
> > > %td, as the type should be ptrdiff_t.
> >
> > I think %tu is better. d specifies a signed type. I don't doubt that the
> > warning is fixed but I think %tu represents the type semantics here.
> >
>
> While I agree that this should never be negative, I'd still lean on
> this being a signed type, for two reasons:
> - I think, if there's a bug in this code, it's easier to debug this if
> a 'negative' value were to appear as such.
> - While, as I understand it, the C spec does provide for a
> ptrdiff_t-sized unsigned printf specifier in '%tu', the difference
> between two pointers is always signed:
>
> "When two pointers are subtracted, both shall point to elements of the
> same array object,
> or one past the last element of the array object; the result is the
> difference of the
> subscripts of the two array elements. The size of the result is
> implementation-defined,
> and its type (a signed integer type) is ptrdiff_t defined in the
> <stddef.h> header"
>
> (Technically, the kernel's ptrdiff_t type isn't defined in stddef.h,
> so a bit of deviation from the spec is happening anyway, though.)
>
> If there's a particularly good reason to make this unsigned in this
> case, I'd be happy to change it, of course. But I'd otherwise prefer
> to keep it as-is.

Copying the line for context, it's about `p-r` where
p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0]));
`p-r` should never be negative unless something has gone horribly
horribly wrong.

So in this particular case, either %tu or %td would be fine.

(sorta bikeshedding warning)
But, I'd personally lean towards using the signed %td in tests to
guard against typos in test code as _a guiding principle._

This is especially true given that the failure messages aren't
verified since they are mostly "dead code."
You can have crazy incorrect things going on in the format arguments,
see patch 1/9 in this series [1]. One of kunit's own tests would do a
read from a ~random memory region if that specific assertion failed.
Not a good look ;)
We never noticed until this series enabled the format string checks.
You also can't expect reviewers to go through and modify every
assertion to fail to see what the failure mode looks like, so these
kinds of errors will continue to slip through.

*So IMO, we should generally adopt a more defensive stance when it
comes to these.*

Also consider the user experience if there is a failure and I
accidentally wrote `r-p` here.
Someone else sees an error report from this test and needs to investigate.

What message is easier to deal with?
> in test 18 at -5 out of bound
or
> in test 18 at 18446744073709551611 out of bound

Sure, I can eventually figure out what both messages mean, but it's a
immediately obvious from the first that there's a
a) real error: something is wrong at index 5
b) test code error: there's a flipped sign somewhere

So I'd strongly prefer the current version of the patch over one with %tu.
Reviewed-by: Daniel Latypov <[email protected]>

[1] https://lore.kernel.org/linux-kselftest/[email protected]/

Thanks,
Daniel

2024-02-22 18:10:21

by Linus Torvalds

Subject: Re: [PATCH 2/9] lib/cmdline: Fix an invalid format specifier in an assertion msg

On Thu, 22 Feb 2024 at 09:36, Daniel Latypov <[email protected]> wrote:
>
> Copying the line for context, it's about `p-r` where
> p = memchr_inv(&r[1], 0, sizeof(r) - sizeof(r[0]));
> `p-r` should never be negative unless something has gone horribly
> horribly wrong.

Sure it would - if 'p' is NULL.

Of course, then a negative value wouldn't be helpful either, and in
this case that's what the EXPECT_PTR_EQ checking is testing in the
first place, so it's a non-issue.

IOW, in practice clearly the sign should simply not matter here.

I do think that the default case for pointer differences should be
that they are signed, because they *can* be.

Just because of that "default case", unless there's some actual reason
to use '%tu', I think '%td' should be seen as the normal case to use.

That said, just as a quick aside: be careful with pointer differences
in the kernel.

For this particular case, when we're talking about just 'char *', it's
not a big deal, but we've had code where people didn't think about
what it means to do a pointer difference in C, and how it can be often
unnecessarily expensive due to the implied "divide by the size of the
pointed object".

Sometimes it's actually worth writing the code in ways that avoids
pointer differences entirely (which might involve passing around
indexes instead of pointers).

Linus

2024-02-22 20:59:22

by Rae Moar

Subject: Re: [PATCH 1/9] kunit: test: Log the correct filter string in executor_test

On Wed, Feb 21, 2024 at 4:28 AM David Gow <[email protected]> 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 <[email protected]>

Hello!

This change looks good to me. Thanks for fixing this mistake.

Thanks!
-Rae

Reviewed-by: Rae Moar <[email protected]>

> ---
> 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
>

2024-02-23 07:02:08

by David Gow

Subject: Re: [tip: timers/core] time/kunit: Use correct format specifier

Hi,

On Thu, 22 Feb 2024 at 02:21, tip-bot2 for David Gow
<[email protected]> wrote:
>
> The following commit has been merged into the timers/core branch of tip:
>
> Commit-ID: e0a1284b293bdf91a68a6d1a0479ad476d0d8ec2
> Gitweb: https://git.kernel.org/tip/e0a1284b293bdf91a68a6d1a0479ad476d0d8ec2
> Author: David Gow <[email protected]>
> AuthorDate: Wed, 21 Feb 2024 17:27:17 +08:00
> Committer: Thomas Gleixner <[email protected]>
> CommitterDate: Wed, 21 Feb 2024 12:00:42 +01:00
>
> time/kunit: Use correct format specifier
>
> '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: 276010551664 ("time: Improve performance of time64_to_tm()")
> Signed-off-by: David Gow <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
>

We're hoping to take this series in via the KUnit tree, so that we can
enable the warning in one place without annoying Linus with lots of
dependencies between PRs.
(See, e.g,. https://lore.kernel.org/linux-kselftest/CAHk-=wgafXXX17eKx9wH_uHg=UgvXkngxGhPcZwhpj7Uz=_0Pw@mail.gmail.com/
)

Would that cause a problem?

Thanks,
-- David

Attachments:

2024-02-23 23:06:14

by Shuah Khan

Subject: Re: [tip: timers/core] time/kunit: Use correct format specifier

On 2/23/24 00:01, David Gow wrote:
> Hi,
>
> On Thu, 22 Feb 2024 at 02:21, tip-bot2 for David Gow
> <[email protected]> wrote:
>>
>> The following commit has been merged into the timers/core branch of tip:
>>
>> Commit-ID: e0a1284b293bdf91a68a6d1a0479ad476d0d8ec2
>> Gitweb: https://git.kernel.org/tip/e0a1284b293bdf91a68a6d1a0479ad476d0d8ec2
>> Author: David Gow <[email protected]>
>> AuthorDate: Wed, 21 Feb 2024 17:27:17 +08:00
>> Committer: Thomas Gleixner <[email protected]>
>> CommitterDate: Wed, 21 Feb 2024 12:00:42 +01:00
>>
>> time/kunit: Use correct format specifier
>>
>> '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: 276010551664 ("time: Improve performance of time64_to_tm()")
>> Signed-off-by: David Gow <[email protected]>
>> Signed-off-by: Thomas Gleixner <[email protected]>
>> Link: https://lore.kernel.org/r/[email protected]
>>
>
> We're hoping to take this series in via the KUnit tree, so that we can
> enable the warning in one place without annoying Linus with lots of
> dependencies between PRs.
> (See, e.g,. https://lore.kernel.org/linux-kselftest/CAHk-=wgafXXX17eKx9wH_uHg=UgvXkngxGhPcZwhpj7Uz=_0Pw@mail.gmail.com/
> )
>
> Would that cause a problem?
>

Linus wants this series to go through kunit tree. I will take care.

I will pick these up for Linux 6.9-rc1 as per Linus's request.

thanks,
-- Shuah

2024-02-27 21:16:59

by Alexandre Belloni

Subject: Re: [PATCH 5/9] rtc: test: Fix invalid format specifier.

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 <[email protected]>

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
>
>

--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2024-02-27 21:33:07

by Shuah Khan

Subject: Re: [PATCH 5/9] rtc: test: Fix invalid format specifier.

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 <[email protected]>
>
> 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

2024-02-27 22:49:11

by Alexandre Belloni

Subject: Re: [PATCH 5/9] rtc: test: Fix invalid format specifier.

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 <[email protected]>
> >
> > 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 <[email protected]>

>
> thanks,
> -- Shuah
>
>

--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2024-02-27 23:30:34

by Shuah Khan

Subject: Re: [PATCH 7/9] drm: tests: Fix invalid printf format specifiers in KUnit tests

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 <[email protected]>
>
> Reviewed-by: Justin Stitt <[email protected]>

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

2024-02-27 23:33:27

by Shuah Khan

Subject: Re: [PATCH 0/9] kunit: Fix printf format specifier issues in KUnit assertions

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 <[email protected]>
> 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