[PATCH v5 0/8] Add printf attribute to kselftest functions

[PATCH v5 0/8] Add printf attribute to kselftest functions

[Maciej Wieczor-Retman]

kselftest.h declares many variadic functions that can print some
formatted message while also executing selftest logic. These
declarations don't have any compiler mechanism to verify if passed
arguments are valid in comparison with format specifiers used in
printf() calls.

Attribute addition can make debugging easier, the code more consistent
and prevent mismatched or missing variables.

The first patch adds __printf() macro and applies it to all functions
in kselftest.h that use printf format specifiers. After compiling all
selftests using:
	make -C tools/testing/selftests
many instances of format specifier mismatching are exposed in the form
of -Wformat warnings.

Fix the mismatched format specifiers caught by __printf() attribute in
multiple tests.

Series is based on kselftests next branch.

Changelog v5:
- Mention in the cover letter what methodology was used to find the
  mismatched format specifiers.
- No functional changes in the patches.

Changelog v4:
- Fix patch 1/8 subject typo.
- Add Reinette's reviewed-by tags.
- Rebase onto new kselftest/next patches.

Changelog v3:
- Changed git signature from Wieczor-Retman Maciej to Maciej
  Wieczor-Retman.
- Added one review tag.
- Rebased onto updated kselftests next branch.

Changelog v2:
- Add review and fixes tags to patches.
- Add two patches with mismatch fixes.
- Fix missed attribute in selftests/kvm. (Andrew)
- Fix previously missed issues in selftests/mm (Ilpo)

[v3] https://lore.kernel.org/all/cover.1695373131.git.maciej.wieczor-retman@intel.com/
[v2] https://lore.kernel.org/all/cover.1693829810.git.maciej.wieczor-retman@intel.com/
[v1] https://lore.kernel.org/all/cover.1693216959.git.maciej.wieczor-retman@intel.com/

Maciej Wieczor-Retman (8):
  selftests: Add printf attribute to kselftest prints
  selftests/cachestat: Fix print_cachestat format
  selftests/openat2: Fix wrong format specifier
  selftests/pidfd: Fix ksft print formats
  selftests/sigaltstack: Fix wrong format specifier
  selftests/kvm: Replace attribute with macro
  selftests/mm: Substitute attribute with a macro
  selftests/resctrl: Fix wrong format specifier

 .../selftests/cachestat/test_cachestat.c       |  2 +-
 tools/testing/selftests/kselftest.h            | 18 ++++++++++--------
 .../testing/selftests/kvm/include/test_util.h  |  8 ++++----
 tools/testing/selftests/mm/mremap_test.c       |  2 +-
 tools/testing/selftests/mm/pkey-helpers.h      |  2 +-
 tools/testing/selftests/openat2/openat2_test.c |  2 +-
 .../selftests/pidfd/pidfd_fdinfo_test.c        |  2 +-
 tools/testing/selftests/pidfd/pidfd_test.c     | 12 ++++++------
 tools/testing/selftests/resctrl/cache.c        |  2 +-
 tools/testing/selftests/sigaltstack/sas.c      |  2 +-
 10 files changed, 27 insertions(+), 25 deletions(-)


base-commit: 2531f374f922e77ba51f24d1aa6fa11c7f4c36b8
-- 
2.42.0

[PATCH v5 1/8] selftests: Add printf attribute to kselftest prints

[Maciej Wieczor-Retman]

Kselftest header defines multiple variadic functions that use printf
along with other logic.

There is no format checking for the variadic functions that use
printing inside kselftest.h. Because of this the compiler won't
be able to catch instances of mismatched printf formats and debugging
tests might be more difficult.

Add the common __printf attribute macro to kselftest.h.

Add __printf attribute to every function using formatted printing with
variadic arguments.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changelog v4:
- Fix typo in patch subject. (Reinette)
- Add Reinette's reviewed-by tag.

 tools/testing/selftests/kselftest.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index af9f1202d423..5696199c16f9 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -78,6 +78,8 @@
 #define KSFT_XPASS 3
 #define KSFT_SKIP  4
 
