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 v3: - Make getpagesize() return "int" - Simplify validation of read() return value - Don't make functions static that are to be used as breakpoints - Drop -s from LDFLAGS - Use proper types for read()/write() return values - Fix unused parameter warnings in new setvbuf() - Link to v2: https://lore.kernel.org/r/20230801-nolibc-warnings-v2-0-1ba5ca57bd9b@weisssc...
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 (14): tools/nolibc: drop unused variables tools/nolibc: fix return type of getpagesize() tools/nolibc: setvbuf: avoid unused parameter warnings 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 parameter warnings selftests/nolibc: avoid sign-compare warnings selftests/nolibc: use correct return type for read() and write() selftests/nolibc: prevent out of bounds access in expect_vfprintf selftests/nolibc: don't strip nolibc-test selftests/nolibc: enable compiler warnings
tools/include/nolibc/stdint.h | 4 + tools/include/nolibc/stdio.h | 5 +- tools/include/nolibc/sys.h | 7 +- tools/testing/selftests/nolibc/Makefile | 4 +- tools/testing/selftests/nolibc/nolibc-test.c | 111 ++++++++++++++++----------- 5 files changed, 80 insertions(+), 51 deletions(-) --- base-commit: bc87f9562af7b2b4cb07dcaceccfafcf05edaff8 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 56f63eb48a1b..e12dd962c578 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;
It's documented as returning int which is also implemented by glibc and musl, so adopt that return type.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/include/nolibc/sys.h | 4 ++-- tools/testing/selftests/nolibc/nolibc-test.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h index e12dd962c578..c151533ba8e9 100644 --- a/tools/include/nolibc/sys.h +++ b/tools/include/nolibc/sys.h @@ -460,11 +460,11 @@ pid_t gettid(void) static unsigned long getauxval(unsigned long key);
/* - * long getpagesize(void); + * int getpagesize(void); */
static __attribute__((unused)) -long getpagesize(void) +int getpagesize(void) { return __sysret(getauxval(AT_PAGESZ) ?: -ENOENT); } diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 3a21bee360ea..52489455add8 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -636,7 +636,7 @@ int test_getdents64(const char *dir)
static int test_getpagesize(void) { - long x = getpagesize(); + int x = getpagesize(); int c;
if (x < 0)
This warning will be enabled later so avoid triggering it.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/include/nolibc/stdio.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/include/nolibc/stdio.h b/tools/include/nolibc/stdio.h index a3778aff4fa9..cae402c11e57 100644 --- a/tools/include/nolibc/stdio.h +++ b/tools/include/nolibc/stdio.h @@ -356,7 +356,10 @@ void perror(const char *msg) }
static __attribute__((unused)) -int setvbuf(FILE *stream, char *buf, int mode, size_t size) +int setvbuf(FILE *stream __attribute__((unused)), + char *buf __attribute__((unused)), + int mode, + size_t size __attribute__((unused))) { /* * nolibc does not support buffering so this is a nop. Just check mode
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 c151533ba8e9..833d6c5e86dc 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)) int getpagesize(void) { - return __sysret(getauxval(AT_PAGESZ) ?: -ENOENT); + return __sysret((int)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;
Hi Thomas,
On Thu, Aug 03, 2023 at 09:28:49AM +0200, Thomas Weißschuh wrote:
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
This one breaks gcc < 7 for me because __SIZE_WIDTH__ is not defined there. However I could trace __SIZE_TYPE__ to be defined since at least gcc-3.4 so instead we can do this, which will always match the type set by the compiler (either "unsigned int" or "unsigned long int") :
#ifdef __SIZE_TYPE__ typedef __SIZE_TYPE__ size_t; #else typedef unsigned long size_t; #endif
Please just let me know if you want me to modify your patch accordingly. I'm still continuing the tests.
Thanks, Willy
On 2023-08-05 18:19:29+0200, Willy Tarreau wrote:
Hi Thomas,
On Thu, Aug 03, 2023 at 09:28:49AM +0200, Thomas Weißschuh wrote:
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
This one breaks gcc < 7 for me because __SIZE_WIDTH__ is not defined there. However I could trace __SIZE_TYPE__ to be defined since at least gcc-3.4 so instead we can do this, which will always match the type set by the compiler (either "unsigned int" or "unsigned long int") :
#ifdef __SIZE_TYPE__ typedef __SIZE_TYPE__ size_t; #else typedef unsigned long size_t; #endif
Sounds good. But do we need the fallback?
Further below we are also unconditionally using preprocessor-defines like __INT_MAX__ and __LONG_MAX__.
So I guess we can drop the proposed #ifdef.
Please just let me know if you want me to modify your patch accordingly. I'm still continuing the tests.
Feel free to modify the patch.
Thanks! Thomas
On Sat, Aug 05, 2023 at 06:25:52PM +0200, Thomas Weißschuh wrote:
On 2023-08-05 18:19:29+0200, Willy Tarreau wrote:
Hi Thomas,
On Thu, Aug 03, 2023 at 09:28:49AM +0200, Thomas Weißschuh wrote:
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
This one breaks gcc < 7 for me because __SIZE_WIDTH__ is not defined there. However I could trace __SIZE_TYPE__ to be defined since at least gcc-3.4 so instead we can do this, which will always match the type set by the compiler (either "unsigned int" or "unsigned long int") :
#ifdef __SIZE_TYPE__ typedef __SIZE_TYPE__ size_t; #else typedef unsigned long size_t; #endif
Sounds good. But do we need the fallback?
I don't know. It's always the same when using a compiler-defined macro that you discover when you need it, you never know how spread it is. At least I've also found it in clang as old as 3.8, so maybe it can be considered safe enough.
Further below we are also unconditionally using preprocessor-defines like __INT_MAX__ and __LONG_MAX__.
So I guess we can drop the proposed #ifdef.
I'll try with this, the risk is quite low anyway (famous last words).
Please just let me know if you want me to modify your patch accordingly. I'm still continuing the tests.
Feel free to modify the patch.
Will do, thanks!
Willy
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 52489455add8..cff441c17f3e 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -908,9 +908,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 */ @@ -1051,9 +1049,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 cff441c17f3e..154ec4787e8d 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -165,7 +165,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);
@@ -178,7 +179,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);
@@ -191,7 +193,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);
@@ -204,7 +207,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);
@@ -217,7 +221,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);
@@ -230,7 +235,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);
@@ -243,7 +249,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);
@@ -256,7 +263,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);
@@ -269,7 +277,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;
@@ -288,7 +297,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;
@@ -307,7 +317,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;
@@ -329,7 +340,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; @@ -352,7 +364,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;
@@ -370,7 +383,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;
@@ -387,7 +401,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;
@@ -404,7 +419,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;
@@ -421,7 +437,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);
@@ -433,7 +450,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);
@@ -446,7 +464,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);
@@ -459,7 +478,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);
@@ -474,7 +494,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; @@ -496,7 +517,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;
@@ -514,7 +536,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;
@@ -532,7 +555,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;
@@ -550,7 +574,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.
Functions that are supposed to be used as breakpoints should not be static, so un-statify those if necessary.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 154ec4787e8d..ea420c794536 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"; @@ -659,7 +659,7 @@ int test_getdents64(const char *dir) return ret; }
-static int test_getpagesize(void) +int test_getpagesize(void) { int x = getpagesize(); int c; @@ -688,7 +688,7 @@ static int test_getpagesize(void) return !c; }
-static int test_fork(void) +int test_fork(void) { int status; pid_t pid; @@ -713,7 +713,7 @@ static int test_fork(void) } }
-static int test_stat_timestamps(void) +int test_stat_timestamps(void) { struct stat st;
@@ -793,7 +793,7 @@ int test_mmap_munmap(void) return !!ret; }
-static int test_pipe(void) +int test_pipe(void) { const char *const msg = "hello, nolibc"; int pipefd[2]; @@ -1231,7 +1231,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;
This warning 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 ea420c794536..d6aa12c3f9ff 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1112,7 +1112,8 @@ static int smash_stack(void) return 1; }
-static int run_protection(int min, int max) +static int run_protection(int min __attribute__((unused)), + int max __attribute__((unused))) { pid_t pid; int llen = 0, status;
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 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index d6aa12c3f9ff..0ab241d61508 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -737,9 +737,9 @@ int test_stat_timestamps(void)
int test_mmap_munmap(void) { - int ret, fd, i; + int ret, fd, i, page_size; void *mem; - size_t page_size, file_size, length; + size_t file_size, length; off_t offset, pa_offset; struct stat stat_buf; const char * const files[] = { @@ -1021,7 +1021,7 @@ 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]; @@ -1045,7 +1045,7 @@ static int expect_vfprintf(int llen, size_t c, const char *expected, const char va_end(args);
if (w != c) { - llen += printf(" written(%d) != %d", w, (int) c); + llen += printf(" written(%d) != %d", w, c); result(llen, FAIL); return 1; }
Avoid truncating values before comparing them.
As printf in nolibc doesn't support ssize_t add casts to int for printing.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 0ab241d61508..63f09800057d 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1023,7 +1023,8 @@ int run_stdlib(int min, int max)
static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...) { - int ret, fd, w, r; + int ret, fd; + ssize_t w, r; char buf[100]; FILE *memfile; va_list args; @@ -1045,7 +1046,7 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm va_end(args);
if (w != c) { - llen += printf(" written(%d) != %d", w, c); + llen += printf(" written(%d) != %d", (int)w, c); result(llen, FAIL); return 1; } @@ -1059,7 +1060,7 @@ 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)", w, r); + llen += printf(" written(%d) != read(%d)", (int)w, (int)r); result(llen, FAIL); return 1; }
If read() fails and returns -1 (or returns garbage for some other reason) buf would be accessed out of bounds. Only use the return value of read() after it has been validated.
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 63f09800057d..0145a44af990 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -1055,7 +1055,6 @@ 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); - buf[r] = '\0';
fclose(memfile);
@@ -1065,6 +1064,7 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm return 1; }
+ buf[r] = '\0'; llen += printf(" "%s" = "%s"", expected, buf); ret = strncmp(expected, buf, c);
Binary size is not important for nolibc-test and some debugging information is nice to have, so don't strip the binary during linking.
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..b82d29b6c37f 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -82,7 +82,7 @@ CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \ $(call cc-option,-fno-stack-protector) \ $(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR) -LDFLAGS := -s +LDFLAGS :=
REPORT ?= awk '/[OK][\r]*$$/{p++} /[FAIL][\r]*$$/{if (!f) printf("\n"); f++; print;} /[SKIPPED][\r]*$$/{s++} \ END{ printf("\n%3d test(s): %3d passed, %3d skipped, %3d failed => status: ", p+s+f, p, s, f); \
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 b82d29b6c37f..e8d09cbee2ab 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 :=
On Thu, Aug 03, 2023 at 09:28:58AM +0200, Thomas Weißschuh wrote:
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 b82d29b6c37f..e8d09cbee2ab 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 :=
I'm now getting this with gcc < 9:
nolibc-test.c: In function 'test_pipe': nolibc-test.c:811:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (len != strlen(msg)) ^ The reason is that len is ssize_t and strlen() is size_t. I tried different approaches here but the cleanest remains turning len to size_t (we don't use its sign anyway), so I'll do that one as well.
Cheers, Willy
On Sat, Aug 05, 2023 at 06:23:27PM +0200, Willy Tarreau wrote:
On Thu, Aug 03, 2023 at 09:28:58AM +0200, Thomas Weißschuh wrote:
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 b82d29b6c37f..e8d09cbee2ab 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 :=
I'm now getting this with gcc < 9:
nolibc-test.c: In function 'test_pipe': nolibc-test.c:811:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if (len != strlen(msg)) ^ The reason is that len is ssize_t and strlen() is size_t. I tried different approaches here but the cleanest remains turning len to size_t (we don't use its sign anyway), so I'll do that one as well.
By the way since the original commit is still not upstream I'd rather reinject this change directly in it. Yuan, are you OK with this ?
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 0145a44af990..95884168cea0 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -798,7 +798,7 @@ int test_pipe(void) const char *const msg = "hello, nolibc"; int pipefd[2]; char buf[32]; - ssize_t len; + size_t len;
if (pipe(pipefd) == -1)
Thanks, Willy
On Thu, Aug 03, 2023 at 09:28:44AM +0200, Thomas Weißschuh wrote:
To help the developers to avoid mistakes and keep the code smaller let's enable compiler warnings.
All the series looks good, I've now queued it, thanks!
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.
I tend to prefer to avoid spreading macros in nolibc itself unless strictly necessary as we'd need to put them under a "nolibc" namespace to avoid a risk of clash, and it becomes less interesting in terms of number of characters saved per line when everything is prefixed with "nolibc_" or so. It's convenient however when there are multiple choices to be replicated at multiple places. So let's keep it like this for now.
Cheers, Willy
linux-kselftest-mirror@lists.linaro.org
-
Thomas Weißschuh -
Willy Tarreau