Date:   Fri, 19 Jul 2019 10:54:44 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...com>,
        Kernel Team <kernel-team@...com>, Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...il.com>
Subject: Re: [PATCH bpf] libbpf: fix missing __WORDSIZE definition

On Thu, Jul 18, 2019 at 6:16 PM Arnaldo Carvalho de Melo
<arnaldo.melo@...il.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@...il.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@...co linux-perf-tools-build]$ export PERF_TARBALL=http://192.168.124.1/perf/perf-5.2.0.tar.xz
> > > [perfbuilder@...co 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@...co 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@...co 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@...r.kernel.org/netdev@...r.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@...co 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@...co perf]$
>
> Anyway, I'll resubmit the patches that you acked to bpf@...r 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@...com>
>
> Ok, will collect this, change the prefix to: "libbpf: ..." and send to
> bpf@...r
>
> >
> > >
> > > 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

Powered by blists - more mailing lists