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. > 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
