Messages in this thread | | | Subject | Re: [PATCH v2] perf build: Build error in libbpf with EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2" | From | Daniel Borkmann <> | Date | Fri, 27 Jul 2018 04:16:23 +0200 |
| |
On 07/26/2018 03:48 AM, Jakub Kicinski wrote: > On Wed, 25 Jul 2018 09:21:26 +0200, Thomas Richter wrote: >> commit a5b8bd47dcc57 ("bpf tools: Collect eBPF programs from their own sections") > > Hmm.. are you sure it's not 531b014e7a2f ("tools: bpf: make use of > reallocarray") that caused the issue? That commit made us switch from > XSI-compliant to GNU-specific strerror_r() implementation.. > > /me checks > > Yes it looks like 531b014e7a2f~ builds just fine. > > Daniel, did you try to apply v1 to the bpf tree? Perhaps there is a > confusion about the trees here, if this is caused by my recent change > it's a bpf-next material. strerror() works, but strerror_r() seems > nicer, so perhaps we could keep it if the patch worked in bpf-next?
Yeah indeed, the issue is only in bpf-next. When I compile libbpf from bpf tree with the below flags then it's all good.
Agree that we should rather use strerror_r() given this is a library.
>> causes a compiler error when building the perf tool in the linux-next tree. >> I compile it using a FEDORA 28 installation, my gcc compiler version: >> gcc (GCC) 8.0.1 20180324 (Red Hat 8.0.1-0.20) >> >> The file that causes the error is tools/lib/bpf/libbpf.c >> >> Here is the error message: [...] >> @@ -2334,7 +2331,7 @@ bpf_perf_event_read_simple(void *mem, unsigned long size, >> __u64 data_tail = header->data_tail; >> __u64 data_head = header->data_head; >> void *base, *begin, *end; >> - int ret; >> + int ret = 0; >> >> asm volatile("" ::: "memory"); /* in real code it should be smp_rmb() */ >> if (data_head == data_tail) > > This looks like a separate issue. The ret variable should really be > enum bpf_perf_event_ret, so could you please initialize it to one of the > values of this enum? > > The uninitilized condition can only happen if (data_head != data_tail) > but at the same time (data_head % size == data_tail % size) which > should never really happen... Perhaps initializing to > LIBBPF_PERF_EVENT_ERROR would make sense? > > Or better still adding: > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index f732237610e5..fa5a25945f19 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -2289,6 +2289,8 @@ bpf_perf_event_read_simple(void *mem, unsigned long size, > > begin = base + data_tail % size; > end = base + data_head % size; > + if (being == end) > + return LIBBPF_PERF_EVENT_ERROR;
Sounds good to me.
> while (begin != end) { > struct perf_event_header *ehdr; >
| |