On Wed, Oct 20, 2021 at 4:46 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Wed, Oct 20, 2021 at 3:30 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Wed, Oct 20, 2021 at 2:49 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Oct 20, 2021 at 2:27 PM Andrii Nakryiko
> > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > >
> > > > On Wed, Oct 20, 2021 at 2:23 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Wed, Oct 20, 2021 at 2:12 PM Andrii Nakryiko
> > > > > <andrii.nakryiko@xxxxxxxxx> wrote:
> > > > > >
> > > > > > On Thu, Oct 14, 2021 at 2:20 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > BTF_KIND_TAG is present in libbtf 6.0 but not libbtf in 5.15rc4. Make
> > > > > > > the code requiring it conditionally compiled in.
> > > > > > >
> > > > > > > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > > > > > > ---
> > > > > > > btf_encoder.c | 7 +++++++
> > > > > > > lib/bpf | 2 +-
> > > > > > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > > > > index c341f95..400d64b 100644
> > > > > > > --- a/btf_encoder.c
> > > > > > > +++ b/btf_encoder.c
> > > > > > > @@ -141,7 +141,9 @@ static const char * const btf_kind_str[NR_BTF_KINDS] = {
> > > > > > > [BTF_KIND_VAR] = "VAR",
> > > > > > > [BTF_KIND_DATASEC] = "DATASEC",
> > > > > > > [BTF_KIND_FLOAT] = "FLOAT",
> > > > > > > +#ifdef BTF_KIND_TAG /* BTF_KIND_TAG was added in 6.0 */
> > > > > > > [BTF_KIND_TAG] = "TAG",
> > > > > > > +#endif
> > > > > > > };
> > > > > > >
> > > > > > > static const char *btf__printable_name(const struct btf *btf, uint32_t offset)
> > > > > > > @@ -648,6 +650,7 @@ static int32_t btf_encoder__add_datasec(struct btf_encoder *encoder, const char
> > > > > > > static int32_t btf_encoder__add_tag(struct btf_encoder *encoder, const char *value, uint32_t type,
> > > > > > > int component_idx)
> > > > > > > {
> > > > > > > +#ifdef BTF_KIND_TAG /* Proxy for libbtf 6.0 */
> > > > > >
> > > > > > How will this work when libbpf is loaded dynamically? I believe pahole
> > > > > > has this mode as well.
> > > > >
> > > > > Well it won't have a compilation error because BTF_KIND_TAG isn't
> > > >
> > > > Great, you traded compile-time error for runtime linking error, I hope
> > > > that trade off makes sense to Arnaldo.
> > > >
> > > > > undefined :-) Tbh, I'm not sure but it seems that you'd be limited to
> > > > > features in the version of libbpf you compiled against.
> > > >
> > > > I've been consistently advocating for statically linking against
> > > > libbpf exactly to control what APIs and features are supported. But
> > > > people stubbornly want dynamic linking. I hope added complexity and
> > > > feature detection makes sense in practice for pahole.
> > > >
> > > > >
> > > > > > Also, note that libbpf now provides LIBBPF_MAJOR_VERSION and
> > > > > > LIBBPF_MINOR_VERSION macros, starting from 0.5, so no need for
> > > > > > guessing the version
> > > > >
> > > > > This was moved to a header file in:
> > > > > https://lore.kernel.org/bpf/CAADnVQJ2qd095mvj3z9u9BXQYCe2OTDn4=Gsu9nv1tjFHc2yqQ@xxxxxxxxxxxxxx/T/
> > > > >
> > > > > But that header doesn't appear any more:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/tools/lib/bpf
> > > > >
> > > > > Is that a bug?
> > > >
> > > > You should be checking here:
> > > >
> > > > https://github.com/libbpf/libbpf/blob/master/src/libbpf_version.h
> > >
> > > We don't currently mirror this or bpf-next, but presumably the
> >
> > Sorry, who's "we" and what's the use case we are talking about here?
> > pahole itself is using libbpf from Github mirror and that's what all
> > distros either are already doing or strongly encouraged to start
> > doing.
>
> I work for Google. When I spoke with Arnaldo it seemed uncommon that a
> distro would be tracking bpf-next. There's a policy of a single
> library version within Google and a different version for pahole has
> some issues for us.
I'm not following. Distros don't track bpf-next for libbpf. They track
https://github.com/libbpf/libbpf as an official libbpf repo. Not sure
what you mean by "different version for pahole"? Use pahole version
that statically links against its own (submodule) libbpf.
>
> > > released version of libbpf is that in the Linus' tree [1]? There are
> > > some things like traceevent that are planned for removal. It seems
> > > like a bug that these trees are missing libbpf_version.h.
> >
> > I misremembered versions, LIBBPF_MAJOR_VERSION/LIBBPF_MINOR_VERSION
> > are available starting from v0.6 (unreleased yet), not v0.5. It's a
> > pretty recent change, so might have not made it to the tip tree. But
> > Github repo does have it, it's synced from bpf-next directly.
>
> Ok, do you suggest something like:
>
> #if defined(LIBBPF_MAJOR_VERSION)
> #if LIBBPF_MAJOR_VERSION > 5
> ..
> #endif
> #endif
>
> rather than #ifdef BTF_KIND_TAG ? I couldn't see similar examples to
> cargo cult from, so there's a likelihood that this could become a
> pattern others copy.
That's more reliable than checking BTF_KIND_TAG for sure. BTF_KIND_TAG
comes from kernel UAPI headers, it doesn't directly correlate with
libbpf version, unless you make sure that you only use UAPI headers
that libbpf uses for its own build.
But then, if you use pahole in the mode that dynamically links against
libbpf, then this whole LIBBPF_MAJOR_VERSION business is bogus,
because it only checks what version of libbpf pahole was compiled
against, not which libbpf version will it dynamically link against.
Regardless, it's probably a good idea to add libbpf version APIs that
could be queried at runtime, e.g., int libbpf_major_version(), int
libbpf_minor_version(), etc. That would be usable even with shared
library.
>
> Thanks,
> Ian
>
> > >
> > > Thanks,
> > > Ian
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf
> > > > >
> > > > > Thanks,
> > > > > Ian
> > > > >
> > > > > > > struct btf *btf = encoder->btf;
> > > > > > > const struct btf_type *t;
> > > > > > > int32_t id;
> > > > > > > @@ -663,6 +666,10 @@ static int32_t btf_encoder__add_tag(struct btf_encoder *encoder, const char *val
> > > > > > > }
> > > > > > >
> > > > > > > return id;
> > > > > > > +#else
> > > > > > > + fprintf(stderr, "error: unable to encode BTF_KIND_TAG due to old libbtf\n");
> > > > > > > + return -ENOTSUP;
> > > > > > > +#endif
> > > > > > > }
> > > > > > >
> > > > > > > /*
> > > > > > > diff --git a/lib/bpf b/lib/bpf
> > > > > > > index 980777c..986962f 160000
> > > > > > > --- a/lib/bpf
> > > > > > > +++ b/lib/bpf
> > > > > > > @@ -1 +1 @@
> > > > > > > -Subproject commit 980777cc16db75d5628a537c892aefc2640bb242
> > > > > > > +Subproject commit 986962fade5dfa89c2890f3854eb040d2a64ab38
> > > > > > > --
> > > > > > > 2.33.0.1079.g6e70778dc9-goog
> > > > > > >
