On Wed, May 1, 2024 at 10:43 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On 30/04/2024 00:40, Andrii Nakryiko wrote:
> > On Wed, Apr 24, 2024 at 8:48 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
> >>
> >> Options cover existing parsing scenarios (ELF, raw, retrieving
> >> .BTF.ext) and also allow specification of the ELF section name
> >> containing BTF. This will allow consumers to retrieve BTF from
> >> .BTF.base sections (BTF_BASE_ELF_SEC) also.
> >>
> >> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> >> ---
> >> tools/lib/bpf/btf.c | 50 ++++++++++++++++++++++++++++------------
> >> tools/lib/bpf/btf.h | 32 +++++++++++++++++++++++++
> >> tools/lib/bpf/libbpf.map | 1 +
> >> 3 files changed, 68 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> >> index 419cc4fa2e86..9036c1dc45d0 100644
> >> --- a/tools/lib/bpf/btf.c
> >> +++ b/tools/lib/bpf/btf.c
> >> @@ -1084,7 +1084,7 @@ 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,
> >> +static struct btf *btf_parse_elf(const char *path, const char *btf_sec, struct btf *base_btf,
> >> struct btf_ext **btf_ext)
> >> {
> >> Elf_Data *btf_data = NULL, *btf_ext_data = NULL;
> >> @@ -1146,7 +1146,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
> >> idx, path);
> >> goto done;
> >> }
> >> - if (strcmp(name, BTF_ELF_SEC) == 0) {
> >> + if (strcmp(name, btf_sec) == 0) {
> >> btf_data = elf_getdata(scn, 0);
> >> if (!btf_data) {
> >> pr_warn("failed to get section(%d, %s) data from %s\n",
> >> @@ -1166,7 +1166,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
> >> }
> >>
> >> if (!btf_data) {
> >> - pr_warn("failed to find '%s' ELF section in %s\n", BTF_ELF_SEC, path);
> >> + pr_warn("failed to find '%s' ELF section in %s\n", btf_sec, path);
> >> err = -ENODATA;
> >> goto done;
> >> }
> >> @@ -1212,12 +1212,12 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
> >>
> >> struct btf *btf__parse_elf(const char *path, struct btf_ext **btf_ext)
> >> {
> >> - return libbpf_ptr(btf_parse_elf(path, NULL, btf_ext));
> >> + return libbpf_ptr(btf_parse_elf(path, BTF_ELF_SEC, NULL, btf_ext));
> >> }
> >>
> >> struct btf *btf__parse_elf_split(const char *path, struct btf *base_btf)
> >> {
> >> - return libbpf_ptr(btf_parse_elf(path, base_btf, NULL));
> >> + return libbpf_ptr(btf_parse_elf(path, BTF_ELF_SEC, base_btf, NULL));
> >> }
> >>
> >> static struct btf *btf_parse_raw(const char *path, struct btf *base_btf)
> >> @@ -1293,7 +1293,8 @@ struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf)
> >> return libbpf_ptr(btf_parse_raw(path, base_btf));
> >> }
> >>
> >> -static struct btf *btf_parse(const char *path, struct btf *base_btf, struct btf_ext **btf_ext)
> >> +static struct btf *btf_parse(const char *path, const char *btf_elf_sec, struct btf *base_btf,
> >> + struct btf_ext **btf_ext)
> >> {
> >> struct btf *btf;
> >> int err;
> >> @@ -1301,23 +1302,42 @@ static struct btf *btf_parse(const char *path, struct btf *base_btf, struct btf_
> >> if (btf_ext)
> >> *btf_ext = NULL;
> >>
> >> - btf = btf_parse_raw(path, base_btf);
> >> - err = libbpf_get_error(btf);
> >> - if (!err)
> >> - return btf;
> >> - if (err != -EPROTO)
> >> - return ERR_PTR(err);
> >> - return btf_parse_elf(path, base_btf, btf_ext);
> >> + if (!btf_elf_sec) {
> >> + btf = btf_parse_raw(path, base_btf);
> >> + err = libbpf_get_error(btf);
> >> + if (!err)
> >> + return btf;
> >> + if (err != -EPROTO)
> >> + return ERR_PTR(err);
> >> + }
> >> + if (!btf_elf_sec)
> >> + btf_elf_sec = BTF_ELF_SEC;
> >> +
> >> + return btf_parse_elf(path, btf_elf_sec, base_btf, btf_ext);
> >
> > nit: btf_elf_sec ?: BTF_ELF_SEC
> >
>
> sure, will fix.
>
> >
> >> +}
> >> +
> >> +struct btf *btf__parse_opts(const char *path, struct btf_parse_opts *opts)
> >> +{
> >> + struct btf *base_btf;
> >> + const char *btf_sec;
> >> + struct btf_ext **btf_ext;
> >> +
> >> + if (!OPTS_VALID(opts, btf_parse_opts))
> >> + return libbpf_err_ptr(-EINVAL);
> >> + base_btf = OPTS_GET(opts, base_btf, NULL);
> >> + btf_sec = OPTS_GET(opts, btf_sec, NULL);
> >> + btf_ext = OPTS_GET(opts, btf_ext, NULL);
> >> + return libbpf_ptr(btf_parse(path, btf_sec, base_btf, btf_ext));
> >> }
> >>
> >> struct btf *btf__parse(const char *path, struct btf_ext **btf_ext)
> >> {
> >> - return libbpf_ptr(btf_parse(path, NULL, btf_ext));
> >> + return libbpf_ptr(btf_parse(path, NULL, NULL, btf_ext));
> >> }
> >>
> >> struct btf *btf__parse_split(const char *path, struct btf *base_btf)
> >> {
> >> - return libbpf_ptr(btf_parse(path, base_btf, NULL));
> >> + return libbpf_ptr(btf_parse(path, NULL, base_btf, NULL));
> >> }
> >>
> >> static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endian);
> >> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> >> index 025ed28b7fe8..94dfdfdef617 100644
> >> --- a/tools/lib/bpf/btf.h
> >> +++ b/tools/lib/bpf/btf.h
> >> @@ -18,6 +18,7 @@ extern "C" {
> >>
> >> #define BTF_ELF_SEC ".BTF"
> >> #define BTF_EXT_ELF_SEC ".BTF.ext"
> >> +#define BTF_BASE_ELF_SEC ".BTF.base"
> >
> > Does libbpf code itself use this? If not, let's get rid of it.
> >
>
> We could, but I wonder would there be value to keeping it around as
> multiple consumers need to agree on this name (pahole, resolve_btfids,
> bpftool)?
Ok, I can see how it might be a bit more generic thing beyond just
kernel use, let's keep it then.
>
> >> #define MAPS_ELF_SEC ".maps"
> >>
> >> struct btf;
> >> @@ -134,6 +135,37 @@ LIBBPF_API struct btf *btf__parse_elf_split(const char *path, struct btf *base_b
> >> LIBBPF_API struct btf *btf__parse_raw(const char *path);
> >> LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf);
> >>
> >> +struct btf_parse_opts {
> >> + size_t sz;
> >> + /* use base BTF to parse split BTF */
> >> + struct btf *base_btf;
> >> + /* retrieve optional .BTF.ext info */
> >> + struct btf_ext **btf_ext;
> >> + /* BTF section name */
> >
> > let's mention that if not set, libbpf will default to trying to parse
> > data as raw BTF, and then will fallback to .BTF in ELF. If it is set
> > to non-NULL, we'll assume ELF and use that section to fetch BTF data.
> >
>
> sure, will do.
>
> >> + const char *btf_sec;
> >> + size_t:0;
> >
> > nit: size_t :0; (consistency)
> >
> >> +};
> >> +
> >> +#define btf_parse_opts__last_field btf_sec
> >> +
> >> +/* @brief **btf__parse_opts()** parses BTF information from either a
> >> + * raw BTF file (*btf_sec* is NULL) or from the specified BTF section,
> >> + * also retrieving .BTF.ext info if *btf_ext* is non-NULL. If
> >> + * *base_btf* is specified, use it to parse split BTF from the
> >> + * specified location.
> >> + *
> >> + * @return new BTF object instance which has to be eventually freed with
> >> + * **btf__free()**
> >> + *
> >> + * On error, error-code-encoded-as-pointer is returned, not a NULL. To extract
> >
> > this is false, we don't encode error as pointer anymore. starting from
> > v1.0 it's always NULL + errno.
> >
>
> ah good catch, I must have cut-and-pasted this..
>
> Thanks again for all the review help!
>
> Alan
>
> >> + * error code from such a pointer `libbpf_get_error()` should be used. If
> >> + * `libbpf_set_strict_mode(LIBBPF_STRICT_CLEAN_PTRS)` is enabled, NULL is
> >> + * returned on error instead. In both cases thread-local `errno` variable is
> >> + * always set to error code as well.
> >> + */
> >> +
> >> +LIBBPF_API struct btf *btf__parse_opts(const char *path, struct btf_parse_opts *opts);
> >> +
> >> LIBBPF_API struct btf *btf__load_vmlinux_btf(void);
> >> LIBBPF_API struct btf *btf__load_module_btf(const char *module_name, struct btf *vmlinux_btf);
> >>
> >> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> >> index c4d9bd7d3220..a9151e31dfa9 100644
> >> --- a/tools/lib/bpf/libbpf.map
> >> +++ b/tools/lib/bpf/libbpf.map
> >> @@ -421,6 +421,7 @@ LIBBPF_1.5.0 {
> >> global:
> >> bpf_program__attach_sockmap;
> >> btf__distill_base;
> >> + btf__parse_opts;
> >> ring__consume_n;
> >> ring_buffer__consume_n;
> >> } LIBBPF_1.4.0;
> >> --
> >> 2.31.1
> >>
