[PATCH v3 00/14] tools/nolibc: enable compiler warnings

[PATCH v3 00/14] 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 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@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 (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,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v3 01/14] 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 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;

-- 
2.41.0

[PATCH v3 02/14] tools/nolibc: fix return type of getpagesize()

[Thomas Weißschuh]

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)

-- 
2.41.0

[PATCH v3 03/14] tools/nolibc: setvbuf: avoid unused parameter warnings

[Thomas Weißschuh]

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

-- 
2.41.0

[PATCH v3 04/14] 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 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);
 }
 
 

-- 
2.41.0

[PATCH v3 05/14] 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

Re: [PATCH v3 05/14] tools/nolibc: stdint: use int for size_t on 32bit

[Willy Tarreau]

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

Re: [PATCH v3 05/14] tools/nolibc: stdint: use int for size_t on 32bit

[Thomas Weißschuh]

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

Re: [PATCH v3 05/14] tools/nolibc: stdint: use int for size_t on 32bit

[Willy Tarreau]

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

[PATCH v3 06/14] 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 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 */

-- 
2.41.0

[PATCH v3 07/14] 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 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;
 

-- 
2.41.0

[PATCH v3 08/14] selftests/nolibc: make functions static if possible

[Thomas Weißschuh]

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;

-- 
2.41.0

[PATCH v3 09/14] selftests/nolibc: avoid unused parameter warnings

[Thomas Weißschuh]

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;

-- 
2.41.0

[PATCH v3 10/14] 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 | 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;
 	}

-- 
2.41.0

[PATCH v3 11/14] selftests/nolibc: use correct return type for read() and write()

[Thomas Weißschuh]

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

-- 
2.41.0

[PATCH v3 12/14] selftests/nolibc: prevent out of bounds access in expect_vfprintf

[Thomas Weißschuh]

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

-- 
2.41.0

[PATCH v3 13/14] selftests/nolibc: don't strip nolibc-test

[Thomas Weißschuh]

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

-- 
2.41.0

[PATCH v3 14/14] 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 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 :=

-- 
2.41.0

Re: [PATCH v3 14/14] selftests/nolibc: enable compiler warnings

[Willy Tarreau]

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

Re: [PATCH v3 00/14] tools/nolibc: enable compiler warnings

[Willy Tarreau]

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