| Message ID | cover.1721903630.git.tony.ambardar@gmail.com |
|---|---|
| Headers | show |
| Series | selftests/bpf: Improve libc portability / musl support (part 2) | expand |
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 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: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 > >
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> 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(-)