To help the developers to avoid mistakes and keep the code smaller let's enable compiler warnings.
I stuck with __attribute__((unused)) over __maybe_unused in nolibc-test.c for consistency with nolibc proper. If we want to add a define it needs to be added twice once for nolibc proper and once for nolibc-test otherwise libc-test wouldn't build anymore.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- Changes in v2: - Don't drop unused test helpers, mark them as __attribute__((unused)) - Make some function in nolibc-test static - Also handle -W and -Wextra - Link to v1: https://lore.kernel.org/r/20230731-nolibc-warnings-v1-0-74973d2a52d7@weisssc...
--- Thomas Weißschuh (10): tools/nolibc: drop unused variables tools/nolibc: sys: avoid implicit sign cast tools/nolibc: stdint: use int for size_t on 32bit selftests/nolibc: drop unused variables selftests/nolibc: mark test helpers as potentially unused selftests/nolibc: make functions static if possible selftests/nolibc: avoid unused arguments warnings selftests/nolibc: avoid sign-compare warnings selftests/nolibc: test return value of read() in test_vfprintf selftests/nolibc: enable compiler warnings
tools/include/nolibc/stdint.h | 4 + tools/include/nolibc/sys.h | 3 +- tools/testing/selftests/nolibc/Makefile | 2 +- tools/testing/selftests/nolibc/nolibc-test.c | 108 +++++++++++++++++---------- 4 files changed, 74 insertions(+), 43 deletions(-) --- base-commit: dfef4fc45d5713eb23d87f0863aff9c33bd4bfaf change-id: 20230731-nolibc-warnings-c6e47284ac03
Best regards,
Nobody needs it, get rid of it.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/include/nolibc/sys.h | 1 - 1 file changed, 1 deletion(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index 8bfe7db20b80..889ccf5f0d56 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -738,7 +738,6 @@ static __attribute__((unused)) int open(const char *path, int flags, ...) { mode_t mode = 0; - int ret;
if (flags & O_CREAT) { va_list args;
getauxval() returns an unsigned long but the overall type of the ternary operator needs to be signed.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/include/nolibc/sys.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index 889ccf5f0d56..2c5d9b06acf5 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -466,7 +466,7 @@ static unsigned long getauxval(unsigned long key); static __attribute__((unused)) long getpagesize(void) { - return __sysret(getauxval(AT_PAGESZ) ?: -ENOENT); + return __sysret((long)getauxval(AT_PAGESZ) ?: -ENOENT); }
Otherwise both gcc and clang may generate warnings about type mismatches:
sysroot/mips/include/string.h:12:14: warning: mismatch in argument 1 type of built-in function 'malloc'; expected 'unsigned int' [-Wbuiltin-declaration-mismatch] 12 | static void *malloc(size_t len); | ^~~~~~
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/include/nolibc/stdint.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/include/nolibc/stdint.h b/tools/include/nolibc/stdint.h index 4b282435a59a..0f390c3028d8 100644 --- a/tools/include/nolibc/stdint.h +++ b/tools/include/nolibc/stdint.h @@ -15,7 +15,11 @@ typedef unsigned int uint32_t; typedef signed int int32_t; typedef unsigned long long uint64_t; typedef signed long long int64_t; +#if __SIZE_WIDTH__ == 64 typedef unsigned long size_t; +#else +typedef unsigned int size_t; +#endif typedef signed long ssize_t; typedef unsigned long uintptr_t; typedef signed long intptr_t;
These got copied around as new testcases where created.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 03b1d30f5507..9a5a212ea55c 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -885,9 +885,7 @@ int run_syscall(int min, int max) int run_stdlib(int min, int max) { int test; - int tmp; int ret = 0; - void *p1, *p2;
for (test = min; test >= 0 && test <= max; test++) { int llen = 0; /* line length */ @@ -1028,9 +1026,7 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char static int run_vfprintf(int min, int max) { int test; - int tmp; int ret = 0; - void *p1, *p2;
for (test = min; test >= 0 && test <= max; test++) { int llen = 0; /* line length */
When warning about unused functions these would be reported by we want to keep them for future use.
Suggested-by: Zhangjin Wu falcon@tinylab.org Link: https://lore.kernel.org/lkml/20230731064826.16584-1-falcon@tinylab.org/ Suggested-by: Willy Tarreau w@1wt.eu Link: https://lore.kernel.org/lkml/20230731224929.GA18296@1wt.eu/ Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 75 ++++++++++++++++++---------- 1 file changed, 50 insertions(+), 25 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 9a5a212ea55c..1555759bb164 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -164,7 +164,8 @@ static void result(int llen, enum RESULT r) #define EXPECT_ZR(cond, expr) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_zr(expr, llen); } while (0)
-static int expect_zr(int expr, int llen) +static __attribute__((unused)) +int expect_zr(int expr, int llen) { int ret = !(expr == 0);
@@ -177,7 +178,8 @@ static int expect_zr(int expr, int llen) #define EXPECT_NZ(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_nz(expr, llen; } while (0)
-static int expect_nz(int expr, int llen) +static __attribute__((unused)) +int expect_nz(int expr, int llen) { int ret = !(expr != 0);
@@ -190,7 +192,8 @@ static int expect_nz(int expr, int llen) #define EXPECT_EQ(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_eq(expr, llen, val); } while (0)
-static int expect_eq(uint64_t expr, int llen, uint64_t val) +static __attribute__((unused)) +int expect_eq(uint64_t expr, int llen, uint64_t val) { int ret = !(expr == val);
@@ -203,7 +206,8 @@ static int expect_eq(uint64_t expr, int llen, uint64_t val) #define EXPECT_NE(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ne(expr, llen, val); } while (0)
-static int expect_ne(int expr, int llen, int val) +static __attribute__((unused)) +int expect_ne(int expr, int llen, int val) { int ret = !(expr != val);
@@ -216,7 +220,8 @@ static int expect_ne(int expr, int llen, int val) #define EXPECT_GE(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ge(expr, llen, val); } while (0)
-static int expect_ge(int expr, int llen, int val) +static __attribute__((unused)) +int expect_ge(int expr, int llen, int val) { int ret = !(expr >= val);
@@ -229,7 +234,8 @@ static int expect_ge(int expr, int llen, int val) #define EXPECT_GT(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_gt(expr, llen, val); } while (0)
-static int expect_gt(int expr, int llen, int val) +static __attribute__((unused)) +int expect_gt(int expr, int llen, int val) { int ret = !(expr > val);
@@ -242,7 +248,8 @@ static int expect_gt(int expr, int llen, int val) #define EXPECT_LE(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_le(expr, llen, val); } while (0)
-static int expect_le(int expr, int llen, int val) +static __attribute__((unused)) +int expect_le(int expr, int llen, int val) { int ret = !(expr <= val);
@@ -255,7 +262,8 @@ static int expect_le(int expr, int llen, int val) #define EXPECT_LT(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_lt(expr, llen, val); } while (0)
-static int expect_lt(int expr, int llen, int val) +static __attribute__((unused)) +int expect_lt(int expr, int llen, int val) { int ret = !(expr < val);
@@ -268,7 +276,8 @@ static int expect_lt(int expr, int llen, int val) #define EXPECT_SYSZR(cond, expr) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_syszr(expr, llen); } while (0)
-static int expect_syszr(int expr, int llen) +static __attribute__((unused)) +int expect_syszr(int expr, int llen) { int ret = 0;
@@ -287,7 +296,8 @@ static int expect_syszr(int expr, int llen) #define EXPECT_SYSEQ(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_syseq(expr, llen, val); } while (0)
-static int expect_syseq(int expr, int llen, int val) +static __attribute__((unused)) +int expect_syseq(int expr, int llen, int val) { int ret = 0;
@@ -306,7 +316,8 @@ static int expect_syseq(int expr, int llen, int val) #define EXPECT_SYSNE(cond, expr, val) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_sysne(expr, llen, val); } while (0)
-static int expect_sysne(int expr, int llen, int val) +static __attribute__((unused)) +int expect_sysne(int expr, int llen, int val) { int ret = 0;
@@ -328,7 +339,8 @@ static int expect_sysne(int expr, int llen, int val) #define EXPECT_SYSER(cond, expr, expret, experr) \ EXPECT_SYSER2(cond, expr, expret, experr, 0)
-static int expect_syserr2(int expr, int expret, int experr1, int experr2, int llen) +static __attribute__((unused)) +int expect_syserr2(int expr, int expret, int experr1, int experr2, int llen) { int ret = 0; int _errno = errno; @@ -351,7 +363,8 @@ static int expect_syserr2(int expr, int expret, int experr1, int experr2, int ll #define EXPECT_PTRZR(cond, expr) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrzr(expr, llen); } while (0)
-static int expect_ptrzr(const void *expr, int llen) +static __attribute__((unused)) +int expect_ptrzr(const void *expr, int llen) { int ret = 0;
@@ -369,7 +382,8 @@ static int expect_ptrzr(const void *expr, int llen) #define EXPECT_PTRNZ(cond, expr) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrnz(expr, llen); } while (0)
-static int expect_ptrnz(const void *expr, int llen) +static __attribute__((unused)) +int expect_ptrnz(const void *expr, int llen) { int ret = 0;
@@ -386,7 +400,8 @@ static int expect_ptrnz(const void *expr, int llen) #define EXPECT_PTREQ(cond, expr, cmp) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptreq(expr, llen, cmp); } while (0)
-static int expect_ptreq(const void *expr, int llen, const void *cmp) +static __attribute__((unused)) +int expect_ptreq(const void *expr, int llen, const void *cmp) { int ret = 0;
@@ -403,7 +418,8 @@ static int expect_ptreq(const void *expr, int llen, const void *cmp) #define EXPECT_PTRNE(cond, expr, cmp) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrne(expr, llen, cmp); } while (0)
-static int expect_ptrne(const void *expr, int llen, const void *cmp) +static __attribute__((unused)) +int expect_ptrne(const void *expr, int llen, const void *cmp) { int ret = 0;
@@ -420,7 +436,8 @@ static int expect_ptrne(const void *expr, int llen, const void *cmp) #define EXPECT_PTRGE(cond, expr, cmp) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrge(expr, llen, cmp); } while (0)
-static int expect_ptrge(const void *expr, int llen, const void *cmp) +static __attribute__((unused)) +int expect_ptrge(const void *expr, int llen, const void *cmp) { int ret = !(expr >= cmp);
@@ -432,7 +449,8 @@ static int expect_ptrge(const void *expr, int llen, const void *cmp) #define EXPECT_PTRGT(cond, expr, cmp) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrgt(expr, llen, cmp); } while (0)
-static int expect_ptrgt(const void *expr, int llen, const void *cmp) +static __attribute__((unused)) +int expect_ptrgt(const void *expr, int llen, const void *cmp) { int ret = !(expr > cmp);
@@ -445,7 +463,8 @@ static int expect_ptrgt(const void *expr, int llen, const void *cmp) #define EXPECT_PTRLE(cond, expr, cmp) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrle(expr, llen, cmp); } while (0)
-static int expect_ptrle(const void *expr, int llen, const void *cmp) +static __attribute__((unused)) +int expect_ptrle(const void *expr, int llen, const void *cmp) { int ret = !(expr <= cmp);
@@ -458,7 +477,8 @@ static int expect_ptrle(const void *expr, int llen, const void *cmp) #define EXPECT_PTRLT(cond, expr, cmp) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrlt(expr, llen, cmp); } while (0)
-static int expect_ptrlt(const void *expr, int llen, const void *cmp) +static __attribute__((unused)) +int expect_ptrlt(const void *expr, int llen, const void *cmp) { int ret = !(expr < cmp);
@@ -473,7 +493,8 @@ static int expect_ptrlt(const void *expr, int llen, const void *cmp) #define EXPECT_PTRER(cond, expr, expret, experr) \ EXPECT_PTRER2(cond, expr, expret, experr, 0)
-static int expect_ptrerr2(const void *expr, const void *expret, int experr1, int experr2, int llen) +static __attribute__((unused)) +int expect_ptrerr2(const void *expr, const void *expret, int experr1, int experr2, int llen) { int ret = 0; int _errno = errno; @@ -495,7 +516,8 @@ static int expect_ptrerr2(const void *expr, const void *expret, int experr1, int #define EXPECT_STRZR(cond, expr) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_strzr(expr, llen); } while (0)
-static int expect_strzr(const char *expr, int llen) +static __attribute__((unused)) +int expect_strzr(const char *expr, int llen) { int ret = 0;
@@ -513,7 +535,8 @@ static int expect_strzr(const char *expr, int llen) #define EXPECT_STRNZ(cond, expr) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_strnz(expr, llen); } while (0)
-static int expect_strnz(const char *expr, int llen) +static __attribute__((unused)) +int expect_strnz(const char *expr, int llen) { int ret = 0;
@@ -531,7 +554,8 @@ static int expect_strnz(const char *expr, int llen) #define EXPECT_STREQ(cond, expr, cmp) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_streq(expr, llen, cmp); } while (0)
-static int expect_streq(const char *expr, int llen, const char *cmp) +static __attribute__((unused)) +int expect_streq(const char *expr, int llen, const char *cmp) { int ret = 0;
@@ -549,7 +573,8 @@ static int expect_streq(const char *expr, int llen, const char *cmp) #define EXPECT_STRNE(cond, expr, cmp) \ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_strne(expr, llen, cmp); } while (0)
-static int expect_strne(const char *expr, int llen, const char *cmp) +static __attribute__((unused)) +int expect_strne(const char *expr, int llen, const char *cmp) { int ret = 0;
This allows the compiler to generate warnings if they go unused.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 1555759bb164..53a3773c7790 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -80,7 +80,7 @@ char *itoa(int i) /* returns the error name (e.g. "ENOENT") for common errors, "SUCCESS" for 0, * or the decimal value for less common ones. */ -const char *errorname(int err) +static const char *errorname(int err) { switch (err) { case 0: return "SUCCESS"; @@ -593,7 +593,7 @@ int expect_strne(const char *expr, int llen, const char *cmp) #define CASE_TEST(name) \ case __LINE__: llen += printf("%d %s", test, #name);
-int run_startup(int min, int max) +static int run_startup(int min, int max) { int test; int ret = 0; @@ -640,7 +640,7 @@ int run_startup(int min, int max)
/* used by some syscall tests below */ -int test_getdents64(const char *dir) +static int test_getdents64(const char *dir) { char buffer[4096]; int fd, ret; @@ -734,7 +734,7 @@ static int test_stat_timestamps(void) return 0; }
-int test_mmap_munmap(void) +static int test_mmap_munmap(void) { int ret, fd, i; void *mem; @@ -796,7 +796,7 @@ int test_mmap_munmap(void) /* Run syscall tests between IDs <min> and <max>. * Return 0 on success, non-zero on failure. */ -int run_syscall(int min, int max) +static int run_syscall(int min, int max) { struct timeval tv; struct timezone tz; @@ -907,7 +907,7 @@ int run_syscall(int min, int max) return ret; }
-int run_stdlib(int min, int max) +static int run_stdlib(int min, int max) { int test; int ret = 0; @@ -1141,7 +1141,7 @@ static int run_protection(int min, int max) }
/* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */ -int prepare(void) +static int prepare(void) { struct stat stat_buf;
@@ -1208,7 +1208,7 @@ static const struct test test_names[] = { { 0 } };
-int is_setting_valid(char *test) +static int is_setting_valid(char *test) { int idx, len, test_len, valid = 0; char delimiter;
On Tue, Aug 01, 2023 at 07:30:13AM +0200, Thomas Weißschuh wrote:
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 1555759bb164..53a3773c7790 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -80,7 +80,7 @@ char *itoa(int i) /* returns the error name (e.g. "ENOENT") for common errors, "SUCCESS" for 0,
- or the decimal value for less common ones.
*/ -const char *errorname(int err) +static const char *errorname(int err) { switch (err) { case 0: return "SUCCESS";
OK for this one, even desired for such a use case.
@@ -593,7 +593,7 @@ int expect_strne(const char *expr, int llen, const char *cmp) #define CASE_TEST(name) \ case __LINE__: llen += printf("%d %s", test, #name); -int run_startup(int min, int max) +static int run_startup(int min, int max) { int test; int ret = 0; @@ -640,7 +640,7 @@ int run_startup(int min, int max) /* used by some syscall tests below */ -int test_getdents64(const char *dir) +static int test_getdents64(const char *dir) { char buffer[4096]; int fd, ret; @@ -734,7 +734,7 @@ static int test_stat_timestamps(void) return 0; } -int test_mmap_munmap(void) +static int test_mmap_munmap(void) { int ret, fd, i; void *mem; @@ -796,7 +796,7 @@ int test_mmap_munmap(void) /* Run syscall tests between IDs <min> and <max>.
- Return 0 on success, non-zero on failure.
*/ -int run_syscall(int min, int max) +static int run_syscall(int min, int max) { struct timeval tv; struct timezone tz; @@ -907,7 +907,7 @@ int run_syscall(int min, int max) return ret; } -int run_stdlib(int min, int max) +static int run_stdlib(int min, int max) { int test; int ret = 0; @@ -1141,7 +1141,7 @@ static int run_protection(int min, int max) } /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */ -int prepare(void) +static int prepare(void) { struct stat stat_buf; @@ -1208,7 +1208,7 @@ static const struct test test_names[] = { { 0 } };
For these ones it will prevent gcc from putting breakpoints there, which is counter-productive.
-int is_setting_valid(char *test) +static int is_setting_valid(char *test) { int idx, len, test_len, valid = 0; char delimiter;
OK for this one.
Willy
On 2023-08-01 08:52:19+0200, Willy Tarreau wrote:
On Tue, Aug 01, 2023 at 07:30:13AM +0200, Thomas Weißschuh wrote:
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 1555759bb164..53a3773c7790 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c
[..]
/* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */ -int prepare(void) +static int prepare(void) { struct stat stat_buf; @@ -1208,7 +1208,7 @@ static const struct test test_names[] = { { 0 } };
For these ones it will prevent gcc from putting breakpoints there, which is counter-productive.
Indeed.
An alternative would be to add -g to CFLAGS (and remove -s from LDFLAGS). This way we get full debugability including breakpoints for everything.
I didn't find the reasoning for -s in LDFLAGS.
Thomas
On Tue, Aug 01, 2023 at 09:34:18AM +0200, Thomas Weißschuh wrote:
On 2023-08-01 08:52:19+0200, Willy Tarreau wrote:
On Tue, Aug 01, 2023 at 07:30:13AM +0200, Thomas Weißschuh wrote:
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 1555759bb164..53a3773c7790 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c
[..]
/* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */ -int prepare(void) +static int prepare(void) { struct stat stat_buf; @@ -1208,7 +1208,7 @@ static const struct test test_names[] = { { 0 } };
For these ones it will prevent gcc from putting breakpoints there, which is counter-productive.
Indeed.
An alternative would be to add -g to CFLAGS (and remove -s from LDFLAGS). This way we get full debugability including breakpoints for everything.
It wouldn't change much because while it would allow the debugger to know where the function was possibly inlined, it's still not very convenient: you believe you're in a function but in fact you're in the caller. It really depends what you're debugging but here I don't see all that as providing a value, at least it brings more annoyance and little to no gain IMHO.
I didn't find the reasoning for -s in LDFLAGS.
It's historic, because normally when you want small binaries you strip them, and the command line was reused as-is, but I agree that we could get rid of it!
Willy
On 2023-08-01 10:13:07+0200, Willy Tarreau wrote:
On Tue, Aug 01, 2023 at 09:34:18AM +0200, Thomas Weißschuh wrote:
On 2023-08-01 08:52:19+0200, Willy Tarreau wrote:
On Tue, Aug 01, 2023 at 07:30:13AM +0200, Thomas Weißschuh wrote:
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 1555759bb164..53a3773c7790 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c
[..]
/* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */ -int prepare(void) +static int prepare(void) { struct stat stat_buf; @@ -1208,7 +1208,7 @@ static const struct test test_names[] = { { 0 } };
For these ones it will prevent gcc from putting breakpoints there, which is counter-productive.
Indeed.
An alternative would be to add -g to CFLAGS (and remove -s from LDFLAGS). This way we get full debugability including breakpoints for everything.
It wouldn't change much because while it would allow the debugger to know where the function was possibly inlined, it's still not very convenient: you believe you're in a function but in fact you're in the caller. It really depends what you're debugging but here I don't see all that as providing a value, at least it brings more annoyance and little to no gain IMHO.
Even if it doesn't work 100% properly it wouldn't it still be a superset of the previous functionality? And we don't have to manually keep track of which ones should be static and which shouldn't (See this discussion).
Would it be better with -ggdb?
If you are still not conviced I'll drop the argument here :-) (And the changes in the next revision)
I didn't find the reasoning for -s in LDFLAGS.
It's historic, because normally when you want small binaries you strip them, and the command line was reused as-is, but I agree that we could get rid of it!
I'll remove it. It was annoying to figure out why my "-g" CFLAG didn't work at all.
Thomas
On Tue, Aug 01, 2023 at 10:50:05AM +0200, Thomas Weißschuh wrote:
An alternative would be to add -g to CFLAGS (and remove -s from LDFLAGS). This way we get full debugability including breakpoints for everything.
It wouldn't change much because while it would allow the debugger to know where the function was possibly inlined, it's still not very convenient: you believe you're in a function but in fact you're in the caller. It really depends what you're debugging but here I don't see all that as providing a value, at least it brings more annoyance and little to no gain IMHO.
Even if it doesn't work 100% properly it wouldn't it still be a superset of the previous functionality?
No, we need to be able to disassemble this to understand what was done to the code we believe is being tested. All of us have been dealing with this already, and making that code less mappable from asm to C is quite annoying.
And we don't have to manually keep track of which ones should be static and which shouldn't (See this discussion).
We should not have to be concerned about this, because it's out of the scope of what this "program" is used for. If we're wondering too much, we're wasting our time on the wrong topic. So we have to find a reasonable rule. One that sounds fine to me is to say: - all that's part of the framework to help with testing (i.e. the expect functions, errorname() etc) could be static because we don't really care about them (at least we won't be placing breakpoints there). They may be marked inline or unused and we can be done with them.
- user code and the calling stack from main should be easily traceable using gdb and objdump -dr so that when you start with a new arch and it breaks early (as happens by definition when syscalls or crt code don't all work well) then it's possible to accurately trace it without having to worry too much about what was transformed how.
Would it be better with -ggdb?
It doesn't change. The thing is: by saying "static" you tell the compiler "I promise it cannot be used anywhere else, do what you want with it", and it can trivially decide to inline all or part of it, or change its number of arguments or whatever as it sees fit because no other code part relies on that function. And when you're trying to disassemble your test_mmap_munmap() and can't find it and can only infer its inlined presence in run_syscall() by seeing a value in a register that vaguely reminds you about __NR_mmap, it's clearly much less easy.
If you are still not conviced I'll drop the argument here :-) (And the changes in the next revision)
No problem, it's fine to discuss the current situation. I've just noticed a number of static on some test functions that would deserve being removed precisely for the reasons above. But that justifies the need for some doc about all this.
I didn't find the reasoning for -s in LDFLAGS.
It's historic, because normally when you want small binaries you strip them, and the command line was reused as-is, but I agree that we could get rid of it!
I'll remove it. It was annoying to figure out why my "-g" CFLAG didn't work at all.
Yes I can definitely understand!
Thanks, Willy
This warnings will be enabled later so avoid triggering it.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 53a3773c7790..cb17cccd0bc7 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1089,7 +1089,8 @@ static int smash_stack(void) return 1; }
-static int run_protection(int min, int max) +static int run_protection(int __attribute__((unused)) min, + int __attribute__((unused)) max) { pid_t pid; int llen = 0, status;
On Tue, Aug 01, 2023 at 07:30:14AM +0200, Thomas Weißschuh wrote:
This warnings will be enabled later so avoid triggering it.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 53a3773c7790..cb17cccd0bc7 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1089,7 +1089,8 @@ static int smash_stack(void) return 1; } -static int run_protection(int min, int max) +static int run_protection(int __attribute__((unused)) min,
int __attribute__((unused)) max)
This one is used to silence -Wunused-parameter I guess. It's one of the rare warnings that I find totally useless in field, because it's simply against the principle of using function pointers with different functions having the same interface but different implementations. As your code evolves you end up with unused on absolutely *all* of the arguments of *all* such functions, which makes them a real pain to add and tends to encourage poor practices such as excessive code reuse just by laziness or boredom. BTW it's one of those that are already disabled in the kernel and we could very well do the same here.
Willy
On 2023-08-01 10:07:28+0200, Willy Tarreau wrote:
On Tue, Aug 01, 2023 at 07:30:14AM +0200, Thomas Weißschuh wrote:
This warnings will be enabled later so avoid triggering it.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 53a3773c7790..cb17cccd0bc7 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1089,7 +1089,8 @@ static int smash_stack(void) return 1; } -static int run_protection(int min, int max) +static int run_protection(int __attribute__((unused)) min,
int __attribute__((unused)) max)This one is used to silence -Wunused-parameter I guess.
Yep.
It's one of the rare warnings that I find totally useless in field, because it's simply against the principle of using function pointers with different functions having the same interface but different implementations. As your code evolves you end up with unused on absolutely *all* of the arguments of *all* such functions, which makes them a real pain to add and tends to encourage poor practices such as excessive code reuse just by laziness or boredom. BTW it's one of those that are already disabled in the kernel and we could very well do the same here.
It's indeed unfortunate.
As long as we don't have too many of them I would prefer to keep the explicit annotations. While they are ugly we then can still reap the positive aspects of the warning.
This is where -std=c89 bites us. With extensions (or C2X) we could also just leave off the argument name to mark it as unused: run_protection(int, int)
Hi, Thomas
On 2023-08-01 10:07:28+0200, Willy Tarreau wrote:
On Tue, Aug 01, 2023 at 07:30:14AM +0200, Thomas Weißschuh wrote:
This warnings will be enabled later so avoid triggering it.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 53a3773c7790..cb17cccd0bc7 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1089,7 +1089,8 @@ static int smash_stack(void) return 1; } -static int run_protection(int min, int max) +static int run_protection(int __attribute__((unused)) min,
int __attribute__((unused)) max)This one is used to silence -Wunused-parameter I guess.
Yep.
It's one of the rare warnings that I find totally useless in field, because it's simply against the principle of using function pointers with different functions having the same interface but different implementations. As your code evolves you end up with unused on absolutely *all* of the arguments of *all* such functions, which makes them a real pain to add and tends to encourage poor practices such as excessive code reuse just by laziness or boredom. BTW it's one of those that are already disabled in the kernel and we could very well do the same here.
It's indeed unfortunate.
As long as we don't have too many of them I would prefer to keep the explicit annotations. While they are ugly we then can still reap the positive aspects of the warning.
This is where -std=c89 bites us. With extensions (or C2X) we could also just leave off the argument name to mark it as unused: run_protection(int, int)
what about further simply ignore the arguments like we did for main(void)?
Thanks, Zhangjin
Aug 1, 2023 12:15:27 Zhangjin Wu falcon@tinylab.org:
Hi, Thomas
On 2023-08-01 10:07:28+0200, Willy Tarreau wrote:
On Tue, Aug 01, 2023 at 07:30:14AM +0200, Thomas Weißschuh wrote:
This warnings will be enabled later so avoid triggering it.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 53a3773c7790..cb17cccd0bc7 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1089,7 +1089,8 @@ static int smash_stack(void) return 1; }
-static int run_protection(int min, int max) +static int run_protection(int __attribute__((unused)) min, + int __attribute__((unused)) max)
This one is used to silence -Wunused-parameter I guess.
Yep.
It's one of the rare warnings that I find totally useless in field, because it's simply against the principle of using function pointers with different functions having the same interface but different implementations. As your code evolves you end up with unused on absolutely *all* of the arguments of *all* such functions, which makes them a real pain to add and tends to encourage poor practices such as excessive code reuse just by laziness or boredom. BTW it's one of those that are already disabled in the kernel and we could very well do the same here.
It's indeed unfortunate.
As long as we don't have too many of them I would prefer to keep the explicit annotations. While they are ugly we then can still reap the positive aspects of the warning.
This is where -std=c89 bites us. With extensions (or C2X) we could also just leave off the argument name to mark it as unused: run_protection(int, int)
what about further simply ignore the arguments like we did for main(void)?
That doesn't work because it is stored as a function pointer in the testcases array. And these members all take the two parameters.
These warnings will be enabled later so avoid triggering them.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index cb17cccd0bc7..82714051c72f 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -749,7 +749,7 @@ static int test_mmap_munmap(void) };
page_size = getpagesize(); - if (page_size < 0) + if (page_size == 0) return -1;
/* find a right file to mmap, existed and accessible */ @@ -998,7 +998,7 @@ static int run_stdlib(int min, int max) #define EXPECT_VFPRINTF(c, expected, fmt, ...) \ ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
-static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...) +static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) { int ret, fd, w, r; char buf[100];
Hi, Thomas
These warnings will be enabled later so avoid triggering them.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index cb17cccd0bc7..82714051c72f 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -749,7 +749,7 @@ static int test_mmap_munmap(void) }; page_size = getpagesize();
- if (page_size < 0)
- if (page_size == 0) return -1;
It was my mistake before, but do we need to align with the one used in test_getpagesize():
static int test_getpagesize(void) { long x = getpagesize(); int c;
if (x < 0) return x;
Use 'long' instead of 'size_t' to declare page_size?
Thanks, Zhangjin
/* find a right file to mmap, existed and accessible */ @@ -998,7 +998,7 @@ static int run_stdlib(int min, int max) #define EXPECT_VFPRINTF(c, expected, fmt, ...) \ ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__) -static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...) +static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) { int ret, fd, w, r; char buf[100];
-- 2.41.0
Hi Zhangjin!
On 2023-08-01 13:48:19+0800, Zhangjin Wu wrote:
These warnings will be enabled later so avoid triggering them.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index cb17cccd0bc7..82714051c72f 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -749,7 +749,7 @@ static int test_mmap_munmap(void) }; page_size = getpagesize();
- if (page_size < 0)
- if (page_size == 0) return -1;
It was my mistake before, but do we need to align with the one used in test_getpagesize():
static int test_getpagesize(void) { long x = getpagesize(); int c;if (x < 0) return x;
Use 'long' instead of 'size_t' to declare page_size?
Good point.
Given that getpagesize() is documented as returning "int" I guess we should actually change the implementation in nolibc.
Thanks, Zhangjin
/* find a right file to mmap, existed and accessible */ @@ -998,7 +998,7 @@ static int run_stdlib(int min, int max) #define EXPECT_VFPRINTF(c, expected, fmt, ...) \ ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__) -static int expect_vfprintf(int llen, size_t c, const char *expected, const char *fmt, ...) +static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) { int ret, fd, w, r; char buf[100];
-- 2.41.0
On 2023-08-01 13:48:19+0800, Zhangjin Wu wrote:
These warnings will be enabled later so avoid triggering them.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index cb17cccd0bc7..82714051c72f 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -749,7 +749,7 @@ static int test_mmap_munmap(void) }; page_size = getpagesize();
- if (page_size < 0)
- if (page_size == 0) return -1;
It was my mistake before, but do we need to align with the one used in test_getpagesize():
static int test_getpagesize(void) { long x = getpagesize(); int c;if (x < 0) return x;
Use 'long' instead of 'size_t' to declare page_size?
Good point.
Given that getpagesize() is documented as returning "int" I guess we should actually change the implementation in nolibc.
Yes, it is documented at [1], perhaps Willy looked at this line before:
This interface, returning an int, may have problems representing appropriate values in the future. Applications should use the sysconf() function instead.
[1]: https://pubs.opengroup.org/onlinepubs/7908799/xsh/getpagesize.html
Thanks, Zhangjin
If read() fails and returns -1 buf would be accessed out of bounds.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 82714051c72f..a334f8450a34 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1031,6 +1031,12 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm lseek(fd, 0, SEEK_SET);
r = read(fd, buf, sizeof(buf) - 1); + if (r == -1) { + llen += printf(" read() = %s", errorname(errno)); + result(llen, FAIL); + return 1; + } + buf[r] = '\0';
fclose(memfile);
On Tue, Aug 01, 2023 at 07:30:16AM +0200, Thomas Weißschuh wrote:
If read() fails and returns -1 buf would be accessed out of bounds.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 82714051c72f..a334f8450a34 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1031,6 +1031,12 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm lseek(fd, 0, SEEK_SET); r = read(fd, buf, sizeof(buf) - 1);
- if (r == -1) {
llen += printf(" read() = %s", errorname(errno));result(llen, FAIL);return 1;- }
- buf[r] = '\0';
In fact given the nature of this file (test if we properly implemented our syscalls), I think that a more conservative approach is deserved because if we messed up on read() we can have anything on return and we don't want to trust that. As such I would suggest that we declare r as ssize_t and verify that it's neither negative nor larger than sizeof(buf)-1, which becomes:
if ((size_t)r >= sizeof(buf)) { ... fail ... }
You'll also have to turn w to ssize_t then due to the test later BTW.
Willy
On 2023-08-01 08:59:17+0200, Willy Tarreau wrote:
On Tue, Aug 01, 2023 at 07:30:16AM +0200, Thomas Weißschuh wrote:
If read() fails and returns -1 buf would be accessed out of bounds.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
tools/testing/selftests/nolibc/nolibc-test.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 82714051c72f..a334f8450a34 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1031,6 +1031,12 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm lseek(fd, 0, SEEK_SET); r = read(fd, buf, sizeof(buf) - 1);
- if (r == -1) {
llen += printf(" read() = %s", errorname(errno));result(llen, FAIL);return 1;- }
- buf[r] = '\0';
In fact given the nature of this file (test if we properly implemented our syscalls), I think that a more conservative approach is deserved because if we messed up on read() we can have anything on return and we don't want to trust that. As such I would suggest that we declare r as ssize_t and verify that it's neither negative nor larger than sizeof(buf)-1, which becomes:
if ((size_t)r >= sizeof(buf)) { ... fail ... }
As r == w is validated just below anyways we could move the assignment buf[r] = '\0' after that check and then we don't need a new block.
You'll also have to turn w to ssize_t then due to the test later BTW.
Will do in any case.
It will help the developers to avoid cruft and detect some bugs.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index f42adef87e12..d6c24c37120c 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -79,7 +79,7 @@ endif CFLAGS_s390 = -m64 CFLAGS_mips = -EL CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all)) -CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \ +CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 -W -Wall -Wextra \ $(call cc-option,-fno-stack-protector) \ $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR) LDFLAGS := -s
Hi Thomas,
On Tue, Aug 01, 2023 at 07:30:07AM +0200, Thomas Weißschuh wrote:
To help the developers to avoid mistakes and keep the code smaller let's enable compiler warnings.
(...)
OK, that's overall OK to me, and I noted thta you'll likely update 6 and 9. Just let me know how you prefer to proceed, I hope to send the PR to Shuah this week-end so I want to be sure we avoid a last minute rush and the risks of breakage that comes with it.
Thanks! Willy
On 2023-08-02 22:22:43+0200, Willy Tarreau wrote:
Hi Thomas,
On Tue, Aug 01, 2023 at 07:30:07AM +0200, Thomas Weißschuh wrote:
To help the developers to avoid mistakes and keep the code smaller let's enable compiler warnings.
(...)
OK, that's overall OK to me, and I noted thta you'll likely update 6 and 9. Just let me know how you prefer to proceed, I hope to send the PR to Shuah this week-end so I want to be sure we avoid a last minute rush and the risks of breakage that comes with it.
I'm not yet sure when I can rework those patches. Could you already pick up those you are fine with? (And also leave out the final one to not have spurious warnings)
Thomas
On Wed, Aug 02, 2023 at 11:10:17PM +0200, Thomas Weißschuh wrote:
On 2023-08-02 22:22:43+0200, Willy Tarreau wrote:
Hi Thomas,
On Tue, Aug 01, 2023 at 07:30:07AM +0200, Thomas Weißschuh wrote:
To help the developers to avoid mistakes and keep the code smaller let's enable compiler warnings.
(...)
OK, that's overall OK to me, and I noted thta you'll likely update 6 and 9. Just let me know how you prefer to proceed, I hope to send the PR to Shuah this week-end so I want to be sure we avoid a last minute rush and the risks of breakage that comes with it.
I'm not yet sure when I can rework those patches.
No problem.
Could you already pick up those you are fine with? (And also leave out the final one to not have spurious warnings)
OK, I'll stick to this for now, thank you! Willy
linux-kselftest-mirror@lists.linaro.org
-
Thomas Weißschuh -
Thomas Weißschuh -
Willy Tarreau -
Zhangjin Wu