On Wed, 2024-08-21 at 21:29 -0700, Andrii Nakryiko wrote:
[...]
> > + btf_fd = bpf_btf_load(&raw_btf, sizeof(raw_btf), &opts);
> > + saved_errno = errno;
> > + if (btf_fd < 0 || env.verbosity > VERBOSE_NORMAL) {
> > + printf("-------- BTF load log start --------\n");
> > + printf("%s", log);
> > + printf("-------- BTF load log end ----------\n");
> > + }
> > + if (btf_fd < 0) {
> > + PRINT_FAIL("bpf_btf_load() failed, errno=%d\n", saved_errno);
> > + return;
> > + }
> > +
> > + memset(log, 0, sizeof(log));
>
> generally speaking there is no need to memset log buffer (maybe just a
> first byte, to be safe)
Will change.
> on the other hand, just `union bpf_attr attr = {};` is breakage
> waiting to happen, I'd do memset(0) on that, we did run into problems
> with that before (I believe it was systemd)
Compilers optimize out 'smth = {}' where 'smth' escapes?
I mean, I will change it to memset(0), but the fact that you observed
such behaviour is disturbing beyond limit...
I already run into gcc vs clang behaviour differences for the first
iteration of this test where I had:
union bpf_attr {
.prog_type = ...
};
clang did not zero out all members of the union, while gcc did.
> > + attr.prog_btf_fd = btf_fd;
> > + attr.prog_type = BPF_TRACE_RAW_TP;
> > + attr.license = (__u64)"GPL";
> > + attr.insns = (__u64)&insns;
> > + attr.insn_cnt = sizeof(insns) / sizeof(*insns);
> > + attr.log_buf = (__u64)log;
> > + attr.log_size = sizeof(log);
> > + attr.log_level = log_level;
> > + attr.func_info = (__u64)funcs;
> > + attr.func_info_cnt = sizeof(funcs) / sizeof(*funcs);
> > + attr.func_info_rec_size = sizeof(*funcs);
> > + attr.core_relos = (__u64)relos;
> > + attr.core_relo_cnt = sizeof(relos) / sizeof(*relos);
> > + attr.core_relo_rec_size = sizeof(*relos);
>
> I was wondering for a bit why you didn't just use bpf_prog_load(), and
> it seems like it's due to core_relos fields?
Yes, it is in commit message :)
> I don't see why we can't extend the bpf_prog_load() API to allow to
> specify those. (would allow to avoid open-coding this whole bpf_attr
> business, but it's fine as is as well)
Maybe extend API as a followup?
The test won't change much, just options instead of bpf_attr.
[...]
