| Message ID | 20190718172513.2394157-1-andriin@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] libbpf: fix missing __WORDSIZE definition | expand |
On Thu, Jul 18, 2019 at 10:25 AM Andrii Nakryiko <andriin@fb.com> wrote: > > hashmap.h depends on __WORDSIZE being defined. It is defined by > glibc/musl in different headers. It's an explicit goal for musl to be > "non-detectable" at compilation time, so instead include glibc header if > glibc is explicitly detected and fall back to musl header otherwise. > > Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap") > Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > --- > tools/lib/bpf/hashmap.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h > index 03748a742146..46a8cb667994 100644 > --- a/tools/lib/bpf/hashmap.h > +++ b/tools/lib/bpf/hashmap.h > @@ -10,7 +10,12 @@ > > #include <stdbool.h> > #include <stddef.h> > +#ifdef __GLIBC__ > +#include <bits/wordsize.h> > +#else > +#include <bits/reg.h> > #include "libbpf_internal.h" > +#endif Disregard this version, #endif on the wrong line. Fixing in v2. > > static inline size_t hash_bits(size_t h, int bits) > { > -- > 2.17.1 >
Em Thu, Jul 18, 2019 at 10:25:13AM -0700, Andrii Nakryiko escreveu: > hashmap.h depends on __WORDSIZE being defined. It is defined by > glibc/musl in different headers. It's an explicit goal for musl to be > "non-detectable" at compilation time, so instead include glibc header if > glibc is explicitly detected and fall back to musl header otherwise. > > Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap") > Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com> > Signed-off-by: Andrii Nakryiko <andriin@fb.com> I fixed this here differently, as below, I didn't send it because I'm still testing it, so far, with a few other fixes and cherry-picking "libbpf: fix ptr to u64 conversion warning on 32-bit platforms" that is still in the bpf tree and is needed for the cross build containers in my suite that are 32-bit, I have the results below, this builds perf + libbpf (where elfutils is available, which is in most cases, except the uCLibc containers due to missing argp-devel), with gcc and with clang: [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz [perfbuilder@quaco linux-perf-tools-build]$ time dm 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final) 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final) 3 alpine:3.6 : Ok gcc (Alpine 6.3.0) 6.3.0, clang version 4.0.0 (tags/RELEASE_400/final) 4 alpine:3.7 : Ok gcc (Alpine 6.4.0) 6.4.0, Alpine clang version 5.0.0 (tags/RELEASE_500/final) (based on LLVM 5.0.0) 5 alpine:3.8 : Ok gcc (Alpine 6.4.0) 6.4.0, Alpine clang version 5.0.1 (tags/RELEASE_501/final) (based on LLVM 5.0.1) 6 alpine:3.9 : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 5.0.1 (tags/RELEASE_502/final) (based on LLVM 5.0.1) 7 alpine:3.10 : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 8.0.0 (tags/RELEASE_800/final) (based on LLVM 8.0.0) 8 alpine:edge : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 7.0.1 (tags/RELEASE_701/final) (based on LLVM 7.0.1) 9 amazonlinux:1 : Ok gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2), clang version 3.6.2 (tags/RELEASE_362/final) 10 amazonlinux:2 : Ok gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5), clang version 7.0.1 (Amazon Linux 2 7.0.1-1.amzn2.0.2) 11 android-ndk:r12b-arm : Ok arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease) 12 android-ndk:r15c-arm : Ok arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease) 13 centos:5 : Ok gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-55) 14 centos:6 : Ok gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) 15 centos:7 : Ok gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-36), clang version 3.4.2 (tags/RELEASE_34/dot2-final) 16 clearlinux:latest : Ok gcc (Clear Linux OS for Intel Architecture) 9.1.1 20190628 gcc-9-branch@272773, clang version 8.0.0 (tags/RELEASE_800/final) 17 debian:8 : Ok gcc (Debian 4.9.2-10+deb8u2) 4.9.2, Debian clang version 3.5.0-10 (tags/RELEASE_350/final) (based on LLVM 3.5.0) 18 debian:9 : Ok gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516, clang version 3.8.1-24 (tags/RELEASE_381/final) 19 debian:10 : Ok gcc (Debian 8.3.0-6) 8.3.0, clang version 7.0.1-8 (tags/RELEASE_701/final) 20 debian:experimental : Ok gcc (Debian 8.3.0-19) 8.3.0, clang version 7.0.1-8 (tags/RELEASE_701/final) 21 debian:experimental-x-arm64 : Ok aarch64-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 22 debian:experimental-x-mips : Ok mips-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 23 debian:experimental-x-mips64 : Ok mips64-linux-gnuabi64-gcc (Debian 8.3.0-7) 8.3.0 24 debian:experimental-x-mipsel : Ok mipsel-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 25 fedora:20 : Ok gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-7), clang version 3.4.2 (tags/RELEASE_34/dot2-final) 26 fedora:22 : Ok gcc (GCC) 5.3.1 20160406 (Red Hat 5.3.1-6), clang version 3.5.0 (tags/RELEASE_350/final) 27 fedora:23 : Ok gcc (GCC) 5.3.1 20160406 (Red Hat 5.3.1-6), clang version 3.7.0 (tags/RELEASE_370/final) 28 fedora:24 : Ok gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1), clang version 3.8.1 (tags/RELEASE_381/final) 29 fedora:24-x-ARC-uClibc : Ok arc-linux-gcc (ARCompact ISA Linux uClibc toolchain 2017.09-rc2) 7.1.1 20170710 I've pushed it to a tmp.perf/core branch in my git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tree, that has these: d5e1f2d60d41 (HEAD -> perf/core, acme.korg/tmp.perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms 7c08fd16f917 tools lib bpf: Avoid designated initializers for unnamed union members 4c9f83c95ad6 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems bdb07df4a0ad tools lib bpf: Fix endianness macro usage for some compilers 66dbf3caff52 tools lib bpf: Replace __WORDSIZE with BITS_PER_LONG to build on the musl libc Please take a look and check if everything is fine on your side. The HEAD I'll remove if Daniel thinks it should wait that landing via the BPF tree, I just put it there for the test builds. commit 66dbf3caff52be0d004bcb9ac4cea4c19eb75dfc Author: Arnaldo Carvalho de Melo <acme@redhat.com> Date: Thu Jul 18 09:46:28 2019 -0300 tools lib bpf: Replace __WORDSIZE with BITS_PER_LONG to build on the musl libc BITS_PER_LONG is more generally available and equivalent to __WORDSIZE, so use it instead to keep it building in systems using the mustl libc where __WORDSIZE is in a different place than in glibc. And do this by explicitely adding the header where this definition is (asm/bitsperlong.h) instead of getting it indirectly. Cc: Adrian Hunter <adrian.hunter@intel.com> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Andrii Nakryiko <andriin@fb.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: Namhyung Kim <namhyung@kernel.org> Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap") Link: https://lkml.kernel.org/n/tip-61vydgldzmmz5w2mf6rv3ryl@git.kernel.org Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h index 03748a742146..f1f37b574d9c 100644 --- a/tools/lib/bpf/hashmap.h +++ b/tools/lib/bpf/hashmap.h @@ -10,12 +10,13 @@ #include <stdbool.h> #include <stddef.h> +#include <asm/bitsperlong.h> #include "libbpf_internal.h" static inline size_t hash_bits(size_t h, int bits) { /* shuffle bits and return requested number of upper bits */ - return (h * 11400714819323198485llu) >> (__WORDSIZE - bits); + return (h * 11400714819323198485llu) >> (BITS_PER_LONG - bits); } typedef size_t (*hashmap_hash_fn)(const void *key, void *ctx);
On Thu, Jul 18, 2019 at 10:55 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Thu, Jul 18, 2019 at 10:25:13AM -0700, Andrii Nakryiko escreveu: > > hashmap.h depends on __WORDSIZE being defined. It is defined by > > glibc/musl in different headers. It's an explicit goal for musl to be > > "non-detectable" at compilation time, so instead include glibc header if > > glibc is explicitly detected and fall back to musl header otherwise. > > > > Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap") > > Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > I fixed this here differently, as below, I didn't send it because I'm still > testing it, so far, with a few other fixes and cherry-picking "libbpf: fix ptr > to u64 conversion warning on 32-bit platforms" that is still in the bpf tree > and is needed for the cross build containers in my suite that are 32-bit, I > have the results below, this builds perf + libbpf (where elfutils is available, > which is in most cases, except the uCLibc containers due to missing argp-devel), > with gcc and with clang: > > [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz > [perfbuilder@quaco linux-perf-tools-build]$ time dm > 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final) > 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final) > 3 alpine:3.6 : Ok gcc (Alpine 6.3.0) 6.3.0, clang version 4.0.0 (tags/RELEASE_400/final) > 4 alpine:3.7 : Ok gcc (Alpine 6.4.0) 6.4.0, Alpine clang version 5.0.0 (tags/RELEASE_500/final) (based on LLVM 5.0.0) > 5 alpine:3.8 : Ok gcc (Alpine 6.4.0) 6.4.0, Alpine clang version 5.0.1 (tags/RELEASE_501/final) (based on LLVM 5.0.1) > 6 alpine:3.9 : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 5.0.1 (tags/RELEASE_502/final) (based on LLVM 5.0.1) > 7 alpine:3.10 : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 8.0.0 (tags/RELEASE_800/final) (based on LLVM 8.0.0) > 8 alpine:edge : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 7.0.1 (tags/RELEASE_701/final) (based on LLVM 7.0.1) > 9 amazonlinux:1 : Ok gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2), clang version 3.6.2 (tags/RELEASE_362/final) > 10 amazonlinux:2 : Ok gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5), clang version 7.0.1 (Amazon Linux 2 7.0.1-1.amzn2.0.2) > 11 android-ndk:r12b-arm : Ok arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease) > 12 android-ndk:r15c-arm : Ok arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease) > 13 centos:5 : Ok gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-55) > 14 centos:6 : Ok gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) > 15 centos:7 : Ok gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-36), clang version 3.4.2 (tags/RELEASE_34/dot2-final) > 16 clearlinux:latest : Ok gcc (Clear Linux OS for Intel Architecture) 9.1.1 20190628 gcc-9-branch@272773, clang version 8.0.0 (tags/RELEASE_800/final) > 17 debian:8 : Ok gcc (Debian 4.9.2-10+deb8u2) 4.9.2, Debian clang version 3.5.0-10 (tags/RELEASE_350/final) (based on LLVM 3.5.0) > 18 debian:9 : Ok gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516, clang version 3.8.1-24 (tags/RELEASE_381/final) > 19 debian:10 : Ok gcc (Debian 8.3.0-6) 8.3.0, clang version 7.0.1-8 (tags/RELEASE_701/final) > 20 debian:experimental : Ok gcc (Debian 8.3.0-19) 8.3.0, clang version 7.0.1-8 (tags/RELEASE_701/final) > 21 debian:experimental-x-arm64 : Ok aarch64-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 > 22 debian:experimental-x-mips : Ok mips-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 > 23 debian:experimental-x-mips64 : Ok mips64-linux-gnuabi64-gcc (Debian 8.3.0-7) 8.3.0 > 24 debian:experimental-x-mipsel : Ok mipsel-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 > 25 fedora:20 : Ok gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-7), clang version 3.4.2 (tags/RELEASE_34/dot2-final) > 26 fedora:22 : Ok gcc (GCC) 5.3.1 20160406 (Red Hat 5.3.1-6), clang version 3.5.0 (tags/RELEASE_350/final) > 27 fedora:23 : Ok gcc (GCC) 5.3.1 20160406 (Red Hat 5.3.1-6), clang version 3.7.0 (tags/RELEASE_370/final) > 28 fedora:24 : Ok gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1), clang version 3.8.1 (tags/RELEASE_381/final) > 29 fedora:24-x-ARC-uClibc : Ok arc-linux-gcc (ARCompact ISA Linux uClibc toolchain 2017.09-rc2) 7.1.1 20170710 > > I've pushed it to a tmp.perf/core branch in my > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tree, that has > these: > > d5e1f2d60d41 (HEAD -> perf/core, acme.korg/tmp.perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms > 7c08fd16f917 tools lib bpf: Avoid designated initializers for unnamed union members > 4c9f83c95ad6 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems > bdb07df4a0ad tools lib bpf: Fix endianness macro usage for some compilers > 66dbf3caff52 tools lib bpf: Replace __WORDSIZE with BITS_PER_LONG to build on the musl libc > > Please take a look and check if everything is fine on your side. The HEAD I'll > remove if Daniel thinks it should wait that landing via the BPF tree, I just put it > there for the test builds. > > commit 66dbf3caff52be0d004bcb9ac4cea4c19eb75dfc > Author: Arnaldo Carvalho de Melo <acme@redhat.com> > Date: Thu Jul 18 09:46:28 2019 -0300 > > tools lib bpf: Replace __WORDSIZE with BITS_PER_LONG to build on the musl libc > > BITS_PER_LONG is more generally available and equivalent to __WORDSIZE, > so use it instead to keep it building in systems using the mustl libc > where __WORDSIZE is in a different place than in glibc. > > And do this by explicitely adding the header where this definition is > (asm/bitsperlong.h) instead of getting it indirectly. > > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Andrii Nakryiko <andriin@fb.com> > Cc: Jiri Olsa <jolsa@kernel.org> > Cc: Namhyung Kim <namhyung@kernel.org> > Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap") > Link: https://lkml.kernel.org/n/tip-61vydgldzmmz5w2mf6rv3ryl@git.kernel.org > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h > index 03748a742146..f1f37b574d9c 100644 > --- a/tools/lib/bpf/hashmap.h > +++ b/tools/lib/bpf/hashmap.h > @@ -10,12 +10,13 @@ > > #include <stdbool.h> > #include <stddef.h> > +#include <asm/bitsperlong.h> Relying on this header is problematic when syncing libbpf into Github mirror. There we'll need to re-implement it anyway, and again, probably through __WORDSIZE or some other tricks. So if we can do away without kernel specific header that would be great. > #include "libbpf_internal.h" > > static inline size_t hash_bits(size_t h, int bits) > { > /* shuffle bits and return requested number of upper bits */ > - return (h * 11400714819323198485llu) >> (__WORDSIZE - bits); > + return (h * 11400714819323198485llu) >> (BITS_PER_LONG - bits); > } > > typedef size_t (*hashmap_hash_fn)(const void *key, void *ctx);
Em Thu, Jul 18, 2019 at 11:04:04AM -0700, Andrii Nakryiko escreveu: > On Thu, Jul 18, 2019 at 10:55 AM Arnaldo Carvalho de Melo > <arnaldo.melo@gmail.com> wrote: > > > > Em Thu, Jul 18, 2019 at 10:25:13AM -0700, Andrii Nakryiko escreveu: > > > hashmap.h depends on __WORDSIZE being defined. It is defined by > > > glibc/musl in different headers. It's an explicit goal for musl to be > > > "non-detectable" at compilation time, so instead include glibc header if > > > glibc is explicitly detected and fall back to musl header otherwise. > > > > > > Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap") > > > Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com> > > > > I fixed this here differently, as below, I didn't send it because I'm still > > testing it, so far, with a few other fixes and cherry-picking "libbpf: fix ptr > > to u64 conversion warning on 32-bit platforms" that is still in the bpf tree > > and is needed for the cross build containers in my suite that are 32-bit, I > > have the results below, this builds perf + libbpf (where elfutils is available, > > which is in most cases, except the uCLibc containers due to missing argp-devel), > > with gcc and with clang: > > > > [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz > > [perfbuilder@quaco linux-perf-tools-build]$ time dm > > 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final) > > 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final) > > 3 alpine:3.6 : Ok gcc (Alpine 6.3.0) 6.3.0, clang version 4.0.0 (tags/RELEASE_400/final) > > 4 alpine:3.7 : Ok gcc (Alpine 6.4.0) 6.4.0, Alpine clang version 5.0.0 (tags/RELEASE_500/final) (based on LLVM 5.0.0) > > 5 alpine:3.8 : Ok gcc (Alpine 6.4.0) 6.4.0, Alpine clang version 5.0.1 (tags/RELEASE_501/final) (based on LLVM 5.0.1) > > 6 alpine:3.9 : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 5.0.1 (tags/RELEASE_502/final) (based on LLVM 5.0.1) > > 7 alpine:3.10 : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 8.0.0 (tags/RELEASE_800/final) (based on LLVM 8.0.0) > > 8 alpine:edge : Ok gcc (Alpine 8.3.0) 8.3.0, Alpine clang version 7.0.1 (tags/RELEASE_701/final) (based on LLVM 7.0.1) > > 9 amazonlinux:1 : Ok gcc (GCC) 7.2.1 20170915 (Red Hat 7.2.1-2), clang version 3.6.2 (tags/RELEASE_362/final) > > 10 amazonlinux:2 : Ok gcc (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5), clang version 7.0.1 (Amazon Linux 2 7.0.1-1.amzn2.0.2) > > 11 android-ndk:r12b-arm : Ok arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease) > > 12 android-ndk:r15c-arm : Ok arm-linux-androideabi-gcc (GCC) 4.9.x 20150123 (prerelease) > > 13 centos:5 : Ok gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-55) > > 14 centos:6 : Ok gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) > > 15 centos:7 : Ok gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-36), clang version 3.4.2 (tags/RELEASE_34/dot2-final) > > 16 clearlinux:latest : Ok gcc (Clear Linux OS for Intel Architecture) 9.1.1 20190628 gcc-9-branch@272773, clang version 8.0.0 (tags/RELEASE_800/final) > > 17 debian:8 : Ok gcc (Debian 4.9.2-10+deb8u2) 4.9.2, Debian clang version 3.5.0-10 (tags/RELEASE_350/final) (based on LLVM 3.5.0) > > 18 debian:9 : Ok gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516, clang version 3.8.1-24 (tags/RELEASE_381/final) > > 19 debian:10 : Ok gcc (Debian 8.3.0-6) 8.3.0, clang version 7.0.1-8 (tags/RELEASE_701/final) > > 20 debian:experimental : Ok gcc (Debian 8.3.0-19) 8.3.0, clang version 7.0.1-8 (tags/RELEASE_701/final) > > 21 debian:experimental-x-arm64 : Ok aarch64-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 > > 22 debian:experimental-x-mips : Ok mips-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 > > 23 debian:experimental-x-mips64 : Ok mips64-linux-gnuabi64-gcc (Debian 8.3.0-7) 8.3.0 > > 24 debian:experimental-x-mipsel : Ok mipsel-linux-gnu-gcc (Debian 8.3.0-19) 8.3.0 > > 25 fedora:20 : Ok gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-7), clang version 3.4.2 (tags/RELEASE_34/dot2-final) > > 26 fedora:22 : Ok gcc (GCC) 5.3.1 20160406 (Red Hat 5.3.1-6), clang version 3.5.0 (tags/RELEASE_350/final) > > 27 fedora:23 : Ok gcc (GCC) 5.3.1 20160406 (Red Hat 5.3.1-6), clang version 3.7.0 (tags/RELEASE_370/final) > > 28 fedora:24 : Ok gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1), clang version 3.8.1 (tags/RELEASE_381/final) > > 29 fedora:24-x-ARC-uClibc : Ok arc-linux-gcc (ARCompact ISA Linux uClibc toolchain 2017.09-rc2) 7.1.1 20170710 > > > > I've pushed it to a tmp.perf/core branch in my > > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tree, that has > > these: > > > > d5e1f2d60d41 (HEAD -> perf/core, acme.korg/tmp.perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms > > 7c08fd16f917 tools lib bpf: Avoid designated initializers for unnamed union members > > 4c9f83c95ad6 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems > > bdb07df4a0ad tools lib bpf: Fix endianness macro usage for some compilers > > 66dbf3caff52 tools lib bpf: Replace __WORDSIZE with BITS_PER_LONG to build on the musl libc > > > > Please take a look and check if everything is fine on your side. The HEAD I'll > > remove if Daniel thinks it should wait that landing via the BPF tree, I just put it > > there for the test builds. > > > > commit 66dbf3caff52be0d004bcb9ac4cea4c19eb75dfc > > Author: Arnaldo Carvalho de Melo <acme@redhat.com> > > Date: Thu Jul 18 09:46:28 2019 -0300 > > > > tools lib bpf: Replace __WORDSIZE with BITS_PER_LONG to build on the musl libc > > > > BITS_PER_LONG is more generally available and equivalent to __WORDSIZE, > > so use it instead to keep it building in systems using the mustl libc > > where __WORDSIZE is in a different place than in glibc. > > > > And do this by explicitely adding the header where this definition is > > (asm/bitsperlong.h) instead of getting it indirectly. > > > > Cc: Adrian Hunter <adrian.hunter@intel.com> > > Cc: Alexei Starovoitov <ast@kernel.org> > > Cc: Andrii Nakryiko <andriin@fb.com> > > Cc: Jiri Olsa <jolsa@kernel.org> > > Cc: Namhyung Kim <namhyung@kernel.org> > > Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap") > > Link: https://lkml.kernel.org/n/tip-61vydgldzmmz5w2mf6rv3ryl@git.kernel.org > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > > > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h > > index 03748a742146..f1f37b574d9c 100644 > > --- a/tools/lib/bpf/hashmap.h > > +++ b/tools/lib/bpf/hashmap.h > > @@ -10,12 +10,13 @@ > > > > #include <stdbool.h> > > #include <stddef.h> > > +#include <asm/bitsperlong.h> > > Relying on this header is problematic when syncing libbpf into Github > mirror. There we'll need to re-implement it anyway, and again, > probably through __WORDSIZE or some other tricks. So if we can do away > without kernel specific header that would be great. [acme@quaco perf]$ rpm -qf /usr/include/asm/bitsperlong.h kernel-headers-5.1.16-300.fc30.x86_64 It is kernel specific, but it comes from a distro package, and one that is even required to build anything that uses glibc: [acme@quaco perf]$ rpm -q --whatrequires kernel-headers glibc-headers-2.29-15.fc30.x86_64 audit-libs-devel-3.0-0.9.20190507gitf58ec40.fc30.x86_64 libnl3-devel-3.4.0-8.fc30.x86_64 [acme@quaco perf]$ [acme@quaco perf]$ rpm -q --whatrequires glibc-headers glibc-devel-2.29-15.fc30.x86_64 [acme@quaco perf]$ It is available everywhere: Alpine, for instance: $ dsh alpine:3.10 / $ apk info --who-owns /usr/include/asm/bitsperlong.h /usr/include/asm/bitsperlong.h is owned by linux-headers-4.19.36-r0 / $ / $ cat /etc/alpine-release 3.10.0 OpenSuSE: $ dsh opensuse:42.3 sh-4.3$ grep PRETTY /etc/os-release PRETTY_NAME="openSUSE Leap 42.3" sh-4.3$ ls -la /usr/include/asm/bitsperlong.h -rw-r--r--. 1 root root 258 Jan 15 2016 /usr/include/asm/bitsperlong.h sh-4.3$ set -o vi sh-4.3$ rpm -qf /usr/include/asm/bitsperlong.h linux-glibc-devel-4.4-6.3.1.noarch sh-4.3$ sh-4.3$ exit ClearLinux: $ dsh clearlinux:latest sh-5.0$ ls -la /usr/include/asm/bitsperlong.h -rw-r--r-- 2 root root 321 Feb 1 20:26 /usr/include/asm/bitsperlong.h sh-5.0$ I see what you mean tho, tools/arch and tools/include _have_ bitsperlong.h as well and the perf build is using that... And I checked and the above don't have BITS_PER_LONG, just some have __BITS_PER_LONG, etc... /me scratches head, so what we ended up in the tools/include to solve that was: #ifdef __SIZEOF_LONG__ #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) #else #define BITS_PER_LONG __WORDSIZE #endif To help in you deciding how you want to fix this: commit e81fcd43723d32e9c9dbb8e8d66f147b5b84256b Author: Peter Zijlstra <peterz@infradead.org> Date: Fri Jul 15 12:38:18 2016 -0300 tools: Simplify BITS_PER_LONG define Do it using (__CHAR_BIT__ * __SIZEOF_LONG__), simpler, works everywhere, reduces the complexity by ditching CONFIG_64BIT, that was being synthesized from yet another set of defines, which proved fragile, breaking the build on linux-next for no obvious reasons. Committer Note: Except on: gcc version 4.1.2 20080704 (Red Hat 4.1.2-55) Fallback to __WORDSIZE in that case... Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Signed-off-by: Peter Zijlstra <peterz@infradead.org> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Andy Lutomirski <luto@amacapital.net> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/20160715072243.GP30154@twins.programming.kicks-ass.net Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> Using the tools/{include,arch} headers as perf does it is continuing to build without failures, in addition to the set of containers first reported: 30 fedora:25 : Ok gcc (GCC) 6.4.1 20170727 (Red Hat 6.4.1-1), clang version 3.9.1 (tags/RELEASE_391/final) 31 fedora:26 : Ok gcc (GCC) 7.3.1 20180130 (Red Hat 7.3.1-2), clang version 4.0.1 (tags/RELEASE_401/final) 32 fedora:27 : Ok gcc (GCC) 7.3.1 20180712 (Red Hat 7.3.1-6), clang version 5.0.2 (tags/RELEASE_502/final) 33 fedora:28 : Ok gcc (GCC) 8.3.1 20190223 (Red Hat 8.3.1-2), clang version 6.0.1 (tags/RELEASE_601/final) 34 fedora:29 : Ok gcc (GCC) 8.3.1 20190223 (Red Hat 8.3.1-2), clang version 7.0.1 (Fedora 7.0.1-6.fc29) 35 fedora:30 : Ok gcc (GCC) 9.1.1 20190503 (Red Hat 9.1.1-1), clang version 8.0.0 (Fedora 8.0.0-1.fc30) 36 fedora:30-x-ARC-glibc : Ok arc-linux-gcc (ARC HS GNU/Linux glibc toolchain 2019.03-rc1) 8.3.1 20190225 37 fedora:30-x-ARC-uClibc : Ok arc-linux-gcc (ARCv2 ISA Linux uClibc toolchain 2019.03-rc1) 8.3.1 20190225 38 fedora:31 : Ok gcc (GCC) 9.1.1 20190605 (Red Hat 9.1.1-2), clang version 8.0.0 (Fedora 8.0.0-3.fc31) 39 fedora:rawhide : Ok gcc (GCC) 9.1.1 20190605 (Red Hat 9.1.1-2), clang version 8.0.0 (Fedora 8.0.0-3.fc31) 40 gentoo-stage3-amd64:latest : Ok gcc (Gentoo 8.3.0-r1 p1.1) 8.3.0 41 mageia:5 : Ok gcc (GCC) 4.9.2, clang version 3.5.2 (tags/RELEASE_352/final) 42 mageia:6 : Ok gcc (Mageia 5.5.0-1.mga6) 5.5.0, clang version 3.9.1 (tags/RELEASE_391/final) I'll stop and replace my patch with yours to see if it survives all the test builds... - Arnaldo > > #include "libbpf_internal.h" > > > > static inline size_t hash_bits(size_t h, int bits) > > { > > /* shuffle bits and return requested number of upper bits */ > > - return (h * 11400714819323198485llu) >> (__WORDSIZE - bits); > > + return (h * 11400714819323198485llu) >> (BITS_PER_LONG - bits); > > } > > > > typedef size_t (*hashmap_hash_fn)(const void *key, void *ctx);
Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu: > I'll stop and replace my patch with yours to see if it survives all the > test builds... So, Alpine:3.4, the first image for this distro I did when I started these builds, survives the 6 builds with gcc and clang with your patch: [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz [perfbuilder@quaco linux-perf-tools-build]$ dm 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final) [perfbuilder@quaco linux-perf-tools-build]$ grep "+ make" dm.log/alpine\:3.4 + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf CC=clang + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf [perfbuilder@quaco linux-perf-tools-build]$ Probably all the rest will go well, will let you know. Daniel, do you mind if I carry this one in my perf/core branch? Its small and shouldn't clash with other patches, I think. It should go upstream soon: Andrii, there are these others: 8dfb6ed300bf tools lib bpf: Avoid designated initializers for unnamed union members 80f7f8f21441 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems d93fe741e291 tools lib bpf: Fix endianness macro usage for some compilers If you could take a look at them at my tmp.perf/core branch at: https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/core I'm force pushing it now to replace my __WORDSIZE patch with yours, the SHAs should be the 3 above and the one below. And to build cleanly I had to cherry pick this one: 3091dafc1884 (HEAD -> perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms Alpine:3.5 just finished: 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final) - Arnaldo
On Thu, Jul 18, 2019 at 12:14 PM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu: > > I'll stop and replace my patch with yours to see if it survives all the > > test builds... > > So, Alpine:3.4, the first image for this distro I did when I started > these builds, survives the 6 builds with gcc and clang with your patch: > > > [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz > [perfbuilder@quaco linux-perf-tools-build]$ dm > 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final) > > > [perfbuilder@quaco linux-perf-tools-build]$ grep "+ make" dm.log/alpine\:3.4 > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf > [perfbuilder@quaco linux-perf-tools-build]$ > > Probably all the rest will go well, will let you know. > > Daniel, do you mind if I carry this one in my perf/core branch? Its > small and shouldn't clash with other patches, I think. It should go > upstream soon: > > Andrii, there are these others: I took a look at them, but I think it would be better, if you could post them as proper patches to bpf@vger.kernel.org/netdev@vger.kernel.org, so that others can check and comment, if necessary. One nit for all three of them: we typically prefix subject with just "libbpf: " instead of "tools lib libbpf". > > 8dfb6ed300bf tools lib bpf: Avoid designated initializers for unnamed union members + attr.sample_period = attr.wakeup_events = 1; let's instead + attr.sample_period = 1; + attr.wakeup_events = 1; I don't like multi-assignments. Also, if we are doing explicit assignment, let's do it for all the fields, not split initialization like that. > 80f7f8f21441 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems For this one I'm confused. What compiler/system you are getting it on? I tried to reproduce it with this example (trying both global variable, as well as function): #include <stdio.h> //int link = 1; void link() {} int f(int link) { return link; } int main() { printf("%d\n", f(123)); return 0; } I haven't gotten any errors nor warnings. I'm certainly liking existing naming better, but my main concern is that we'll probably add more code like this, and we'll forget about this problem and will re-introduce. So I'd rather figure out why it's happening and some rare system and see if we can mitigate that without all the renames. > d93fe741e291 tools lib bpf: Fix endianness macro usage for some compilers This one looks good to me, thanks! Acked-by: Andrii Nakryiko <andriin@fb.com> > > If you could take a look at them at my tmp.perf/core branch at: > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/core > > I'm force pushing it now to replace my __WORDSIZE patch with yours, the > SHAs should be the 3 above and the one below. > > And to build cleanly I had to cherry pick this one: > > 3091dafc1884 (HEAD -> perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms > > Alpine:3.5 just finished: > > 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final) > > - Arnaldo
Em Thu, Jul 18, 2019 at 02:16:29PM -0700, Andrii Nakryiko escreveu: > On Thu, Jul 18, 2019 at 12:14 PM Arnaldo Carvalho de Melo > <arnaldo.melo@gmail.com> wrote: > > > > Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu: > > > I'll stop and replace my patch with yours to see if it survives all the > > > test builds... > > > > So, Alpine:3.4, the first image for this distro I did when I started > > these builds, survives the 6 builds with gcc and clang with your patch: > > > > > > [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz > > [perfbuilder@quaco linux-perf-tools-build]$ dm > > 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final) > > > > > > [perfbuilder@quaco linux-perf-tools-build]$ grep "+ make" dm.log/alpine\:3.4 > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf > > [perfbuilder@quaco linux-perf-tools-build]$ > > > > Probably all the rest will go well, will let you know. > > > > Daniel, do you mind if I carry this one in my perf/core branch? Its > > small and shouldn't clash with other patches, I think. It should go > > upstream soon: > > > > Andrii, there are these others: > > I took a look at them, but I think it would be better, if you could > post them as proper patches to > bpf@vger.kernel.org/netdev@vger.kernel.org, so that others can check > and comment, if necessary. > > One nit for all three of them: we typically prefix subject with just > "libbpf: " instead of "tools lib libbpf". Sure, that was mechanic, I do it like that for the patches I upstream, and that was like that in the beginning: [acme@quaco perf]$ git log --oneline tools/lib/bpf | grep lib | tail 9d759a9b4ac2 tools lib bpf: Collect map definition in bpf_object d8ad6a15cc3a tools lib bpf: Don't do a feature check when cleaning 6371ca3b541c bpf tools: Improve libbpf error reporting 0c77c04aa9c2 tools lib bpf: Change FEATURE-DUMP to FEATURE-DUMP.libbpf 715f8db9102f tools lib bpf: Fix compiler warning on CentOS 6 7c422f557266 tools build: Build fixdep helper from perf and basic libs 65f041bee783 tools lib bpf: Use FEATURE_USER to allow building in the same dir as perf 20517cd9c593 tools lib bpf: Fix up FEATURE_{TESTS,DISPLAY} usage cc4228d57c4c bpf tools: Check endianness and make libbpf fail early 1b76c13e4b36 bpf tools: Introduce 'bpf' library and add bpf feature check [acme@quaco perf]$ Anyway, I'll resubmit the patches that you acked to bpf@vger and will let my container tests fail for those cases, sticking a warning so that Ingo knows that this is being dealt with and those problems will get fixed soon when the bpf tree merges upstream. > > > > 8dfb6ed300bf tools lib bpf: Avoid designated initializers for unnamed union members > > + attr.sample_period = attr.wakeup_events = 1; > > let's instead > > + attr.sample_period = 1; > + attr.wakeup_events = 1; > > I don't like multi-assignments. Meh, what's wrong with it? :) > Also, if we are doing explicit assignment, let's do it for all the > fields, not split initialization like that. If that is what takes to get it to build everywhere, no problem. In tools/perf I'm used to doing it, documents that this is an oddity to support more systems :) > > 80f7f8f21441 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems > > For this one I'm confused. What compiler/system you are getting it on? > I tried to reproduce it with this example (trying both global > variable, as well as function): > > #include <stdio.h> > > //int link = 1; > void link() {} > > int f(int link) { > return link; > } > int main() { > printf("%d\n", f(123)); > return 0; > } > > I haven't gotten any errors nor warnings. I'm certainly liking > existing naming better, but my main concern is that we'll probably add > more code like this, and we'll forget about this problem and will > re-introduce. yeah, this happens from time to time with centos:6 and IIRC amazonlinux:1, oraclelinux:6. I still remember when I got reports from the twitter guys when something broke on rhel:5, that was the main reason to get these container tests in place, you never know where people are using this, and since before upstreaming I do the tests, fixing those became second nature 8-) > So I'd rather figure out why it's happening and some rare system and > see if we can mitigate that without all the renames. > > > > d93fe741e291 tools lib bpf: Fix endianness macro usage for some compilers > > This one looks good to me, thanks! > > Acked-by: Andrii Nakryiko <andriin@fb.com> Ok, will collect this, change the prefix to: "libbpf: ..." and send to bpf@vger > > > > > If you could take a look at them at my tmp.perf/core branch at: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/core > > > > I'm force pushing it now to replace my __WORDSIZE patch with yours, the > > SHAs should be the 3 above and the one below. > > > > And to build cleanly I had to cherry pick this one: > > > > 3091dafc1884 (HEAD -> perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms > > > > Alpine:3.5 just finished: > > > > 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final) > > > > - Arnaldo
On Thu, Jul 18, 2019 at 6:16 PM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Thu, Jul 18, 2019 at 02:16:29PM -0700, Andrii Nakryiko escreveu: > > On Thu, Jul 18, 2019 at 12:14 PM Arnaldo Carvalho de Melo > > <arnaldo.melo@gmail.com> wrote: > > > > > > Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > I'll stop and replace my patch with yours to see if it survives all the > > > > test builds... > > > > > > So, Alpine:3.4, the first image for this distro I did when I started > > > these builds, survives the 6 builds with gcc and clang with your patch: > > > > > > > > > [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz > > > [perfbuilder@quaco linux-perf-tools-build]$ dm > > > 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final) > > > > > > > > > [perfbuilder@quaco linux-perf-tools-build]$ grep "+ make" dm.log/alpine\:3.4 > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf > > > [perfbuilder@quaco linux-perf-tools-build]$ > > > > > > Probably all the rest will go well, will let you know. > > > > > > Daniel, do you mind if I carry this one in my perf/core branch? Its > > > small and shouldn't clash with other patches, I think. It should go > > > upstream soon: > > > > > > Andrii, there are these others: > > > > I took a look at them, but I think it would be better, if you could > > post them as proper patches to > > bpf@vger.kernel.org/netdev@vger.kernel.org, so that others can check > > and comment, if necessary. > > > > One nit for all three of them: we typically prefix subject with just > > "libbpf: " instead of "tools lib libbpf". > > Sure, that was mechanic, I do it like that for the patches I upstream, > and that was like that in the beginning: > > [acme@quaco perf]$ git log --oneline tools/lib/bpf | grep lib | tail > 9d759a9b4ac2 tools lib bpf: Collect map definition in bpf_object > d8ad6a15cc3a tools lib bpf: Don't do a feature check when cleaning > 6371ca3b541c bpf tools: Improve libbpf error reporting > 0c77c04aa9c2 tools lib bpf: Change FEATURE-DUMP to FEATURE-DUMP.libbpf > 715f8db9102f tools lib bpf: Fix compiler warning on CentOS 6 > 7c422f557266 tools build: Build fixdep helper from perf and basic libs > 65f041bee783 tools lib bpf: Use FEATURE_USER to allow building in the same dir as perf > 20517cd9c593 tools lib bpf: Fix up FEATURE_{TESTS,DISPLAY} usage > cc4228d57c4c bpf tools: Check endianness and make libbpf fail early > 1b76c13e4b36 bpf tools: Introduce 'bpf' library and add bpf feature check > [acme@quaco perf]$ > > Anyway, I'll resubmit the patches that you acked to bpf@vger and will > let my container tests fail for those cases, sticking a warning so that > Ingo knows that this is being dealt with and those problems will get > fixed soon when the bpf tree merges upstream. Great, thanks! > > > > > > > 8dfb6ed300bf tools lib bpf: Avoid designated initializers for unnamed union members > > > > + attr.sample_period = attr.wakeup_events = 1; > > > > let's instead > > > > + attr.sample_period = 1; > > + attr.wakeup_events = 1; > > > > I don't like multi-assignments. > > Meh, what's wrong with it? :) Nothing, objectively :) But I don't remember seeing multi-assignments in libbpf code base, so nitpicking for consistency's sake.... > > > Also, if we are doing explicit assignment, let's do it for all the > > fields, not split initialization like that. > > If that is what takes to get it to build everywhere, no problem. In > tools/perf I'm used to doing it, documents that this is an oddity to > support more systems :) > > > > 80f7f8f21441 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems > > > > For this one I'm confused. What compiler/system you are getting it on? > > > I tried to reproduce it with this example (trying both global > > variable, as well as function): > > > > #include <stdio.h> > > > > //int link = 1; > > void link() {} > > > > int f(int link) { > > return link; > > } > > int main() { > > printf("%d\n", f(123)); > > return 0; > > } > > > > I haven't gotten any errors nor warnings. I'm certainly liking > > existing naming better, but my main concern is that we'll probably add > > more code like this, and we'll forget about this problem and will > > re-introduce. > > yeah, this happens from time to time with centos:6 and IIRC > amazonlinux:1, oraclelinux:6. > > I still remember when I got reports from the twitter guys when something > broke on rhel:5, that was the main reason to get these container tests > in place, you never know where people are using this, and since before > upstreaming I do the tests, fixing those became second nature 8-) > > > So I'd rather figure out why it's happening and some rare system and > > see if we can mitigate that without all the renames. Ok, did some more googling. This warning (turned error in your setup) is emitted when -Wshadow option is enabled for GCC/clang. It appears to be disabled by default, so it must be enabled somewhere for perf build or something. Would it be possible to disable it at least for libbpf when building from perf either everywhere or for those systems where you see this warning? I don't think this warning is useful, to be honest, just random name conflict between any local and global variables will cause this. > > > > > > > d93fe741e291 tools lib bpf: Fix endianness macro usage for some compilers > > > > This one looks good to me, thanks! > > > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > Ok, will collect this, change the prefix to: "libbpf: ..." and send to > bpf@vger > > > > > > > > > If you could take a look at them at my tmp.perf/core branch at: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/log/?h=tmp.perf/core > > > > > > I'm force pushing it now to replace my __WORDSIZE patch with yours, the > > > SHAs should be the 3 above and the one below. > > > > > > And to build cleanly I had to cherry pick this one: > > > > > > 3091dafc1884 (HEAD -> perf/core) libbpf: fix ptr to u64 conversion warning on 32-bit platforms > > > > > > Alpine:3.5 just finished: > > > > > > 2 alpine:3.5 : Ok gcc (Alpine 6.2.1) 6.2.1 20160822, clang version 3.8.1 (tags/RELEASE_381/final) > > > > > > - Arnaldo > > -- > > - Arnaldo
Em Fri, Jul 19, 2019 at 10:54:44AM -0700, Andrii Nakryiko escreveu: > On Thu, Jul 18, 2019 at 6:16 PM Arnaldo Carvalho de Melo > <arnaldo.melo@gmail.com> wrote: > > > > Em Thu, Jul 18, 2019 at 02:16:29PM -0700, Andrii Nakryiko escreveu: > > > On Thu, Jul 18, 2019 at 12:14 PM Arnaldo Carvalho de Melo > > > <arnaldo.melo@gmail.com> wrote: > > > > > > > > Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > > I'll stop and replace my patch with yours to see if it survives all the > > > > > test builds... > > > > > > > > So, Alpine:3.4, the first image for this distro I did when I started > > > > these builds, survives the 6 builds with gcc and clang with your patch: > > > > > > > > > > > > [perfbuilder@quaco linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz > > > > [perfbuilder@quaco linux-perf-tools-build]$ dm > > > > 1 alpine:3.4 : Ok gcc (Alpine 5.3.0) 5.3.0, clang version 3.8.0 (tags/RELEASE_380/final) > > > > > > > > > > > > [perfbuilder@quaco linux-perf-tools-build]$ grep "+ make" dm.log/alpine\:3.4 > > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf > > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf > > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang > > > > + make ARCH= CROSS_COMPILE= EXTRA_CFLAGS= LIBCLANGLLVM=1 -C /git/linux/tools/perf O=/tmp/build/perf > > > > [perfbuilder@quaco linux-perf-tools-build]$ > > > > > > > > Probably all the rest will go well, will let you know. > > > > > > > > Daniel, do you mind if I carry this one in my perf/core branch? Its > > > > small and shouldn't clash with other patches, I think. It should go > > > > upstream soon: > > > > > > > > Andrii, there are these others: > > > > > > I took a look at them, but I think it would be better, if you could > > > post them as proper patches to > > > bpf@vger.kernel.org/netdev@vger.kernel.org, so that others can check > > > and comment, if necessary. > > > > > > One nit for all three of them: we typically prefix subject with just > > > "libbpf: " instead of "tools lib libbpf". > > > > Sure, that was mechanic, I do it like that for the patches I upstream, > > and that was like that in the beginning: > > > > [acme@quaco perf]$ git log --oneline tools/lib/bpf | grep lib | tail > > 9d759a9b4ac2 tools lib bpf: Collect map definition in bpf_object > > d8ad6a15cc3a tools lib bpf: Don't do a feature check when cleaning > > 6371ca3b541c bpf tools: Improve libbpf error reporting > > 0c77c04aa9c2 tools lib bpf: Change FEATURE-DUMP to FEATURE-DUMP.libbpf > > 715f8db9102f tools lib bpf: Fix compiler warning on CentOS 6 > > 7c422f557266 tools build: Build fixdep helper from perf and basic libs > > 65f041bee783 tools lib bpf: Use FEATURE_USER to allow building in the same dir as perf > > 20517cd9c593 tools lib bpf: Fix up FEATURE_{TESTS,DISPLAY} usage > > cc4228d57c4c bpf tools: Check endianness and make libbpf fail early > > 1b76c13e4b36 bpf tools: Introduce 'bpf' library and add bpf feature check > > [acme@quaco perf]$ > > > > Anyway, I'll resubmit the patches that you acked to bpf@vger and will > > let my container tests fail for those cases, sticking a warning so that > > Ingo knows that this is being dealt with and those problems will get > > fixed soon when the bpf tree merges upstream. > > Great, thanks! > > > > > > > > > > > 8dfb6ed300bf tools lib bpf: Avoid designated initializers for unnamed union members > > > > > > + attr.sample_period = attr.wakeup_events = 1; > > > > > > let's instead > > > > > > + attr.sample_period = 1; > > > + attr.wakeup_events = 1; > > > > > > I don't like multi-assignments. > > > > Meh, what's wrong with it? :) > > Nothing, objectively :) But I don't remember seeing multi-assignments > in libbpf code base, so nitpicking for consistency's sake.... > > > > > > > Also, if we are doing explicit assignment, let's do it for all the > > > fields, not split initialization like that. > > > > If that is what takes to get it to build everywhere, no problem. In > > tools/perf I'm used to doing it, documents that this is an oddity to > > support more systems :) > > > > > > 80f7f8f21441 tools lib bpf: Avoid using 'link' as it shadows a global definition in some systems > > > > > > For this one I'm confused. What compiler/system you are getting it on? > > > > > I tried to reproduce it with this example (trying both global > > > variable, as well as function): > > > > > > #include <stdio.h> > > > > > > //int link = 1; > > > void link() {} > > > > > > int f(int link) { > > > return link; > > > } > > > int main() { > > > printf("%d\n", f(123)); > > > return 0; > > > } > > > > > > I haven't gotten any errors nor warnings. I'm certainly liking > > > existing naming better, but my main concern is that we'll probably add > > > more code like this, and we'll forget about this problem and will > > > re-introduce. > > > > yeah, this happens from time to time with centos:6 and IIRC > > amazonlinux:1, oraclelinux:6. > > > > I still remember when I got reports from the twitter guys when something > > broke on rhel:5, that was the main reason to get these container tests > > in place, you never know where people are using this, and since before > > upstreaming I do the tests, fixing those became second nature 8-) > > > > > So I'd rather figure out why it's happening and some rare system and > > > see if we can mitigate that without all the renames. > > Ok, did some more googling. This warning (turned error in your setup) > is emitted when -Wshadow option is enabled for GCC/clang. It appears > to be disabled by default, so it must be enabled somewhere for perf > build or something. Right, I came to the exact same conclusion, doing tests here: [perfbuilder@3a58896a648d tmp]$ gcc -Wshadow shadow_global_decl.c -o shadow_global_decl shadow_global_decl.c: In function 'main': shadow_global_decl.c:9: warning: declaration of 'link' shadows a global declaration shadow_global_decl.c:4: warning: shadowed declaration is here [perfbuilder@3a58896a648d tmp]$ gcc --version |& head -1 gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) [perfbuilder@3a58896a648d tmp]$ gcc shadow_global_decl.c -o shadow_global_decl [perfbuilder@3a58896a648d tmp]$ So I'm going to remove this warning from the places where it causes problems. > Would it be possible to disable it at least for libbpf when building > from perf either everywhere or for those systems where you see this > warning? I don't think this warning is useful, to be honest, just > random name conflict between any local and global variables will cause > this. Yeah, I might end up having this applied. [acme@quaco perf]$ git diff diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include index 495066bafbe3..b6e902a2312f 100644 --- a/tools/scripts/Makefile.include +++ b/tools/scripts/Makefile.include @@ -32,7 +32,6 @@ EXTRA_WARNINGS += -Wno-system-headers EXTRA_WARNINGS += -Wold-style-definition EXTRA_WARNINGS += -Wpacked EXTRA_WARNINGS += -Wredundant-decls -EXTRA_WARNINGS += -Wshadow EXTRA_WARNINGS += -Wstrict-prototypes EXTRA_WARNINGS += -Wswitch-default EXTRA_WARNINGS += -Wswitch-enum [acme@quaco perf]$ Sorry for the noise... - Arnaldo
On Fri, Jul 19, 2019 at 11:14 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Fri, Jul 19, 2019 at 10:54:44AM -0700, Andrii Nakryiko escreveu: > > On Thu, Jul 18, 2019 at 6:16 PM Arnaldo Carvalho de Melo > > <arnaldo.melo@gmail.com> wrote: > > > > > > Em Thu, Jul 18, 2019 at 02:16:29PM -0700, Andrii Nakryiko escreveu: > > > > On Thu, Jul 18, 2019 at 12:14 PM Arnaldo Carvalho de Melo > > > > <arnaldo.melo@gmail.com> wrote: > > > > > > > > > > Em Thu, Jul 18, 2019 at 03:56:19PM -0300, Arnaldo Carvalho de Melo escreveu: > > > > > > I'll stop and replace my patch with yours to see if it survives all the > > > > > > test builds... > > > > > > > > > > So, Alpine:3.4, the first image for this distro I did when I started > > > > > these builds, survives the 6 builds with gcc and clang with your patch: > > > > > > > > > > [...] > > > > Ok, did some more googling. This warning (turned error in your setup) > > is emitted when -Wshadow option is enabled for GCC/clang. It appears > > to be disabled by default, so it must be enabled somewhere for perf > > build or something. > > Right, I came to the exact same conclusion, doing tests here: > > [perfbuilder@3a58896a648d tmp]$ gcc -Wshadow shadow_global_decl.c -o shadow_global_decl > shadow_global_decl.c: In function 'main': > shadow_global_decl.c:9: warning: declaration of 'link' shadows a global declaration > shadow_global_decl.c:4: warning: shadowed declaration is here > [perfbuilder@3a58896a648d tmp]$ gcc --version |& head -1 > gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) > [perfbuilder@3a58896a648d tmp]$ gcc shadow_global_decl.c -o shadow_global_decl > [perfbuilder@3a58896a648d tmp]$ > > So I'm going to remove this warning from the places where it causes > problems. > > > Would it be possible to disable it at least for libbpf when building > > from perf either everywhere or for those systems where you see this > > warning? I don't think this warning is useful, to be honest, just > > random name conflict between any local and global variables will cause > > this. > > Yeah, I might end up having this applied. Thanks! > > [acme@quaco perf]$ git diff > diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include > index 495066bafbe3..b6e902a2312f 100644 > --- a/tools/scripts/Makefile.include > +++ b/tools/scripts/Makefile.include > @@ -32,7 +32,6 @@ EXTRA_WARNINGS += -Wno-system-headers > EXTRA_WARNINGS += -Wold-style-definition > EXTRA_WARNINGS += -Wpacked > EXTRA_WARNINGS += -Wredundant-decls > -EXTRA_WARNINGS += -Wshadow > EXTRA_WARNINGS += -Wstrict-prototypes > EXTRA_WARNINGS += -Wswitch-default > EXTRA_WARNINGS += -Wswitch-enum > [acme@quaco perf]$ > > > Sorry for the noise... No worries, I learned something new today :) > > - Arnaldo
Em Fri, Jul 19, 2019 at 11:26:50AM -0700, Andrii Nakryiko escreveu: > On Fri, Jul 19, 2019 at 11:14 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Fri, Jul 19, 2019 at 10:54:44AM -0700, Andrii Nakryiko escreveu: > > > Ok, did some more googling. This warning (turned error in your setup) > > > is emitted when -Wshadow option is enabled for GCC/clang. It appears > > > to be disabled by default, so it must be enabled somewhere for perf > > > build or something. > > Right, I came to the exact same conclusion, doing tests here: > > [perfbuilder@3a58896a648d tmp]$ gcc -Wshadow shadow_global_decl.c -o shadow_global_decl > > shadow_global_decl.c: In function 'main': > > shadow_global_decl.c:9: warning: declaration of 'link' shadows a global declaration > > shadow_global_decl.c:4: warning: shadowed declaration is here > > [perfbuilder@3a58896a648d tmp]$ gcc --version |& head -1 > > gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) > > [perfbuilder@3a58896a648d tmp]$ gcc shadow_global_decl.c -o shadow_global_decl > > [perfbuilder@3a58896a648d tmp]$ > > So I'm going to remove this warning from the places where it causes > > problems. > > > Would it be possible to disable it at least for libbpf when building > > > from perf either everywhere or for those systems where you see this > > > warning? I don't think this warning is useful, to be honest, just > > > random name conflict between any local and global variables will cause > > > this. > > Yeah, I might end up having this applied. > Thanks! So, I'm ending up with the patch below, there is some value after all in Wshadow, that is, from gcc 4.8 onwards :-) - Arnaldo diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include index 495066bafbe3..ded7a950dc40 100644 --- a/tools/scripts/Makefile.include +++ b/tools/scripts/Makefile.include @@ -32,7 +32,6 @@ EXTRA_WARNINGS += -Wno-system-headers EXTRA_WARNINGS += -Wold-style-definition EXTRA_WARNINGS += -Wpacked EXTRA_WARNINGS += -Wredundant-decls -EXTRA_WARNINGS += -Wshadow EXTRA_WARNINGS += -Wstrict-prototypes EXTRA_WARNINGS += -Wswitch-default EXTRA_WARNINGS += -Wswitch-enum @@ -69,8 +68,16 @@ endif # will do for now and keep the above -Wstrict-aliasing=3 in place # in newer systems. # Needed for the __raw_cmpxchg in tools/arch/x86/include/asm/cmpxchg.h +# +# See https://lkml.org/lkml/2006/11/28/253 and https://gcc.gnu.org/gcc-4.8/changes.html, +# that takes into account Linus's comments (search for Wshadow) for the reasoning about +# -Wshadow not being interesting before gcc 4.8. + ifneq ($(filter 3.%,$(MAKE_VERSION)),) # make-3 EXTRA_WARNINGS += -fno-strict-aliasing +EXTRA_WARNINGS += -Wno-shadow +else +EXTRA_WARNINGS += -Wshadow endif ifneq ($(findstring $(MAKEFLAGS), w),w)
On Fri, Jul 19, 2019 at 11:34 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Fri, Jul 19, 2019 at 11:26:50AM -0700, Andrii Nakryiko escreveu: > > On Fri, Jul 19, 2019 at 11:14 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > > Em Fri, Jul 19, 2019 at 10:54:44AM -0700, Andrii Nakryiko escreveu: > > > > Ok, did some more googling. This warning (turned error in your setup) > > > > is emitted when -Wshadow option is enabled for GCC/clang. It appears > > > > to be disabled by default, so it must be enabled somewhere for perf > > > > build or something. > > > > Right, I came to the exact same conclusion, doing tests here: > > > > [perfbuilder@3a58896a648d tmp]$ gcc -Wshadow shadow_global_decl.c -o shadow_global_decl > > > shadow_global_decl.c: In function 'main': > > > shadow_global_decl.c:9: warning: declaration of 'link' shadows a global declaration > > > shadow_global_decl.c:4: warning: shadowed declaration is here > > > [perfbuilder@3a58896a648d tmp]$ gcc --version |& head -1 > > > gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) > > > [perfbuilder@3a58896a648d tmp]$ gcc shadow_global_decl.c -o shadow_global_decl > > > [perfbuilder@3a58896a648d tmp]$ > > > > So I'm going to remove this warning from the places where it causes > > > problems. > > > > > Would it be possible to disable it at least for libbpf when building > > > > from perf either everywhere or for those systems where you see this > > > > warning? I don't think this warning is useful, to be honest, just > > > > random name conflict between any local and global variables will cause > > > > this. > > > > Yeah, I might end up having this applied. > > > Thanks! > > So, I'm ending up with the patch below, there is some value after all in > Wshadow, that is, from gcc 4.8 onwards :-) I agree with the intent, but see below. > > - Arnaldo > > diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include > index 495066bafbe3..ded7a950dc40 100644 > --- a/tools/scripts/Makefile.include > +++ b/tools/scripts/Makefile.include > @@ -32,7 +32,6 @@ EXTRA_WARNINGS += -Wno-system-headers > EXTRA_WARNINGS += -Wold-style-definition > EXTRA_WARNINGS += -Wpacked > EXTRA_WARNINGS += -Wredundant-decls > -EXTRA_WARNINGS += -Wshadow > EXTRA_WARNINGS += -Wstrict-prototypes > EXTRA_WARNINGS += -Wswitch-default > EXTRA_WARNINGS += -Wswitch-enum > @@ -69,8 +68,16 @@ endif > # will do for now and keep the above -Wstrict-aliasing=3 in place > # in newer systems. > # Needed for the __raw_cmpxchg in tools/arch/x86/include/asm/cmpxchg.h > +# > +# See https://lkml.org/lkml/2006/11/28/253 and https://gcc.gnu.org/gcc-4.8/changes.html, > +# that takes into account Linus's comments (search for Wshadow) for the reasoning about > +# -Wshadow not being interesting before gcc 4.8. > + > ifneq ($(filter 3.%,$(MAKE_VERSION)),) # make-3 This is checking make version, not GCC version. So code comment and configurations are not in sync? > EXTRA_WARNINGS += -fno-strict-aliasing > +EXTRA_WARNINGS += -Wno-shadow > +else > +EXTRA_WARNINGS += -Wshadow > endif > > ifneq ($(findstring $(MAKEFLAGS), w),w)
Em Fri, Jul 19, 2019 at 01:04:32PM -0700, Andrii Nakryiko escreveu: > On Fri, Jul 19, 2019 at 11:34 AM Arnaldo Carvalho de Melo > <arnaldo.melo@gmail.com> wrote: > > > > Em Fri, Jul 19, 2019 at 11:26:50AM -0700, Andrii Nakryiko escreveu: > > > On Fri, Jul 19, 2019 at 11:14 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > > > Em Fri, Jul 19, 2019 at 10:54:44AM -0700, Andrii Nakryiko escreveu: > > > > > Ok, did some more googling. This warning (turned error in your setup) > > > > > is emitted when -Wshadow option is enabled for GCC/clang. It appears > > > > > to be disabled by default, so it must be enabled somewhere for perf > > > > > build or something. > > > > > > Right, I came to the exact same conclusion, doing tests here: > > > > > > [perfbuilder@3a58896a648d tmp]$ gcc -Wshadow shadow_global_decl.c -o shadow_global_decl > > > > shadow_global_decl.c: In function 'main': > > > > shadow_global_decl.c:9: warning: declaration of 'link' shadows a global declaration > > > > shadow_global_decl.c:4: warning: shadowed declaration is here > > > > [perfbuilder@3a58896a648d tmp]$ gcc --version |& head -1 > > > > gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) > > > > [perfbuilder@3a58896a648d tmp]$ gcc shadow_global_decl.c -o shadow_global_decl > > > > [perfbuilder@3a58896a648d tmp]$ > > > > > > So I'm going to remove this warning from the places where it causes > > > > problems. > > > > > > > Would it be possible to disable it at least for libbpf when building > > > > > from perf either everywhere or for those systems where you see this > > > > > warning? I don't think this warning is useful, to be honest, just > > > > > random name conflict between any local and global variables will cause > > > > > this. > > > > > > Yeah, I might end up having this applied. > > > > > Thanks! > > > > So, I'm ending up with the patch below, there is some value after all in > > Wshadow, that is, from gcc 4.8 onwards :-) > > I agree with the intent, but see below. > > > > > - Arnaldo > > > > diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include > > index 495066bafbe3..ded7a950dc40 100644 > > --- a/tools/scripts/Makefile.include > > +++ b/tools/scripts/Makefile.include > > @@ -32,7 +32,6 @@ EXTRA_WARNINGS += -Wno-system-headers > > EXTRA_WARNINGS += -Wold-style-definition > > EXTRA_WARNINGS += -Wpacked > > EXTRA_WARNINGS += -Wredundant-decls > > -EXTRA_WARNINGS += -Wshadow > > EXTRA_WARNINGS += -Wstrict-prototypes > > EXTRA_WARNINGS += -Wswitch-default > > EXTRA_WARNINGS += -Wswitch-enum > > @@ -69,8 +68,16 @@ endif > > # will do for now and keep the above -Wstrict-aliasing=3 in place > > # in newer systems. > > # Needed for the __raw_cmpxchg in tools/arch/x86/include/asm/cmpxchg.h > > +# > > +# See https://lkml.org/lkml/2006/11/28/253 and https://gcc.gnu.org/gcc-4.8/changes.html, > > +# that takes into account Linus's comments (search for Wshadow) for the reasoning about > > +# -Wshadow not being interesting before gcc 4.8. > > + > > ifneq ($(filter 3.%,$(MAKE_VERSION)),) # make-3 > > This is checking make version, not GCC version. So code comment and > configurations are not in sync? Ah, I should have added a few lines back: # Hack to avoid type-punned warnings on old systems such as RHEL5: # We should be changing CFLAGS and checking gcc version, but this # will do for now and keep the above -Wstrict-aliasing=3 in place # in newer systems. # Needed for the __raw_cmpxchg in tools/arch/x86/include/asm/cmpxchg.h # # See https://lkml.org/lkml/2006/11/28/253 and https://gcc.gnu.org/gcc-4.8/changes.html, # that takes into account Linus's comments (search for Wshadow) for the reasoning about # -Wshadow not being interesting before gcc 4.8. In time I'll try and get it to use the gcc version to be strict. - Arnaldo
On Fri, Jul 19, 2019 at 1:27 PM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Em Fri, Jul 19, 2019 at 01:04:32PM -0700, Andrii Nakryiko escreveu: > > On Fri, Jul 19, 2019 at 11:34 AM Arnaldo Carvalho de Melo > > <arnaldo.melo@gmail.com> wrote: > > > > > > Em Fri, Jul 19, 2019 at 11:26:50AM -0700, Andrii Nakryiko escreveu: > > > > On Fri, Jul 19, 2019 at 11:14 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > > > > Em Fri, Jul 19, 2019 at 10:54:44AM -0700, Andrii Nakryiko escreveu: > > > > > > Ok, did some more googling. This warning (turned error in your setup) > > > > > > is emitted when -Wshadow option is enabled for GCC/clang. It appears > > > > > > to be disabled by default, so it must be enabled somewhere for perf > > > > > > build or something. > > > > > > > > Right, I came to the exact same conclusion, doing tests here: > > > > > > > > [perfbuilder@3a58896a648d tmp]$ gcc -Wshadow shadow_global_decl.c -o shadow_global_decl > > > > > shadow_global_decl.c: In function 'main': > > > > > shadow_global_decl.c:9: warning: declaration of 'link' shadows a global declaration > > > > > shadow_global_decl.c:4: warning: shadowed declaration is here > > > > > [perfbuilder@3a58896a648d tmp]$ gcc --version |& head -1 > > > > > gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-23) > > > > > [perfbuilder@3a58896a648d tmp]$ gcc shadow_global_decl.c -o shadow_global_decl > > > > > [perfbuilder@3a58896a648d tmp]$ > > > > > > > > So I'm going to remove this warning from the places where it causes > > > > > problems. > > > > > > > > > Would it be possible to disable it at least for libbpf when building > > > > > > from perf either everywhere or for those systems where you see this > > > > > > warning? I don't think this warning is useful, to be honest, just > > > > > > random name conflict between any local and global variables will cause > > > > > > this. > > > > > > > > Yeah, I might end up having this applied. > > > > > > > Thanks! > > > > > > So, I'm ending up with the patch below, there is some value after all in > > > Wshadow, that is, from gcc 4.8 onwards :-) > > > > I agree with the intent, but see below. > > > > > > > > - Arnaldo > > > > > > diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include > > > index 495066bafbe3..ded7a950dc40 100644 > > > --- a/tools/scripts/Makefile.include > > > +++ b/tools/scripts/Makefile.include > > > @@ -32,7 +32,6 @@ EXTRA_WARNINGS += -Wno-system-headers > > > EXTRA_WARNINGS += -Wold-style-definition > > > EXTRA_WARNINGS += -Wpacked > > > EXTRA_WARNINGS += -Wredundant-decls > > > -EXTRA_WARNINGS += -Wshadow > > > EXTRA_WARNINGS += -Wstrict-prototypes > > > EXTRA_WARNINGS += -Wswitch-default > > > EXTRA_WARNINGS += -Wswitch-enum > > > @@ -69,8 +68,16 @@ endif > > > # will do for now and keep the above -Wstrict-aliasing=3 in place > > > # in newer systems. > > > # Needed for the __raw_cmpxchg in tools/arch/x86/include/asm/cmpxchg.h > > > +# > > > +# See https://lkml.org/lkml/2006/11/28/253 and https://gcc.gnu.org/gcc-4.8/changes.html, > > > +# that takes into account Linus's comments (search for Wshadow) for the reasoning about > > > +# -Wshadow not being interesting before gcc 4.8. > > > + > > > ifneq ($(filter 3.%,$(MAKE_VERSION)),) # make-3 > > > > This is checking make version, not GCC version. So code comment and > > configurations are not in sync? > > Ah, I should have added a few lines back: > > # Hack to avoid type-punned warnings on old systems such as RHEL5: > # We should be changing CFLAGS and checking gcc version, but this > # will do for now and keep the above -Wstrict-aliasing=3 in place > # in newer systems. > # Needed for the __raw_cmpxchg in tools/arch/x86/include/asm/cmpxchg.h > # > # See https://lkml.org/lkml/2006/11/28/253 and https://gcc.gnu.org/gcc-4.8/changes.html, > # that takes into account Linus's comments (search for Wshadow) for the reasoning about > # -Wshadow not being interesting before gcc 4.8. > > > In time I'll try and get it to use the gcc version to be strict. Oh well, if it's how it's done right now :) Acked-by: Andrii Nakryiko <andriin@fb.com> > > - Arnaldo
diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h index 03748a742146..46a8cb667994 100644 --- a/tools/lib/bpf/hashmap.h +++ b/tools/lib/bpf/hashmap.h @@ -10,7 +10,12 @@ #include <stdbool.h> #include <stddef.h> +#ifdef __GLIBC__ +#include <bits/wordsize.h> +#else +#include <bits/reg.h> #include "libbpf_internal.h" +#endif static inline size_t hash_bits(size_t h, int bits) {
hashmap.h depends on __WORDSIZE being defined. It is defined by glibc/musl in different headers. It's an explicit goal for musl to be "non-detectable" at compilation time, so instead include glibc header if glibc is explicitly detected and fall back to musl header otherwise. Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap") Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/lib/bpf/hashmap.h | 5 +++++ 1 file changed, 5 insertions(+)