[PATCH bpf] libbpf: fix missing __WORDSIZE definition

[PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Andrii Nakryiko]

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
 
 static inline size_t hash_bits(size_t h, int bits)
 {
-- 
2.17.1

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Andrii Nakryiko]

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
>

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Arnaldo Carvalho de Melo]

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);

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Andrii Nakryiko]

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);

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Arnaldo Carvalho de Melo]

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);

-- 

- Arnaldo

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Arnaldo Carvalho de Melo]

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

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Andrii Nakryiko]

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

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Arnaldo Carvalho de Melo]

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

-- 

- Arnaldo

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Andrii Nakryiko]

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

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Arnaldo Carvalho de Melo]

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

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Andrii Nakryiko]

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

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Arnaldo Carvalho de Melo]

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)

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Andrii Nakryiko]

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)

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Arnaldo Carvalho de Melo]

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

Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

[Andrii Nakryiko]

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