On Fri, Feb 15, 2019 at 9:15 AM Arnaldo Carvalho de Melo <arnaldo.melo@xxxxxxxxx> wrote: > > 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@xxxxxxxxx> 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. We already did that a couple of days ago, that code wasn't yet mirrored into github.com/libbpf/libbpf until yesterday (it was merged this morning). We now have btf__new(data, size) that just constructs in-memory BTF struct with index for types. And we have btf__load(struct btf* btf) that can load that into kernel. We already have equivalent of btf__new_from_kernel(fd) -- btf__get_from_id(fd). Adding something akin to btf__new_from_elf() might be a good idea as well, for completeness. > > > 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
