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)?
>> #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
>>
