Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2] perf build: Build error in libbpf with EXTRA_CFLAGS="-Wp,-D_FORTIFY_SOURCE=2 -O2" | From | Thomas-Mich Richter <> | Date | Fri, 27 Jul 2018 09:22:03 +0200 |
| |
On 07/27/2018 04:16 AM, Daniel Borkmann wrote: > 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.
Are you sure to stick with strerror_r? I ask because it is the only occurence of strerror_r in this file. All other error messages use strerror as in: pr_warning("failed to create map (name: '%s'): %s\n", map->name, strerror(errno));
$ fgrep strerror tools/lib/bpf/libbpf.c strerror(errno)); issue I try to solve---> strerror_r(-err, errmsg, sizeof(errmsg)); map->name, strerror(errno), errno); strerror(errno)); pr_warning("load bpf program failed: %s\n", strerror(errno)); pr_warning("failed to statfs %s: %s\n", dir, strerror(errno)); pr_warning("failed to pin program: %s\n", strerror(errno)); pr_warning("failed to mkdir %s: %s\n", path, strerror(-err)); pr_warning("failed to pin map: %s\n", strerror(errno)); $
The next issue with strerror_r is to assign the return value to a variable. Then we end up with variable set but not used: libbpf.c: In function ‘bpf_object__elf_collect’: libbpf.c:809:35: error: variable ‘cp’ set but not used [-Werror=unused-but-set-variable] char errmsg[STRERR_BUFSIZE], *cp; ^ cc1: all warnings being treated as errors Here is the source: if (err) { char errmsg[STRERR_BUFSIZE], *cp;
cp = strerror_r(-err, errmsg, sizeof(errmsg)); pr_warning("failed to alloc program %s (%s): %s", name, obj->path, errmsg); }
To fix this requires something like: pr_warning("failed to alloc program %s (%s): %s", name, obj->path, cp);
And after pr_warning() is done, the local array errmsg is deleted.
A lot of overkill in my opinion, unless I miss something.
>>> 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. >
If you want I can send you a separate patch for this.
-- Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany -- Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
| |