This series introduces a new test harness for nolibc. It is similar to kselftest-harness and google test. More information in patch 1.
This is an RFC to gather feedback, especially if it can be integrated with the original kselftest-harness somehow.
Note:
When run under qemu-loongarch64 8.1.2 the test "mmap_munmap_good" fails. This is a bug in qemu-user and already fixed there on master.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- Thomas Weißschuh (3): selftests/nolibc: add custom test harness selftests/nolibc: migrate startup tests to new harness selftests/nolibc: migrate vfprintf tests to new harness
tools/testing/selftests/nolibc/nolibc-harness.h | 269 ++++++++++++++++++++++++ tools/testing/selftests/nolibc/nolibc-test.c | 180 ++++++++-------- 2 files changed, 353 insertions(+), 96 deletions(-) --- base-commit: d38d5366cb1c51f687b4720277adee97074b22e9 change-id: 20231105-nolibc-harness-28c59029d7a5
Best regards,
The harness provides a framework to write unit tests for nolibc itself and kernel selftests using nolibc.
Advantages over the current harness: * Makes it possible to emit KTAP for integration into kselftests. * Provides familiarity with the kselftest harness and google test. * It is nicer to write testcases that are longer than one line.
Design goals: * Compatibility with nolibc. kselftest-harness requires setjmp() and signals which are not supported on nolibc. * Provide the same output as the existing unittests. * Provide a way to emit KTAP.
Notes: * This differs from kselftest-harness in its support for test suites, the same as google test.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-harness.h | 269 ++++++++++++++++++++++++ 1 file changed, 269 insertions(+)
diff --git a/tools/testing/selftests/nolibc/nolibc-harness.h b/tools/testing/selftests/nolibc/nolibc-harness.h new file mode 100644 index 000000000000..4c82581fab86 --- /dev/null +++ b/tools/testing/selftests/nolibc/nolibc-harness.h @@ -0,0 +1,269 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Test harness for NOLIBC + * + * Copyright (C) 2023 Thomas Weißschuh linux@weissschuh.net + * Copyright (c) 2012 The Chromium OS Authors. All rights reserved. + */ + +#include <stdint.h> +#include <stdio.h> +#include <string.h> + +static void putcharn(char c, size_t n) +{ + char buf[64]; + + memset(buf, c, n); + buf[n] = '\0'; + fputs(buf, stdout); +} + +struct __test_setup; + +struct __test_execution { + int finished, failed, skipped, llen; +}; + +struct __test_metadata { + const char *name; + const char *suite; + unsigned int suite_count; + void (*func)(struct __test_metadata *metadata); + struct __test_metadata *next; + struct __test_setup *setup; + struct __test_execution exe; +}; + +struct __test_setup { + struct __test_metadata *start, *end; +} __test_setup __attribute__((weak)); + +static void __add_metadata(struct __test_metadata *metadata) +{ + struct __test_metadata *m; + + if (!__test_setup.start) + __test_setup.start = metadata; + + if (!__test_setup.end) { + __test_setup.end = metadata; + } else { + __test_setup.end->next = metadata; + __test_setup.end = metadata; + } + + metadata->setup = &__test_setup; + + for (m = __test_setup.start; m; m = m->next) { + if (strcmp(metadata->suite, m->suite) == 0) + metadata->suite_count++; + } +} + +#define TEST(_suite, _name) \ + static void _testfunc_ ## _suite ## _name(struct __test_metadata *); \ + static struct __test_metadata _metadata_ ## _suite ## _name = { \ + .name = #_name, \ + .suite = #_suite, \ + .func = _testfunc_ ## _suite ## _name, \ + }; \ + __attribute__((constructor)) \ + static void _register_testfunc_ ## _suite ## _name(void) \ + { __add_metadata(&_metadata_ ## _suite ## _name); } \ + static void _testfunc_ ## _suite ## _name( \ + struct __test_metadata *_metadata __attribute__((unused))) + +#define SKIP(statement) __extension__ ({ \ + _metadata->exe.skipped = 1; \ + statement; \ +}) + +#define FAIL(statement) __extension__ ({ \ + _metadata->exe.failed = 1; \ + statement; \ +}) + +static void __run_test(struct __test_metadata *metadata) +{ + metadata->func(metadata); + metadata->exe.finished = 1; +} + +static __attribute__((unused)) +unsigned int count_test_suites(void) +{ + struct __test_metadata *m; + unsigned int r = 0; + + for (m = __test_setup.start; m && m->suite_count; m = m->next) + r++; + + return r; +} + +static __attribute__((unused)) +int count_tests(void) +{ + struct __test_metadata *m; + unsigned int r = 0; + + for (m = __test_setup.start; m; m = m->next) + r++; + + return r; +} + +static unsigned int count_suite_tests(const char *suite) +{ + struct __test_metadata *m; + unsigned int c = 0; + + for (m = __test_setup.start; m && m->suite_count; m = m->next) + if (strcmp(m->suite, suite) == 0) + c = m->suite_count; + + return c; +} + +static __attribute__((unused)) +void dump_test_plan(void) +{ + struct __test_metadata *m; + const char *suite = ""; + + printf("PLAN:\n"); + for (m = __test_setup.start; m; m = m->next) { + if (strcmp(suite, m->suite)) { + suite = m->suite; + printf(" Suite %s (%d):\n", suite, count_suite_tests(suite)); + } + printf(" %10s:%s %d\n", m->suite, m->name, m->suite_count - 1); + } +} + +static unsigned int run_test_suite(const char *suite, int min, int max) +{ + struct __test_metadata *m; + const char *status; + unsigned int errors = 0; + int printed; + + for (m = __test_setup.start; m; m = m->next) { + int testnum = m->suite_count - 1; + + if (strcmp(suite, m->suite) == 0 && testnum >= min && testnum <= max) { + printed = printf("%d %s", testnum, m->name); + __run_test(m); + printed += m->exe.llen; + if (printed < 64) + putcharn(' ', 64 - printed); + + if (m->exe.failed) + status = " [FAIL]"; + else if (m->exe.skipped) + status = "[SKIPPED]"; + else + status = " [OK]"; + + printf("%s\n", status); + if (m->exe.failed) + errors++; + } + } + return errors; +}; + +static __attribute__((unused)) +void reset_tests(void) +{ + struct __test_metadata *m; + + for (m = __test_setup.start; m; m = m->next) + memset(&m->exe, 0, sizeof(m->exe)); +} + +static __attribute__((unused)) +unsigned int run_all_tests(void) +{ + struct __test_metadata *m; + unsigned int suite_errors, errors = 0; + + for (m = __test_setup.start; m; m = m->next) { + if (!m->exe.finished) { + printf("Running test '%s'\n", m->suite); + suite_errors = run_test_suite(m->suite, 0, INT_MAX); + printf("Errors during this test: %d\n\n", suite_errors); + errors += suite_errors; + } + } + + printf("Total number of errors: %d\n", errors); + return errors; +} + +#define ASSERT_EQ(expected, seen) \ + __ASSERT(expected, #expected, seen, #seen, ==) +#define ASSERT_NE(expected, seen) \ + __ASSERT(expected, #expected, seen, #seen, !=) +#define ASSERT_LT(expected, seen) \ + __ASSERT(expected, #expected, seen, #seen, <) +#define ASSERT_LE(expected, seen) \ + __ASSERT(expected, #expected, seen, #seen, <=) +#define ASSERT_GT(expected, seen) \ + __ASSERT(expected, #expected, seen, #seen, >) +#define ASSERT_GE(expected, seen) \ + __ASSERT(expected, #expected, seen, #seen, >=) +#define ASSERT_NULL(seen) \ + __ASSERT(NULL, "NULL", seen, #seen, ==) +#define ASSERT_TRUE(seen) \ + __ASSERT(0, "0", seen, #seen, !=) +#define ASSERT_FALSE(seen) \ + __ASSERT(0, "0", seen, #seen, ==) + +#define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1)) +#define is_pointer_type(var) (__builtin_classify_type(var) == 5) + +#define __ASSERT(_expected, _expected_str, _seen, _seen_str, _t) __extension__ ({ \ + /* Avoid multiple evaluation of the cases */ \ + __typeof__(_expected) __exp = (_expected); \ + __typeof__(_seen) __seen = (_seen); \ + int __ok = __exp _t __seen; \ + if (!__ok) \ + _metadata->exe.failed = 1; \ + if (is_pointer_type(__exp)) { \ + void * __expected_print = (void *)(uintptr_t)__exp; \ + _metadata->exe.llen = printf(" = <%p> ", __expected_print); \ + } else if (is_signed_type(__exp)) { \ + long long __expected_print = (intptr_t)__exp; \ + _metadata->exe.llen = printf(" = %lld ", __expected_print); \ + } else { \ + unsigned long long __expected_print = (uintptr_t)__exp; \ + _metadata->exe.llen = printf(" = %llu ", __expected_print); \ + } \ + __ok; \ +}) + +#define ASSERT_STREQ(expected, seen) \ + __ASSERT_STR(expected, seen, ==) +#define ASSERT_STRNE(expected, seen) \ + __ASSERT_STR(expected, seen, !=) + +#define __ASSERT_STR(_expected, _seen, _t) __extension__ ({ \ + const char *__exp = (_expected); \ + const char *__seen = (_seen); \ + int __ok = __seen && __exp && strcmp(__exp, __seen) _t 0; \ + if (!__ok) \ + _metadata->exe.failed = 1; \ + _metadata->exe.llen = printf(" = <%s> ", __exp ? __exp : ""); \ + __ok; \ +}) + +#define ASSERT_STRNZ(seen) __extension__ ({ \ + const char *__seen = (seen); \ + int __ok = !!__seen; \ + if (!__ok) \ + _metadata->exe.failed = 1; \ + _metadata->exe.llen = printf(" = <%s> ", __seen); \ + __ok; \ +})
Hi Thomas,
On Wed, Nov 15, 2023 at 10:08:19PM +0100, Thomas Weißschuh wrote:
The harness provides a framework to write unit tests for nolibc itself and kernel selftests using nolibc.
Advantages over the current harness:
- Makes it possible to emit KTAP for integration into kselftests.
- Provides familiarity with the kselftest harness and google test.
- It is nicer to write testcases that are longer than one line.
Design goals:
- Compatibility with nolibc. kselftest-harness requires setjmp() and signals which are not supported on nolibc.
- Provide the same output as the existing unittests.
- Provide a way to emit KTAP.
Notes:
- This differs from kselftest-harness in its support for test suites, the same as google test.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
Nice intro to present the benefits, but you forgot to explain what the patch itself does among these points, the decisions you took, tradeoffs if any etc. All of these are particularly important so as to figure what to expect from the patch itself, because, tob be honest, for me it's a bit difficult to estimate the suitability of the code for a given purpose, thus for now I'll mostly focus on general code.
A few comments below:
+static void putcharn(char c, size_t n) +{
- char buf[64];
- memset(buf, c, n);
- buf[n] = '\0';
- fputs(buf, stdout);
+}
You should really check that n < 64 here, not only because it's test code that will trigger about any possible bug around, but also because you want others to easily contribute and not get trapped by calling this with a larger value without figuring it will do whatever. And that way you can remove the tests from the callers which don't need to hard-code this limit.
+#define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1)) +#define is_pointer_type(var) (__builtin_classify_type(var) == 5)
The hard-coded "5" above should either be replaced with pointer_type_class (if available here) or left as-is with a comment at the end of the line saying e.g. "// pointer_type_class" so that the value can be looked up more easily if needed.
Willy
On 2023-11-16 08:16:22+0100, Willy Tarreau wrote:
Hi Thomas,
On Wed, Nov 15, 2023 at 10:08:19PM +0100, Thomas Weißschuh wrote:
The harness provides a framework to write unit tests for nolibc itself and kernel selftests using nolibc.
Advantages over the current harness:
- Makes it possible to emit KTAP for integration into kselftests.
- Provides familiarity with the kselftest harness and google test.
- It is nicer to write testcases that are longer than one line.
Design goals:
- Compatibility with nolibc. kselftest-harness requires setjmp() and signals which are not supported on nolibc.
- Provide the same output as the existing unittests.
- Provide a way to emit KTAP.
Notes:
- This differs from kselftest-harness in its support for test suites, the same as google test.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
Nice intro to present the benefits, but you forgot to explain what the patch itself does among these points, the decisions you took, tradeoffs if any etc. All of these are particularly important so as to figure what to expect from the patch itself, because, tob be honest, for me it's a bit difficult to estimate the suitability of the code for a given purpose, thus for now I'll mostly focus on general code.
Good points. I'll expand more in v2 after we are through this round.
A few comments below:
+static void putcharn(char c, size_t n) +{
- char buf[64];
- memset(buf, c, n);
- buf[n] = '\0';
- fputs(buf, stdout);
+}
You should really check that n < 64 here, not only because it's test code that will trigger about any possible bug around, but also because you want others to easily contribute and not get trapped by calling this with a larger value without figuring it will do whatever. And that way you can remove the tests from the callers which don't need to hard-code this limit.
Ack.
+#define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1)) +#define is_pointer_type(var) (__builtin_classify_type(var) == 5)
The hard-coded "5" above should either be replaced with pointer_type_class (if available here) or left as-is with a comment at the end of the line saying e.g. "// pointer_type_class" so that the value can be looked up more easily if needed.
Ack.
Migrate part of nolibc-test.c to the new test harness.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 106 ++++++++++++++------------- 1 file changed, 56 insertions(+), 50 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index e173014f6b66..6c1b42b58e3e 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -42,6 +42,7 @@ #endif
#include "nolibc-test-linkage.h" +#include "nolibc-harness.h"
/* for the type of int_fast16_t and int_fast32_t, musl differs from glibc and nolibc */ #define SINT_MAX_OF_TYPE(type) (((type)1 << (sizeof(type) * 8 - 2)) - (type)1 + ((type)1 << (sizeof(type) * 8 - 2))) @@ -130,15 +131,6 @@ static const char *errorname(int err) } }
-static void putcharn(char c, size_t n) -{ - char buf[64]; - - memset(buf, c, n); - buf[n] = '\0'; - fputs(buf, stdout); -} - enum RESULT { OK, FAIL, @@ -599,6 +591,19 @@ int expect_strne(const char *expr, int llen, const char *cmp) #define CASE_TEST(name) \ case __LINE__: llen += printf("%d %s", test, #name);
+#if defined(NOLIBC) + +#define ASSUME_NOLIBC(stmt) + +#else /* defined(NOLIBC) */ + +/* differ from nolibc, both glibc and musl have no global _auxv */ +unsigned long *_auxv = (void *)-1; +#define ASSUME_NOLIBC(stmt) SKIP(stmt) + +#endif /* defined(NOLIBC) */ + + /* constructors validate that they are executed in definition order */ __attribute__((constructor)) static void constructor1(void) @@ -612,53 +617,54 @@ static void constructor2(void) constructor_test_value *= 2; }
-int run_startup(int min, int max) +static const void *pbrk(void) { - int test; - int ret = 0; - /* kernel at least passes HOME and TERM, shell passes more */ - int env_total = 2; - /* checking NULL for argv/argv0, environ and _auxv is not enough, let's compare with sbrk(0) or &end */ extern char end; - char *brk = sbrk(0) != (void *)-1 ? sbrk(0) : &end; - /* differ from nolibc, both glibc and musl have no global _auxv */ - const unsigned long *test_auxv = (void *)-1; -#ifdef NOLIBC - test_auxv = _auxv; -#endif + static char *brk;
- for (test = min; test >= 0 && test <= max; test++) { - int llen = 0; /* line length */ + if (brk) + return brk;
- /* avoid leaving empty lines below, this will insert holes into - * test numbers. - */ - switch (test + __LINE__ + 1) { - CASE_TEST(argc); EXPECT_GE(1, test_argc, 1); break; - CASE_TEST(argv_addr); EXPECT_PTRGT(1, test_argv, brk); break; - CASE_TEST(argv_environ); EXPECT_PTRLT(1, test_argv, environ); break; - CASE_TEST(argv_total); EXPECT_EQ(1, environ - test_argv - 1, test_argc ?: 1); break; - CASE_TEST(argv0_addr); EXPECT_PTRGT(1, argv0, brk); break; - CASE_TEST(argv0_str); EXPECT_STRNZ(1, argv0 > brk ? argv0 : NULL); break; - CASE_TEST(argv0_len); EXPECT_GE(1, argv0 > brk ? strlen(argv0) : 0, 1); break; - CASE_TEST(environ_addr); EXPECT_PTRGT(1, environ, brk); break; - CASE_TEST(environ_envp); EXPECT_PTREQ(1, environ, test_envp); break; - CASE_TEST(environ_auxv); EXPECT_PTRLT(test_auxv != (void *)-1, environ, test_auxv); break; - CASE_TEST(environ_total); EXPECT_GE(test_auxv != (void *)-1, (void *)test_auxv - (void *)environ - 1, env_total); break; - CASE_TEST(environ_HOME); EXPECT_PTRNZ(1, getenv("HOME")); break; - CASE_TEST(auxv_addr); EXPECT_PTRGT(test_auxv != (void *)-1, test_auxv, brk); break; - CASE_TEST(auxv_AT_UID); EXPECT_EQ(1, getauxval(AT_UID), getuid()); break; - CASE_TEST(constructor); EXPECT_EQ(1, constructor_test_value, 2); break; - CASE_TEST(linkage_errno); EXPECT_PTREQ(1, linkage_test_errno_addr(), &errno); break; - CASE_TEST(linkage_constr); EXPECT_EQ(1, linkage_test_constructor_test_value, 6); break; - case __LINE__: - return ret; /* must be last */ - /* note: do not set any defaults so as to permit holes above */ - } - } - return ret; + brk = sbrk(0); + + if (brk == (void *)-1) + brk = &end; + + return brk; }
+TEST(startup, argc) { ASSERT_GE(test_argc, 1); } +TEST(startup, argv_addr) { ASSERT_GT((void *)test_argv, pbrk()); } +TEST(startup, argv_environ) { ASSERT_LT(test_argv, environ); } +TEST(startup, argv_total) { ASSERT_EQ(environ - test_argv - 1, test_argc ?: 1); } +TEST(startup, argv0_addr) { ASSERT_GT((void *)argv0, pbrk()); } +TEST(startup, argv0_str) { ASSERT_STRNZ((void *)argv0 > pbrk() ? argv0 : NULL); } +TEST(startup, argv0_len) { ASSERT_GE((void *)argv0 > pbrk() ? strlen(argv0) : 0U, 1U); } +TEST(startup, environ_addr) { ASSERT_GT((void *)environ, pbrk()); } +TEST(startup, environ_envp) { ASSERT_EQ(environ, test_envp); } +TEST(startup, environ_auxv) { + ASSUME_NOLIBC(return); + ASSERT_LT((void *)environ, (void *)_auxv); +} +TEST(startup, environ_total) { + ASSUME_NOLIBC(return); + /* kernel at least passes HOME and TERM, shell passes more */ + ASSERT_GE((void *)_auxv - (void *)environ - 1, 2); +} +TEST(startup, environ_HOME) { ASSERT_NE(getenv("HOME"), NULL); } +TEST(startup, auxv_addr) { + ASSUME_NOLIBC(return); + ASSERT_GT((void *)_auxv, pbrk()); +} +TEST(startup, auxv_AT_UID) { ASSERT_EQ(getauxval(AT_UID), getuid()); } +TEST(startup, constructor) { ASSERT_EQ(constructor_test_value, 2); } +TEST(startup, linkage_errno) { ASSERT_EQ(linkage_test_errno_addr(), &errno); } +TEST(startup, linkage_constr) { ASSERT_EQ(linkage_test_constructor_test_value, 6); } + +int run_startup(int min, int max) +{ + return run_test_suite("startup", min, max); +}
/* used by some syscall tests below */ int test_getdents64(const char *dir)
On Wed, Nov 15, 2023 at 10:08:20PM +0100, Thomas Weißschuh wrote:
Migrate part of nolibc-test.c to the new test harness.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
A few points, mostly questions and food for thoughts.
-static void putcharn(char c, size_t n) -{
- char buf[64];
- memset(buf, c, n);
- buf[n] = '\0';
- fputs(buf, stdout);
-}
Ah now I see how the other one came from :-) My comment about the size check still stands anyway, especially when placed in an include file.
+#if defined(NOLIBC)
+#define ASSUME_NOLIBC(stmt)
+#else /* defined(NOLIBC) */
+/* differ from nolibc, both glibc and musl have no global _auxv */ +unsigned long *_auxv = (void *)-1; +#define ASSUME_NOLIBC(stmt) SKIP(stmt)
+#endif /* defined(NOLIBC) */
I've seen below how it's used and don't find this very clear. In general, passing a statement as an argument to a macro, especially control statements such as "return" is a bit difficult to grasp. If the macro is only used for this, maybe it should integrate the return statement and be called something like "RETURN_UNLESS_NOLIBC()" which is quite explicit this time. If you really need to keep the statement adjustable, then most likely that calling the macro "UNLESS_NOLIBC()" would help, because I understand more naturally that the following will perform a return if we're not on nolibc:
UNLESS_NOLIBC(return);
than:
ASSUME_NOLIBC(return);
- for (test = min; test >= 0 && test <= max; test++) {
int llen = 0; /* line length */
- if (brk)
return brk;
/* avoid leaving empty lines below, this will insert holes into* test numbers.*/switch (test + __LINE__ + 1) {CASE_TEST(argc); EXPECT_GE(1, test_argc, 1); break;CASE_TEST(argv_addr); EXPECT_PTRGT(1, test_argv, brk); break;CASE_TEST(argv_environ); EXPECT_PTRLT(1, test_argv, environ); break;CASE_TEST(argv_total); EXPECT_EQ(1, environ - test_argv - 1, test_argc ?: 1); break;CASE_TEST(argv0_addr); EXPECT_PTRGT(1, argv0, brk); break;CASE_TEST(argv0_str); EXPECT_STRNZ(1, argv0 > brk ? argv0 : NULL); break;CASE_TEST(argv0_len); EXPECT_GE(1, argv0 > brk ? strlen(argv0) : 0, 1); break;CASE_TEST(environ_addr); EXPECT_PTRGT(1, environ, brk); break;CASE_TEST(environ_envp); EXPECT_PTREQ(1, environ, test_envp); break;CASE_TEST(environ_auxv); EXPECT_PTRLT(test_auxv != (void *)-1, environ, test_auxv); break;CASE_TEST(environ_total); EXPECT_GE(test_auxv != (void *)-1, (void *)test_auxv - (void *)environ - 1, env_total); break;CASE_TEST(environ_HOME); EXPECT_PTRNZ(1, getenv("HOME")); break;CASE_TEST(auxv_addr); EXPECT_PTRGT(test_auxv != (void *)-1, test_auxv, brk); break;CASE_TEST(auxv_AT_UID); EXPECT_EQ(1, getauxval(AT_UID), getuid()); break;CASE_TEST(constructor); EXPECT_EQ(1, constructor_test_value, 2); break;CASE_TEST(linkage_errno); EXPECT_PTREQ(1, linkage_test_errno_addr(), &errno); break;CASE_TEST(linkage_constr); EXPECT_EQ(1, linkage_test_constructor_test_value, 6); break;case __LINE__:return ret; /* must be last *//* note: do not set any defaults so as to permit holes above */}- }
- return ret;
- brk = sbrk(0);
- if (brk == (void *)-1)
brk = &end;- return brk;
} +TEST(startup, argc) { ASSERT_GE(test_argc, 1); } +TEST(startup, argv_addr) { ASSERT_GT((void *)test_argv, pbrk()); } +TEST(startup, argv_environ) { ASSERT_LT(test_argv, environ); } +TEST(startup, argv_total) { ASSERT_EQ(environ - test_argv - 1, test_argc ?: 1); } +TEST(startup, argv0_addr) { ASSERT_GT((void *)argv0, pbrk()); } +TEST(startup, argv0_str) { ASSERT_STRNZ((void *)argv0 > pbrk() ? argv0 : NULL); } +TEST(startup, argv0_len) { ASSERT_GE((void *)argv0 > pbrk() ? strlen(argv0) : 0U, 1U); } +TEST(startup, environ_addr) { ASSERT_GT((void *)environ, pbrk()); } +TEST(startup, environ_envp) { ASSERT_EQ(environ, test_envp); } +TEST(startup, environ_auxv) {
- ASSUME_NOLIBC(return);
- ASSERT_LT((void *)environ, (void *)_auxv);
+} +TEST(startup, environ_total) {
- ASSUME_NOLIBC(return);
- /* kernel at least passes HOME and TERM, shell passes more */
- ASSERT_GE((void *)_auxv - (void *)environ - 1, 2);
+} +TEST(startup, environ_HOME) { ASSERT_NE(getenv("HOME"), NULL); } +TEST(startup, auxv_addr) {
- ASSUME_NOLIBC(return);
- ASSERT_GT((void *)_auxv, pbrk());
+} +TEST(startup, auxv_AT_UID) { ASSERT_EQ(getauxval(AT_UID), getuid()); } +TEST(startup, constructor) { ASSERT_EQ(constructor_test_value, 2); } +TEST(startup, linkage_errno) { ASSERT_EQ(linkage_test_errno_addr(), &errno); } +TEST(startup, linkage_constr) { ASSERT_EQ(linkage_test_constructor_test_value, 6); }
I do appreciate the much lower indent level that still manages to enumerate tests easily. But given that test suites are grouped, shouldn't we go a bit further and state that TEST() operates on the suite defined by the TEST_SUITE macro that must be defined before it ? This way you would have:
#define TEST_SUITE startup TEST(argc) { ASSERT_GE(test_argc, 1); } TEST(argv_addr) { ASSERT_GT((void *)test_argv, pbrk()); } ... #undef TEST_SUITE
One thing that was not immediately obvious to me upon first read was if TEST() defines or executes a test (i.e. "test" is both a noun and a verb). Of course, spending 10 more seconds on the patch makes it obvious it's a definition, but maybe following the same logic we have with run_test_suite(), we should place the verb in front, for example "DEF_TEST()" which then makes it quite unambiguous. Any opinion ?
Willy
On 2023-11-16 08:33:27+0100, Willy Tarreau wrote:
On Wed, Nov 15, 2023 at 10:08:20PM +0100, Thomas Weißschuh wrote:
Migrate part of nolibc-test.c to the new test harness.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
A few points, mostly questions and food for thoughts.
-static void putcharn(char c, size_t n) -{
- char buf[64];
- memset(buf, c, n);
- buf[n] = '\0';
- fputs(buf, stdout);
-}
Ah now I see how the other one came from :-) My comment about the size check still stands anyway, especially when placed in an include file.
+#if defined(NOLIBC)
+#define ASSUME_NOLIBC(stmt)
+#else /* defined(NOLIBC) */
+/* differ from nolibc, both glibc and musl have no global _auxv */ +unsigned long *_auxv = (void *)-1; +#define ASSUME_NOLIBC(stmt) SKIP(stmt)
+#endif /* defined(NOLIBC) */
I've seen below how it's used and don't find this very clear. In general, passing a statement as an argument to a macro, especially control statements such as "return" is a bit difficult to grasp. If the macro is only used for this, maybe it should integrate the return statement and be called something like "RETURN_UNLESS_NOLIBC()" which is quite explicit this time. If you really need to keep the statement adjustable, then most likely that calling the macro "UNLESS_NOLIBC()" would help, because I understand more naturally that the following will perform a return if we're not on nolibc:
UNLESS_NOLIBC(return);than:
ASSUME_NOLIBC(return);
The statement arguments is modelled after SKIP() from kselftest_harness.h.
But the wrapper you proposed is indeed much better, I'll switch to that.
- for (test = min; test >= 0 && test <= max; test++) {
int llen = 0; /* line length */
- if (brk)
return brk;
/* avoid leaving empty lines below, this will insert holes into* test numbers.*/switch (test + __LINE__ + 1) {CASE_TEST(argc); EXPECT_GE(1, test_argc, 1); break;CASE_TEST(argv_addr); EXPECT_PTRGT(1, test_argv, brk); break;CASE_TEST(argv_environ); EXPECT_PTRLT(1, test_argv, environ); break;CASE_TEST(argv_total); EXPECT_EQ(1, environ - test_argv - 1, test_argc ?: 1); break;CASE_TEST(argv0_addr); EXPECT_PTRGT(1, argv0, brk); break;CASE_TEST(argv0_str); EXPECT_STRNZ(1, argv0 > brk ? argv0 : NULL); break;CASE_TEST(argv0_len); EXPECT_GE(1, argv0 > brk ? strlen(argv0) : 0, 1); break;CASE_TEST(environ_addr); EXPECT_PTRGT(1, environ, brk); break;CASE_TEST(environ_envp); EXPECT_PTREQ(1, environ, test_envp); break;CASE_TEST(environ_auxv); EXPECT_PTRLT(test_auxv != (void *)-1, environ, test_auxv); break;CASE_TEST(environ_total); EXPECT_GE(test_auxv != (void *)-1, (void *)test_auxv - (void *)environ - 1, env_total); break;CASE_TEST(environ_HOME); EXPECT_PTRNZ(1, getenv("HOME")); break;CASE_TEST(auxv_addr); EXPECT_PTRGT(test_auxv != (void *)-1, test_auxv, brk); break;CASE_TEST(auxv_AT_UID); EXPECT_EQ(1, getauxval(AT_UID), getuid()); break;CASE_TEST(constructor); EXPECT_EQ(1, constructor_test_value, 2); break;CASE_TEST(linkage_errno); EXPECT_PTREQ(1, linkage_test_errno_addr(), &errno); break;CASE_TEST(linkage_constr); EXPECT_EQ(1, linkage_test_constructor_test_value, 6); break;case __LINE__:return ret; /* must be last *//* note: do not set any defaults so as to permit holes above */}- }
- return ret;
- brk = sbrk(0);
- if (brk == (void *)-1)
brk = &end;- return brk;
} +TEST(startup, argc) { ASSERT_GE(test_argc, 1); } +TEST(startup, argv_addr) { ASSERT_GT((void *)test_argv, pbrk()); } +TEST(startup, argv_environ) { ASSERT_LT(test_argv, environ); } +TEST(startup, argv_total) { ASSERT_EQ(environ - test_argv - 1, test_argc ?: 1); } +TEST(startup, argv0_addr) { ASSERT_GT((void *)argv0, pbrk()); } +TEST(startup, argv0_str) { ASSERT_STRNZ((void *)argv0 > pbrk() ? argv0 : NULL); } +TEST(startup, argv0_len) { ASSERT_GE((void *)argv0 > pbrk() ? strlen(argv0) : 0U, 1U); } +TEST(startup, environ_addr) { ASSERT_GT((void *)environ, pbrk()); } +TEST(startup, environ_envp) { ASSERT_EQ(environ, test_envp); } +TEST(startup, environ_auxv) {
- ASSUME_NOLIBC(return);
- ASSERT_LT((void *)environ, (void *)_auxv);
+} +TEST(startup, environ_total) {
- ASSUME_NOLIBC(return);
- /* kernel at least passes HOME and TERM, shell passes more */
- ASSERT_GE((void *)_auxv - (void *)environ - 1, 2);
+} +TEST(startup, environ_HOME) { ASSERT_NE(getenv("HOME"), NULL); } +TEST(startup, auxv_addr) {
- ASSUME_NOLIBC(return);
- ASSERT_GT((void *)_auxv, pbrk());
+} +TEST(startup, auxv_AT_UID) { ASSERT_EQ(getauxval(AT_UID), getuid()); } +TEST(startup, constructor) { ASSERT_EQ(constructor_test_value, 2); } +TEST(startup, linkage_errno) { ASSERT_EQ(linkage_test_errno_addr(), &errno); } +TEST(startup, linkage_constr) { ASSERT_EQ(linkage_test_constructor_test_value, 6); }
I do appreciate the much lower indent level that still manages to enumerate tests easily. But given that test suites are grouped, shouldn't we go a bit further and state that TEST() operates on the suite defined by the TEST_SUITE macro that must be defined before it ? This way you would have:
#define TEST_SUITE startup TEST(argc) { ASSERT_GE(test_argc, 1); } TEST(argv_addr) { ASSERT_GT((void *)test_argv, pbrk()); } ... #undef TEST_SUITE
One thing that was not immediately obvious to me upon first read was if TEST() defines or executes a test (i.e. "test" is both a noun and a verb). Of course, spending 10 more seconds on the patch makes it obvious it's a definition, but maybe following the same logic we have with run_test_suite(), we should place the verb in front, for example "DEF_TEST()" which then makes it quite unambiguous. Any opinion ?
The TEST() macro is modelled after kselftest_harness (which only takes one argument, as it doesn't support suites) and google test which works the same as the new TEST().
So I would prefer to keep the name.
As for specifying the suite via a macro: I like that it saves even more indentation but at the same time it feels a bit too implicit.
I'm not sure...
Migrate part of nolibc-test.c to the new test harness.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++----------------- 1 file changed, 28 insertions(+), 46 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 6c1b42b58e3e..c0e7e090a05a 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max) return ret; }
-#define EXPECT_VFPRINTF(c, expected, fmt, ...) \ - ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__) +#define ASSERT_VFPRINTF(c, expected, fmt, ...) \ + enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \ + if (res == SKIPPED) SKIP(return); \ + if (res == FAIL) FAIL(return);
-static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) +static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c, + const char *expected, const char *fmt, ...) { int ret, fd; ssize_t w, r; @@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm va_list args;
fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600); - if (fd == -1) { - result(llen, SKIPPED); - return 0; - } + if (fd == -1) + return SKIPPED;
memfile = fdopen(fd, "w+"); - if (!memfile) { - result(llen, FAIL); - return 1; - } + if (!memfile) + return FAIL;
va_start(args, fmt); w = vfprintf(memfile, fmt, args); va_end(args);
if (w != c) { - llen += printf(" written(%d) != %d", (int)w, c); - result(llen, FAIL); - return 1; + _metadata->exe.llen += printf(" written(%d) != %d", (int)w, c); + return FAIL; }
fflush(memfile); @@ -1080,46 +1078,30 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm fclose(memfile);
if (r != w) { - llen += printf(" written(%d) != read(%d)", (int)w, (int)r); - result(llen, FAIL); - return 1; + _metadata->exe.llen += printf(" written(%d) != read(%d)", (int)w, (int)r); + return FAIL; }
buf[r] = '\0'; - llen += printf(" "%s" = "%s"", expected, buf); + _metadata->exe.llen += printf(" "%s" = "%s"", expected, buf); ret = strncmp(expected, buf, c);
- result(llen, ret ? FAIL : OK); - return ret; + return ret ? FAIL : OK; }
-static int run_vfprintf(int min, int max) +TEST(vfprintf, empty) { ASSERT_VFPRINTF(0, "", ""); } +TEST(vfprintf, simple) { ASSERT_VFPRINTF(3, "foo", "foo"); } +TEST(vfprintf, string) { ASSERT_VFPRINTF(3, "foo", "%s", "foo"); } +TEST(vfprintf, number) { ASSERT_VFPRINTF(4, "1234", "%d", 1234); } +TEST(vfprintf, negnumber) { ASSERT_VFPRINTF(5, "-1234", "%d", -1234); } +TEST(vfprintf, unsigned) { ASSERT_VFPRINTF(5, "12345", "%u", 12345); } +TEST(vfprintf, char) { ASSERT_VFPRINTF(1, "c", "%c", 'c'); } +TEST(vfprintf, hex) { ASSERT_VFPRINTF(1, "f", "%x", 0xf); } +TEST(vfprintf, pointer) { ASSERT_VFPRINTF(3, "0x1", "%p", (void *) 0x1); } + +int run_vfprintf(int min, int max) { - int test; - int ret = 0; - - for (test = min; test >= 0 && test <= max; test++) { - int llen = 0; /* line length */ - - /* avoid leaving empty lines below, this will insert holes into - * test numbers. - */ - switch (test + __LINE__ + 1) { - CASE_TEST(empty); EXPECT_VFPRINTF(0, "", ""); break; - CASE_TEST(simple); EXPECT_VFPRINTF(3, "foo", "foo"); break; - CASE_TEST(string); EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break; - CASE_TEST(number); EXPECT_VFPRINTF(4, "1234", "%d", 1234); break; - CASE_TEST(negnumber); EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break; - CASE_TEST(unsigned); EXPECT_VFPRINTF(5, "12345", "%u", 12345); break; - CASE_TEST(char); EXPECT_VFPRINTF(1, "c", "%c", 'c'); break; - CASE_TEST(hex); EXPECT_VFPRINTF(1, "f", "%x", 0xf); break; - CASE_TEST(pointer); EXPECT_VFPRINTF(3, "0x1", "%p", (void *) 0x1); break; - case __LINE__: - return ret; /* must be last */ - /* note: do not set any defaults so as to permit holes above */ - } - } - return ret; + return run_test_suite("vfprintf", min, max); }
static int smash_stack(void)
On Wed, Nov 15, 2023 at 10:08:21PM +0100, Thomas Weißschuh wrote:
Migrate part of nolibc-test.c to the new test harness.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++----------------- 1 file changed, 28 insertions(+), 46 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 6c1b42b58e3e..c0e7e090a05a 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max) return ret; } -#define EXPECT_VFPRINTF(c, expected, fmt, ...) \
- ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
+#define ASSERT_VFPRINTF(c, expected, fmt, ...) \
- enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \
- if (res == SKIPPED) SKIP(return); \
- if (res == FAIL) FAIL(return);
Please enclose this in a "do { } while (0)" block when using more than one statement, because you can be certain that sooner or later someone will put an "if" or "for" above it. This will also avoid the double colon caused by the trailing one.
-static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) +static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c,
const char *expected, const char *fmt, ...){ int ret, fd; ssize_t w, r; @@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm va_list args; fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600);
- if (fd == -1) {
result(llen, SKIPPED);return 0;- }
- if (fd == -1)
return SKIPPED;memfile = fdopen(fd, "w+");
- if (!memfile) {
result(llen, FAIL);return 1;- }
- if (!memfile)
return FAIL;
Till now it looks easier and more readable.
va_start(args, fmt); w = vfprintf(memfile, fmt, args); va_end(args); if (w != c) {
llen += printf(" written(%d) != %d", (int)w, c);result(llen, FAIL);return 1;
_metadata->exe.llen += printf(" written(%d) != %d", (int)w, c);return FAIL;
Here however I feel like we're already hacking internals of the test system and that doesn't seem natural nor logical. OK I understand that llen contains the lenght of the emitted message, but how should the user easily guess that it's placed into ->exe.llen, and they may or may not touch other fields there, etc ? Also the fact that the variable is prefixed with an underscore signals a warning to the user that they should not fiddle too much with its internals.
I'm seeing basically two possible approaches: - one consisting in having a wrapper around printf() that transparently sets the llen field in _metadata->exe. This at least offload this knowledge from the user who can then just know they have to pass an opaque metadata argument and that's all.
- one consisting in saying that such functions should not affect the test's metadata themselves and that it ought to be the caller's job instead. The function then only ought to return the printed length, and will not need the metadata at all.
I tend to prefer the second option. In addition, you seem to be returning only 3 statuses (ok/skip/fail) so returning a single composite value for this is easy, e.g. (result | llen) with result made of macros only touching the high bits. If in the future more returns are needed, either a larger int or shift can be used, or we can then return pairs or structs.
Regards, Willy
On 2023-11-16 08:48:02+0100, Willy Tarreau wrote:
On Wed, Nov 15, 2023 at 10:08:21PM +0100, Thomas Weißschuh wrote:
Migrate part of nolibc-test.c to the new test harness.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++----------------- 1 file changed, 28 insertions(+), 46 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 6c1b42b58e3e..c0e7e090a05a 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max) return ret; } -#define EXPECT_VFPRINTF(c, expected, fmt, ...) \
- ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
+#define ASSERT_VFPRINTF(c, expected, fmt, ...) \
- enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \
- if (res == SKIPPED) SKIP(return); \
- if (res == FAIL) FAIL(return);
Please enclose this in a "do { } while (0)" block when using more than one statement, because you can be certain that sooner or later someone will put an "if" or "for" above it. This will also avoid the double colon caused by the trailing one.
Ack.
-static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) +static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c,
const char *expected, const char *fmt, ...){ int ret, fd; ssize_t w, r; @@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm va_list args; fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600);
- if (fd == -1) {
result(llen, SKIPPED);return 0;- }
- if (fd == -1)
return SKIPPED;memfile = fdopen(fd, "w+");
- if (!memfile) {
result(llen, FAIL);return 1;- }
- if (!memfile)
return FAIL;Till now it looks easier and more readable.
va_start(args, fmt); w = vfprintf(memfile, fmt, args); va_end(args); if (w != c) {
llen += printf(" written(%d) != %d", (int)w, c);result(llen, FAIL);return 1;
_metadata->exe.llen += printf(" written(%d) != %d", (int)w, c);return FAIL;Here however I feel like we're already hacking internals of the test system and that doesn't seem natural nor logical. OK I understand that llen contains the lenght of the emitted message, but how should the user easily guess that it's placed into ->exe.llen, and they may or may not touch other fields there, etc ? Also the fact that the variable is prefixed with an underscore signals a warning to the user that they should not fiddle too much with its internals.
Agree that this is ugly.
I'm seeing basically two possible approaches:
one consisting in having a wrapper around printf() that transparently sets the llen field in _metadata->exe. This at least offload this knowledge from the user who can then just know they have to pass an opaque metadata argument and that's all.
one consisting in saying that such functions should not affect the test's metadata themselves and that it ought to be the caller's job instead. The function then only ought to return the printed length, and will not need the metadata at all.
I tend to prefer the second option. In addition, you seem to be returning only 3 statuses (ok/skip/fail) so returning a single composite value for this is easy, e.g. (result | llen) with result made of macros only touching the high bits. If in the future more returns are needed, either a larger int or shift can be used, or we can then return pairs or structs.
I am prefering the first option. It will make it easier to adapt the backend of the harness to KTAP I think.
If you are fine with the basics of the harness I can convert all of nolibc-test.c and then also add the KTAP output at the end.
WDYT?
Thomas
linux-kselftest-mirror@lists.linaro.org
-
Thomas Weißschuh -
Willy Tarreau