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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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