[PATCH v2 00/10] tools/nolibc: enable compiler warnings

[PATCH v2 00/10] tools/nolibc: enable compiler warnings

[Thomas Weißschuh]

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@weissschuh.net

---
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,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v2 01/10] tools/nolibc: drop unused variables

[Thomas Weißschuh]

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;

-- 
2.41.0

[PATCH v2 02/10] tools/nolibc: sys: avoid implicit sign cast

[Thomas Weißschuh]

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);
 }
 
 

-- 
2.41.0

[PATCH v2 03/10] tools/nolibc: stdint: use int for size_t on 32bit

[Thomas Weißschuh]

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;

-- 
2.41.0

[PATCH v2 04/10] selftests/nolibc: drop unused variables

[Thomas Weißschuh]

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 */

-- 
2.41.0

[PATCH v2 05/10] selftests/nolibc: mark test helpers as potentially unused

[Thomas Weißschuh]

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;
 

-- 
2.41.0

[PATCH v2 06/10] selftests/nolibc: make functions static if possible

[Thomas Weißschuh]

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;

-- 
2.41.0

Re: [PATCH v2 06/10] selftests/nolibc: make functions static if possible

[Willy Tarreau]

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

Re: [PATCH v2 06/10] selftests/nolibc: make functions static if possible

[Thomas Weißschuh]

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

Re: [PATCH v2 06/10] selftests/nolibc: make functions static if possible

[Willy Tarreau]

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

Re: [PATCH v2 06/10] selftests/nolibc: make functions static if possible

[Thomas Weißschuh]

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

Re: [PATCH v2 06/10] selftests/nolibc: make functions static if possible

[Willy Tarreau]

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

[PATCH v2 07/10] selftests/nolibc: avoid unused arguments warnings

[Thomas Weißschuh]

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;

-- 
2.41.0

Re: [PATCH v2 07/10] selftests/nolibc: avoid unused arguments warnings

[Willy Tarreau]

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

Re: [PATCH v2 07/10] selftests/nolibc: avoid unused arguments warnings

[Thomas Weißschuh]

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)

Re: [PATCH v2 07/10] selftests/nolibc: avoid unused arguments warnings

[Zhangjin Wu]

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

Re: [PATCH v2 07/10] selftests/nolibc: avoid unused arguments warnings

[Thomas Weißschuh]

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.

[PATCH v2 08/10] selftests/nolibc: avoid sign-compare warnings

[Thomas Weißschuh]

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];

-- 
2.41.0

[PATCH v2 08/10] selftests/nolibc: avoid sign-compare warnings

[Zhangjin Wu]

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

Re: [PATCH v2 08/10] selftests/nolibc: avoid sign-compare warnings

[Thomas Weißschuh]

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

Re: [PATCH v2 08/10] selftests/nolibc: avoid sign-compare warnings

[Zhangjin Wu]

> 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

[PATCH v2 09/10] selftests/nolibc: test return value of read() in test_vfprintf

[Thomas Weißschuh]

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);

-- 
2.41.0

Re: [PATCH v2 09/10] selftests/nolibc: test return value of read() in test_vfprintf

[Willy Tarreau]

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

Re: [PATCH v2 09/10] selftests/nolibc: test return value of read() in test_vfprintf

[Thomas Weißschuh]

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.

[PATCH v2 10/10] selftests/nolibc: enable compiler warnings

[Thomas Weißschuh]

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

-- 
2.41.0

Re: [PATCH v2 00/10] tools/nolibc: enable compiler warnings

[Willy Tarreau]

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

Re: [PATCH v2 00/10] tools/nolibc: enable compiler warnings

[Thomas Weißschuh]

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

Re: [PATCH v2 00/10] tools/nolibc: enable compiler warnings

[Willy Tarreau]

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