On 11/28/23 11:23 AM, Jose E. Marchesi wrote:
[During LPC 2023 we talked about improving communication between the GCC
BPF toolchain port and the kernel side. This is the first periodical
report that we plan to publish in the GCC wiki and send to interested
parties. Hopefully this will help.]
GCC wiki page for the port: https://gcc.gnu.org/wiki/BPFBackEnd
IRC channel: #gccbpf at irc.oftc.net.
Help on using the port: gcc@xxxxxxxxxxx
Patches and/or development discussions: gcc-patches@xxxxxxx
Thanks a lot for detailed report. Really helpful to nail down
issues facing one or both compilers. See comments below for
some mentioned issues.
Assembler
=========
[...]
- In the Pseudo-C syntax register names are not preceded by % characters
nor any other prefix. A consequence of that is that in contexts like
instruction operands, where both register names and expressions
involving symbols are expected, there is no way to disambiguate
between them. GAS was allowing symbols like `w3' or `r5' in syntactic
contexts where no registers were expected, such as in:
r0 = w3 ll ; GAS interpreted w3 as symbol, clang emits error
The clang assembler wasn't allowing that. During LPC we agreed that
the simplest approach is to not allow any symbol to have the same name
than a register, in any context. So we changed GAS so it now doesn't
allow to use register names as symbols in any expression, such as:
r0 = w3 + 1 ll ; This now fails for both GAS and llvm.
r0 = 1 + w3 ll ; NOTE this does not fail with llvm, but it should.
Could you provide a reproducible case above for llvm? llvm does not
support syntax like 'r0 = 1 + w3 ll'. For add, it only supports
'r1 += r2' or 'r1 += 100' syntax.
We installed a patch in GAS for this.
Jose E. Marchesi
https://sourceware.org/pipermail/binutils/2023-November/130684.html
Pending Patches for bpf-next
============================
- bpf: avoid VLAs in progs/test_xdp_dynptr.c
In the progs/test_xdp_dynptr.c there are a bunch of VLAs in the
handle_ipv4 and handle_ipv6 functions:
const size_t tcphdr_sz = sizeof(struct tcphdr);
const size_t udphdr_sz = sizeof(struct udphdr);
const size_t ethhdr_sz = sizeof(struct ethhdr);
const size_t iphdr_sz = sizeof(struct iphdr);
const size_t ipv6hdr_sz = sizeof(struct ipv6hdr);
[...]
static __always_inline int handle_ipv6(struct xdp_md *xdp, struct bpf_dynptr *xdp_ptr)
{
__u8 eth_buffer[ethhdr_sz + ipv6hdr_sz + ethhdr_sz];
__u8 ip6h_buffer_tcp[ipv6hdr_sz + tcphdr_sz];
__u8 ip6h_buffer_udp[ipv6hdr_sz + udphdr_sz];
[...]
}
static __always_inline int handle_ipv6(struct xdp_md *xdp, struct bpf_dynptr *xdp_ptr)
{
__u8 eth_buffer[ethhdr_sz + ipv6hdr_sz + ethhdr_sz];
__u8 ip6h_buffer_tcp[ipv6hdr_sz + tcphdr_sz];
__u8 ip6h_buffer_udp[ipv6hdr_sz + udphdr_sz];
[...]
}
In both GCC and clang we are not allowing dynamic stack allocation (we
used to support it in GCC using one register as an auxiliary stack
pointer, but not any longer).
The above code builds with clang but not with GCC:
progs/test_xdp_dynptr.c:79:14: error: BPF does not support dynamic stack allocation
79 | __u8 eth_buffer[ethhdr_sz + iphdr_sz + ethhdr_sz];
| ^~~~~~~~~~
We are guessing that clang turns these arrays from VLAs into normal
statically sized arrays because ethhdr_sz and friends are constant and
set to sizeof, which is always known at compile time. This patch
changes the selftest to use preprocessor constants instead of
variables:
#define tcphdr_sz sizeof(struct tcphdr)
#define udphdr_sz sizeof(struct udphdr)
#define ethhdr_sz sizeof(struct ethhdr)
#define iphdr_sz sizeof(struct iphdr)
#define ipv6hdr_sz sizeof(struct ipv6hdr)
Indeed, clang frontend (before generating IR) did some optimization
and calculates the real array size and that is why dynamic stack
allocation didn't happen. Since this is an optimizaiton, there is
no guarantee that frontend is able to calculate the precise
array size in all cases. See llvm patch https://reviews.llvm.org/D111897.
So your above change looks good to me.
- bpf_helpers.h: define bpf_tail_call_static when building with GCC
- bpf: fix constraint in test_tcpbpf_kern.c
GCC emits a warning:
progs/test_tcpbpf_kern.c:60:9: error: ‘op’ is used uninitialized [-Werror=uninitialized]
when the uninitialized automatic `op' is used with a "+r" constraint
in:
asm volatile (
"%[op] = *(u32 *)(%[skops] +96)"
: [op] "+r"(op)
: [skops] "r"(skops)
:);
The constraint shall be "=r" instead.
We may miss an error case like above in llvm. Will double check.
Open Questions
==============
- BPF programs including libc headers.
BPF programs run on their own without an operating system or a C
library. Implementing C implies providing certain definitions and
headers, such as stdint.h and stdarg.h. For such targets, known as
"bare metal targets", the compiler has to provide these definitions
and headers in order to implement the language.
GCC provides the following C headers for BPF targets:
float.h
gcov.h
iso646.h
limits.h
stdalign.h
stdarg.h
stdatomic.h
stdbool.h
stdckdint.h
stddef.h
stdfix.h
stdint.h
stdnoreturn.h
syslimits.h
tgmath.h
unwind.h
varargs.h
However, we have found that there is at least one BPF kernel self test
that include glibc headers that, indirectly, include glibc's own
definitions of stdint.h and friends. This leads to compile-time
errors due to conflicting types. We think that including headers from
a glibc built for some host target is very questionable. For example,
in BPF a C `char' is defined to be signed. But if a BPF program
includes glibc headers in an android system, that code will assume an
unsigned char instead.
Currently clang side does not have compiler side bpf specific header so
we do not have this issues. We do encourage users to use vmlinux.h, e.g.,
for tracing programs, or for all kinds of programs using kfunc's
where parameters likely being kernel structures. In the future, kfunc
definitions are likely to be included in vmlinux.h itself.
For selftests, we also slowly move to vmlinux.h.
