From: Tony Ambardar tony.ambardar@gmail.com
Hello all,
This is part 2 of a series of fixes for libc-related issues encountered building for musl-based systems. The series has been tested with the kernel-patches/bpf CI and locally using mips64el-gcc/musl-libc and QEMU with an OpenWrt rootfs.
The patches cover a few areas of portability issues:
- improper libc usage (strtok_r(), reserved identifiers) - gcc compile errors (include header ordering, sequence-point errors) - POSIX vs GNU basename() - missing GNU extensions (<execinfo.h>, C++ <stdbool.h>) - Y2038 and setsockopt() / SO_TIMESTAMPNS_NEW
Feedback and suggestions are appreciated!
Thanks, Tony
Tony Ambardar (8): selftests/bpf: Use portable POSIX basename() selftests/bpf: Fix arg parsing in veristat, test_progs selftests/bpf: Fix error compiling test_lru_map.c selftests/bpf: Fix C++ compile error from missing _Bool type selftests/bpf: Fix order-of-include compile errors in lwt_reroute.c selftests/bpf: Fix compile if backtrace support missing in libc selftests/bpf: Fix using stdout, stderr as struct field names selftests/bpf: Fix error compiling tc_redirect.c with musl libc
.../selftests/bpf/prog_tests/lwt_helpers.h | 3 +- .../selftests/bpf/prog_tests/reg_bounds.c | 2 +- .../selftests/bpf/prog_tests/tc_redirect.c | 12 +-- tools/testing/selftests/bpf/test_cpp.cpp | 4 + tools/testing/selftests/bpf/test_lru_map.c | 3 +- tools/testing/selftests/bpf/test_progs.c | 75 ++++++++++--------- tools/testing/selftests/bpf/test_progs.h | 8 +- tools/testing/selftests/bpf/testing_helpers.c | 2 +- tools/testing/selftests/bpf/veristat.c | 12 +-- tools/testing/selftests/bpf/xskxceiver.c | 1 + 10 files changed, 68 insertions(+), 54 deletions(-)
From: Tony Ambardar tony.ambardar@gmail.com
Use the POSIX version of basename() to allow compilation against non-gnu libc (e.g. musl). Include <libgen.h> ahead of <string.h> to enable using functions from the latter while preferring POSIX over GNU basename().
In veristat.c, rely on strdupa() to avoid basename() altering the passed "const char" argument. This is not needed in xskxceiver.c since the arg is mutable and the program exits immediately after usage.
Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/veristat.c | 8 +++++--- tools/testing/selftests/bpf/xskxceiver.c | 1 + 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index b2854238d4a0..11ec1190d582 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -2,6 +2,7 @@ /* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */ #define _GNU_SOURCE #include <argp.h> +#include <libgen.h> #include <string.h> #include <stdlib.h> #include <sched.h> @@ -988,8 +989,8 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch
static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog) { + const char *base_filename = basename(strdupa(filename)); const char *prog_name = bpf_program__name(prog); - const char *base_filename = basename(filename); char *buf; int buf_sz, log_level; struct verif_stats *stats; @@ -1056,13 +1057,14 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
static int process_obj(const char *filename) { + const char *base_filename = basename(strdupa(filename)); struct bpf_object *obj = NULL, *tobj; struct bpf_program *prog, *tprog, *lprog; libbpf_print_fn_t old_libbpf_print_fn; LIBBPF_OPTS(bpf_object_open_opts, opts); int err = 0, prog_cnt = 0;
- if (!should_process_file_prog(basename(filename), NULL)) { + if (!should_process_file_prog(base_filename, NULL)) { if (env.verbose) printf("Skipping '%s' due to filters...\n", filename); env.files_skipped++; @@ -1076,7 +1078,7 @@ static int process_obj(const char *filename) }
if (!env.quiet && env.out_fmt == RESFMT_TABLE) - printf("Processing '%s'...\n", basename(filename)); + printf("Processing '%s'...\n", base_filename);
old_libbpf_print_fn = libbpf_set_print(libbpf_print_fn); obj = bpf_object__open_file(filename, &opts); diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c index 8144fd145237..92af633faea8 100644 --- a/tools/testing/selftests/bpf/xskxceiver.c +++ b/tools/testing/selftests/bpf/xskxceiver.c @@ -90,6 +90,7 @@ #include <signal.h> #include <stdio.h> #include <stdlib.h> +#include <libgen.h> #include <string.h> #include <stddef.h> #include <sys/mman.h>
From: Tony Ambardar tony.ambardar@gmail.com
Current code parses arguments with strtok_r() using a construct like
char *state = NULL; while ((next = strtok_r(state ? NULL : input, ",", &state))) { ... }
where logic assumes the 'state' var can distinguish between first and subsequent strtok_r() calls, and adjusts parameters accordingly. However, 'state' is strictly internal context for strtok_r() and no such assumptions are supported in the man page. Moreover, the exact behaviour of 'state' depends on the libc implementation, making the above code fragile.
Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang, with the above code in an infinite loop.
Similarly, we see strange behaviour running 'veristat' on mips64el/musl:
$ ./veristat -e file,prog,verdict,insns -C two-ok add-failure Can't specify more than 9 stats
Rewrite code using a 'for' loop without logic dependent on var 'state', the same approach already used in cgroup_helpers.c.
Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests") Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat") Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/testing_helpers.c | 2 +- tools/testing/selftests/bpf/veristat.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c index ac7c66f4fc7b..2a73b72feb18 100644 --- a/tools/testing/selftests/bpf/testing_helpers.c +++ b/tools/testing/selftests/bpf/testing_helpers.c @@ -227,7 +227,7 @@ int parse_test_list(const char *s, if (!input) return -ENOMEM;
- while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) { + for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) { err = insert_test(set, test_spec, is_glob_pattern); if (err) break; diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 11ec1190d582..6808679827ac 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -791,7 +791,7 @@ static int parse_stats(const char *stats_str, struct stat_specs *specs) if (!input) return -ENOMEM;
- while ((next = strtok_r(state ? NULL : input, ",", &state))) { + for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) { err = parse_stat(next, specs); if (err) { free(input); @@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs, *stat_cntp += 1; }
- while ((next = strtok_r(state ? NULL : input, ",\n", &state))) { + for (next = strtok_r(input, ",\n", &state); next; next = strtok_r(NULL, ",\n", &state)) { if (header) { /* for the first line, set up spec stats */ err = parse_stat(next, specs);
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Current code parses arguments with strtok_r() using a construct like
char *state = NULL; while ((next = strtok_r(state ? NULL : input, ",", &state))) { ... }where logic assumes the 'state' var can distinguish between first and subsequent strtok_r() calls, and adjusts parameters accordingly. However, 'state' is strictly internal context for strtok_r() and no such assumptions are supported in the man page. Moreover, the exact behaviour of 'state' depends on the libc implementation, making the above code fragile.
Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang, with the above code in an infinite loop.
Similarly, we see strange behaviour running 'veristat' on mips64el/musl:
$ ./veristat -e file,prog,verdict,insns -C two-ok add-failure Can't specify more than 9 statsRewrite code using a 'for' loop without logic dependent on var 'state', the same approach already used in cgroup_helpers.c.
Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests") Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat") Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com
tools/testing/selftests/bpf/testing_helpers.c | 2 +- tools/testing/selftests/bpf/veristat.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c index ac7c66f4fc7b..2a73b72feb18 100644 --- a/tools/testing/selftests/bpf/testing_helpers.c +++ b/tools/testing/selftests/bpf/testing_helpers.c @@ -227,7 +227,7 @@ int parse_test_list(const char *s, if (!input) return -ENOMEM;
while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) {
oh, this is so long and verbose, let's just add a counter and use that to determine whether to pass NULL or input, ok?
err = insert_test(set, test_spec, is_glob_pattern); if (err) break;diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 11ec1190d582..6808679827ac 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -791,7 +791,7 @@ static int parse_stats(const char *stats_str, struct stat_specs *specs) if (!input) return -ENOMEM;
while ((next = strtok_r(state ? NULL : input, ",", &state))) {
for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) {
ditto, let's not duplicate strtok_r() calls
err = parse_stat(next, specs); if (err) { free(input);@@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs, *stat_cntp += 1; }
while ((next = strtok_r(state ? NULL : input, ",\n", &state))) {
for (next = strtok_r(input, ",\n", &state); next; next = strtok_r(NULL, ",\n", &state)) { if (header) { /* for the first line, set up spec stats */ err = parse_stat(next, specs);-- 2.34.1
On Thu, Jul 25, 2024 at 01:09:24PM -0700, Andrii Nakryiko wrote:
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
...
--- a/tools/testing/selftests/bpf/testing_helpers.c +++ b/tools/testing/selftests/bpf/testing_helpers.c @@ -227,7 +227,7 @@ int parse_test_list(const char *s, if (!input) return -ENOMEM;
while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) {
for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) {oh, this is so long and verbose, let's just add a counter and use that to determine whether to pass NULL or input, ok?
...
while ((next = strtok_r(state ? NULL : input, ",", &state))) {
for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) {ditto, let's not duplicate strtok_r() calls
Sounds good. I'll update for v2 and thanks for the suggestion.
...
From: Tony Ambardar tony.ambardar@gmail.com
Although the post-increment in macro 'CPU_SET(next++, &cpuset)' seems safe, the sequencing can raise compile errors, so move the increment outside the macro. This avoids an error seen using gcc 12.3.0 for mips64el/musl-libc:
In file included from test_lru_map.c:11: test_lru_map.c: In function 'sched_next_online': test_lru_map.c:129:29: error: operation on 'next' may be undefined [-Werror=sequence-point] 129 | CPU_SET(next++, &cpuset); | ^ cc1: all warnings being treated as errors
Fixes: 3fbfadce6012 ("bpf: Fix test_lru_sanity5() in test_lru_map.c") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/test_lru_map.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_lru_map.c b/tools/testing/selftests/bpf/test_lru_map.c index 4d0650cfb5cd..fda7589c5023 100644 --- a/tools/testing/selftests/bpf/test_lru_map.c +++ b/tools/testing/selftests/bpf/test_lru_map.c @@ -126,7 +126,8 @@ static int sched_next_online(int pid, int *next_to_try)
while (next < nr_cpus) { CPU_ZERO(&cpuset); - CPU_SET(next++, &cpuset); + CPU_SET(next, &cpuset); + next++; if (!sched_setaffinity(pid, sizeof(cpuset), &cpuset)) { ret = 0; break;
From: Tony Ambardar tony.ambardar@gmail.com
While building, bpftool makes a skeleton from test_core_extern.c, which itself includes <stdbool.h> and uses the 'bool' type. However, the skeleton test_core_extern.skel.h generated *does not* include <stdbool.h> or use the 'bool' type, instead using the C-only '_Bool' type. Compiling test_cpp.cpp with g++ 12.3 for mips64el/musl-libc then fails with error:
In file included from test_cpp.cpp:9: test_core_extern.skel.h:45:17: error: '_Bool' does not name a type 45 | _Bool CONFIG_BOOL; | ^~~~~
This was likely missed previously because glibc uses a GNU extension for <stdbool.h> with C++ (#define _Bool bool), not supported by musl libc.
Normally, a C fragment would include <stdbool.h> and use the 'bool' type, and thus cleanly work after import by C++. The ideal fix would be for 'bpftool gen skeleton' to output the correct type/include supporting C++, but in the meantime add a conditional define as above.
Fixes: 7c8dce4b1661 ("bpftool: Make skeleton C code compilable with C++ compiler") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/test_cpp.cpp | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_cpp.cpp b/tools/testing/selftests/bpf/test_cpp.cpp index dde0bb16e782..abc2a56ab261 100644 --- a/tools/testing/selftests/bpf/test_cpp.cpp +++ b/tools/testing/selftests/bpf/test_cpp.cpp @@ -6,6 +6,10 @@ #include <bpf/libbpf.h> #include <bpf/bpf.h> #include <bpf/btf.h> + +#ifndef _Bool +#define _Bool bool +#endif #include "test_core_extern.skel.h" #include "struct_ops_module.skel.h"
From: Tony Ambardar tony.ambardar@gmail.com
Fix redefinition errors seen compiling lwt_reroute.c for mips64el/musl-libc by adjusting the order of includes in lwt_helpers.h. The ordering required is: <net/if.h> --> <arpa/inet.h> (from "test_progs.h") --> <linux/icmp.h>.
Because of the complexity and large number of includes, ordering appears to be fragile however. Previously, with "test_progs.h" at the end of this sequence, compiling with GCC 12.3 for mips64el/musl-libc yields errors:
In file included from .../include/arpa/inet.h:9, from ./test_progs.h:18, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: .../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr' 23 | struct in6_addr { | ^~~~~~~~ In file included from .../include/linux/icmp.h:24, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9: .../include/linux/in6.h:33:8: note: originally defined here 33 | struct in6_addr { | ^~~~~~~~ .../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6' 34 | struct sockaddr_in6 { | ^~~~~~~~~~~~ .../include/linux/in6.h:50:8: note: originally defined here 50 | struct sockaddr_in6 { | ^~~~~~~~~~~~ .../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq' 42 | struct ipv6_mreq { | ^~~~~~~~~ .../include/linux/in6.h:60:8: note: originally defined here 60 | struct ipv6_mreq { | ^~~~~~~~~
Similarly, with "test_progs.h" at the beginning of this sequence, compiling with GCC 12.3 for x86_64 using glibc would fail like this:
In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:83:9: error: redeclaration of enumerator ‘IFF_UP’ 83 | IFF_UP = 1<<0, /* sysfs */ | ^~~~~~ /usr/include/net/if.h:44:5: note: previous definition of ‘IFF_UP’ with type ‘enum <anonymous>’ 44 | IFF_UP = 0x1, /* Interface is up. */ | ^~~~~~ /usr/include/linux/if.h:84:9: error: redeclaration of enumerator ‘IFF_BROADCAST’ 84 | IFF_BROADCAST = 1<<1, /* __volatile__ */ | ^~~~~~~~~~~~~ /usr/include/net/if.h:46:5: note: previous definition of ‘IFF_BROADCAST’ with type ‘enum <anonymous>’ 46 | IFF_BROADCAST = 0x2, /* Broadcast address valid. */ | ^~~~~~~~~~~~~
...
In file included from /usr/include/linux/icmp.h:23, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:194:8: error: redefinition of ‘struct ifmap’ 194 | struct ifmap { | ^~~~~ In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/net/if.h:111:8: note: originally defined here 111 | struct ifmap | ^~~~~ In file included from /usr/include/linux/icmp.h:23, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:232:8: error: redefinition of ‘struct ifreq’ 232 | struct ifreq { | ^~~~~ In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/net/if.h:126:8: note: originally defined here 126 | struct ifreq | ^~~~~
Fixes: 43a7c3ef8a15 ("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/prog_tests/lwt_helpers.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h index fb1eb8c67361..8e5e28af03c5 100644 --- a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h +++ b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h @@ -6,10 +6,9 @@ #include <time.h> #include <net/if.h> #include <linux/if_tun.h> +#include "test_progs.h" /* between <net/if.h> and <linux/icmp.h> or errors */ #include <linux/icmp.h>
-#include "test_progs.h" - #define log_err(MSG, ...) \ fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \ __FILE__, __LINE__, strerror(errno), ##__VA_ARGS__)
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Fix redefinition errors seen compiling lwt_reroute.c for mips64el/musl-libc by adjusting the order of includes in lwt_helpers.h. The ordering required is: <net/if.h> --> <arpa/inet.h> (from "test_progs.h") --> <linux/icmp.h>.
Because of the complexity and large number of includes, ordering appears to be fragile however. Previously, with "test_progs.h" at the end of this sequence, compiling with GCC 12.3 for mips64el/musl-libc yields errors:
In file included from .../include/arpa/inet.h:9, from ./test_progs.h:18, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: .../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr' 23 | struct in6_addr { | ^~~~~~~~ In file included from .../include/linux/icmp.h:24, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9: .../include/linux/in6.h:33:8: note: originally defined here 33 | struct in6_addr { | ^~~~~~~~ .../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6' 34 | struct sockaddr_in6 { | ^~~~~~~~~~~~ .../include/linux/in6.h:50:8: note: originally defined here 50 | struct sockaddr_in6 { | ^~~~~~~~~~~~ .../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq' 42 | struct ipv6_mreq { | ^~~~~~~~~ .../include/linux/in6.h:60:8: note: originally defined here 60 | struct ipv6_mreq { | ^~~~~~~~~
Similarly, with "test_progs.h" at the beginning of this sequence, compiling with GCC 12.3 for x86_64 using glibc would fail like this:
In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:83:9: error: redeclaration of enumerator ‘IFF_UP’ 83 | IFF_UP = 1<<0, /* sysfs */ | ^~~~~~ /usr/include/net/if.h:44:5: note: previous definition of ‘IFF_UP’ with type ‘enum <anonymous>’ 44 | IFF_UP = 0x1, /* Interface is up. */ | ^~~~~~ /usr/include/linux/if.h:84:9: error: redeclaration of enumerator ‘IFF_BROADCAST’ 84 | IFF_BROADCAST = 1<<1, /* __volatile__ */ | ^~~~~~~~~~~~~ /usr/include/net/if.h:46:5: note: previous definition of ‘IFF_BROADCAST’ with type ‘enum <anonymous>’ 46 | IFF_BROADCAST = 0x2, /* Broadcast address valid. */ | ^~~~~~~~~~~~~
...
In file included from /usr/include/linux/icmp.h:23, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:194:8: error: redefinition of ‘struct ifmap’ 194 | struct ifmap { | ^~~~~ In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/net/if.h:111:8: note: originally defined here 111 | struct ifmap | ^~~~~ In file included from /usr/include/linux/icmp.h:23, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:232:8: error: redefinition of ‘struct ifreq’ 232 | struct ifreq { | ^~~~~ In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/net/if.h:126:8: note: originally defined here 126 | struct ifreq | ^~~~~
Fixes: 43a7c3ef8a15 ("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com
tools/testing/selftests/bpf/prog_tests/lwt_helpers.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h index fb1eb8c67361..8e5e28af03c5 100644 --- a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h +++ b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h @@ -6,10 +6,9 @@ #include <time.h> #include <net/if.h> #include <linux/if_tun.h> +#include "test_progs.h" /* between <net/if.h> and <linux/icmp.h> or errors */
Now we'll be papering over the real issue. Can you see if you can untangle this mess and ensure that we consistently use either net/if.h or linux/if.h headers?
pw-bot: cr
#include <linux/icmp.h>
-#include "test_progs.h"
#define log_err(MSG, ...) \ fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \ __FILE__, __LINE__, strerror(errno), ##__VA_ARGS__) -- 2.34.1
On Thu, Jul 25, 2024 at 01:18:04PM -0700, Andrii Nakryiko wrote:
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Fix redefinition errors seen compiling lwt_reroute.c for mips64el/musl-libc by adjusting the order of includes in lwt_helpers.h. The ordering required is: <net/if.h> --> <arpa/inet.h> (from "test_progs.h") --> <linux/icmp.h>.
Because of the complexity and large number of includes, ordering appears to be fragile however. Previously, with "test_progs.h" at the end of this sequence, compiling with GCC 12.3 for mips64el/musl-libc yields errors:
In file included from .../include/arpa/inet.h:9, from ./test_progs.h:18, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: .../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr' 23 | struct in6_addr { | ^~~~~~~~ In file included from .../include/linux/icmp.h:24, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9: .../include/linux/in6.h:33:8: note: originally defined here 33 | struct in6_addr { | ^~~~~~~~ .../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6' 34 | struct sockaddr_in6 { | ^~~~~~~~~~~~ .../include/linux/in6.h:50:8: note: originally defined here 50 | struct sockaddr_in6 { | ^~~~~~~~~~~~ .../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq' 42 | struct ipv6_mreq { | ^~~~~~~~~ .../include/linux/in6.h:60:8: note: originally defined here 60 | struct ipv6_mreq { | ^~~~~~~~~
Similarly, with "test_progs.h" at the beginning of this sequence, compiling with GCC 12.3 for x86_64 using glibc would fail like this:
In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:83:9: error: redeclaration of enumerator ‘IFF_UP’ 83 | IFF_UP = 1<<0, /* sysfs */ | ^~~~~~ /usr/include/net/if.h:44:5: note: previous definition of ‘IFF_UP’ with type ‘enum <anonymous>’ 44 | IFF_UP = 0x1, /* Interface is up. */ | ^~~~~~ /usr/include/linux/if.h:84:9: error: redeclaration of enumerator ‘IFF_BROADCAST’ 84 | IFF_BROADCAST = 1<<1, /* __volatile__ */ | ^~~~~~~~~~~~~ /usr/include/net/if.h:46:5: note: previous definition of ‘IFF_BROADCAST’ with type ‘enum <anonymous>’ 46 | IFF_BROADCAST = 0x2, /* Broadcast address valid. */ | ^~~~~~~~~~~~~
...
In file included from /usr/include/linux/icmp.h:23, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:194:8: error: redefinition of ‘struct ifmap’ 194 | struct ifmap { | ^~~~~ In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/net/if.h:111:8: note: originally defined here 111 | struct ifmap | ^~~~~ In file included from /usr/include/linux/icmp.h:23, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:10, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/linux/if.h:232:8: error: redefinition of ‘struct ifreq’ 232 | struct ifreq { | ^~~~~ In file included from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:8, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: /usr/include/net/if.h:126:8: note: originally defined here 126 | struct ifreq | ^~~~~
Fixes: 43a7c3ef8a15 ("selftests/bpf: Add lwt_xmit tests for BPF_REDIRECT") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com
tools/testing/selftests/bpf/prog_tests/lwt_helpers.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h index fb1eb8c67361..8e5e28af03c5 100644 --- a/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h +++ b/tools/testing/selftests/bpf/prog_tests/lwt_helpers.h @@ -6,10 +6,9 @@ #include <time.h> #include <net/if.h> #include <linux/if_tun.h> +#include "test_progs.h" /* between <net/if.h> and <linux/icmp.h> or errors */
Now we'll be papering over the real issue. Can you see if you can untangle this mess and ensure that we consistently use either net/if.h or linux/if.h headers?
Well, "untangle this mess" is certainly the right phrase, so I'll give it another go and see what I can find.
pw-bot: cr
#include <linux/icmp.h>
-#include "test_progs.h"
#define log_err(MSG, ...) \ fprintf(stderr, "(%s:%d: errno: %s) " MSG "\n", \ __FILE__, __LINE__, strerror(errno), ##__VA_ARGS__) -- 2.34.1
From: Tony Ambardar tony.ambardar@gmail.com
Use backtrace functions only with glibc and otherwise provide stubs in test_progs.c. This avoids compile errors (e.g. with musl libc) like:
test_progs.c:13:10: fatal error: execinfo.h: No such file or directory 13 | #include <execinfo.h> /* backtrace */ | ^~~~~~~~~~~~ test_progs.c: In function 'crash_handler': test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration] 1034 | sz = backtrace(bt, ARRAY_SIZE(bt)); | ^~~~~~~~~ test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration] 1045 | backtrace_symbols_fd(bt, sz, STDERR_FILENO); | ^~~~~~~~~~~~~~~~~~~~
Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/test_progs.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 60c5ec0f6abf..f6cfc6a8e8f0 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -10,7 +10,6 @@ #include <sched.h> #include <signal.h> #include <string.h> -#include <execinfo.h> /* backtrace */ #include <sys/sysinfo.h> /* get_nprocs */ #include <netinet/in.h> #include <sys/select.h> @@ -19,6 +18,14 @@ #include <bpf/btf.h> #include "json_writer.h"
+#ifdef __GLIBC__ +#include <execinfo.h> /* backtrace */ +#else +#define backtrace(...) (0) +#define backtrace_symbols_fd(bt, sz, fd) \ + dprintf(fd, "<backtrace not supported>\n", bt, sz) +#endif + static bool verbose(void) { return env.verbosity > VERBOSE_NONE;
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Use backtrace functions only with glibc and otherwise provide stubs in test_progs.c. This avoids compile errors (e.g. with musl libc) like:
test_progs.c:13:10: fatal error: execinfo.h: No such file or directory 13 | #include <execinfo.h> /* backtrace */ | ^~~~~~~~~~~~ test_progs.c: In function 'crash_handler': test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration] 1034 | sz = backtrace(bt, ARRAY_SIZE(bt)); | ^~~~~~~~~ test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration] 1045 | backtrace_symbols_fd(bt, sz, STDERR_FILENO); | ^~~~~~~~~~~~~~~~~~~~
Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com
tools/testing/selftests/bpf/test_progs.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 60c5ec0f6abf..f6cfc6a8e8f0 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -10,7 +10,6 @@ #include <sched.h> #include <signal.h> #include <string.h> -#include <execinfo.h> /* backtrace */ #include <sys/sysinfo.h> /* get_nprocs */ #include <netinet/in.h> #include <sys/select.h> @@ -19,6 +18,14 @@ #include <bpf/btf.h> #include "json_writer.h"
+#ifdef __GLIBC__ +#include <execinfo.h> /* backtrace */ +#else +#define backtrace(...) (0) +#define backtrace_symbols_fd(bt, sz, fd) \
dprintf(fd, "<backtrace not supported>\n", bt, sz)+#endif
First, let's define backtrace() and backtrace_symbols_fd() as proper functions, not a macro?
And second, what if we then make those functions __weak, so they provide default implementations if libc doesn't provide those functions?
This parts seems unavoidable, though:
#ifdef __GLIBC__ #include <execinfo.h> #endif
static bool verbose(void) { return env.verbosity > VERBOSE_NONE; -- 2.34.1
On Thu, Jul 25, 2024 at 01:22:37PM -0700, Andrii Nakryiko wrote:
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Use backtrace functions only with glibc and otherwise provide stubs in test_progs.c. This avoids compile errors (e.g. with musl libc) like:
test_progs.c:13:10: fatal error: execinfo.h: No such file or directory 13 | #include <execinfo.h> /* backtrace */ | ^~~~~~~~~~~~ test_progs.c: In function 'crash_handler': test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration] 1034 | sz = backtrace(bt, ARRAY_SIZE(bt)); | ^~~~~~~~~ test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration] 1045 | backtrace_symbols_fd(bt, sz, STDERR_FILENO); | ^~~~~~~~~~~~~~~~~~~~
Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com
tools/testing/selftests/bpf/test_progs.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 60c5ec0f6abf..f6cfc6a8e8f0 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -10,7 +10,6 @@ #include <sched.h> #include <signal.h> #include <string.h> -#include <execinfo.h> /* backtrace */ #include <sys/sysinfo.h> /* get_nprocs */ #include <netinet/in.h> #include <sys/select.h> @@ -19,6 +18,14 @@ #include <bpf/btf.h> #include "json_writer.h"
+#ifdef __GLIBC__ +#include <execinfo.h> /* backtrace */ +#else +#define backtrace(...) (0) +#define backtrace_symbols_fd(bt, sz, fd) \
dprintf(fd, "<backtrace not supported>\n", bt, sz)+#endif
First, let's define backtrace() and backtrace_symbols_fd() as proper functions, not a macro?
And second, what if we then make those functions __weak, so they provide default implementations if libc doesn't provide those functions?
This parts seems unavoidable, though:
#ifdef __GLIBC__ #include <execinfo.h> #endif
I agree that would be cleaner, will work on a v2 with this.
Out of curiosity, I saw that tools/build includes feature-detection code (incl backtrace) and wondered if selftests/bpf ever used this facility?
static bool verbose(void) { return env.verbosity > VERBOSE_NONE; -- 2.34.1
On Fri, Jul 26, 2024 at 8:48 PM Tony Ambardar tony.ambardar@gmail.com wrote:
On Thu, Jul 25, 2024 at 01:22:37PM -0700, Andrii Nakryiko wrote:
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Use backtrace functions only with glibc and otherwise provide stubs in test_progs.c. This avoids compile errors (e.g. with musl libc) like:
test_progs.c:13:10: fatal error: execinfo.h: No such file or directory 13 | #include <execinfo.h> /* backtrace */ | ^~~~~~~~~~~~ test_progs.c: In function 'crash_handler': test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration] 1034 | sz = backtrace(bt, ARRAY_SIZE(bt)); | ^~~~~~~~~ test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration] 1045 | backtrace_symbols_fd(bt, sz, STDERR_FILENO); | ^~~~~~~~~~~~~~~~~~~~
Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com
tools/testing/selftests/bpf/test_progs.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 60c5ec0f6abf..f6cfc6a8e8f0 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -10,7 +10,6 @@ #include <sched.h> #include <signal.h> #include <string.h> -#include <execinfo.h> /* backtrace */ #include <sys/sysinfo.h> /* get_nprocs */ #include <netinet/in.h> #include <sys/select.h> @@ -19,6 +18,14 @@ #include <bpf/btf.h> #include "json_writer.h"
+#ifdef __GLIBC__ +#include <execinfo.h> /* backtrace */ +#else +#define backtrace(...) (0) +#define backtrace_symbols_fd(bt, sz, fd) \
dprintf(fd, "<backtrace not supported>\n", bt, sz)+#endif
First, let's define backtrace() and backtrace_symbols_fd() as proper functions, not a macro?
And second, what if we then make those functions __weak, so they provide default implementations if libc doesn't provide those functions?
This parts seems unavoidable, though:
#ifdef __GLIBC__ #include <execinfo.h> #endif
I agree that would be cleaner, will work on a v2 with this.
v2 looks good, thanks
Out of curiosity, I saw that tools/build includes feature-detection code (incl backtrace) and wondered if selftests/bpf ever used this facility?
I don't remember, tbh, it might have at some point in the past.
static bool verbose(void) { return env.verbosity > VERBOSE_NONE; -- 2.34.1
From: Tony Ambardar tony.ambardar@gmail.com
Typically stdin, stdout, stderr are treated as reserved identifiers under ISO/ANSI C, and a libc implementation is free to define these as macros. This is the case in musl libc and results in compile errors when these names are reused as struct fields, as with 'struct test_env' and related usage in test_progs.[ch] and reg_bounds.c.
Rename the fields to _stdout and _stderr to avoid many errors seen building against musl, e.g.:
In file included from test_progs.h:6, from test_progs.c:5: test_progs.c: In function 'print_test_result': test_progs.c:237:21: error: expected identifier before '(' token 237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); | ^~~~~~ test_progs.c:237:9: error: too few arguments to function 'fprintf' 237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); | ^~~~~~~
Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- .../selftests/bpf/prog_tests/reg_bounds.c | 2 +- tools/testing/selftests/bpf/test_progs.c | 66 +++++++++---------- tools/testing/selftests/bpf/test_progs.h | 8 +-- 3 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c index 0da4225749bd..ff4ebc9eaf3f 100644 --- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c +++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c @@ -1487,7 +1487,7 @@ static int verify_case_opt(struct ctx *ctx, enum num_t init_t, enum num_t cond_t u64 elapsed_ns = get_time_ns() - ctx->start_ns; double remain_ns = elapsed_ns / progress * (1 - progress);
- fprintf(env.stderr, "PROGRESS (%s): %d/%d (%.2lf%%), " + fprintf(env._stderr, "PROGRESS (%s): %d/%d (%.2lf%%), " "elapsed %llu mins (%.2lf hrs), " "ETA %.0lf mins (%.2lf hrs)\n", ctx->progress_ctx, diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index f6cfc6a8e8f0..091b49bf671a 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -44,15 +44,15 @@ static void stdio_hijack_init(char **log_buf, size_t *log_cnt)
stdout = open_memstream(log_buf, log_cnt); if (!stdout) { - stdout = env.stdout; + stdout = env._stdout; perror("open_memstream"); return; }
if (env.subtest_state) - env.subtest_state->stdout = stdout; + env.subtest_state->_stdout = stdout; else - env.test_state->stdout = stdout; + env.test_state->_stdout = stdout;
stderr = stdout; #endif @@ -66,8 +66,8 @@ static void stdio_hijack(char **log_buf, size_t *log_cnt) return; }
- env.stdout = stdout; - env.stderr = stderr; + env._stdout = stdout; + env._stderr = stderr;
stdio_hijack_init(log_buf, log_cnt); #endif @@ -84,13 +84,13 @@ static void stdio_restore_cleanup(void) fflush(stdout);
if (env.subtest_state) { - fclose(env.subtest_state->stdout); - env.subtest_state->stdout = NULL; - stdout = env.test_state->stdout; - stderr = env.test_state->stdout; + fclose(env.subtest_state->_stdout); + env.subtest_state->_stdout = NULL; + stdout = env.test_state->_stdout; + stderr = env.test_state->_stdout; } else { - fclose(env.test_state->stdout); - env.test_state->stdout = NULL; + fclose(env.test_state->_stdout); + env.test_state->_stdout = NULL; } #endif } @@ -103,13 +103,13 @@ static void stdio_restore(void) return; }
- if (stdout == env.stdout) + if (stdout == env._stdout) return;
stdio_restore_cleanup();
- stdout = env.stdout; - stderr = env.stderr; + stdout = env._stdout; + stderr = env._stderr; #endif }
@@ -237,25 +237,25 @@ static void print_test_result(const struct prog_test_def *test, const struct tes int skipped_cnt = test_state->skip_cnt; int subtests_cnt = test_state->subtest_num;
- fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); + fprintf(env._stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); if (test_state->error_cnt) - fprintf(env.stdout, "FAIL"); + fprintf(env._stdout, "FAIL"); else if (!skipped_cnt) - fprintf(env.stdout, "OK"); + fprintf(env._stdout, "OK"); else if (skipped_cnt == subtests_cnt || !subtests_cnt) - fprintf(env.stdout, "SKIP"); + fprintf(env._stdout, "SKIP"); else - fprintf(env.stdout, "OK (SKIP: %d/%d)", skipped_cnt, subtests_cnt); + fprintf(env._stdout, "OK (SKIP: %d/%d)", skipped_cnt, subtests_cnt);
- fprintf(env.stdout, "\n"); + fprintf(env._stdout, "\n"); }
static void print_test_log(char *log_buf, size_t log_cnt) { log_buf[log_cnt] = '\0'; - fprintf(env.stdout, "%s", log_buf); + fprintf(env._stdout, "%s", log_buf); if (log_buf[log_cnt - 1] != '\n') - fprintf(env.stdout, "\n"); + fprintf(env._stdout, "\n"); }
static void print_subtest_name(int test_num, int subtest_num, @@ -266,14 +266,14 @@ static void print_subtest_name(int test_num, int subtest_num,
snprintf(test_num_str, sizeof(test_num_str), "%d/%d", test_num, subtest_num);
- fprintf(env.stdout, "#%-*s %s/%s", + fprintf(env._stdout, "#%-*s %s/%s", TEST_NUM_WIDTH, test_num_str, test_name, subtest_name);
if (result) - fprintf(env.stdout, ":%s", result); + fprintf(env._stdout, ":%s", result);
- fprintf(env.stdout, "\n"); + fprintf(env._stdout, "\n"); }
static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt) @@ -458,7 +458,7 @@ bool test__start_subtest(const char *subtest_name) memset(subtest_state, 0, sub_state_size);
if (!subtest_name || !subtest_name[0]) { - fprintf(env.stderr, + fprintf(env._stderr, "Subtest #%d didn't provide sub-test name!\n", state->subtest_num); return false; @@ -466,7 +466,7 @@ bool test__start_subtest(const char *subtest_name)
subtest_state->name = strdup(subtest_name); if (!subtest_state->name) { - fprintf(env.stderr, + fprintf(env._stderr, "Subtest #%d: failed to copy subtest name!\n", state->subtest_num); return false; @@ -1036,7 +1036,7 @@ void crash_handler(int signum)
sz = backtrace(bt, ARRAY_SIZE(bt));
- if (env.stdout) + if (env._stdout) stdio_restore(); if (env.test) { env.test_state->error_cnt++; @@ -1352,7 +1352,7 @@ static void calculate_summary_and_print_errors(struct test_env *env) if (env->json) { w = jsonw_new(env->json); if (!w) - fprintf(env->stderr, "Failed to create new JSON stream."); + fprintf(env->_stderr, "Failed to create new JSON stream."); }
if (w) { @@ -1701,8 +1701,8 @@ int main(int argc, char **argv) return -1; }
- env.stdout = stdout; - env.stderr = stderr; + env._stdout = stdout; + env._stderr = stderr;
env.has_testmod = true; if (!env.list_test_names) { @@ -1710,7 +1710,7 @@ int main(int argc, char **argv) unload_bpf_testmod(verbose());
if (load_bpf_testmod(verbose())) { - fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n"); + fprintf(env._stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n"); env.has_testmod = false; } } @@ -1788,7 +1788,7 @@ int main(int argc, char **argv) }
if (env.list_test_names) { - fprintf(env.stdout, "%s\n", test->test_name); + fprintf(env._stdout, "%s\n", test->test_name); env.succ_cnt++; continue; } diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index b1e949fb16cf..f42f1ae59c6e 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -75,7 +75,7 @@ struct subtest_state { bool skipped; bool filtered;
- FILE *stdout; + FILE *_stdout; };
struct test_state { @@ -92,7 +92,7 @@ struct test_state { size_t log_cnt; char *log_buf;
- FILE *stdout; + FILE *_stdout; };
struct test_env { @@ -111,8 +111,8 @@ struct test_env { struct test_state *test_state; /* current running test state */ struct subtest_state *subtest_state; /* current running subtest state */
- FILE *stdout; - FILE *stderr; + FILE *_stdout; + FILE *_stderr; int nr_cpus; FILE *json;
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Typically stdin, stdout, stderr are treated as reserved identifiers under ISO/ANSI C, and a libc implementation is free to define these as macros.
Ok, wow that. Do you have a pointer to where in the standard it is said that stdin/stdout/stderr is some sort of reserved identifier that can't be used as a field name?
I really don't like these underscored field names. If we have to rename, I'd prefer something like env.saved_stdout instead of env._stdout. But I'd prefer even more if musl wasn't doing this macro definition, of course...
This is the case in musl libc and results in compile errors when these names are reused as struct fields, as with 'struct test_env' and related usage in test_progs.[ch] and reg_bounds.c.
Rename the fields to _stdout and _stderr to avoid many errors seen building against musl, e.g.:
In file included from test_progs.h:6, from test_progs.c:5: test_progs.c: In function 'print_test_result': test_progs.c:237:21: error: expected identifier before '(' token 237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); | ^~~~~~ test_progs.c:237:9: error: too few arguments to function 'fprintf' 237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); | ^~~~~~~
Signed-off-by: Tony Ambardar tony.ambardar@gmail.com
.../selftests/bpf/prog_tests/reg_bounds.c | 2 +- tools/testing/selftests/bpf/test_progs.c | 66 +++++++++---------- tools/testing/selftests/bpf/test_progs.h | 8 +-- 3 files changed, 38 insertions(+), 38 deletions(-)
[...]
On Thu, Jul 25, 2024 at 01:27:03PM -0700, Andrii Nakryiko wrote:
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Typically stdin, stdout, stderr are treated as reserved identifiers under ISO/ANSI C, and a libc implementation is free to define these as macros.
Ok, wow that. Do you have a pointer to where in the standard it is said that stdin/stdout/stderr is some sort of reserved identifier that can't be used as a field name?
I'll need to dig around to share some references. The short answer IIRC is there's enough potential variation in their definitions that their use requires care (or better avoidance).
I really don't like these underscored field names. If we have to rename, I'd prefer something like env.saved_stdout instead of env._stdout. But I'd prefer even more if musl wasn't doing this macro definition, of course...
OK, I'll use clearer names for a v2.
I believe the macro definitions are quite common and old, but "how" makes a difference: specifically, using parenthesis happens to break our .stdxxx field names.
In glibc <stdio.h> we have for example: ... /* Standard streams. */ extern FILE *stdin; /* Standard input stream. */ extern FILE *stdout; /* Standard output stream. */ extern FILE *stderr; /* Standard error output stream. */ /* C89/C99 say they're macros. Make them happy. */ #define stdin stdin #define stdout stdout #define stderr stderr ...
while in musl <stdio.h> we have: ... extern FILE *const stdin; extern FILE *const stdout; extern FILE *const stderr;
#define stdin (stdin) #define stdout (stdout) #define stderr (stderr) ...
which borks code in test_progs.c: ... env.stderr = stderr; env.stdout = stdout; ...
This is the case in musl libc and results in compile errors when these names are reused as struct fields, as with 'struct test_env' and related usage in test_progs.[ch] and reg_bounds.c.
Rename the fields to _stdout and _stderr to avoid many errors seen building against musl, e.g.:
In file included from test_progs.h:6, from test_progs.c:5: test_progs.c: In function 'print_test_result': test_progs.c:237:21: error: expected identifier before '(' token 237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); | ^~~~~~ test_progs.c:237:9: error: too few arguments to function 'fprintf' 237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); | ^~~~~~~
Signed-off-by: Tony Ambardar tony.ambardar@gmail.com
.../selftests/bpf/prog_tests/reg_bounds.c | 2 +- tools/testing/selftests/bpf/test_progs.c | 66 +++++++++---------- tools/testing/selftests/bpf/test_progs.h | 8 +-- 3 files changed, 38 insertions(+), 38 deletions(-)
[...]
On Fri, Jul 26, 2024 at 09:22:38PM -0700, Tony Ambardar wrote:
On Thu, Jul 25, 2024 at 01:27:03PM -0700, Andrii Nakryiko wrote:
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar tony.ambardar@gmail.com wrote:
From: Tony Ambardar tony.ambardar@gmail.com
Typically stdin, stdout, stderr are treated as reserved identifiers under ISO/ANSI C, and a libc implementation is free to define these as macros.
Ok, wow that. Do you have a pointer to where in the standard it is said that stdin/stdout/stderr is some sort of reserved identifier that can't be used as a field name?
I'll need to dig around to share some references. The short answer IIRC is there's enough potential variation in their definitions that their use requires care (or better avoidance).
Hi Andrii,
Following up on your request for pointers, some excerpts from a quasi-draft C17 ISO doc located here: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf
7.1.2 Standard headers (2) The standard headers are ... <stdio.h> ... (5) Any definition of an object-like macro ... shall expand to code that is fully protected by parentheses ...
7.1.3 Reserved identifiers (1) ... Each macro name in any of the following subclauses ... is reserved for use as specified if any of its associated headers is included ...
7.21.1 Input/output <stdio.h>, Introduction (1) The header <stdio.h> defines several macros ... (3) The macros are ... stderr stdin stdout which are expressions of type "pointer to FILE" ...
7.21.5.4 The freopen function (2) (Footnote 278) The primary use of the freopen function is to change the file associated with a standard text stream (stderr, stdin, or stdout), as those identifiers need not be modifiable lvalues ...
So we have reserved idents (IANALL so not sure of field names), macros, parentheses, and potentially unassignable stdout/stderr that might break the output redirection hack in test_progs.c. More than enough to tread carefully I think...
Cheers, Tony
From: Tony Ambardar tony.ambardar@gmail.com
Linux 5.1 implemented 64-bit time types and related syscalls to address the Y2038 problem generally across archs. Userspace handling of Y2038 varies with the libc however. While musl libc uses 64-bit time across all 32-bit and 64-bit platforms, GNU glibc uses 64-bit time on 64-bit platforms but defaults to 32-bit time on 32-bit platforms unless they "opt-in" to 64-bit time or explicitly use 64-bit syscalls and time structures.
One specific area is the standard setsockopt() call, SO_TIMESTAMPNS option used for timestamping, and the related output 'struct timespec'. GNU glibc defaults as above, also exposing the SO_TIMESTAMPNS_NEW flag to explicitly use a 64-bit call and 'struct __kernel_timespec'. Since these are not exposed or needed with musl libc, their use in tc_redirect.c leads to compile errors building for mips64el/musl:
tc_redirect.c: In function 'rcv_tstamp': tc_redirect.c:425:32: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'? 425 | cmsg->cmsg_type == SO_TIMESTAMPNS_NEW) | ^~~~~~~~~~~~~~~~~~ | SO_TIMESTAMPNS tc_redirect.c:425:32: note: each undeclared identifier is reported only once for each function it appears in tc_redirect.c: In function 'test_inet_dtime': tc_redirect.c:491:49: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'? 491 | err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW, | ^~~~~~~~~~~~~~~~~~ | SO_TIMESTAMPNS
However, using SO_TIMESTAMPNS_NEW isn't strictly needed, nor is Y2038 being explicitly tested. The timestamp checks in tc_redirect.c are simple: the packet receive timestamp is non-zero and processed/handled in less than 5 seconds.
Switch to using the standard setsockopt() call and SO_TIMESTAMPNS option to ensure compatibility across glibc and musl libc. In the worst-case, there is a 5-second window 14 years from now where tc_redirect tests may fail on 32-bit systems. However, we should reasonably expect glibc to adopt a 64-bit mandate rather than the current "opt-in" policy before the Y2038 roll-over.
Fixes: ce6f6cffaeaa ("selftests/bpf: Wait for the netstamp_needed_key static key to be turned on") Fixes: c803475fd8dd ("bpf: selftests: test skb->tstamp in redirect_neigh") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/prog_tests/tc_redirect.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c index 327d51f59142..53b8ffc943dc 100644 --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c @@ -471,7 +471,7 @@ static int set_forwarding(bool enable)
static int __rcv_tstamp(int fd, const char *expected, size_t s, __u64 *tstamp) { - struct __kernel_timespec pkt_ts = {}; + struct timespec pkt_ts = {}; char ctl[CMSG_SPACE(sizeof(pkt_ts))]; struct timespec now_ts; struct msghdr msg = {}; @@ -495,7 +495,7 @@ static int __rcv_tstamp(int fd, const char *expected, size_t s, __u64 *tstamp)
cmsg = CMSG_FIRSTHDR(&msg); if (cmsg && cmsg->cmsg_level == SOL_SOCKET && - cmsg->cmsg_type == SO_TIMESTAMPNS_NEW) + cmsg->cmsg_type == SO_TIMESTAMPNS) memcpy(&pkt_ts, CMSG_DATA(cmsg), sizeof(pkt_ts));
pkt_ns = pkt_ts.tv_sec * NSEC_PER_SEC + pkt_ts.tv_nsec; @@ -537,9 +537,9 @@ static int wait_netstamp_needed_key(void) if (!ASSERT_GE(srv_fd, 0, "start_server")) goto done;
- err = setsockopt(srv_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW, + err = setsockopt(srv_fd, SOL_SOCKET, SO_TIMESTAMPNS, &opt, sizeof(opt)); - if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS_NEW)")) + if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS)")) goto done;
cli_fd = connect_to_fd(srv_fd, TIMEOUT_MILLIS); @@ -621,9 +621,9 @@ static void test_inet_dtime(int family, int type, const char *addr, __u16 port) return;
/* Ensure the kernel puts the (rcv) timestamp for all skb */ - err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW, + err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS, &opt, sizeof(opt)); - if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS_NEW)")) + if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS)")) goto done;
if (type == SOCK_STREAM) {
Hello all,
This is part 2 of a series of fixes for libc-related issues encountered building for musl-based systems. The series has been tested with the kernel-patches/bpf CI and locally using mips64el-gcc/musl-libc and QEMU with an OpenWrt rootfs.
The patches cover a few areas of portability issues:
- problematic libc usage (strtok_r(), stdio macros/reserved identifiers) - gcc compile errors (include header ordering, sequence-point errors) - POSIX vs GNU basename() - missing GNU extensions (<execinfo.h>, C++ <stdbool.h>) - Y2038 and setsockopt() / SO_TIMESTAMPNS_NEW
Feedback and suggestions are appreciated!
Thanks, Tony
Changelog: ----------
v1->v2: (feedback from Andrii) - P2: rewrite simpler code using counter - P5: update description/fix after more research - P6: use weak functions for backtrace stubs - P7: use stdxxx_saved names, update desc
Tony Ambardar (8): selftests/bpf: Use portable POSIX basename() selftests/bpf: Fix arg parsing in veristat, test_progs selftests/bpf: Fix error compiling test_lru_map.c selftests/bpf: Fix C++ compile error from missing _Bool type selftests/bpf: Fix redefinition errors compiling lwt_reroute.c selftests/bpf: Fix compile if backtrace support missing in libc selftests/bpf: Fix using stdout, stderr as struct field names selftests/bpf: Fix error compiling tc_redirect.c with musl libc
.../selftests/bpf/prog_tests/lwt_reroute.c | 1 + .../selftests/bpf/prog_tests/reg_bounds.c | 2 +- .../selftests/bpf/prog_tests/tc_redirect.c | 12 +-- tools/testing/selftests/bpf/test_cpp.cpp | 4 + tools/testing/selftests/bpf/test_lru_map.c | 3 +- tools/testing/selftests/bpf/test_progs.c | 82 +++++++++++-------- tools/testing/selftests/bpf/test_progs.h | 8 +- tools/testing/selftests/bpf/testing_helpers.c | 4 +- tools/testing/selftests/bpf/veristat.c | 16 ++-- tools/testing/selftests/bpf/xskxceiver.c | 1 + 10 files changed, 78 insertions(+), 55 deletions(-)
Use the POSIX version of basename() to allow compilation against non-gnu libc (e.g. musl). Include <libgen.h> ahead of <string.h> to enable using functions from the latter while preferring POSIX over GNU basename().
In veristat.c, rely on strdupa() to avoid basename() altering the passed "const char" argument. This is not needed in xskxceiver.c since the arg is mutable and the program exits immediately after usage.
Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/veristat.c | 8 +++++--- tools/testing/selftests/bpf/xskxceiver.c | 1 + 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index b2854238d4a0..11ec1190d582 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -2,6 +2,7 @@ /* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */ #define _GNU_SOURCE #include <argp.h> +#include <libgen.h> #include <string.h> #include <stdlib.h> #include <sched.h> @@ -988,8 +989,8 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch
static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog) { + const char *base_filename = basename(strdupa(filename)); const char *prog_name = bpf_program__name(prog); - const char *base_filename = basename(filename); char *buf; int buf_sz, log_level; struct verif_stats *stats; @@ -1056,13 +1057,14 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
static int process_obj(const char *filename) { + const char *base_filename = basename(strdupa(filename)); struct bpf_object *obj = NULL, *tobj; struct bpf_program *prog, *tprog, *lprog; libbpf_print_fn_t old_libbpf_print_fn; LIBBPF_OPTS(bpf_object_open_opts, opts); int err = 0, prog_cnt = 0;
- if (!should_process_file_prog(basename(filename), NULL)) { + if (!should_process_file_prog(base_filename, NULL)) { if (env.verbose) printf("Skipping '%s' due to filters...\n", filename); env.files_skipped++; @@ -1076,7 +1078,7 @@ static int process_obj(const char *filename) }
if (!env.quiet && env.out_fmt == RESFMT_TABLE) - printf("Processing '%s'...\n", basename(filename)); + printf("Processing '%s'...\n", base_filename);
old_libbpf_print_fn = libbpf_set_print(libbpf_print_fn); obj = bpf_object__open_file(filename, &opts); diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c index 8144fd145237..92af633faea8 100644 --- a/tools/testing/selftests/bpf/xskxceiver.c +++ b/tools/testing/selftests/bpf/xskxceiver.c @@ -90,6 +90,7 @@ #include <signal.h> #include <stdio.h> #include <stdlib.h> +#include <libgen.h> #include <string.h> #include <stddef.h> #include <sys/mman.h>
Hello:
This series was applied to bpf/bpf-next.git (master) by Andrii Nakryiko andrii@kernel.org:
On Mon, 29 Jul 2024 02:24:17 -0700 you wrote:
Use the POSIX version of basename() to allow compilation against non-gnu libc (e.g. musl). Include <libgen.h> ahead of <string.h> to enable using functions from the latter while preferring POSIX over GNU basename().
In veristat.c, rely on strdupa() to avoid basename() altering the passed "const char" argument. This is not needed in xskxceiver.c since the arg is mutable and the program exits immediately after usage.
[...]
Here is the summary with links: - [bpf-next,v2,1/8] selftests/bpf: Use portable POSIX basename() https://git.kernel.org/bpf/bpf-next/c/c0247800ee7d - [bpf-next,v2,2/8] selftests/bpf: Fix arg parsing in veristat, test_progs https://git.kernel.org/bpf/bpf-next/c/03bfcda1fbc3 - [bpf-next,v2,3/8] selftests/bpf: Fix error compiling test_lru_map.c https://git.kernel.org/bpf/bpf-next/c/cacf2a5a78cd - [bpf-next,v2,4/8] selftests/bpf: Fix C++ compile error from missing _Bool type https://git.kernel.org/bpf/bpf-next/c/aa95073fd290 - [bpf-next,v2,5/8] selftests/bpf: Fix redefinition errors compiling lwt_reroute.c https://git.kernel.org/bpf/bpf-next/c/16b795cc5952 - [bpf-next,v2,6/8] selftests/bpf: Fix compile if backtrace support missing in libc https://git.kernel.org/bpf/bpf-next/c/c9a83e76b5a9 - [bpf-next,v2,7/8] selftests/bpf: Fix using stdout, stderr as struct field names https://git.kernel.org/bpf/bpf-next/c/06eeca1217a8 - [bpf-next,v2,8/8] selftests/bpf: Fix error compiling tc_redirect.c with musl libc https://git.kernel.org/bpf/bpf-next/c/21c5f4f55da7
You are awesome, thank you!
Current code parses arguments with strtok_r() using a construct like
char *state = NULL; while ((next = strtok_r(state ? NULL : input, ",", &state))) { ... }
where logic assumes the 'state' var can distinguish between first and subsequent strtok_r() calls, and adjusts parameters accordingly. However, 'state' is strictly internal context for strtok_r() and no such assumptions are supported in the man page. Moreover, the exact behaviour of 'state' depends on the libc implementation, making the above code fragile.
Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang, with the above code in an infinite loop.
Similarly, we see strange behaviour running 'veristat' on mips64el/musl:
$ ./veristat -e file,prog,verdict,insns -C two-ok add-failure Can't specify more than 9 stats
Rewrite code using a counter to distinguish between strtok_r() calls.
Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests") Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat") Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/testing_helpers.c | 4 ++-- tools/testing/selftests/bpf/veristat.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c index ac7c66f4fc7b..c217e12bd9da 100644 --- a/tools/testing/selftests/bpf/testing_helpers.c +++ b/tools/testing/selftests/bpf/testing_helpers.c @@ -221,13 +221,13 @@ int parse_test_list(const char *s, bool is_glob_pattern) { char *input, *state = NULL, *test_spec; - int err = 0; + int err = 0, cnt = 0;
input = strdup(s); if (!input) return -ENOMEM;
- while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) { + while ((test_spec = strtok_r(cnt++ ? NULL : input, ",", &state))) { err = insert_test(set, test_spec, is_glob_pattern); if (err) break; diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 11ec1190d582..1ec5c4c47235 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -785,13 +785,13 @@ static int parse_stat(const char *stat_name, struct stat_specs *specs) static int parse_stats(const char *stats_str, struct stat_specs *specs) { char *input, *state = NULL, *next; - int err; + int err, cnt = 0;
input = strdup(stats_str); if (!input) return -ENOMEM;
- while ((next = strtok_r(state ? NULL : input, ",", &state))) { + while ((next = strtok_r(cnt++ ? NULL : input, ",", &state))) { err = parse_stat(next, specs); if (err) { free(input); @@ -1495,7 +1495,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs, while (fgets(line, sizeof(line), f)) { char *input = line, *state = NULL, *next; struct verif_stats *st = NULL; - int col = 0; + int col = 0, cnt = 0;
if (!header) { void *tmp; @@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs, *stat_cntp += 1; }
- while ((next = strtok_r(state ? NULL : input, ",\n", &state))) { + while ((next = strtok_r(cnt++ ? NULL : input, ",\n", &state))) { if (header) { /* for the first line, set up spec stats */ err = parse_stat(next, specs);
Although the post-increment in macro 'CPU_SET(next++, &cpuset)' seems safe, the sequencing can raise compile errors, so move the increment outside the macro. This avoids an error seen using gcc 12.3.0 for mips64el/musl-libc:
In file included from test_lru_map.c:11: test_lru_map.c: In function 'sched_next_online': test_lru_map.c:129:29: error: operation on 'next' may be undefined [-Werror=sequence-point] 129 | CPU_SET(next++, &cpuset); | ^ cc1: all warnings being treated as errors
Fixes: 3fbfadce6012 ("bpf: Fix test_lru_sanity5() in test_lru_map.c") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/test_lru_map.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_lru_map.c b/tools/testing/selftests/bpf/test_lru_map.c index 4d0650cfb5cd..fda7589c5023 100644 --- a/tools/testing/selftests/bpf/test_lru_map.c +++ b/tools/testing/selftests/bpf/test_lru_map.c @@ -126,7 +126,8 @@ static int sched_next_online(int pid, int *next_to_try)
while (next < nr_cpus) { CPU_ZERO(&cpuset); - CPU_SET(next++, &cpuset); + CPU_SET(next, &cpuset); + next++; if (!sched_setaffinity(pid, sizeof(cpuset), &cpuset)) { ret = 0; break;
While building, bpftool makes a skeleton from test_core_extern.c, which itself includes <stdbool.h> and uses the 'bool' type. However, the skeleton test_core_extern.skel.h generated *does not* include <stdbool.h> or use the 'bool' type, instead using the C-only '_Bool' type. Compiling test_cpp.cpp with g++ 12.3 for mips64el/musl-libc then fails with error:
In file included from test_cpp.cpp:9: test_core_extern.skel.h:45:17: error: '_Bool' does not name a type 45 | _Bool CONFIG_BOOL; | ^~~~~
This was likely missed previously because glibc uses a GNU extension for <stdbool.h> with C++ (#define _Bool bool), not supported by musl libc.
Normally, a C fragment would include <stdbool.h> and use the 'bool' type, and thus cleanly work after import by C++. The ideal fix would be for 'bpftool gen skeleton' to output the correct type/include supporting C++, but in the meantime add a conditional define as above.
Fixes: 7c8dce4b1661 ("bpftool: Make skeleton C code compilable with C++ compiler") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/test_cpp.cpp | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_cpp.cpp b/tools/testing/selftests/bpf/test_cpp.cpp index dde0bb16e782..abc2a56ab261 100644 --- a/tools/testing/selftests/bpf/test_cpp.cpp +++ b/tools/testing/selftests/bpf/test_cpp.cpp @@ -6,6 +6,10 @@ #include <bpf/libbpf.h> #include <bpf/bpf.h> #include <bpf/btf.h> + +#ifndef _Bool +#define _Bool bool +#endif #include "test_core_extern.skel.h" #include "struct_ops_module.skel.h"
Compiling lwt_reroute.c with GCC 12.3 for mips64el/musl-libc yields errors:
In file included from .../include/arpa/inet.h:9, from ./test_progs.h:18, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11, from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52: .../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr' 23 | struct in6_addr { | ^~~~~~~~ In file included from .../include/linux/icmp.h:24, from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9: .../include/linux/in6.h:33:8: note: originally defined here 33 | struct in6_addr { | ^~~~~~~~ .../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6' 34 | struct sockaddr_in6 { | ^~~~~~~~~~~~ .../include/linux/in6.h:50:8: note: originally defined here 50 | struct sockaddr_in6 { | ^~~~~~~~~~~~ .../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq' 42 | struct ipv6_mreq { | ^~~~~~~~~ .../include/linux/in6.h:60:8: note: originally defined here 60 | struct ipv6_mreq { | ^~~~~~~~~
These errors occur because <linux/in6.h> is included before <netinet/in.h>, bypassing the Linux uapi/libc compat mechanism's partial musl support. As described in [1] and [2], fix these errors by including <netinet/in.h> in lwt_reroute.c before any uapi headers.
[1]: commit c0bace798436 ("uapi libc compat: add fallback for unsupported libcs") [2]: https://git.musl-libc.org/cgit/musl/commit/?id=04983f227238
Fixes: 6c77997bc639 ("selftests/bpf: Add lwt_xmit tests for BPF_REROUTE") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/prog_tests/lwt_reroute.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/lwt_reroute.c b/tools/testing/selftests/bpf/prog_tests/lwt_reroute.c index 03825d2b45a8..6c50c0f63f43 100644 --- a/tools/testing/selftests/bpf/prog_tests/lwt_reroute.c +++ b/tools/testing/selftests/bpf/prog_tests/lwt_reroute.c @@ -49,6 +49,7 @@ * is not crashed, it is considered successful. */ #define NETNS "ns_lwt_reroute" +#include <netinet/in.h> #include "lwt_helpers.h" #include "network_helpers.h" #include <linux/net_tstamp.h>
Include GNU <execinfo.h> header only with glibc and provide weak, stubbed backtrace functions as a fallback in test_progs.c. This allows for non-GNU replacements while avoiding compile errors (e.g. with musl libc) like:
test_progs.c:13:10: fatal error: execinfo.h: No such file or directory 13 | #include <execinfo.h> /* backtrace */ | ^~~~~~~~~~~~ test_progs.c: In function 'crash_handler': test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration] 1034 | sz = backtrace(bt, ARRAY_SIZE(bt)); | ^~~~~~~~~ test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration] 1045 | backtrace_symbols_fd(bt, sz, STDERR_FILENO); | ^~~~~~~~~~~~~~~~~~~~
Fixes: 9fb156bb82a3 ("selftests/bpf: Print backtrace on SIGSEGV in test_progs") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/test_progs.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 60c5ec0f6abf..d5d0cb4eb197 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -10,7 +10,6 @@ #include <sched.h> #include <signal.h> #include <string.h> -#include <execinfo.h> /* backtrace */ #include <sys/sysinfo.h> /* get_nprocs */ #include <netinet/in.h> #include <sys/select.h> @@ -19,6 +18,21 @@ #include <bpf/btf.h> #include "json_writer.h"
+#ifdef __GLIBC__ +#include <execinfo.h> /* backtrace */ +#endif + +/* Default backtrace funcs if missing at link */ +__weak int backtrace(void **buffer, int size) +{ + return 0; +} + +__weak void backtrace_symbols_fd(void *const *buffer, int size, int fd) +{ + dprintf(fd, "<backtrace not supported>\n"); +} + static bool verbose(void) { return env.verbosity > VERBOSE_NONE;
Typically stdin, stdout, stderr are treated as reserved identifiers under ISO/ANSI C and libc implementations further define these as macros, both in glibc and musl <stdio.h>.
However, while glibc defines: ... /* Standard streams. */ extern FILE *stdin; /* Standard input stream. */ extern FILE *stdout; /* Standard output stream. */ extern FILE *stderr; /* Standard error output stream. */ /* C89/C99 say they're macros. Make them happy. */ #define stdin stdin #define stdout stdout #define stderr stderr ...
musl instead uses (legally): ... extern FILE *const stdin; extern FILE *const stdout; extern FILE *const stderr;
#define stdin (stdin) #define stdout (stdout) #define stderr (stderr) ...
The latter results in compile errors when the names are reused as fields of 'struct test_env' and elsewhere in test_progs.[ch] and reg_bounds.c.
Rename the fields to stdout_saved and stderr_saved to avoid many errors seen building against musl, e.g.:
In file included from test_progs.h:6, from test_progs.c:5: test_progs.c: In function 'print_test_result': test_progs.c:237:21: error: expected identifier before '(' token 237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); | ^~~~~~ test_progs.c:237:9: error: too few arguments to function 'fprintf' 237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); | ^~~~~~~
Link: https://lore.kernel.org/bpf/ZqR2DuHdBXPX%2Fyx8@kodidev-ubuntu/ Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- .../selftests/bpf/prog_tests/reg_bounds.c | 2 +- tools/testing/selftests/bpf/test_progs.c | 66 +++++++++---------- tools/testing/selftests/bpf/test_progs.h | 8 +-- 3 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c index 0da4225749bd..467027236d30 100644 --- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c +++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c @@ -1487,7 +1487,7 @@ static int verify_case_opt(struct ctx *ctx, enum num_t init_t, enum num_t cond_t u64 elapsed_ns = get_time_ns() - ctx->start_ns; double remain_ns = elapsed_ns / progress * (1 - progress);
- fprintf(env.stderr, "PROGRESS (%s): %d/%d (%.2lf%%), " + fprintf(env.stderr_saved, "PROGRESS (%s): %d/%d (%.2lf%%), " "elapsed %llu mins (%.2lf hrs), " "ETA %.0lf mins (%.2lf hrs)\n", ctx->progress_ctx, diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index d5d0cb4eb197..60fafa2f1ed7 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -51,15 +51,15 @@ static void stdio_hijack_init(char **log_buf, size_t *log_cnt)
stdout = open_memstream(log_buf, log_cnt); if (!stdout) { - stdout = env.stdout; + stdout = env.stdout_saved; perror("open_memstream"); return; }
if (env.subtest_state) - env.subtest_state->stdout = stdout; + env.subtest_state->stdout_saved = stdout; else - env.test_state->stdout = stdout; + env.test_state->stdout_saved = stdout;
stderr = stdout; #endif @@ -73,8 +73,8 @@ static void stdio_hijack(char **log_buf, size_t *log_cnt) return; }
- env.stdout = stdout; - env.stderr = stderr; + env.stdout_saved = stdout; + env.stderr_saved = stderr;
stdio_hijack_init(log_buf, log_cnt); #endif @@ -91,13 +91,13 @@ static void stdio_restore_cleanup(void) fflush(stdout);
if (env.subtest_state) { - fclose(env.subtest_state->stdout); - env.subtest_state->stdout = NULL; - stdout = env.test_state->stdout; - stderr = env.test_state->stdout; + fclose(env.subtest_state->stdout_saved); + env.subtest_state->stdout_saved = NULL; + stdout = env.test_state->stdout_saved; + stderr = env.test_state->stdout_saved; } else { - fclose(env.test_state->stdout); - env.test_state->stdout = NULL; + fclose(env.test_state->stdout_saved); + env.test_state->stdout_saved = NULL; } #endif } @@ -110,13 +110,13 @@ static void stdio_restore(void) return; }
- if (stdout == env.stdout) + if (stdout == env.stdout_saved) return;
stdio_restore_cleanup();
- stdout = env.stdout; - stderr = env.stderr; + stdout = env.stdout_saved; + stderr = env.stderr_saved; #endif }
@@ -244,25 +244,25 @@ static void print_test_result(const struct prog_test_def *test, const struct tes int skipped_cnt = test_state->skip_cnt; int subtests_cnt = test_state->subtest_num;
- fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); + fprintf(env.stdout_saved, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name); if (test_state->error_cnt) - fprintf(env.stdout, "FAIL"); + fprintf(env.stdout_saved, "FAIL"); else if (!skipped_cnt) - fprintf(env.stdout, "OK"); + fprintf(env.stdout_saved, "OK"); else if (skipped_cnt == subtests_cnt || !subtests_cnt) - fprintf(env.stdout, "SKIP"); + fprintf(env.stdout_saved, "SKIP"); else - fprintf(env.stdout, "OK (SKIP: %d/%d)", skipped_cnt, subtests_cnt); + fprintf(env.stdout_saved, "OK (SKIP: %d/%d)", skipped_cnt, subtests_cnt);
- fprintf(env.stdout, "\n"); + fprintf(env.stdout_saved, "\n"); }
static void print_test_log(char *log_buf, size_t log_cnt) { log_buf[log_cnt] = '\0'; - fprintf(env.stdout, "%s", log_buf); + fprintf(env.stdout_saved, "%s", log_buf); if (log_buf[log_cnt - 1] != '\n') - fprintf(env.stdout, "\n"); + fprintf(env.stdout_saved, "\n"); }
static void print_subtest_name(int test_num, int subtest_num, @@ -273,14 +273,14 @@ static void print_subtest_name(int test_num, int subtest_num,
snprintf(test_num_str, sizeof(test_num_str), "%d/%d", test_num, subtest_num);
- fprintf(env.stdout, "#%-*s %s/%s", + fprintf(env.stdout_saved, "#%-*s %s/%s", TEST_NUM_WIDTH, test_num_str, test_name, subtest_name);
if (result) - fprintf(env.stdout, ":%s", result); + fprintf(env.stdout_saved, ":%s", result);
- fprintf(env.stdout, "\n"); + fprintf(env.stdout_saved, "\n"); }
static void jsonw_write_log_message(json_writer_t *w, char *log_buf, size_t log_cnt) @@ -465,7 +465,7 @@ bool test__start_subtest(const char *subtest_name) memset(subtest_state, 0, sub_state_size);
if (!subtest_name || !subtest_name[0]) { - fprintf(env.stderr, + fprintf(env.stderr_saved, "Subtest #%d didn't provide sub-test name!\n", state->subtest_num); return false; @@ -473,7 +473,7 @@ bool test__start_subtest(const char *subtest_name)
subtest_state->name = strdup(subtest_name); if (!subtest_state->name) { - fprintf(env.stderr, + fprintf(env.stderr_saved, "Subtest #%d: failed to copy subtest name!\n", state->subtest_num); return false; @@ -1043,7 +1043,7 @@ void crash_handler(int signum)
sz = backtrace(bt, ARRAY_SIZE(bt));
- if (env.stdout) + if (env.stdout_saved) stdio_restore(); if (env.test) { env.test_state->error_cnt++; @@ -1359,7 +1359,7 @@ static void calculate_summary_and_print_errors(struct test_env *env) if (env->json) { w = jsonw_new(env->json); if (!w) - fprintf(env->stderr, "Failed to create new JSON stream."); + fprintf(env->stderr_saved, "Failed to create new JSON stream."); }
if (w) { @@ -1708,8 +1708,8 @@ int main(int argc, char **argv) return -1; }
- env.stdout = stdout; - env.stderr = stderr; + env.stdout_saved = stdout; + env.stderr_saved = stderr;
env.has_testmod = true; if (!env.list_test_names) { @@ -1717,7 +1717,7 @@ int main(int argc, char **argv) unload_bpf_testmod(verbose());
if (load_bpf_testmod(verbose())) { - fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n"); + fprintf(env.stderr_saved, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n"); env.has_testmod = false; } } @@ -1795,7 +1795,7 @@ int main(int argc, char **argv) }
if (env.list_test_names) { - fprintf(env.stdout, "%s\n", test->test_name); + fprintf(env.stdout_saved, "%s\n", test->test_name); env.succ_cnt++; continue; } diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index b1e949fb16cf..cb9d6d46826b 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -75,7 +75,7 @@ struct subtest_state { bool skipped; bool filtered;
- FILE *stdout; + FILE *stdout_saved; };
struct test_state { @@ -92,7 +92,7 @@ struct test_state { size_t log_cnt; char *log_buf;
- FILE *stdout; + FILE *stdout_saved; };
struct test_env { @@ -111,8 +111,8 @@ struct test_env { struct test_state *test_state; /* current running test state */ struct subtest_state *subtest_state; /* current running subtest state */
- FILE *stdout; - FILE *stderr; + FILE *stdout_saved; + FILE *stderr_saved; int nr_cpus; FILE *json;
Linux 5.1 implemented 64-bit time types and related syscalls to address the Y2038 problem generally across archs. Userspace handling of Y2038 varies with the libc however. While musl libc uses 64-bit time across all 32-bit and 64-bit platforms, GNU glibc uses 64-bit time on 64-bit platforms but defaults to 32-bit time on 32-bit platforms unless they "opt-in" to 64-bit time or explicitly use 64-bit syscalls and time structures.
One specific area is the standard setsockopt() call, SO_TIMESTAMPNS option used for timestamping, and the related output 'struct timespec'. GNU glibc defaults as above, also exposing the SO_TIMESTAMPNS_NEW flag to explicitly use a 64-bit call and 'struct __kernel_timespec'. Since these are not exposed or needed with musl libc, their use in tc_redirect.c leads to compile errors building for mips64el/musl:
tc_redirect.c: In function 'rcv_tstamp': tc_redirect.c:425:32: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'? 425 | cmsg->cmsg_type == SO_TIMESTAMPNS_NEW) | ^~~~~~~~~~~~~~~~~~ | SO_TIMESTAMPNS tc_redirect.c:425:32: note: each undeclared identifier is reported only once for each function it appears in tc_redirect.c: In function 'test_inet_dtime': tc_redirect.c:491:49: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'? 491 | err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW, | ^~~~~~~~~~~~~~~~~~ | SO_TIMESTAMPNS
However, using SO_TIMESTAMPNS_NEW isn't strictly needed, nor is Y2038 being explicitly tested. The timestamp checks in tc_redirect.c are simple: the packet receive timestamp is non-zero and processed/handled in less than 5 seconds.
Switch to using the standard setsockopt() call and SO_TIMESTAMPNS option to ensure compatibility across glibc and musl libc. In the worst-case, there is a 5-second window 14 years from now where tc_redirect tests may fail on 32-bit systems. However, we should reasonably expect glibc to adopt a 64-bit mandate rather than the current "opt-in" policy before the Y2038 roll-over.
Fixes: ce6f6cffaeaa ("selftests/bpf: Wait for the netstamp_needed_key static key to be turned on") Fixes: c803475fd8dd ("bpf: selftests: test skb->tstamp in redirect_neigh") Signed-off-by: Tony Ambardar tony.ambardar@gmail.com --- tools/testing/selftests/bpf/prog_tests/tc_redirect.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c index 327d51f59142..53b8ffc943dc 100644 --- a/tools/testing/selftests/bpf/prog_tests/tc_redirect.c +++ b/tools/testing/selftests/bpf/prog_tests/tc_redirect.c @@ -471,7 +471,7 @@ static int set_forwarding(bool enable)
static int __rcv_tstamp(int fd, const char *expected, size_t s, __u64 *tstamp) { - struct __kernel_timespec pkt_ts = {}; + struct timespec pkt_ts = {}; char ctl[CMSG_SPACE(sizeof(pkt_ts))]; struct timespec now_ts; struct msghdr msg = {}; @@ -495,7 +495,7 @@ static int __rcv_tstamp(int fd, const char *expected, size_t s, __u64 *tstamp)
cmsg = CMSG_FIRSTHDR(&msg); if (cmsg && cmsg->cmsg_level == SOL_SOCKET && - cmsg->cmsg_type == SO_TIMESTAMPNS_NEW) + cmsg->cmsg_type == SO_TIMESTAMPNS) memcpy(&pkt_ts, CMSG_DATA(cmsg), sizeof(pkt_ts));
pkt_ns = pkt_ts.tv_sec * NSEC_PER_SEC + pkt_ts.tv_nsec; @@ -537,9 +537,9 @@ static int wait_netstamp_needed_key(void) if (!ASSERT_GE(srv_fd, 0, "start_server")) goto done;
- err = setsockopt(srv_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW, + err = setsockopt(srv_fd, SOL_SOCKET, SO_TIMESTAMPNS, &opt, sizeof(opt)); - if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS_NEW)")) + if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS)")) goto done;
cli_fd = connect_to_fd(srv_fd, TIMEOUT_MILLIS); @@ -621,9 +621,9 @@ static void test_inet_dtime(int family, int type, const char *addr, __u16 port) return;
/* Ensure the kernel puts the (rcv) timestamp for all skb */ - err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW, + err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS, &opt, sizeof(opt)); - if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS_NEW)")) + if (!ASSERT_OK(err, "setsockopt(SO_TIMESTAMPNS)")) goto done;
if (type == SOCK_STREAM) {
linux-kselftest-mirror@lists.linaro.org
-
Andrii Nakryiko -
patchwork-bot+netdevbpf＠kernel.org -
Tony Ambardar