[PATCH 0/4] tools/nolibc: enable compiler warnings

[PATCH 0/4] 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.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (4):
      selftests/nolibc: drop unused test helpers
      selftests/nolibc: drop unused variables
      tools/nolibc: drop unused variables
      selftests/nolibc: enable -Wall compiler warnings

 tools/include/nolibc/sys.h                   |   1 -
 tools/testing/selftests/nolibc/Makefile      |   2 +-
 tools/testing/selftests/nolibc/nolibc-test.c | 103 ---------------------------
 3 files changed, 1 insertion(+), 105 deletions(-)
---
base-commit: dfef4fc45d5713eb23d87f0863aff9c33bd4bfaf
change-id: 20230731-nolibc-warnings-c6e47284ac03

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/4] selftests/nolibc: drop unused test helpers

[Thomas Weißschuh]

As we want to enable compiler warnings in the future these would be
reported as unused functions.

If we need them in the future they are easy to recreate from their still
existing siblings.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 99 ----------------------------
 1 file changed, 99 deletions(-)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 03b1d30f5507..53e2d448eded 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -161,31 +161,6 @@ static void result(int llen, enum RESULT r)
  * of failures, thus either 0 or 1.
  */
 
-#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)
-{
-	int ret = !(expr == 0);
-
-	llen += printf(" = %d ", expr);
-	result(llen, ret ? FAIL : OK);
-	return ret;
-}
-
-
-#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)
-{
-	int ret = !(expr != 0);
-
-	llen += printf(" = %d ", expr);
-	result(llen, ret ? FAIL : OK);
-	return ret;
-}
-
 
 #define EXPECT_EQ(cond, expr, val)				\
 	do { if (!(cond)) result(llen, SKIPPED); else ret += expect_eq(expr, llen, val); } while (0)
@@ -239,19 +214,6 @@ 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)
-{
-	int ret = !(expr <= val);
-
-	llen += printf(" = %d ", expr);
-	result(llen, ret ? FAIL : OK);
-	return ret;
-}
-
-
 #define EXPECT_LT(cond, expr, val)				\
 	do { if (!(cond)) result(llen, SKIPPED); else ret += expect_lt(expr, llen, val); } while (0)
 
@@ -348,24 +310,6 @@ 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)
-{
-	int ret = 0;
-
-	llen += printf(" = <%p> ", expr);
-	if (expr) {
-		ret = 1;
-		result(llen, FAIL);
-	} else {
-		result(llen, OK);
-	}
-	return ret;
-}
-
-
 #define EXPECT_PTRNZ(cond, expr)				\
 	do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrnz(expr, llen); } while (0)
 
@@ -417,18 +361,6 @@ static int expect_ptrne(const void *expr, int llen, const void *cmp)
 	return ret;
 }
 
-#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)
-{
-	int ret = !(expr >= cmp);
-
-	llen += printf(" = <%p> ", expr);
-	result(llen, ret ? FAIL : OK);
-	return ret;
-}
-
 #define EXPECT_PTRGT(cond, expr, cmp)				\
 	do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrgt(expr, llen, cmp); } while (0)
 
@@ -442,19 +374,6 @@ 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)
-{
-	int ret = !(expr <= cmp);
-
-	llen += printf(" = <%p> ", expr);
-	result(llen, ret ? FAIL : OK);
-	return ret;
-}
-
-
 #define EXPECT_PTRLT(cond, expr, cmp)				\
 	do { if (!(cond)) result(llen, SKIPPED); else ret += expect_ptrlt(expr, llen, cmp); } while (0)
 
