| Message ID | 20231115-nolibc-harness-v1-2-4d61382d9bf3@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/nolibc: introduce new test harness | expand |
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...
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)
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(-)