On 3/4/20 4:38 PM, Daniel Borkmann wrote:
On 3/4/20 10:37 AM, Toke Høiland-Jørgensen wrote:
Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
On Tue, Mar 3, 2020 at 3:01 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
On 3/3/20 1:32 AM, Andrii Nakryiko wrote:
Switch BPF UAPI constants, previously defined as #define macro, to anonymous
enum values. This preserves constants values and behavior in expressions, but
has added advantaged of being captured as part of DWARF and, subsequently, BTF
type info. Which, in turn, greatly improves usefulness of generated vmlinux.h
for BPF applications, as it will not require BPF users to copy/paste various
flags and constants, which are frequently used with BPF helpers. Only those
constants that are used/useful from BPF program side are converted.
Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
Just thinking out loud, is there some way this could be resolved generically
either from compiler side or via additional tooling where this ends up as BTF
data and thus inside vmlinux.h as anon enum eventually? bpf.h is one single
header and worst case libbpf could also ship a copy of it (?), but what about
all the other things one would need to redefine e.g. for tracing? Small example
that comes to mind are all these TASK_* defines in sched.h etc, and there's
probably dozens of other similar stuff needed too depending on the particular
case; would be nice to have some generic catch-all, hmm.
Enum convertion seems to be the simplest and cleanest way,
unfortunately (as far as I know). DWARF has some extensions capturing
#defines, but values are strings (and need to be parsed, which is pain
already for "1 << 1ULL"), and it's some obscure extension, not a
standard thing. I agree would be nice not to have and change all UAPI
headers for this, but I'm not aware of the solution like that.
Since this is a UAPI header, are we sure that no userspace programs are
using these defines in #ifdefs or something like that?
Hm, yes, anyone doing #ifdefs on them would get build issues. Simple example:
enum {
FOO = 42,
//#define FOO FOO
};
#ifndef FOO
# warning "bar"
#endif
int main(int argc, char **argv)
{
return FOO;
}
$ gcc -Wall -O2 foo.c
foo.c:7:3: warning: #warning "bar" [-Wcpp]
7 | # warning "bar"
| ^~~~~~~
Commenting #define FOO FOO back in fixes it as we discussed in v2:
$ gcc -Wall -O2 foo.c
$
There's also a flag_enum attribute, but with the experiments I tried yesterday
night I couldn't get a warning to trigger for anonymous enums at least, so that
part should be ok.
I was about to push the series out, but agree that there may be a risk for #ifndefs
in the BPF C code. If we want to be on safe side, #define FOO FOO would be needed.
I checked Cilium, LLVM, bcc, bpftrace code, and various others at least there it
seems okay with the current approach, meaning no such if{,n}def seen that would
cause a build warning. Also suricata seems to ship the BPF header itself. But
iproute2 had the following in include/bpf_util.h:
#ifndef BPF_PSEUDO_MAP_FD
# define BPF_PSEUDO_MAP_FD 1
#endif
It's still not what was converted though. I would expect risk might be rather low.
Toke, is there anything on your side affected?
Thanks,
Daniel
