> On Oct 28, 2020, at 5:58 PM, Andrii Nakryiko <and...@kernel.org> wrote:
> 

[...]

> 
> BTF deduplication is not yet supported for split BTF and support for it will
> be added in separate patch.
> 
> Signed-off-by: Andrii Nakryiko <and...@kernel.org>

Acked-by: Song Liu <songliubrav...@fb.com>

With a couple nits:

> ---
> tools/lib/bpf/btf.c      | 205 ++++++++++++++++++++++++++++++---------
> tools/lib/bpf/btf.h      |   8 ++
> tools/lib/bpf/libbpf.map |   9 ++
> 3 files changed, 175 insertions(+), 47 deletions(-)
> 
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index db9331fea672..20c64a8441a8 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -78,10 +78,32 @@ struct btf {
>       void *types_data;
>       size_t types_data_cap; /* used size stored in hdr->type_len */
> 
> -     /* type ID to `struct btf_type *` lookup index */
> +     /* type ID to `struct btf_type *` lookup index
> +      * type_offs[0] corresponds to the first non-VOID type:
> +      *   - for base BTF it's type [1];
> +      *   - for split BTF it's the first non-base BTF type.
> +      */
>       __u32 *type_offs;
>       size_t type_offs_cap;
> +     /* number of types in this BTF instance:
> +      *   - doesn't include special [0] void type;
> +      *   - for split BTF counts number of types added on top of base BTF.
> +      */
>       __u32 nr_types;

This is a little confusing. Maybe add a void type for every split BTF? 

> +     /* if not NULL, points to the base BTF on top of which the current
> +      * split BTF is based
> +      */

[...]

> 
> @@ -252,12 +274,20 @@ static int btf_parse_str_sec(struct btf *btf)
>       const char *start = btf->strs_data;
>       const char *end = start + btf->hdr->str_len;
> 
> -     if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_STR_OFFSET ||
> -         start[0] || end[-1]) {
> -             pr_debug("Invalid BTF string section\n");
> -             return -EINVAL;
> +     if (btf->base_btf) {
> +             if (hdr->str_len == 0)
> +                     return 0;
> +             if (hdr->str_len - 1 > BTF_MAX_STR_OFFSET || end[-1]) {
> +                     pr_debug("Invalid BTF string section\n");
> +                     return -EINVAL;
> +             }
> +     } else {
> +             if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_STR_OFFSET ||
> +                 start[0] || end[-1]) {
> +                     pr_debug("Invalid BTF string section\n");
> +                     return -EINVAL;
> +             }
>       }
> -
>       return 0;

I found this function a little difficult to follow. Maybe rearrange it as 

        /* too long, or not \0 terminated */
        if (hdr->str_len - 1 > BTF_MAX_STR_OFFSET || end[-1])
                goto err_out;

        /* for base btf, .... */
        if (!btf->base_btf && (!hdr->str_len || start[0]))
                goto err_out;

        return 0;
err_out:
        pr_debug("Invalid BTF string section\n");
        return -EINVAL;
}
> }
> 
> @@ -372,19 +402,9 @@ static int btf_parse_type_sec(struct btf *btf)
>       struct btf_header *hdr = btf->hdr;
>       void *next_type = btf->types_data;
>       void *end_type = next_type + hdr->type_len;
> -     int err, i = 0, type_size;

[...]

Reply via email to