On Fri, Aug 26, 2022 at 7:19 AM Jose E. Marchesi jose.marchesi@oracle.com wrote:
On Thu, Aug 25, 2022 at 11:49 PM Martin KaFai Lau kafai@fb.com wrote:
On Thu, Aug 25, 2022 at 11:31:15PM -0600, James Hilliard wrote:
On Thu, Aug 25, 2022 at 11:16 PM Martin KaFai Lau kafai@fb.com wrote:
On Thu, Aug 25, 2022 at 04:17:49PM -0600, James Hilliard wrote: > There is a potential for us to hit a type conflict when including > netinet/tcp.h with sys/socket.h, we can replace both of these includes > with linux/tcp.h to avoid this conflict. > > Fixes errors like: > In file included from /usr/include/netinet/tcp.h:91, > from progs/bind4_prog.c:10: > /home/buildroot/opt/cross/lib/gcc/bpf/13.0.0/include/stdint.h:34:23: error: conflicting types for 'int8_t'; have 'char' > 34 | typedef __INT8_TYPE__ int8_t; > | ^~~~~~ > In file included from /usr/include/x86_64-linux-gnu/sys/types.h:155, > from /usr/include/x86_64-linux-gnu/bits/socket.h:29, > from /usr/include/x86_64-linux-gnu/sys/socket.h:33, > from progs/bind4_prog.c:9: > /usr/include/x86_64-linux-gnu/bits/stdint-intn.h:24:18: note: previous declaration of 'int8_t' with type 'int8_t' {aka 'signed char'} > 24 | typedef __int8_t int8_t; > | ^~~~~~ > /home/buildroot/opt/cross/lib/gcc/bpf/13.0.0/include/stdint.h:43:24: > error: conflicting types for 'int64_t'; have 'long int' > 43 | typedef __INT64_TYPE__ int64_t; > | ^~~~~~~ > /usr/include/x86_64-linux-gnu/bits/stdint-intn.h:27:19: note: > previous declaration of 'int64_t' with type 'int64_t' {aka > 'long long int'} > 27 | typedef __int64_t int64_t; > | ^~~~~~~ > make: *** [Makefile:537: > /home/buildroot/bpf-next/tools/testing/selftests/bpf/bpf_gcc/bind4_prog.o] > Error 1 > > Signed-off-by: James Hilliard james.hilliard1@gmail.com > --- > tools/testing/selftests/bpf/progs/bind4_prog.c | 3 +-- > tools/testing/selftests/bpf/progs/bind6_prog.c | 3 +-- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/bpf/progs/bind4_prog.c > b/tools/testing/selftests/bpf/progs/bind4_prog.c > index 474c6a62078a..6bd20042fd53 100644 > --- a/tools/testing/selftests/bpf/progs/bind4_prog.c > +++ b/tools/testing/selftests/bpf/progs/bind4_prog.c > @@ -6,8 +6,7 @@ > #include <linux/bpf.h> > #include <linux/in.h> > #include <linux/in6.h> > -#include <sys/socket.h> > -#include <netinet/tcp.h> These includes look normal to me. What environment is hitting this.
I was hitting this error with GCC 13(GCC master branch).
These two includes (<sys/socket.h> and <netinet/tcp.h>) are normal, so does it mean all existing programs need to change to use gcc 13 ?
Well I think it's mostly just an issue getting hit with GCC-BPF as it looks to me like a cross compilation host/target header conflict.
This is an interesting issue.
Right now the BPF GCC target is a sort of a bare-metal target. As such, it provides a set of header files that implement ISO C types and other machinery (i.e. it doesn't rely on a C library to provide them):
iso646.h stdalign.h stdarg.h stdatomic.h stdbool.h stddef.h stdfix.h stdint.h stdnoreturn.h tgmath.h unwind.h varargs.h
This is because we were expecting this to be used like:
<compiler-provided std C headers> | | v | <kernel headers> | | | v v <BPF C program>However, if it is expected/intended for C BPF programs to include libc headers, such as sys/socket.h, this can quickly go sour as you have found with that conflict.
So this leads to the question: should we turn the BPF target into a target that assumes a libc? This basically means we will be assuming BPF programs are always compiled in an environment that provides a standard stdint.h, stdbool.h and friends.
Well for a normal GCC BPF setup we're basically cross compiling for the BPF bare metal target while sharing headers with the build host(for libbpf and any other libc headers that get included).
On the other hand when using GCC BPF as part of a full cross toolchain we actually end up sharing headers with our real cross target architecture sysroot(which would provide a libc), essentially in that case BPF is a bare metal cross target which shares headers with the real cross target(which is not a bare metal target). For this libbpf is installed to the real cross target sysroot which is used by both GCC BPF(for bpf progs) and the real cross target GCC compiler(for userspace side). From my understanding with this setup GCC BPF will pick up the real cross target libc headers as a fallback which may sometimes have conflict/compatibility issues with the kernel headers.
I think it's probably best to avoid depending on libc headers as things may otherwise get even more complex. You would essentially have 2 libc's in a normal GCC BPF setup and 3 libc's in a full cross toolchain setup(you'd have one for the build host, one for the real cross target arch and one for the BPF target arch).
Cross build systems will typically allow a libc choice as well(glibc/musl/uclibc) and we don't really want the bpf programs to have to care about the specific libc being used as they are bare metal programs which shouldn't depend on a libc.
I don't understand what do you mean with "real cross target".
From the toolchain perspective, the compiler is targetted to just one platform: bpf-unknown-none. As is usual for bare-metal targets, the compiler provides headers to implement the C standard with things like floating-point types and standard integer types, `bool', etc.
If you then -I directories in order to "share headers with the build host" or with that "real cross target", or to use any other header that may implement the same types (typically a libc) then well, thats when the problem arises.
I don't know how much sense does it makes to include glibc headers like sys/socket.h in BPF C programs: I'm no BPF programmer. But if it is something to be supported, we will have to change the compiler to not provide the standard headers.
Thoughts?
I don't prefer the selftest writers need to remember this rule.
Beside, afaict, tcp.h should be removed because I don't see this test needs it. I tried removing it and it works fine. It should be removed instead of replacing it with another unnecessary tcp.h.
Oh, that does also appear to work, thought I had tried that already but I guess I hadn't, sent a v2 with them removed: https://lore.kernel.org/bpf/20220826052925.980431-1-james.hilliard1@gmail.co...
> +#include <linux/tcp.h> > #include <linux/if.h> > #include <errno.h> > > diff --git a/tools/testing/selftests/bpf/progs/bind6_prog.c > b/tools/testing/selftests/bpf/progs/bind6_prog.c > index c19cfa869f30..f37617b35a55 100644 > --- a/tools/testing/selftests/bpf/progs/bind6_prog.c > +++ b/tools/testing/selftests/bpf/progs/bind6_prog.c > @@ -6,8 +6,7 @@ > #include <linux/bpf.h> > #include <linux/in.h> > #include <linux/in6.h> > -#include <sys/socket.h> > -#include <netinet/tcp.h> > +#include <linux/tcp.h> > #include <linux/if.h> > #include <errno.h> > > -- > 2.34.1 >