@@ -546,24 +465,6 @@ 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)
-{
-	int ret = 0;
-
-	llen += printf(" = <%s> ", expr);
-	if (strcmp(expr, cmp) == 0) {
-		ret = 1;
-		result(llen, FAIL);
-	} else {
-		result(llen, OK);
-	}
-	return ret;
-}
-
-
 /* declare tests based on line numbers. There must be exactly one test per line. */
 #define CASE_TEST(name) \
 	case __LINE__: llen += printf("%d %s", test, #name);

-- 
2.41.0

[PATCH 2/4] 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 53e2d448eded..3064191681d0 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -786,9 +786,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 */
@@ -929,9 +927,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 3/4] 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 4/4] selftests/nolibc: enable -Wall 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..72227d75c6da 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 -Wall \
 		$(call cc-option,-fno-stack-protector) \
 		$(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
 LDFLAGS := -s

-- 
2.41.0

[PATCH 1/4] selftests/nolibc: drop unused test helpers

[Zhangjin Wu]

Hi, Thomas

> As we want to enable compiler warnings in the future these would be
> reported as unused functions.
> 
> If we need them in the future they are easy to recreate from their still
> existing siblings.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  tools/testing/selftests/nolibc/nolibc-test.c | 99 ----------------------------
>  1 file changed, 99 deletions(-)
> 
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 03b1d30f5507..53e2d448eded 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -161,31 +161,6 @@ static void result(int llen, enum RESULT r)
>   * of failures, thus either 0 or 1.
>   */
>  
> -#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)
> -{

Why not a simple 'static __attribute__((unused))' line, then, no need to
add them again next time.

    -static int expect_zr(int expr, int llen)
    +static __attribute__((unused))
    +int expect_zr(int expr, int llen)
     {

Thanks,
Zhangjin

> -	int ret = !(expr == 0);
> -
> -	llen += printf(" = %d ", expr);
> -	result(llen, ret ? FAIL : OK);
> -	return ret;
> -}
> -
> -
> -#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)
> -{
> -	int ret = !(expr != 0);
> -
> -	llen += printf(" = %d ", expr);
> -	result(llen, ret ? FAIL : OK);
> -	return ret;
> -}
> -
> [...]
> -- 
> 2.41.0
> 
> 

Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

[Thomas Weißschuh]

Note:

It seems your mail client does not add the prefix "Re: " to responses.
Is that intentional?

On 2023-07-31 14:48:26+0800, Zhangjin Wu wrote:
> Hi, Thomas
> 
> > As we want to enable compiler warnings in the future these would be
> > reported as unused functions.
> > 
> > If we need them in the future they are easy to recreate from their still
> > existing siblings.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  tools/testing/selftests/nolibc/nolibc-test.c | 99 ----------------------------
> >  1 file changed, 99 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index 03b1d30f5507..53e2d448eded 100644
> > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > @@ -161,31 +161,6 @@ static void result(int llen, enum RESULT r)
> >   * of failures, thus either 0 or 1.
> >   */
> >  
> > -#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)
> > -{
> 
> Why not a simple 'static __attribute__((unused))' line, then, no need to
> add them again next time.
> 
>     -static int expect_zr(int expr, int llen)
>     +static __attribute__((unused))
>     +int expect_zr(int expr, int llen)
>      {

Personally I don't like having dead code lying around that needs to be
maintained and skipped over while reading.
It's not a given that we will need those helpers in the future at all.

Thomas

> 
> Thanks,
> Zhangjin
> 
> > -	int ret = !(expr == 0);
> > -
> > -	llen += printf(" = %d ", expr);
> > -	result(llen, ret ? FAIL : OK);
> > -	return ret;
> > -}
> > -
> > -
> > -#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)
> > -{
> > -	int ret = !(expr != 0);
> > -
> > -	llen += printf(" = %d ", expr);
> > -	result(llen, ret ? FAIL : OK);
> > -	return ret;
> > -}
> > -
> > [...]
> > -- 
> > 2.41.0
> > 
> > 

Re: [PATCH 4/4] selftests/nolibc: enable -Wall compiler warnings

[Zhangjin Wu]

Hi, Thomas

> 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..72227d75c6da 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 -Wall \

Very good static analyzer support.

What about further add more options?

    +CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 -Wall -Wextra -Werror\

A simple test shows, it can catch more issues.

Thanks,
Zhangjin

>  		$(call cc-option,-fno-stack-protector) \
>  		$(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
>  LDFLAGS := -s
> 
> -- 
> 2.41.0

Re: Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

[Zhangjin Wu]

Hi, Thomas

> Note:
> 
> It seems your mail client does not add the prefix "Re: " to responses.
> Is that intentional?
>

I use a lightweight 'b4 + git send-email' method to reply emails,
sometimes, I forgot manually adding the 'Re: ' prefix, perhaps I should
write a simple script to do that or carefully check the Subject title
everytime, Thanks!

> On 2023-07-31 14:48:26+0800, Zhangjin Wu wrote:
> > Hi, Thomas
> > 
> > > As we want to enable compiler warnings in the future these would be
> > > reported as unused functions.
> > > 
> > > If we need them in the future they are easy to recreate from their still
> > > existing siblings.
> > > 
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > >  tools/testing/selftests/nolibc/nolibc-test.c | 99 ----------------------------
> > >  1 file changed, 99 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > index 03b1d30f5507..53e2d448eded 100644
> > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > @@ -161,31 +161,6 @@ static void result(int llen, enum RESULT r)
> > >   * of failures, thus either 0 or 1.
> > >   */
> > >  
> > > -#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)
> > > -{
> > 
> > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > add them again next time.
> > 
> >     -static int expect_zr(int expr, int llen)
> >     +static __attribute__((unused))
> >     +int expect_zr(int expr, int llen)
> >      {
> 
> Personally I don't like having dead code lying around that needs to be
> maintained and skipped over while reading.
> It's not a given that we will need those helpers in the future at all.
>

It is reasonable in some degree from current status, especially for
these ones are newly added, but let us think about these scenes: when we
would drop or change some test cases in the future and the helpers may
would be not referenced by any test cases in a short time, and warnings
there, but some other cases may be added later to use them again ...

I'm ok to drop these ones, but another patch may be required to add
'static __attribute__((unused))' for all of the currently used ones,
otherwise, there will be warnings randomly by a test case change or
drop.

Or even further, is it possible to merge some of them to some more
generic helpers like the ones used from the selftest.h in your last RFC
patchset?

Thanks,
Zhangjin

> Thomas
> 
> > 
> > Thanks,
> > Zhangjin

Re: [PATCH 4/4] selftests/nolibc: enable -Wall compiler warnings

[Thomas Weißschuh]

On 2023-07-31 15:17:18+0800, Zhangjin Wu 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 f42adef87e12..72227d75c6da 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 -Wall \
> 
> Very good static analyzer support.
> 
> What about further add more options?
> 
>     +CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 -Wall -Wextra -Werror\
> 
> A simple test shows, it can catch more issues.

-Wextra will need some further rework for 32bit architectures to avoid
some warnings.
(At least mips for which I tested it)

I don't think -Werror is appropriate. If we want to test the functioning
of nolibc with weird compilers these may very well add new warnings and
that shouldn't break the build.

> 
> Thanks,
> Zhangjin
> 
> >  		$(call cc-option,-fno-stack-protector) \
> >  		$(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
> >  LDFLAGS := -s
> > 
> > -- 
> > 2.41.0

Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

[Thomas Weißschuh]

On 2023-07-31 15:32:43+0800, Zhangjin Wu wrote:
> Hi, Thomas
> 
> > Note:
> > 
> > It seems your mail client does not add the prefix "Re: " to responses.
> > Is that intentional?
> >
> 
> I use a lightweight 'b4 + git send-email' method to reply emails,
> sometimes, I forgot manually adding the 'Re: ' prefix, perhaps I should
> write a simple script to do that or carefully check the Subject title
> everytime, Thanks!

Now there are two "Re: " prefixes :-)

My understanding is that there is exactly one "Re: " prefix iff the
message is any response at all.

> > On 2023-07-31 14:48:26+0800, Zhangjin Wu wrote:
> > > Hi, Thomas
> > > 
> > > > As we want to enable compiler warnings in the future these would be
> > > > reported as unused functions.
> > > > 
> > > > If we need them in the future they are easy to recreate from their still
> > > > existing siblings.
> > > > 
> > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > ---
> > > >  tools/testing/selftests/nolibc/nolibc-test.c | 99 ----------------------------
> > > >  1 file changed, 99 deletions(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 03b1d30f5507..53e2d448eded 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -161,31 +161,6 @@ static void result(int llen, enum RESULT r)
> > > >   * of failures, thus either 0 or 1.
> > > >   */
> > > >  
> > > > -#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)
> > > > -{
> > > 
> > > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > > add them again next time.
> > > 
> > >     -static int expect_zr(int expr, int llen)
> > >     +static __attribute__((unused))
> > >     +int expect_zr(int expr, int llen)
> > >      {
> > 
> > Personally I don't like having dead code lying around that needs to be
> > maintained and skipped over while reading.
> > It's not a given that we will need those helpers in the future at all.
> >
> 
> It is reasonable in some degree from current status, especially for
> these ones are newly added, but let us think about these scenes: when we
> would drop or change some test cases in the future and the helpers may
> would be not referenced by any test cases in a short time, and warnings
> there, but some other cases may be added later to use them again ...

That doesn't seem very likely.
Did it happen recently?

> I'm ok to drop these ones, but another patch may be required to add
> 'static __attribute__((unused))' for all of the currently used ones,
> otherwise, there will be warnings randomly by a test case change or
> drop.

Then we just drop the helper when we don't need it anymore.

I also dislike the __attribute__ spam to be honest.

> Or even further, is it possible to merge some of them to some more
> generic helpers like the ones used from the selftest.h in your last RFC
> patchset?

Something like this will indeed be part of the KTAP rework.
But it's a change for another time.

Thomas

Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

[Zhangjin Wu]

Hi, Willy

> On 2023-07-31 15:32:43+0800, Zhangjin Wu wrote:
> > Hi, Thomas
> > 
> > > Note:
> > > 
> > > It seems your mail client does not add the prefix "Re: " to responses.
> > > Is that intentional?
> > >
> > 
> > I use a lightweight 'b4 + git send-email' method to reply emails,
> > sometimes, I forgot manually adding the 'Re: ' prefix, perhaps I should
> > write a simple script to do that or carefully check the Subject title
> > everytime, Thanks!
> 
> Now there are two "Re: " prefixes :-)
> 
> My understanding is that there is exactly one "Re: " prefix iff the
> message is any response at all.
>

Get it, some clients always add another 'Re: ' for a new response, only
one is better, thanks ;-)

> > > On 2023-07-31 14:48:26+0800, Zhangjin Wu wrote:
> > > > Hi, Thomas
> > > > 
> > > > > As we want to enable compiler warnings in the future these would be
> > > > > reported as unused functions.
> > > > > 
> > > > > If we need them in the future they are easy to recreate from their still
> > > > > existing siblings.
> > > > > 
> > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > > ---
> > > > >  tools/testing/selftests/nolibc/nolibc-test.c | 99 ----------------------------
> > > > >  1 file changed, 99 deletions(-)
> > > > > 
> > > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > > index 03b1d30f5507..53e2d448eded 100644
> > > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > > @@ -161,31 +161,6 @@ static void result(int llen, enum RESULT r)
> > > > >   * of failures, thus either 0 or 1.
> > > > >   */
> > > > >  
> > > > > -#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)
> > > > > -{
> > > > 
> > > > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > > > add them again next time.
> > > > 
> > > >     -static int expect_zr(int expr, int llen)
> > > >     +static __attribute__((unused))
> > > >     +int expect_zr(int expr, int llen)
> > > >      {
> > > 
> > > Personally I don't like having dead code lying around that needs to be
> > > maintained and skipped over while reading.
> > > It's not a given that we will need those helpers in the future at all.
> > >
> > 
> > It is reasonable in some degree from current status, especially for
> > these ones are newly added, but let us think about these scenes: when we
> > would drop or change some test cases in the future and the helpers may
> > would be not referenced by any test cases in a short time, and warnings
> > there, but some other cases may be added later to use them again ...
> 
> That doesn't seem very likely.
> Did it happen recently?
>

Yeah, it did happen, but I can not remember which one, a simple statistic
does show it may be likely:

    $ grep EXPECT_ -ur tools/testing/selftests/nolibc/nolibc-test.c | grep -v define | sed -e 's/.*\(EXPECT_[A-Z0-9]*\).*/\1/g' | sort | uniq -c | sort -k 1 -g -r
         55 EXPECT_EQ
         37 EXPECT_SYSER
         21 EXPECT_SYSZR
         11 EXPECT_SYSNE
          9 EXPECT_VFPRINTF
          4 EXPECT_PTRGT
          4 EXPECT_GE
          3 EXPECT_STRZR
          3 EXPECT_NE
          3 EXPECT_LT
          3 EXPECT_GT
          2 EXPECT_STRNZ
          2 EXPECT_STREQ
          2 EXPECT_PTRLT
          1 EXPECT_SYSER2
          1 EXPECT_SYSEQ
          1 EXPECT_PTRNZ
          1 EXPECT_PTRNE
          1 EXPECT_PTRER2
          1 EXPECT_PTRER
          1 EXPECT_PTREQ

7 helpers are only used by once, another 3 helpers are used twice, and
another 4 are only used by three times.

> > I'm ok to drop these ones, but another patch may be required to add
> > 'static __attribute__((unused))' for all of the currently used ones,
> > otherwise, there will be warnings randomly by a test case change or
> > drop.
> 
> Then we just drop the helper when we don't need it anymore.
> 
> I also dislike the __attribute__ spam to be honest.
>

Me too, but it does help sometimes ;-)

> > Or even further, is it possible to merge some of them to some more
> > generic helpers like the ones used from the selftest.h in your last RFC
> > patchset?
> 
> Something like this will indeed be part of the KTAP rework.
> But it's a change for another time.

Yes, this may be a better solution to such warnings.

Btw, just thought about gc-section, do we need to further remove dead code/data
in the binary? I don't think it is necessary for nolibc-test itself, but with
'-Wl,--gc-sections -Wl,--print-gc-sections' may be a good helper to show us
which ones should be dropped or which ones are wrongly declared as public?

Just found '-O3 + -Wl,--gc-section + -Wl,--print-gc-sections' did tell
us something as below:

    removing unused section '.text.nolibc_raise'
    removing unused section '.text.nolibc_memmove'
    removing unused section '.text.nolibc_abort'
    removing unused section '.text.nolibc_memcpy'
    removing unused section '.text.__stack_chk_init'
    removing unused section '.text.is_setting_valid'

These info may help us further add missing 'static' keyword or find
another method to to drop the wrongly used status of some functions from
the code side.

It is very easy to add the missing 'static' keyword for is_setting_valid(), but
for __stack_chk_init(), is it ok for us to convert it to 'static' and remove
the 'weak' attrbute and even the 'section' attribute? seems it is only used by
our _start_c() currently.

For the left ones, some are related to libgcc for divide by zero or the other
divide functions, which may be not possible to drop in code side, but for
memmove/memset, it is able to add -ffreestanding in our nolibc-test like -Wall
and only wrap the 'weak' attribute with '#if __STDC_HOSTED__ == 1', for the ARM
specific one, '#ifdef __ARM_EABI__'.

And even further, the '_start_c()' should be 'static' too, perhaps the above
issues are worth a new patchset, If you agree, will send a new patchset to fix
up them.

Thanks,
Zhangjin

> 
> Thomas

Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

[Thomas Weißschuh]

On 2023-07-31 19:02:26+0800, Zhangjin Wu wrote:
> Hi, Willy

Thomas here :-)

> > > > > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > > > > add them again next time.
> > > > > 
> > > > >     -static int expect_zr(int expr, int llen)
> > > > >     +static __attribute__((unused))
> > > > >     +int expect_zr(int expr, int llen)
> > > > >      {
> > > > 
> > > > Personally I don't like having dead code lying around that needs to be
> > > > maintained and skipped over while reading.
> > > > It's not a given that we will need those helpers in the future at all.
> > > >
> > > 
> > > It is reasonable in some degree from current status, especially for
> > > these ones are newly added, but let us think about these scenes: when we
> > > would drop or change some test cases in the future and the helpers may
> > > would be not referenced by any test cases in a short time, and warnings
> > > there, but some other cases may be added later to use them again ...
> > 
> > That doesn't seem very likely.
> > Did it happen recently?
> >
> 
> Yeah, it did happen, but I can not remember which one, a simple statistic
> does show it may be likely:

I can't find it.

>     $ grep EXPECT_ -ur tools/testing/selftests/nolibc/nolibc-test.c | grep -v define | sed -e 's/.*\(EXPECT_[A-Z0-9]*\).*/\1/g' | sort | uniq -c | sort -k 1 -g -r
>          55 EXPECT_EQ
>          37 EXPECT_SYSER
>          21 EXPECT_SYSZR
>          11 EXPECT_SYSNE
>           9 EXPECT_VFPRINTF
>           4 EXPECT_PTRGT
>           4 EXPECT_GE
>           3 EXPECT_STRZR
>           3 EXPECT_NE
>           3 EXPECT_LT
>           3 EXPECT_GT
>           2 EXPECT_STRNZ
>           2 EXPECT_STREQ
>           2 EXPECT_PTRLT
>           1 EXPECT_SYSER2
>           1 EXPECT_SYSEQ
>           1 EXPECT_PTRNZ
>           1 EXPECT_PTRNE
>           1 EXPECT_PTRER2
>           1 EXPECT_PTRER
>           1 EXPECT_PTREQ
> 
> 7 helpers are only used by once, another 3 helpers are used twice, and
> another 4 are only used by three times.

Why can't we just drop them when they are not used anymore?

> > > I'm ok to drop these ones, but another patch may be required to add
> > > 'static __attribute__((unused))' for all of the currently used ones,
> > > otherwise, there will be warnings randomly by a test case change or
> > > drop.
> > 
> > Then we just drop the helper when we don't need it anymore.
> > 
> > I also dislike the __attribute__ spam to be honest.
> >
> 
> Me too, but it does help sometimes ;-)
> 
> > > Or even further, is it possible to merge some of them to some more
> > > generic helpers like the ones used from the selftest.h in your last RFC
> > > patchset?
> > 
> > Something like this will indeed be part of the KTAP rework.
> > But it's a change for another time.
> 
> Yes, this may be a better solution to such warnings.
> 
> Btw, just thought about gc-section, do we need to further remove dead code/data
> in the binary? I don't think it is necessary for nolibc-test itself, but with
> '-Wl,--gc-sections -Wl,--print-gc-sections' may be a good helper to show us
> which ones should be dropped or which ones are wrongly declared as public?
> 
> Just found '-O3 + -Wl,--gc-section + -Wl,--print-gc-sections' did tell
> us something as below:
> 
>     removing unused section '.text.nolibc_raise'
>     removing unused section '.text.nolibc_memmove'
>     removing unused section '.text.nolibc_abort'
>     removing unused section '.text.nolibc_memcpy'
>     removing unused section '.text.__stack_chk_init'
>     removing unused section '.text.is_setting_valid'
> 
> These info may help us further add missing 'static' keyword or find
> another method to to drop the wrongly used status of some functions from
> the code side.
> 
> It is very easy to add the missing 'static' keyword for is_setting_valid(), but
> for __stack_chk_init(), is it ok for us to convert it to 'static' and remove
> the 'weak' attrbute and even the 'section' attribute? seems it is only used by
> our _start_c() currently.

Making is_setting_valid(), __stack_chk_init() seems indeed useful.
Also all the run_foo() test functions.

> For the left ones, some are related to libgcc for divide by zero or the other
> divide functions, which may be not possible to drop in code side, but for
> memmove/memset, it is able to add -ffreestanding in our nolibc-test like -Wall
> and only wrap the 'weak' attribute with '#if __STDC_HOSTED__ == 1', for the ARM
> specific one, '#ifdef __ARM_EABI__'.

That seems very excessive.

> And even further, the '_start_c()' should be 'static' too, perhaps the above
> issues are worth a new patchset, If you agree, will send a new patchset to fix
> up them.

_start_c(), too.

Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

[Willy Tarreau]

Hi guys,

On Mon, Jul 31, 2023 at 05:30:23PM +0200, Thomas Weißschuh wrote:

> > > > > > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > > > > > add them again next time.
> > > > > > 
> > > > > >     -static int expect_zr(int expr, int llen)
> > > > > >     +static __attribute__((unused))
> > > > > >     +int expect_zr(int expr, int llen)
> > > > > >      {
> > > > > 
> > > > > Personally I don't like having dead code lying around that needs to be
> > > > > maintained and skipped over while reading.
> > > > > It's not a given that we will need those helpers in the future at all.
> > > > >
> > > > 
> > > > It is reasonable in some degree from current status, especially for
> > > > these ones are newly added, but let us think about these scenes: when we
> > > > would drop or change some test cases in the future and the helpers may
> > > > would be not referenced by any test cases in a short time, and warnings
> > > > there, but some other cases may be added later to use them again ...
> > > 
> > > That doesn't seem very likely.
> > > Did it happen recently?
> > >
> > 
> > Yeah, it did happen, but I can not remember which one, a simple statistic
> > does show it may be likely:
> 
> I can't find it.
> 
> >     $ grep EXPECT_ -ur tools/testing/selftests/nolibc/nolibc-test.c | grep -v define | sed -e 's/.*\(EXPECT_[A-Z0-9]*\).*/\1/g' | sort | uniq -c | sort -k 1 -g -r
> >          55 EXPECT_EQ
> >          37 EXPECT_SYSER
> >          21 EXPECT_SYSZR
> >          11 EXPECT_SYSNE
> >           9 EXPECT_VFPRINTF
> >           4 EXPECT_PTRGT
> >           4 EXPECT_GE
> >           3 EXPECT_STRZR
> >           3 EXPECT_NE
> >           3 EXPECT_LT
> >           3 EXPECT_GT
> >           2 EXPECT_STRNZ
> >           2 EXPECT_STREQ
> >           2 EXPECT_PTRLT
> >           1 EXPECT_SYSER2
> >           1 EXPECT_SYSEQ
> >           1 EXPECT_PTRNZ
> >           1 EXPECT_PTRNE
> >           1 EXPECT_PTRER2
> >           1 EXPECT_PTRER
> >           1 EXPECT_PTREQ
> > 
> > 7 helpers are only used by once, another 3 helpers are used twice, and
> > another 4 are only used by three times.
> 
> Why can't we just drop them when they are not used anymore?

Actually we don't know if they're used or not given that the purpose of
the nolibc-test.c file is for it to be easy to add new tests, and the
collection of macros above serves this purpose. It's not just a series
of test but rather a small test framework. So the fact that right now
no single test uses some of them doesn't mean that someone else will
not have to reimplement them in two months.

However I share your concern that the file has become ugly over time.
I've recently been wondering why we wouldn't move all that to an external
include file. It could also encourage us to differentiate between the
macros used to only evaluate a result, and the tests themselves, as
we'd be certain that none of them could call a test function directly.

> > Btw, just thought about gc-section, do we need to further remove dead code/data
> > in the binary? I don't think it is necessary for nolibc-test itself, but with
> > '-Wl,--gc-sections -Wl,--print-gc-sections' may be a good helper to show us
> > which ones should be dropped or which ones are wrongly declared as public?
> > 
> > Just found '-O3 + -Wl,--gc-section + -Wl,--print-gc-sections' did tell
> > us something as below:
> > 
> >     removing unused section '.text.nolibc_raise'
> >     removing unused section '.text.nolibc_memmove'
> >     removing unused section '.text.nolibc_abort'
> >     removing unused section '.text.nolibc_memcpy'
> >     removing unused section '.text.__stack_chk_init'
> >     removing unused section '.text.is_setting_valid'

Just a note Zhangjin, it would really help if you wouldn't mix different
topics in mails. It's easy enough to start a separate thread since it's
a completely separate one here.

> > These info may help us further add missing 'static' keyword or find
> > another method to to drop the wrongly used status of some functions from
> > the code side.
> > 
> > It is very easy to add the missing 'static' keyword for is_setting_valid(), but
> > for __stack_chk_init(), is it ok for us to convert it to 'static' and remove
> > the 'weak' attrbute and even the 'section' attribute? seems it is only used by
> > our _start_c() currently.
> 
> Making is_setting_valid(), __stack_chk_init() seems indeed useful.
> Also all the run_foo() test functions.

Most of them could theoretically be turned to static. *But* it causes a
problem which is that it will multiply their occurrences in multi-unit
programs, and that's in part why we've started to use weak instead. Also
if you run through gdb and want to mark a break point, you won't have the
symbol when it's static, and the code will appear at multiple locations,
which is really painful. I'd instead really prefer to avoid static when
we don't strictly want to inline the code, and prefer weak when possible
because we know many of them will be dropped at link time (and that's
the exact purpose).

Thanks,
Willy

Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

[Thomas Weißschuh]

On 2023-07-31 18:53:34+0200, Willy Tarreau wrote:
> Hi guys,
> 
> On Mon, Jul 31, 2023 at 05:30:23PM +0200, Thomas Weißschuh wrote:
> 
> > > > > > > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > > > > > > add them again next time.
> > > > > > > 
> > > > > > >     -static int expect_zr(int expr, int llen)
> > > > > > >     +static __attribute__((unused))
> > > > > > >     +int expect_zr(int expr, int llen)
> > > > > > >      {
> > > > > > 
> > > > > > Personally I don't like having dead code lying around that needs to be
> > > > > > maintained and skipped over while reading.
> > > > > > It's not a given that we will need those helpers in the future at all.
> > > > > >
> > > > > 
> > > > > It is reasonable in some degree from current status, especially for
> > > > > these ones are newly added, but let us think about these scenes: when we
> > > > > would drop or change some test cases in the future and the helpers may
> > > > > would be not referenced by any test cases in a short time, and warnings
> > > > > there, but some other cases may be added later to use them again ...
> > > > 
> > > > That doesn't seem very likely.
> > > > Did it happen recently?
> > > >
> > > 
> > > Yeah, it did happen, but I can not remember which one, a simple statistic
> > > does show it may be likely:
> > 
> > I can't find it.
> > 
> > >     $ grep EXPECT_ -ur tools/testing/selftests/nolibc/nolibc-test.c | grep -v define | sed -e 's/.*\(EXPECT_[A-Z0-9]*\).*/\1/g' | sort | uniq -c | sort -k 1 -g -r
> > >          55 EXPECT_EQ
> > >          37 EXPECT_SYSER
> > >          21 EXPECT_SYSZR
> > >          11 EXPECT_SYSNE
> > >           9 EXPECT_VFPRINTF
> > >           4 EXPECT_PTRGT
> > >           4 EXPECT_GE
> > >           3 EXPECT_STRZR
> > >           3 EXPECT_NE
> > >           3 EXPECT_LT
> > >           3 EXPECT_GT
> > >           2 EXPECT_STRNZ
> > >           2 EXPECT_STREQ
> > >           2 EXPECT_PTRLT
> > >           1 EXPECT_SYSER2
> > >           1 EXPECT_SYSEQ
> > >           1 EXPECT_PTRNZ
> > >           1 EXPECT_PTRNE
> > >           1 EXPECT_PTRER2
> > >           1 EXPECT_PTRER
> > >           1 EXPECT_PTREQ
> > > 
> > > 7 helpers are only used by once, another 3 helpers are used twice, and
> > > another 4 are only used by three times.
> > 
> > Why can't we just drop them when they are not used anymore?
> 
> Actually we don't know if they're used or not given that the purpose of
> the nolibc-test.c file is for it to be easy to add new tests, and the
> collection of macros above serves this purpose. It's not just a series
> of test but rather a small test framework. So the fact that right now
> no single test uses some of them doesn't mean that someone else will
> not have to reimplement them in two months.

Reimplementing them would mean to copy one of the sibling test macros
and changing the name and the condition operator in one place.
I regarded that as an acceptable effort instead of having to work around
the warnings.

The warnings themselves I see as useful as they can give developers
early feedback on their code. They would have avoided some of the issues
with the recent pipe() series.

Do you have a preferred solution for the overall situation?

> However I share your concern that the file has become ugly over time.
> I've recently been wondering why we wouldn't move all that to an external
> include file. It could also encourage us to differentiate between the
> macros used to only evaluate a result, and the tests themselves, as
> we'd be certain that none of them could call a test function directly.
> 
> > > Btw, just thought about gc-section, do we need to further remove dead code/data
> > > in the binary? I don't think it is necessary for nolibc-test itself, but with
> > > '-Wl,--gc-sections -Wl,--print-gc-sections' may be a good helper to show us
> > > which ones should be dropped or which ones are wrongly declared as public?
> > > 
> > > Just found '-O3 + -Wl,--gc-section + -Wl,--print-gc-sections' did tell
> > > us something as below:
> > > 
> > >     removing unused section '.text.nolibc_raise'
> > >     removing unused section '.text.nolibc_memmove'
> > >     removing unused section '.text.nolibc_abort'
> > >     removing unused section '.text.nolibc_memcpy'
> > >     removing unused section '.text.__stack_chk_init'
> > >     removing unused section '.text.is_setting_valid'
> 
> Just a note Zhangjin, it would really help if you wouldn't mix different
> topics in mails. It's easy enough to start a separate thread since it's
> a completely separate one here.
> 
> > > These info may help us further add missing 'static' keyword or find
> > > another method to to drop the wrongly used status of some functions from
> > > the code side.
> > > 
> > > It is very easy to add the missing 'static' keyword for is_setting_valid(), but
> > > for __stack_chk_init(), is it ok for us to convert it to 'static' and remove
> > > the 'weak' attrbute and even the 'section' attribute? seems it is only used by
> > > our _start_c() currently.
> > 
> > Making is_setting_valid(), __stack_chk_init() seems indeed useful.
> > Also all the run_foo() test functions.
> 
> Most of them could theoretically be turned to static. *But* it causes a
> problem which is that it will multiply their occurrences in multi-unit
> programs, and that's in part why we've started to use weak instead. Also
> if you run through gdb and want to mark a break point, you won't have the
> symbol when it's static, and the code will appear at multiple locations,
> which is really painful. I'd instead really prefer to avoid static when
> we don't strictly want to inline the code, and prefer weak when possible
> because we know many of them will be dropped at link time (and that's
> the exact purpose).

Thanks for the clarification. I forgot about that completely!

The stuff from nolibc-test.c itself (run_foo() and is_settings_valid())
should still be done.

Thomas

Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers

[Willy Tarreau]

On Mon, Jul 31, 2023 at 08:36:05PM +0200, Thomas Weißschuh wrote:
> > > > 7 helpers are only used by once, another 3 helpers are used twice, and
> > > > another 4 are only used by three times.
> > > 
> > > Why can't we just drop them when they are not used anymore?
> > 
> > Actually we don't know if they're used or not given that the purpose of
> > the nolibc-test.c file is for it to be easy to add new tests, and the
> > collection of macros above serves this purpose. It's not just a series
> > of test but rather a small test framework. So the fact that right now
> > no single test uses some of them doesn't mean that someone else will
> > not have to reimplement them in two months.
> 
> Reimplementing them would mean to copy one of the sibling test macros
> and changing the name and the condition operator in one place.

Yes but that's the difference between us providing a basis for others
to easily contribute tests and just saying "you can implement you test
in this directory". Literally adding just one line is simple and
encouraging enough.

> I regarded that as an acceptable effort instead of having to work around
> the warnings.

Warnings must always be addressed, and there are tools for this. One
of them is the inline keyword which makes them go away. It's fine as
long as we expect that functions are worth inlining (size, debuggability).
A second one is the "unused" attribute. I know you said you don't find
it clean but it's the official clean way to shut some specific warnings,
by passing meta-information to the compiler about the intent for certain
things. We can very well have a define saying that __maybe_unused maps
to __attribute__((unused)) as done everywhere else, but it's in the end
it remains the regular way to do it. Finally the third method consists
in removing "static" so that the compiler doesn't know if we're going
to use them elsewhere. My personal preference goes with the unused
attribute because it's well aligned with the spirit of a test framework
providing tools to those who need them.

> The warnings themselves I see as useful as they can give developers
> early feedback on their code. They would have avoided some of the issues
> with the recent pipe() series.

I totally agree with warnings. I compile my code with -W -Wall -Wextra
for this exact reason. Also inside a lib test we must try to trigger
more of them so as to be in the worst user situation, because if users
detect them first, that's painful.

> Do you have a preferred solution for the overall situation?

I'd rather put unused everywhere (possibly with a define to make it
more readable). And if the code is too large and too ugly (too many
utility functions) really moving it into a .h would significantly
help I think.

> > > > It is very easy to add the missing 'static' keyword for is_setting_valid(), but
> > > > for __stack_chk_init(), is it ok for us to convert it to 'static' and remove
> > > > the 'weak' attrbute and even the 'section' attribute? seems it is only used by
> > > > our _start_c() currently.
> > > 
> > > Making is_setting_valid(), __stack_chk_init() seems indeed useful.
> > > Also all the run_foo() test functions.
> > 
> > Most of them could theoretically be turned to static. *But* it causes a
> > problem which is that it will multiply their occurrences in multi-unit
> > programs, and that's in part why we've started to use weak instead. Also
> > if you run through gdb and want to mark a break point, you won't have the
> > symbol when it's static, and the code will appear at multiple locations,
> > which is really painful. I'd instead really prefer to avoid static when
> > we don't strictly want to inline the code, and prefer weak when possible
> > because we know many of them will be dropped at link time (and that's
> > the exact purpose).
> 
> Thanks for the clarification. I forgot about that completely!
> 
> The stuff from nolibc-test.c itself (run_foo() and is_settings_valid())
> should still be done.

Yes, likely. Nolibc-test should be done just like users expect to use
nolibc, and nolibc should be the most flexible possible.

Cheers,
Willy