Re: [PATCH dwarves v2] btf: Add support for the floating-point types

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

On Tue, Mar 9, 2021 at 1:57 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Tue, 2021-03-09 at 13:37 -0800, Andrii Nakryiko wrote:
> > On Tue, Mar 9, 2021 at 3:48 AM Arnaldo Carvalho de Melo <
> > acme@kernel.org> wrote:
> > >
> > > Em Tue, Mar 09, 2021 at 12:59:13AM +0100, Ilya Leoshkevich
> > > escreveu:
> > > > Some BPF programs compiled on s390 fail to load, because s390
> > > > arch-specific linux headers contain float and double types.
> > > >
> > > > Fix as follows:
> > > >
> > > > - Make the DWARF loader fill base_type.float_type.
> > > >
> > > > - Introduce libbpf compatibility level command-line parameter, so
> > > > that
> > > >   pahole could be used to build both the older and the newer
> > > > kernels.
> > > >
> > > > - libbpf introduced the support for the floating-point types in
> > > > commit
> > > >   986962fade5, so update the libbpf submodule to that version and
> > > > use
> > > >   the new btf__add_float() function in order to emit the
> > > > floating-point
> > > >   types when not in the compatibility mode and
> > > > base_type.float_type is
> > > >   set.
> > > >
> > > > - Make the BTF loader recognize the new BTF kind.
> > > >
> > > > Example of the resulting entry in the vmlinux BTF:
> > > >
> > > >     [7164] FLOAT 'double' size=8
> > > >
> > > > when building with:
> > > >
> > > >     LLVM_OBJCOPY=${OBJCOPY} ${PAHOLE} -J ${1} --
> > > > libbpf_compat=0.4.0
> > >
> > > I'm testing it now, and added as a followup patch the man page
> > > entry,
> > > please check that the wording is appropriate.
> > >
> > > Thanks,
> > >
> > > - Arnaldo
> > >
> > > [acme@five pahole]$ vim man-pages/pahole.1
> > > [acme@five pahole]$ git diff
> > > diff --git a/man-pages/pahole.1 b/man-pages/pahole.1
> > > index 352bb5e45f319da4..787771753d1933b1 100644
> > > --- a/man-pages/pahole.1
> > > +++ b/man-pages/pahole.1
> > > @@ -199,6 +199,12 @@ Path to the base BTF file, for instance:
> > > vmlinux when encoding kernel module BTF
> > >  This may be inferred when asking for a /sys/kernel/btf/MODULE,
> > > when it will be autoconfigured
> > >  to "/sys/kernel/btf/vmlinux".
> > >
> > > +.TP
> > > +.B \-\-libbpf_compat=LIBBPF_VERSION
> > > +Produce output compatible with this libbpf version. For instance,
> > > specifying 0.4.0 as
> > > +the version would encode BTF_KIND_FLOAT entries in systems where
> > > the vmlinux DWARF
> > > +information has float types.
> >
> > TBH, I think it's not exactly right to call out libbpf version here.
> > It's BTF "version" (if we had such a thing) that determines the set
> > of
> > supported BTF kinds. There could be other libraries that might want
> > to
> > parse BTF. So I don't know what this should be called, but
> > libbpf_compat is probably a wrong name for it.
>
> BTF version seems to exist: btf_header.version. Should we maybe bump
> this?

That seems excessive. If the kernel doesn't use FLOATs, then no one
would even notice a difference. While if we bump this version, then
everything will automatically become incompatible.

>
> >
> > If we do want to teach pahole to not emit some parts of BTF, it
> > should
> > probably be a set of BPF features, not some arbitrary library
> > versions.
>
> I thought about just adding --btf-allow-floats, but if new features
> will be added in the future, the list of options will become unwieldy.
> So I thought it would be good to settle for something that increases
> monotonically.
>

BTF_KIND_FLOAT is the first extension in a long while. I'd worry about
the proliferation of new options when we actually see some proof of
that being a problem in practice.