On Tue, May 28, 2024 at 5:26 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> From: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> Update btf_parse_elf() to check if .BTF.base section is present.
> The logic is as follows:
>
> if .BTF.base section exists:
> distilled_base := btf_new(.BTF.base)
> if distilled_base:
> btf := btf_new(.BTF, .base_btf=distilled_base)
> if base_btf:
> btf_relocate(btf, base_btf)
> else:
> btf := btf_new(.BTF)
> return btf
>
> In other words:
> - if .BTF.base section exists, load BTF from it and use it as a base
> for .BTF load;
> - if base_btf is specified and .BTF.base section exist, relocate newly
> loaded .BTF against base_btf.
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> tools/lib/bpf/btf.c | 151 +++++++++++++++++++++++++++++---------------
> tools/lib/bpf/btf.h | 1 +
> 2 files changed, 102 insertions(+), 50 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index cb762d7a5dd7..b57f74eedda0 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -114,7 +114,10 @@ struct btf {
> /* a set of unique strings */
> struct strset *strs_set;
> /* whether strings are already deduplicated */
> - bool strs_deduped;
> + unsigned strs_deduped:1;
> +
> + /* whether base_btf should be freed in btf_free for this instance */
> + unsigned owns_base:1;
nit: let's not do bit counting (i.e., bit fields for bool flags) on
rather big things like struct btf, which are only a few of them and
4/8 extra bytes just doesn't matter compared to all the other memory
used for actual data.
>
> /* BTF object FD, if loaded into kernel */
> int fd;
> @@ -969,6 +972,8 @@ void btf__free(struct btf *btf)
> free(btf->raw_data);
> free(btf->raw_data_swapped);
> free(btf->type_offs);
> + if (btf->owns_base)
> + btf__free(btf->base_btf);
> free(btf);
> }
>
> @@ -1084,53 +1089,38 @@ struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf)
> return libbpf_ptr(btf_new(data, size, base_btf));
> }
>
> -static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
> - struct btf_ext **btf_ext)
> +struct elf_sections_info {
> + Elf_Data *btf_data;
> + Elf_Data *btf_ext_data;
> + Elf_Data *btf_base_data;
bikeshedding time: elf_sections_info -> btf_elf_data (or
btf_elf_secs), btf_data -> btf, btf_ext_data -> btf_ext, btf_base_data
-> btf_base ?
> +};
> +
> +static int btf_find_elf_sections(Elf *elf, const char *path, struct elf_sections_info *info)
> {
> - Elf_Data *btf_data = NULL, *btf_ext_data = NULL;
> - int err = 0, fd = -1, idx = 0;
> - struct btf *btf = NULL;
> Elf_Scn *scn = NULL;
> - Elf *elf = NULL;
> + Elf_Data *data;
> GElf_Ehdr ehdr;
> size_t shstrndx;
> + int idx = 0;
[...]
> + if (!info.btf_data) {
> pr_warn("failed to find '%s' ELF section in %s\n", BTF_ELF_SEC, path);
> err = -ENODATA;
> goto done;
> }
> - btf = btf_new(btf_data->d_buf, btf_data->d_size, base_btf);
> +
> + if (info.btf_base_data) {
> + distilled_base_btf = btf_new(info.btf_base_data->d_buf, info.btf_base_data->d_size,
> + NULL);
with the above bikeshedding suggestion, and distilled_base_btf ->
dist_base_btf, let's get it to be a less verbose single-line statement
> + err = libbpf_get_error(distilled_base_btf);
boo to using libbpf_get_error() in new code. btf_new() is internal, so
IS_ERR()/PTR_ERR(), please
pw-bot: cr
> + if (err) {
> + distilled_base_btf = NULL;
> + goto done;
> + }
> + }
> +
> + btf = btf_new(info.btf_data->d_buf, info.btf_data->d_size,
> + distilled_base_btf ? distilled_base_btf : base_btf);
dist_base_btf ?: base_btf
> err = libbpf_get_error(btf);
ditto, IS_ERR/PTR_ERR
> if (err)
> goto done;
>
> + if (distilled_base_btf && base_btf) {
> + err = btf__relocate(btf, base_btf);
> + if (err)
> + goto done;
> + btf__free(distilled_base_btf);
> + distilled_base_btf = NULL;
> + }
> +
> + if (distilled_base_btf)
> + btf->owns_base = true;
should we reset this to false when changing base in btf__relocate()?
> +
> switch (gelf_getclass(elf)) {
> case ELFCLASS32:
> btf__set_pointer_size(btf, 4);
[...]