+#define __printf(a, b)   __attribute__((format(printf, a, b)))
+
 /* counters */
 struct ksft_count {
 	unsigned int ksft_pass;
@@ -144,7 +146,7 @@ static inline void ksft_print_cnts(void)
 		ksft_cnt.ksft_xskip, ksft_cnt.ksft_error);
 }
 
-static inline void ksft_print_msg(const char *msg, ...)
+static inline __printf(1, 2) void ksft_print_msg(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -169,7 +171,7 @@ static inline void ksft_perror(const char *msg)
 #endif
 }
 
-static inline void ksft_test_result_pass(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_pass(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -183,7 +185,7 @@ static inline void ksft_test_result_pass(const char *msg, ...)
 	va_end(args);
 }
 
-static inline void ksft_test_result_fail(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_fail(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -209,7 +211,7 @@ static inline void ksft_test_result_fail(const char *msg, ...)
 		ksft_test_result_fail(fmt, ##__VA_ARGS__);\
 	} while (0)
 
-static inline void ksft_test_result_xfail(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -223,7 +225,7 @@ static inline void ksft_test_result_xfail(const char *msg, ...)
 	va_end(args);
 }
 
-static inline void ksft_test_result_skip(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_skip(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -238,7 +240,7 @@ static inline void ksft_test_result_skip(const char *msg, ...)
 }
 
 /* TODO: how does "error" differ from "fail" or "skip"? */
-static inline void ksft_test_result_error(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -285,7 +287,7 @@ static inline int ksft_exit_fail(void)
 		  ksft_cnt.ksft_xfail +	\
 		  ksft_cnt.ksft_xskip)
 
-static inline int ksft_exit_fail_msg(const char *msg, ...)
+static inline __printf(1, 2) int ksft_exit_fail_msg(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -312,7 +314,7 @@ static inline int ksft_exit_xpass(void)
 	exit(KSFT_XPASS);
 }
 
-static inline int ksft_exit_skip(const char *msg, ...)
+static inline __printf(1, 2) int ksft_exit_skip(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
-- 
2.42.0

[PATCH v5 2/8] selftests/cachestat: Fix print_cachestat format

[Maciej Wieczor-Retman]

The format specifier in printf() call expects long int variables and
received long long int.

Change format specifiers to long long int so they match passed
variables.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Acked-by: Nhat Pham <nphamcs@gmail.com>
---
Changelog v2:
- Added Acked-by tag (Nhat)

 tools/testing/selftests/cachestat/test_cachestat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
index 4804c7dc7b31..b171fd53b004 100644
--- a/tools/testing/selftests/cachestat/test_cachestat.c
+++ b/tools/testing/selftests/cachestat/test_cachestat.c
@@ -27,7 +27,7 @@ static const char * const dev_files[] = {
 void print_cachestat(struct cachestat *cs)
 {
 	ksft_print_msg(
-	"Using cachestat: Cached: %lu, Dirty: %lu, Writeback: %lu, Evicted: %lu, Recently Evicted: %lu\n",
+	"Using cachestat: Cached: %llu, Dirty: %llu, Writeback: %llu, Evicted: %llu, Recently Evicted: %llu\n",
 	cs->nr_cache, cs->nr_dirty, cs->nr_writeback,
 	cs->nr_evicted, cs->nr_recently_evicted);
 }
-- 
2.42.0

[PATCH v5 3/8] selftests/openat2: Fix wrong format specifier

[Maciej Wieczor-Retman]

Ksft_print_msg() inside test_openat2_flags() uses the wrong format
specifier for printing test.how->flags variable.

Change the format specifier to %llX so it matches the printed variable.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
Changelog v2:
- Added Reviewed-by tag (Ilpo)

 tools/testing/selftests/openat2/openat2_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 7fb902099de4..9024754530b2 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -300,7 +300,7 @@ void test_openat2_flags(void)
 
 			ksft_print_msg("openat2 unexpectedly returned ");
 			if (fdpath)
-				ksft_print_msg("%d['%s'] with %X (!= %X)\n",
+				ksft_print_msg("%d['%s'] with %X (!= %llX)\n",
 					       fd, fdpath, fdflags,
 					       test->how.flags);
 			else
-- 
2.42.0

[PATCH v5 4/8] selftests/pidfd: Fix ksft print formats

[Maciej Wieczor-Retman]

Many calls to ksft print functions have format strings that don't match
with other passed arguments. One call expects a string but doesn't
provide any argument after the format string.

Fix format specifiers so they match the passed variables.

Add a missing variable to ksft_test_result_pass() inside
pidfd_fdinfo_test() so it matches other cases in the switch statement.

Fixes: 2def297ec7fb ("pidfd: add tests for NSpid info in fdinfo")

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v2:
- Add fixes tag to patch message.

 tools/testing/selftests/pidfd/pidfd_fdinfo_test.c |  2 +-
 tools/testing/selftests/pidfd/pidfd_test.c        | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
index 4e86f927880c..01cc37bf611c 100644
--- a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
@@ -62,7 +62,7 @@ static void error_report(struct error *err, const char *test_name)
 		break;
 
 	case PIDFD_PASS:
-		ksft_test_result_pass("%s test: Passed\n");
+		ksft_test_result_pass("%s test: Passed\n", test_name);
 		break;
 
 	default:
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index 00a07e7c571c..c081ae91313a 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -381,13 +381,13 @@ static int test_pidfd_send_signal_syscall_support(void)
 
 static void *test_pidfd_poll_exec_thread(void *priv)
 {
-	ksft_print_msg("Child Thread: starting. pid %d tid %d ; and sleeping\n",
+	ksft_print_msg("Child Thread: starting. pid %d tid %ld ; and sleeping\n",
 			getpid(), syscall(SYS_gettid));
 	ksft_print_msg("Child Thread: doing exec of sleep\n");
 
 	execl("/bin/sleep", "sleep", str(CHILD_THREAD_MIN_WAIT), (char *)NULL);
 
-	ksft_print_msg("Child Thread: DONE. pid %d tid %d\n",
+	ksft_print_msg("Child Thread: DONE. pid %d tid %ld\n",
 			getpid(), syscall(SYS_gettid));
 	return NULL;
 }
@@ -427,7 +427,7 @@ static int child_poll_exec_test(void *args)
 {
 	pthread_t t1;
 
-	ksft_print_msg("Child (pidfd): starting. pid %d tid %d\n", getpid(),
+	ksft_print_msg("Child (pidfd): starting. pid %d tid %ld\n", getpid(),
 			syscall(SYS_gettid));
 	pthread_create(&t1, NULL, test_pidfd_poll_exec_thread, NULL);
 	/*
@@ -480,10 +480,10 @@ static void test_pidfd_poll_exec(int use_waitpid)
 
 static void *test_pidfd_poll_leader_exit_thread(void *priv)
 {
-	ksft_print_msg("Child Thread: starting. pid %d tid %d ; and sleeping\n",
+	ksft_print_msg("Child Thread: starting. pid %d tid %ld ; and sleeping\n",
 			getpid(), syscall(SYS_gettid));
 	sleep(CHILD_THREAD_MIN_WAIT);
-	ksft_print_msg("Child Thread: DONE. pid %d tid %d\n", getpid(), syscall(SYS_gettid));
+	ksft_print_msg("Child Thread: DONE. pid %d tid %ld\n", getpid(), syscall(SYS_gettid));
 	return NULL;
 }
 
@@ -492,7 +492,7 @@ static int child_poll_leader_exit_test(void *args)
 {
 	pthread_t t1, t2;
 
-	ksft_print_msg("Child: starting. pid %d tid %d\n", getpid(), syscall(SYS_gettid));
+	ksft_print_msg("Child: starting. pid %d tid %ld\n", getpid(), syscall(SYS_gettid));
 	pthread_create(&t1, NULL, test_pidfd_poll_leader_exit_thread, NULL);
 	pthread_create(&t2, NULL, test_pidfd_poll_leader_exit_thread, NULL);
 
-- 
2.42.0

[PATCH v5 5/8] selftests/sigaltstack: Fix wrong format specifier

[Maciej Wieczor-Retman]

The format specifier inside ksft printing function expects a long
unsigned int but the passed variable is of unsigned int type.

Fix the format specifier so it matches the passed variable.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
Changelog v2:
- Added Reviewed-by tag (Ilpo)

 tools/testing/selftests/sigaltstack/sas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c
index 98d37cb744fb..07227fab1cc9 100644
--- a/tools/testing/selftests/sigaltstack/sas.c
+++ b/tools/testing/selftests/sigaltstack/sas.c
@@ -111,7 +111,7 @@ int main(void)
 
 	/* Make sure more than the required minimum. */
 	stack_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
-	ksft_print_msg("[NOTE]\tthe stack size is %lu\n", stack_size);
+	ksft_print_msg("[NOTE]\tthe stack size is %u\n", stack_size);
 
 	ksft_print_header();
 	ksft_set_plan(3);
-- 
2.42.0

[PATCH v5 6/8] selftests/kvm: Replace attribute with macro

[Maciej Wieczor-Retman]

The __printf() macro is used in many tools in the linux kernel to
validate the format specifiers in functions that use printf. The kvm
selftest uses it without putting it in a macro definition while it
also imports the kselftests.h header.

Use __printf() from kselftests.h instead of the full attribute.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v2:
- Reword patch message.
- Use __printf() on test_assert().

 tools/testing/selftests/kvm/include/test_util.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 7e614adc6cf4..8e5f413a593d 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -33,7 +33,7 @@ static inline int _no_printf(const char *format, ...) { return 0; }
 #define pr_info(...) _no_printf(__VA_ARGS__)
 #endif
 
-void print_skip(const char *fmt, ...) __attribute__((format(printf, 1, 2)));
+void __printf(1, 2) print_skip(const char *fmt, ...);
 #define __TEST_REQUIRE(f, fmt, ...)				\
 do {								\
 	if (!(f))						\
@@ -46,9 +46,9 @@ ssize_t test_write(int fd, const void *buf, size_t count);
 ssize_t test_read(int fd, void *buf, size_t count);
 int test_seq_read(const char *path, char **bufp, size_t *sizep);
 
-void test_assert(bool exp, const char *exp_str,
-		 const char *file, unsigned int line, const char *fmt, ...)
-		__attribute__((format(printf, 5, 6)));
+void __printf(5, 6) test_assert(bool exp, const char *exp_str,
+				const char *file, unsigned int line,
+				const char *fmt, ...);
 
 #define TEST_ASSERT(e, fmt, ...) \
 	test_assert((e), #e, __FILE__, __LINE__, fmt, ##__VA_ARGS__)
-- 
2.42.0

[PATCH v5 7/8] selftests/mm: Substitute attribute with a macro

[Maciej Wieczor-Retman]

The mm selftest uses the printf attribute in its full form. Since the
header file that uses it also includes kselftests.h it can use the macro
defined there.

Use __printf() included with kselftests.h instead of the full attribute.

Fix a wrong format specifier in ksft_print_msg().

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v2:
- Added this patch to the series.

 tools/testing/selftests/mm/mremap_test.c  | 2 +-
 tools/testing/selftests/mm/pkey-helpers.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c
index 5c3773de9f0f..1dbfcf6df255 100644
--- a/tools/testing/selftests/mm/mremap_test.c
+++ b/tools/testing/selftests/mm/mremap_test.c
@@ -338,7 +338,7 @@ static long long remap_region(struct config c, unsigned int threshold_mb,
 		char c = (char) rand();
 
 		if (((char *) dest_addr)[i] != c) {
-			ksft_print_msg("Data after remap doesn't match at offset %d\n",
+			ksft_print_msg("Data after remap doesn't match at offset %llu\n",
 				       i);
 			ksft_print_msg("Expected: %#x\t Got: %#x\n", c & 0xff,
 					((char *) dest_addr)[i] & 0xff);
diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h
index 92f3be3dd8e5..1af3156a9db8 100644
--- a/tools/testing/selftests/mm/pkey-helpers.h
+++ b/tools/testing/selftests/mm/pkey-helpers.h
@@ -34,7 +34,7 @@ extern int test_nr;
 extern int iteration_nr;
 
 #ifdef __GNUC__
-__attribute__((format(printf, 1, 2)))
+__printf(1, 2)
 #endif
 static inline void sigsafe_printf(const char *format, ...)
 {
-- 
2.42.0

[PATCH v5 8/8] selftests/resctrl: Fix wrong format specifier

[Maciej Wieczor-Retman]

A long unsigned int variable is passed to the ksft_print_msg() and the
format specifier used expects a variable of type int.

Change the format specifier to match the passed variable.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changelog v4:
- Added Reinette's reviewed-by tag.

Changelog v3:
- Added Ilpo's reviewed-by tag.

Changelog v2:
- Added this patch to the series.

 tools/testing/selftests/resctrl/cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
index d3cbb829ff6a..a5d082cd2d53 100644
--- a/tools/testing/selftests/resctrl/cache.c
+++ b/tools/testing/selftests/resctrl/cache.c
@@ -294,7 +294,7 @@ int show_cache_info(unsigned long sum_llc_val, int no_of_bits,
 	ret = platform && abs((int)diff_percent) > max_diff_percent &&
 	      (cmt ? (abs(avg_diff) > max_diff) : true);
 
-	ksft_print_msg("%s Check cache miss rate within %d%%\n",
+	ksft_print_msg("%s Check cache miss rate within %lu%%\n",
 		       ret ? "Fail:" : "Pass:", max_diff_percent);
 
 	ksft_print_msg("Percent diff=%d\n", abs((int)diff_percent));
-- 
2.42.0

Re: [PATCH v5 1/8] selftests: Add printf attribute to kselftest prints

[Shuah]

On 10/11/23 02:23, Maciej Wieczor-Retman wrote:
> Kselftest header defines multiple variadic functions that use printf
> along with other logic.
> 
> There is no format checking for the variadic functions that use
> printing inside kselftest.h. Because of this the compiler won't
> be able to catch instances of mismatched printf formats and debugging
> tests might be more difficult.
> 
> Add the common __printf attribute macro to kselftest.h.
> 
> Add __printf attribute to every function using formatted printing with
> variadic arguments.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Changelog v4:
> - Fix typo in patch subject. (Reinette)
> - Add Reinette's reviewed-by tag.
> 

I still need information on how you found these problems. Please
add it to change log for each of these patches.

I am seeing checkpatch warning:

WARNING: Prefer __printf(a, b) over __attribute__((format(printf, a, b)))
#102: FILE: tools/testing/selftests/kselftest.h:81:
+#define __printf(a, b)   __attribute__((format(printf, a, b)))

thanks,
-- Shuah

Re: [PATCH v5 1/8] selftests: Add printf attribute to kselftest prints

[Maciej Wieczór-Retman]

On 2023-10-11 at 13:40:48 -0600, Shuah wrote:
>On 10/11/23 02:23, Maciej Wieczor-Retman wrote:
>> Kselftest header defines multiple variadic functions that use printf
>> along with other logic.
>> 
>> There is no format checking for the variadic functions that use
>> printing inside kselftest.h. Because of this the compiler won't
>> be able to catch instances of mismatched printf formats and debugging
>> tests might be more difficult.
>> 
>> Add the common __printf attribute macro to kselftest.h.
>> 
>> Add __printf attribute to every function using formatted printing with
>> variadic arguments.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
>> ---
>> Changelog v4:
>> - Fix typo in patch subject. (Reinette)
>> - Add Reinette's reviewed-by tag.
>> 
>
>I still need information on how you found these problems. Please
>add it to change log for each of these patches.

Sure, I'll add notes on methodology to patches 2-8. I understand that
this patch (1/8) message doesn't need that addition since the problems
it exposes are in separate patches.

Or would you like me to also note here more specifically what effect it
has in the rest of the series?

>I am seeing checkpatch warning:
>
>WARNING: Prefer __printf(a, b) over __attribute__((format(printf, a, b)))
>#102: FILE: tools/testing/selftests/kselftest.h:81:
>+#define __printf(a, b)   __attribute__((format(printf, a, b)))

Running checkpatch.pl with --show-types shows the
PREFER_DEFINED_ATTRIBUTE_MACRO is raised. From looking at the error
message in the script it looks like a false positive:
	"Prefer $new over __attribute__(($orig_attr$params))\n"

Please correct me if my train of thought is wrong but I think checkpatch
sees __printf() macro defined and it sees it's raw version
"__attribute__((format(printf, a, b)))" which it wants to replace with
the macro. But since the raw version is found in the define line that is
obviously not possible.

>thanks,
>-- Shuah

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH v5 1/8] selftests: Add printf attribute to kselftest prints

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

On Thu, 12 Oct 2023, Maciej Wieczór-Retman wrote:

> On 2023-10-11 at 13:40:48 -0600, Shuah wrote:
> >On 10/11/23 02:23, Maciej Wieczor-Retman wrote:
> >> Kselftest header defines multiple variadic functions that use printf
> >> along with other logic.
> >> 
> >> There is no format checking for the variadic functions that use
> >> printing inside kselftest.h. Because of this the compiler won't
> >> be able to catch instances of mismatched printf formats and debugging
> >> tests might be more difficult.
> >> 
> >> Add the common __printf attribute macro to kselftest.h.
> >> 
> >> Add __printf attribute to every function using formatted printing with
> >> variadic arguments.
> >> 
> >> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> >> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> >> ---
> >> Changelog v4:
> >> - Fix typo in patch subject. (Reinette)
> >> - Add Reinette's reviewed-by tag.
> >> 
> >
> >I still need information on how you found these problems. Please
> >add it to change log for each of these patches.
> 
> Sure, I'll add notes on methodology to patches 2-8. I understand that
> this patch (1/8) message doesn't need that addition since the problems
> it exposes are in separate patches.
> 
> Or would you like me to also note here more specifically what effect it
> has in the rest of the series?
> 
> >I am seeing checkpatch warning:
> >
> >WARNING: Prefer __printf(a, b) over __attribute__((format(printf, a, b)))
> >#102: FILE: tools/testing/selftests/kselftest.h:81:
> >+#define __printf(a, b)   __attribute__((format(printf, a, b)))
> 
> Running checkpatch.pl with --show-types shows the
> PREFER_DEFINED_ATTRIBUTE_MACRO is raised. From looking at the error
> message in the script it looks like a false positive:
> 	"Prefer $new over __attribute__(($orig_attr$params))\n"
> 
> Please correct me if my train of thought is wrong but I think checkpatch
> sees __printf() macro defined and it sees it's raw version
> "__attribute__((format(printf, a, b)))" which it wants to replace with
> the macro. But since the raw version is found in the define line that is
> obviously not possible.

Yes, this is clearly a false positive from checkpatch. Checkpatch's logic 
cannot differentiate the definition from the use of __printf(), it just 
assumes __printf() is there already, which is not true for selftests.

The patch adds the capability to use __printf() elsewhere in the 
selftests code but of course the definition of __printf() itself has to 
use __attribute__().

-- 
 i.

Re: [PATCH v5 1/8] selftests: Add printf attribute to kselftest prints

[Shuah Khan]

On 10/12/23 01:32, Maciej Wieczór-Retman wrote:
> On 2023-10-11 at 13:40:48 -0600, Shuah wrote:
>> On 10/11/23 02:23, Maciej Wieczor-Retman wrote:
>>> Kselftest header defines multiple variadic functions that use printf
>>> along with other logic.
>>>
>>> There is no format checking for the variadic functions that use
>>> printing inside kselftest.h. Because of this the compiler won't
>>> be able to catch instances of mismatched printf formats and debugging
>>> tests might be more difficult.
>>>
>>> Add the common __printf attribute macro to kselftest.h.
>>>
>>> Add __printf attribute to every function using formatted printing with
>>> variadic arguments.
>>>
>>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
>>> ---
>>> Changelog v4:
>>> - Fix typo in patch subject. (Reinette)
>>> - Add Reinette's reviewed-by tag.
>>>
>>
>> I still need information on how you found these problems. Please
>> add it to change log for each of these patches.
> 
> Sure, I'll add notes on methodology to patches 2-8. I understand that
> this patch (1/8) message doesn't need that addition since the problems
> it exposes are in separate patches.
>

Yes please. As mentioned a couple of times, I would like to see how
the problem is found in each patch commit log.

> Or would you like me to also note here more specifically what effect it
> has in the rest of the series?
> 

Yes please.

>> I am seeing checkpatch warning:
>>
>> WARNING: Prefer __printf(a, b) over __attribute__((format(printf, a, b)))
>> #102: FILE: tools/testing/selftests/kselftest.h:81:
>> +#define __printf(a, b)   __attribute__((format(printf, a, b)))
> 
> Running checkpatch.pl with --show-types shows the
> PREFER_DEFINED_ATTRIBUTE_MACRO is raised. From looking at the error
> message in the script it looks like a false positive:
> 	"Prefer $new over __attribute__(($orig_attr$params))\n"
> 
> Please correct me if my train of thought is wrong but I think checkpatch
> sees __printf() macro defined and it sees it's raw version
> "__attribute__((format(printf, a, b)))" which it wants to replace with
> the macro. But since the raw version is found in the define line that is
> obviously not possible.
> 

This is fine.

thanks,
-- Shuah

Re: [PATCH v5 1/8] selftests: Add printf attribute to kselftest prints

[Maciej Wieczór-Retman]

On 2023-10-12 at 08:30:37 -0600, Shuah Khan wrote:
>On 10/12/23 01:32, Maciej Wieczór-Retman wrote:
>> On 2023-10-11 at 13:40:48 -0600, Shuah wrote:
>> > On 10/11/23 02:23, Maciej Wieczor-Retman wrote:
>> > > Kselftest header defines multiple variadic functions that use printf
>> > > along with other logic.
>> > > 
>> > > There is no format checking for the variadic functions that use
>> > > printing inside kselftest.h. Because of this the compiler won't
>> > > be able to catch instances of mismatched printf formats and debugging
>> > > tests might be more difficult.
>> > > 
>> > > Add the common __printf attribute macro to kselftest.h.
>> > > 
>> > > Add __printf attribute to every function using formatted printing with
>> > > variadic arguments.
>> > > 
>> > > Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> > > Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
>> > > ---
>> > > Changelog v4:
>> > > - Fix typo in patch subject. (Reinette)
>> > > - Add Reinette's reviewed-by tag.
>> > > 
>> > 
>> > I still need information on how you found these problems. Please
>> > add it to change log for each of these patches.
>> 
>> Sure, I'll add notes on methodology to patches 2-8. I understand that
>> this patch (1/8) message doesn't need that addition since the problems
>> it exposes are in separate patches.
>> 
>
>Yes please. As mentioned a couple of times, I would like to see how
>the problem is found in each patch commit log.
>
>> Or would you like me to also note here more specifically what effect it
>> has in the rest of the series?
>> 
>
>Yes please.

Thanks for confirming. I went through all the patches, made corrections
and sent v6 of the series.

-- 
Kind regards
Maciej Wieczór-Retman