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 <[email protected]>
---
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/[email protected]
---
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 <[email protected]>
If read() fails and returns -1 buf would be accessed out of bounds.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
When warning about unused functions these would be reported by we want
to keep them for future use.
Suggested-by: Zhangjin Wu <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Suggested-by: Willy Tarreau <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
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 <[email protected]>
---
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
Nobody needs it, get rid of it.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
These warnings will be enabled later so avoid triggering them.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
This allows the compiler to generate warnings if they go unused.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
getauxval() returns an unsigned long but the overall type of the ternary
operator needs to be signed.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
It will help the developers to avoid cruft and detect some bugs.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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
On Tue, Aug 01, 2023 at 07:30:13AM +0200, Thomas Wei?schuh wrote:
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 1555759bb164..53a3773c7790 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -80,7 +80,7 @@ char *itoa(int i)
> /* returns the error name (e.g. "ENOENT") for common errors, "SUCCESS" for 0,
> * or the decimal value for less common ones.
> */
> -const char *errorname(int err)
> +static const char *errorname(int err)
> {
> switch (err) {
> case 0: return "SUCCESS";
OK for this one, even desired for such a use case.
> @@ -593,7 +593,7 @@ int expect_strne(const char *expr, int llen, const char *cmp)
> #define CASE_TEST(name) \
> case __LINE__: llen += printf("%d %s", test, #name);
>
> -int run_startup(int min, int max)
> +static int run_startup(int min, int max)
> {
> int test;
> int ret = 0;
> @@ -640,7 +640,7 @@ int run_startup(int min, int max)
>
>
> /* used by some syscall tests below */
> -int test_getdents64(const char *dir)
> +static int test_getdents64(const char *dir)
> {
> char buffer[4096];
> int fd, ret;
> @@ -734,7 +734,7 @@ static int test_stat_timestamps(void)
> return 0;
> }
>
> -int test_mmap_munmap(void)
> +static int test_mmap_munmap(void)
> {
> int ret, fd, i;
> void *mem;
> @@ -796,7 +796,7 @@ int test_mmap_munmap(void)
> /* Run syscall tests between IDs <min> and <max>.
> * Return 0 on success, non-zero on failure.
> */
> -int run_syscall(int min, int max)
> +static int run_syscall(int min, int max)
> {
> struct timeval tv;
> struct timezone tz;
> @@ -907,7 +907,7 @@ int run_syscall(int min, int max)
> return ret;
> }
>
> -int run_stdlib(int min, int max)
> +static int run_stdlib(int min, int max)
> {
> int test;
> int ret = 0;
> @@ -1141,7 +1141,7 @@ static int run_protection(int min, int max)
> }
>
> /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */
> -int prepare(void)
> +static int prepare(void)
> {
> struct stat stat_buf;
>
> @@ -1208,7 +1208,7 @@ static const struct test test_names[] = {
> { 0 }
> };
For these ones it will prevent gcc from putting breakpoints there, which
is counter-productive.
> -int is_setting_valid(char *test)
> +static int is_setting_valid(char *test)
> {
> int idx, len, test_len, valid = 0;
> char delimiter;
OK for this one.
Willy
On 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 <[email protected]>
> ---
> tools/testing/selftests/nolibc/nolibc-test.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 82714051c72f..a334f8450a34 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -1031,6 +1031,12 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm
> lseek(fd, 0, SEEK_SET);
>
> r = read(fd, buf, sizeof(buf) - 1);
> + if (r == -1) {
> + llen += printf(" read() = %s", errorname(errno));
> + result(llen, FAIL);
> + return 1;
> + }
> +
> buf[r] = '\0';
In fact given the nature of this file (test if we properly implemented
our syscalls), I think that a more conservative approach is deserved
because if we messed up on read() we can have anything on return and we
don't want to trust that. As such I would suggest that we declare r as
ssize_t and verify that it's neither negative nor larger than
sizeof(buf)-1, which becomes:
if ((size_t)r >= sizeof(buf)) {
... fail ...
}
You'll also have to turn w to ssize_t then due to the test later BTW.
Willy
On 2023-08-01 08:52:19+0200, Willy Tarreau wrote:
> On Tue, Aug 01, 2023 at 07:30:13AM +0200, Thomas Weißschuh wrote:
> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index 1555759bb164..53a3773c7790 100644
> > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> [..]
> > /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */
> > -int prepare(void)
> > +static int prepare(void)
> > {
> > struct stat stat_buf;
> >
> > @@ -1208,7 +1208,7 @@ static const struct test test_names[] = {
> > { 0 }
> > };
>
> For these ones it will prevent gcc from putting breakpoints there, which
> is counter-productive.
Indeed.
An alternative would be to add -g to CFLAGS (and remove -s from LDFLAGS).
This way we get full debugability including breakpoints for everything.
I didn't find the reasoning for -s in LDFLAGS.
Thomas
On 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 <[email protected]>
> > ---
> > 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.
On Tue, Aug 01, 2023 at 09:34:18AM +0200, Thomas Wei?schuh wrote:
> On 2023-08-01 08:52:19+0200, Willy Tarreau wrote:
> > On Tue, Aug 01, 2023 at 07:30:13AM +0200, Thomas Wei?schuh wrote:
> > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > index 1555759bb164..53a3773c7790 100644
> > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
>
> > [..]
>
> > > /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */
> > > -int prepare(void)
> > > +static int prepare(void)
> > > {
> > > struct stat stat_buf;
> > >
> > > @@ -1208,7 +1208,7 @@ static const struct test test_names[] = {
> > > { 0 }
> > > };
> >
> > For these ones it will prevent gcc from putting breakpoints there, which
> > is counter-productive.
>
> Indeed.
>
> An alternative would be to add -g to CFLAGS (and remove -s from LDFLAGS).
> This way we get full debugability including breakpoints for everything.
It wouldn't change much because while it would allow the debugger to know
where the function was possibly inlined, it's still not very convenient:
you believe you're in a function but in fact you're in the caller. It
really depends what you're debugging but here I don't see all that as
providing a value, at least it brings more annoyance and little to no
gain IMHO.
> I didn't find the reasoning for -s in LDFLAGS.
It's historic, because normally when you want small binaries you strip
them, and the command line was reused as-is, but I agree that we could
get rid of it!
Willy
On 2023-08-01 10:13:07+0200, Willy Tarreau wrote:
> On Tue, Aug 01, 2023 at 09:34:18AM +0200, Thomas Weißschuh wrote:
> > On 2023-08-01 08:52:19+0200, Willy Tarreau wrote:
> > > On Tue, Aug 01, 2023 at 07:30:13AM +0200, Thomas Weißschuh wrote:
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 1555759bb164..53a3773c7790 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> >
> > > [..]
> >
> > > > /* prepare what needs to be prepared for pid 1 (stdio, /dev, /proc, etc) */
> > > > -int prepare(void)
> > > > +static int prepare(void)
> > > > {
> > > > struct stat stat_buf;
> > > >
> > > > @@ -1208,7 +1208,7 @@ static const struct test test_names[] = {
> > > > { 0 }
> > > > };
> > >
> > > For these ones it will prevent gcc from putting breakpoints there, which
> > > is counter-productive.
> >
> > Indeed.
> >
> > An alternative would be to add -g to CFLAGS (and remove -s from LDFLAGS).
> > This way we get full debugability including breakpoints for everything.
>
> It wouldn't change much because while it would allow the debugger to know
> where the function was possibly inlined, it's still not very convenient:
> you believe you're in a function but in fact you're in the caller. It
> really depends what you're debugging but here I don't see all that as
> providing a value, at least it brings more annoyance and little to no
> gain IMHO.
Even if it doesn't work 100% properly it wouldn't it still be a superset
of the previous functionality?
And we don't have to manually keep track of which ones should be static
and which shouldn't (See this discussion).
Would it be better with -ggdb?
If you are still not conviced I'll drop the argument here :-)
(And the changes in the next revision)
> > I didn't find the reasoning for -s in LDFLAGS.
>
> It's historic, because normally when you want small binaries you strip
> them, and the command line was reused as-is, but I agree that we could
> get rid of it!
I'll remove it. It was annoying to figure out why my "-g" CFLAG didn't
work at all.
Thomas
On Tue, Aug 01, 2023 at 10:50:05AM +0200, Thomas Wei?schuh wrote:
> > > An alternative would be to add -g to CFLAGS (and remove -s from LDFLAGS).
> > > This way we get full debugability including breakpoints for everything.
> >
> > It wouldn't change much because while it would allow the debugger to know
> > where the function was possibly inlined, it's still not very convenient:
> > you believe you're in a function but in fact you're in the caller. It
> > really depends what you're debugging but here I don't see all that as
> > providing a value, at least it brings more annoyance and little to no
> > gain IMHO.
>
> Even if it doesn't work 100% properly it wouldn't it still be a superset
> of the previous functionality?
No, we need to be able to disassemble this to understand what was done
to the code we believe is being tested. All of us have been dealing with
this already, and making that code less mappable from asm to C is quite
annoying.
> And we don't have to manually keep track of which ones should be static
> and which shouldn't (See this discussion).
We should not have to be concerned about this, because it's out of the
scope of what this "program" is used for. If we're wondering too much,
we're wasting our time on the wrong topic. So we have to find a reasonable
rule. One that sounds fine to me is to say:
- all that's part of the framework to help with testing (i.e. the expect
functions, errorname() etc) could be static because we don't really
care about them (at least we won't be placing breakpoints there). They
may be marked inline or unused and we can be done with them.
- user code and the calling stack from main should be easily traceable
using gdb and objdump -dr so that when you start with a new arch and
it breaks early (as happens by definition when syscalls or crt code
don't all work well) then it's possible to accurately trace it without
having to worry too much about what was transformed how.
> Would it be better with -ggdb?
It doesn't change. The thing is: by saying "static" you tell the
compiler "I promise it cannot be used anywhere else, do what you want
with it", and it can trivially decide to inline all or part of it, or
change its number of arguments or whatever as it sees fit because no
other code part relies on that function. And when you're trying to
disassemble your test_mmap_munmap() and can't find it and can only
infer its inlined presence in run_syscall() by seeing a value in a
register that vaguely reminds you about __NR_mmap, it's clearly much
less easy.
> If you are still not conviced I'll drop the argument here :-)
> (And the changes in the next revision)
No problem, it's fine to discuss the current situation. I've just
noticed a number of static on some test functions that would deserve
being removed precisely for the reasons above. But that justifies the
need for some doc about all this.
> > > I didn't find the reasoning for -s in LDFLAGS.
> >
> > It's historic, because normally when you want small binaries you strip
> > them, and the command line was reused as-is, but I agree that we could
> > get rid of it!
>
> I'll remove it. It was annoying to figure out why my "-g" CFLAG didn't
> work at all.
Yes I can definitely understand!
Thanks,
Willy
Hi Thomas,
On Tue, Aug 01, 2023 at 07:30:07AM +0200, Thomas Wei?schuh wrote:
> To help the developers to avoid mistakes and keep the code smaller let's
> enable compiler warnings.
(...)
OK, that's overall OK to me, and I noted thta you'll likely update
6 and 9. Just let me know how you prefer to proceed, I hope to send
the PR to Shuah this week-end so I want to be sure we avoid a last
minute rush and the risks of breakage that comes with it.
Thanks!
Willy
On 2023-08-02 22:22:43+0200, Willy Tarreau wrote:
> Hi Thomas,
>
> On Tue, Aug 01, 2023 at 07:30:07AM +0200, Thomas Weißschuh wrote:
> > To help the developers to avoid mistakes and keep the code smaller let's
> > enable compiler warnings.
> (...)
>
> OK, that's overall OK to me, and I noted thta you'll likely update
> 6 and 9. Just let me know how you prefer to proceed, I hope to send
> the PR to Shuah this week-end so I want to be sure we avoid a last
> minute rush and the risks of breakage that comes with it.
I'm not yet sure when I can rework those patches.
Could you already pick up those you are fine with?
(And also leave out the final one to not have spurious warnings)
Thomas
On Wed, Aug 02, 2023 at 11:10:17PM +0200, Thomas Wei?schuh wrote:
> On 2023-08-02 22:22:43+0200, Willy Tarreau wrote:
> > Hi Thomas,
> >
> > On Tue, Aug 01, 2023 at 07:30:07AM +0200, Thomas Wei?schuh wrote:
> > > To help the developers to avoid mistakes and keep the code smaller let's
> > > enable compiler warnings.
> > (...)
> >
> > OK, that's overall OK to me, and I noted thta you'll likely update
> > 6 and 9. Just let me know how you prefer to proceed, I hope to send
> > the PR to Shuah this week-end so I want to be sure we avoid a last
> > minute rush and the risks of breakage that comes with it.
>
> I'm not yet sure when I can rework those patches.
No problem.
> Could you already pick up those you are fine with?
> (And also leave out the final one to not have spurious warnings)
OK, I'll stick to this for now, thank you!
Willy