On Mon, Jan 9, 2023 at 10:37 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Mon, Jan 9, 2023 at 10:10 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> >
> > On Mon, Jan 09, 2023 at 12:12:15PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Jan 06, 2023 at 11:06:46AM -0800, Ian Rogers escreveu:
> > > > So trying to get build-test working on my Debian derived distro is a
> > > > PITA with broken feature detection for options I don't normally use.
> > >
> > > Its really difficult to have perf building with so many dependent
> > > libraries, mowing out some should be in order.
> > >
> > > > I'll try to fix this.
> > >
> > > Thanks.
> > >
> > > > In any case I think I've spotted what is really happening here and it
> > > > isn't a failure but a feature :-D The build is specifying
> > >
> > > I get it.
> > >
> > > > LIBBPF_DYNAMIC=1 which means you get the libbpf headers from
> > > > /usr/include. I think the build is trying to do this on a system with
> > > > an old libbpf and hence getting the failures above. Previously, even
> > > > though we wanted the dynamic headers we still had a -I, this time for
> > > > the install_headers version. Now you really are using the system
> > > > version and it is broken. This means a few things:
> > > > - the libbpf feature test should fail if code like above is going to fail,
> > >
> > > Agreed.
> > >
> > > > - we may want to contemplate supporting older libbpfs (I'd rather not),
> > >
> > > I'd rather require everybody to be up to the latest trends, but I really
> > > don't think that is a reasonable expectation.
> > >
> > > > - does build-test have a way to skip known issues like this?
> > >
> > > Unsure, Jiri?
> >
> > I don't think so it just triggers the build, it's up to the features check
> > to disable the feature if the library is not compatible with perf code
> >
> > could we add that specific libbpf call to the libbpf feature check?
>
> Looking at the failure closer, the failing code is code inside a
> feature check trying to workaround the feature not being present. We
> need to do something like:
>
> ```
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 6e9b06cf06ee..a1c3cc230273 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -33,17 +33,18 @@
> #include <internal/xyarray.h>
>
> #ifndef HAVE_LIBBPF_BPF_PROGRAM__SET_INSNS
> -int bpf_program__set_insns(struct bpf_program *prog __maybe_unused,
> - struct bpf_insn *new_insns __maybe_unused,
> size_t new_insn_cnt __maybe_un
> used)
> +static int bpf_program__set_insns(struct bpf_program *prog __maybe_unused,
> + struct bpf_insn *new_insns __maybe_unused,
> + size_t new_insn_cnt __maybe_unused)
> {
> pr_err("%s: not support, update libbpf\n", __func__);
> return -ENOTSUP;
> }
>
> -int libbpf_register_prog_handler(const char *sec __maybe_unused,
> - enum bpf_prog_type prog_type __maybe_unused,
> - enum bpf_attach_type exp_attach_type
> __maybe_unused,
> - const struct
> libbpf_prog_handler_opts *opts __maybe_unused)
> +static int libbpf_register_prog_handler(const char *sec __maybe_unused,
> + enum bpf_prog_type prog_type
> __maybe_unused,
> + enum bpf_attach_type
> exp_attach_type __maybe_unused,
> + const void *opts __maybe_unused)
> {
> pr_err("%s: not support, update libbpf\n", __func__);
> return -ENOTSUP;
> ```
>
> There are some other fixes necessary too. I'll try to write the fuller
> patch but I have no means for testing except for undefining
> HAVE_LIBBPF_BPF_PROGRAM__SET_INSNS.
>
> Thanks,
> Ian
So libbpf_prog_handler_opts is missing in the failing build, this
points to a libbpf before 0.8. I'm somewhat concerned that to work
around these linkage problems we're adding runtime errors - we may
build but the functionality is totally crippled. Is it worth
maintaining these broken builds or to just upfront fail the feature
test?
We can also switch the feature tests for LIBBPF_MAJOR_VERSION and
LIBBPF_MINOR_VERSION checks. This would have the property of letting
us tie the error messages to what version of libbpf is assumed.
In this case we could have a feature test for the libbpf version and
if the version is before libbpf 0.8 fail the feature check. A quick
way to do this is:
```
diff --git a/tools/build/feature/test-libbpf.c
b/tools/build/feature/test-libbpf.c
index a508756cf4cc..dadd8186b71d 100644
--- a/tools/build/feature/test-libbpf.c
+++ b/tools/build/feature/test-libbpf.c
@@ -1,6 +1,10 @@
// SPDX-License-Identifier: GPL-2.0
#include <bpf/libbpf.h>
+#if (LIBBPF_MAJOR_VERSION == 0) && (LIBBPF_MINOR_VERSION < 8)
+#error At least libbpf 0.8 is assumed for Linux tools.
+#endif
+
int main(void)
{
return bpf_object__open("test") ? 0 : -1;
```
Thanks,
Ian
> > jirka
> >
> > >
> > > But yeah, previous experiences with Andrii were that we can do not too
> > > costly feature checks, not using .c programs that would fail if some
> > > required feature wasn't present but instead would just do some grep on a
> > > header and if some "smell" wasn't scent, just fail the cap query.
> > >
> > > - Arnaldo
