Re: pahole: soliciting naming suggestion for struct btf rename

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Thu, Feb 14, 2019 at 08:37:51PM -0800, Andrii Nakryiko escreveu:
> On Thu, Feb 14, 2019 at 6:01 AM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Thu, Feb 14, 2019 at 10:20:29AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Thu, Feb 14, 2019 at 10:11:56AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Thu, Feb 14, 2019 at 09:47:57AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > Em Wed, Feb 13, 2019 at 09:43:43PM -0800, Andrii Nakryiko escreveu:
> > > > > > happy with it. So consider this email a solicitation for naming
> > > > > > suggestion. Keep in mind, that all the pahole's functions of the form
> > > > > > btf__xxx will be renamed as well for consistency. If you like
> > > > > > btf_info, let me know as well, I'll just stick with it.
> > > >
> > > > > Can you try thinking if splitting this further into 'struct btf_loader',
> > > > > 'struct btf_encoder' that would live in the pahole sources and that
> > > > > refer to a 'struct btf' that lives in libbpf (or in libbtf, at some
> > > > > point) is a move that eases your current needs?
> 
> I think pahole would be simpler by using struct btf for btf_loader and
> separate struct btf_enc (or whatever name we come up with) to encode
> BTF during DWARF->BTF conversion. See below about mutable vs immutable
> BTF representations. For immutable parts libbpf's struct btf should be
> enough, though pahole is dealing also with endianness issues, so we'll
> need to resolve that somehow.
> 
> > > >
> > > > So, the btf__new() in tools/lib/bpf/btf.c is basically a variant of
> > > > btf__new() in the pahole sources, probably we should go ahead and make
> > > > pahole use that btf__new() and do changes in tools/lib/bpf/btf.c to
> > > > allow for it to access internal state that it needs to do its job?
> > >
> > > No, we can't, because tools/lib/btf/btf.c btf__new() is centered on
> > > getting some BTF buffer no matter where it comes from and passing it to
> > > the kernel.
> 
> Yes, libbpf's struct btf is immutable read-only view of .BTF section
> that can come from either file or kernel. When I'll be adding BTF
> writing (encoding) API, it probably will be done using something
> similar to pahole's struct btf, that supports dynamic growth of types

Ok, I noticed that libbpf's btf__new() does load things into the kernel,
perhaps we should have it not do that and instead have some other method
for asking it to send the data to kernel, i.e.:

	struct btf *btf = btf__new();
	int err = btf__load_to_kernel(btf, data, size);

Or have multiple constructors, each specifying what it actually does,
i.e.:

To get a btf data + size and insert it into the kernel, getting its fd,
etc:

	struct btf *btf = btf__new_to_kernel(data, size);

For asking for BTF info that is already in the kernel to be obtained for
tooling to parse maps in running bpf programs:

	struct btf *btf = btf__new_from_kernel(fd);

And for the pahole case it would be:

	struct btf *btf = btf__new_from_elf(file-with-BTF-ELF-section)

that would then be used to do the encoding, etc.

> and strings sections. Then, once application is done constructing BTF,
> that modifyable struct can be "sealed" into read-only struct btf.
> Whichever way we'll go about that, there should be minimal changes to
> pahole to be able to reuse that functionality.
 
> > > So probably we should backtrack to my previous suggestion of having
> > > pahole use 'struct btf_loader', or even more explicitely, 'struct
> > > btf_elf' to make extra clear that this has nothing to do with the
> > > kernel, its purely about loading/encoding the BTF info from/to a ELF
> > > file.

> > So, I have this in my private branch, please see if this helps get
> > things moving forward:
 
> Yes, this is exactly what I needed, thanks for doing this massive
> renaming! See just one nit below about probably unintentional file
> name change.

yeah
 
> > @@ -645,7 +636,7 @@ static int btf__write_elf(struct btf *btf)
> >                         llvm_objcopy = "llvm-objcopy";
> >
> >                 /* Use objcopy to add a .BTF section */
> > -               snprintf(tmp_fn, sizeof(tmp_fn), "%s.btf", btf->filename);
> > +               snprintf(tmp_fn, sizeof(tmp_fn), "%s.btfe", btfe->filename);
> 
> This is probably unintentional change, though not a problem per se.
 
Eagle eyes, I'll fix that.

Thanks,

- Arnaldo