On Thu, Jun 13, 2024 at 2:50 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> Map distilled base BTF type ids referenced in split BTF and their
> references to the base BTF passed in, and if the mapping succeeds,
> reparent the split BTF to the base BTF.
>
> Relocation is done by first verifying that distilled base BTF
> only consists of named INT, FLOAT, ENUM, FWD, STRUCT and
> UNION kinds; then we sort these to speed lookups. Once sorted,
> the base BTF is iterated, and for each relevant kind we check
> for an equivalent in distilled base BTF. When found, the
> mapping from distilled -> base BTF id and string offset is recorded.
> In establishing mappings, we need to ensure we check STRUCT/UNION
> size when the STRUCT/UNION is embedded in a split BTF STRUCT/UNION,
> and when duplicate names exist for the same STRUCT/UNION. Otherwise
> size is ignored in matching STRUCT/UNIONs.
>
> Once all mappings are established, we can update type ids
> and string offsets in split BTF and reparent it to the new base.
>
> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> tools/lib/bpf/Build | 2 +-
> tools/lib/bpf/btf.c | 17 ++
> tools/lib/bpf/btf.h | 14 +
> tools/lib/bpf/btf_relocate.c | 506 ++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.map | 1 +
> tools/lib/bpf/libbpf_internal.h | 3 +
> 6 files changed, 542 insertions(+), 1 deletion(-)
> create mode 100644 tools/lib/bpf/btf_relocate.c
>
[...]
> +/* Build a map from distilled base BTF ids to base BTF ids. To do so, iterate
> + * through base BTF looking up distilled type (using binary search) equivalents.
> + */
> +static int btf_relocate_map_distilled_base(struct btf_relocate *r)
> +{
> + struct btf_name_info *dist_base_info_sorted, *dist_base_info_sorted_end;
> + struct btf_type *base_t, *dist_t;
> + __u8 *base_name_cnt = NULL;
> + int err = 0;
> + __u32 id;
> +
> + /* generate a sort index array of name/type ids sorted by name for
> + * distilled base BTF to speed name-based lookups.
> + */
> + dist_base_info_sorted = calloc(r->nr_dist_base_types, sizeof(*dist_base_info_sorted));
s/dist_base_info_sorted/infos/. Do we have any other info here? Not
distilled base one? Not sorted? What's the point of these long verbose
variable names besides making the rest of the code longer and more
distracting?
> + if (!dist_base_info_sorted) {
> + err = -ENOMEM;
> + goto done;
> + }
> + dist_base_info_sorted_end = dist_base_info_sorted + r->nr_dist_base_types;
> + for (id = 0; id < r->nr_dist_base_types; id++) {
> + dist_t = btf_type_by_id(r->dist_base_btf, id);
> + dist_base_info_sorted[id].name = btf__name_by_offset(r->dist_base_btf,
> + dist_t->name_off);
> + dist_base_info_sorted[id].id = id;
> + dist_base_info_sorted[id].size = dist_t->size;
> + dist_base_info_sorted[id].needs_size = true;
> + }
> + qsort(dist_base_info_sorted, r->nr_dist_base_types, sizeof(*dist_base_info_sorted),
> + cmp_btf_name_size);
> +
> + /* Mark distilled base struct/union members of split BTF structs/unions
> + * in id_map with BTF_IS_EMBEDDED; this signals that these types
> + * need to match both name and size, otherwise embeddding the base
typo: embedding
> + * struct/union in the split type is invalid.
> + */
> + for (id = r->nr_dist_base_types; id < r->nr_split_types; id++) {
> + err = btf_mark_embedded_composite_type_ids(r, id);
> + if (err)
> + goto done;
> + }
> +
[...]
> + /* iterate over all matching distilled base types */
> + for (dist_name_info = search_btf_name_size(&base_name_info, dist_base_info_sorted,
> + r->nr_dist_base_types);
> + dist_name_info != NULL; dist_name_info = dist_name_info_next) {
> + /* Are there more distilled matches to process after
> + * this one?
> + */
> + dist_name_info_next = dist_name_info + 1;
> + if (dist_name_info_next >= dist_base_info_sorted_end ||
> + cmp_btf_name_size(&base_name_info, dist_name_info_next))
> + dist_name_info_next = NULL;
Goodness, does this have to be so verbose and ugly?...
First, does "dist_name_info" give us much more information than just
"info" or something like this?
Second,
for (info = search_btf_name_size(&.....);
info && cmp_btf_name_size(...) == 0;
info++) {
...
}
And there is no need for dist_name_info_next and this extra if with
NULL-ing anything out.
Please send a follow up with the clean up, this loop's conditions are
hard to follow (I had to double check that we don't use
dist_name_info_next for any decision making; but I shouldn't even
care, it should be obvious if written as above)
> +
> + if (!dist_name_info->id || dist_name_info->id > r->nr_dist_base_types) {
another off by one? Valid ID is always < number of types, and so `id
>= nr_types` is the condition for invalid ID. Please fix in a follow
up as well.
> + pr_warn("base BTF id [%d] maps to invalid distilled base BTF id [%d]\n",
> + id, dist_name_info->id);
> + err = -EINVAL;
> + goto done;
> + }
> + dist_t = btf_type_by_id(r->dist_base_btf, dist_name_info->id);
> + dist_kind = btf_kind(dist_t);
> +
> + /* Validate that the found distilled type is compatible.
> + * Do not error out on mismatch as another match may
> + * occur for an identically-named type.
> + */
> + switch (dist_kind) {
> + case BTF_KIND_FWD:
> + switch (base_kind) {
> + case BTF_KIND_FWD:
> + if (btf_kflag(dist_t) != btf_kflag(base_t))
> + continue;
> + break;
> + case BTF_KIND_STRUCT:
> + if (btf_kflag(base_t))
> + continue;
> + break;
> + case BTF_KIND_UNION:
> + if (!btf_kflag(base_t))
> + continue;
> + break;
> + default:
> + continue;
> + }
> + break;
I gotta say it's amazing that C allows this intermixing of breaks and
continues to work at completely different "scopes" (switch vs for).
Wonderful language :)
> + case BTF_KIND_INT:
> + if (dist_kind != base_kind ||
> + btf_int_encoding(base_t) != btf_int_encoding(dist_t))
> + continue;
> + break;
[...]
