Re: [RFC PATCH v7 1/7] libbpf: Add BTF permutation support for type reordering

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-11-19 at 10:21 -0800, Andrii Nakryiko wrote:
> On Tue, Nov 18, 2025 at 7:21 PM Donglin Peng <dolinux.peng@gmail.com> wrote:

[...]

> > +       nt = new_types;
> > +       for (i = 0; i < new_nr_types; i++) {
> > +               struct btf_field_iter it;
> > +               const struct btf_type *t;
> > +               __u32 *type_id;
> > +               int type_size;
> > +
> > +               id = order_map[i];
> > +               /* must be a valid type ID */
> 
> redundant comment, please drop
> 
> > +               t = btf__type_by_id(btf, id);
> > +               if (!t) {
> 
> no need to check this, we already validated that all types are valid earlier
> 
> > +                       err = -EINVAL;
> > +                       goto done;
> > +               }
> > +               type_size = btf_type_size(t);
> > +               memcpy(nt, t, type_size);
> > +
> > +               /* Fix up referenced IDs for BTF */
> > +               err = btf_field_iter_init(&it, nt, BTF_FIELD_ITER_IDS);
> > +               if (err)
> > +                       goto done;
> > +               while ((type_id = btf_field_iter_next(&it))) {
> > +                       err = btf_permute_remap_type_id(type_id, &p);
> > +                       if (err)
> > +                               goto done;
> > +               }
> > +
> > +               nt += type_size;
> > +       }
> > +
> > +       /* Fix up referenced IDs for btf_ext */
> > +       btf_ext = OPTS_GET(opts, btf_ext, NULL);
> > +       if (btf_ext) {
> > +               err = btf_ext_visit_type_ids(btf_ext, btf_permute_remap_type_id, &p);
> > +               if (err)
> > +                       goto done;
> > +       }
> > +
> > +       new_type_len = nt - new_types;
> 
> 
> new_type_len has to be exactly the same as the old size, this is redundant
> 
> > +       next_type = new_types;
> > +       end_type = next_type + new_type_len;
> > +       i = 0;
> > +       while (next_type + sizeof(struct btf_type) <= end_type) {
> 
> while (next_type < end_type)?
> 
> Reference to struct btf_type is confusing, as generally type is bigger
> than just sizeof(struct btf_type). But there is no need for this, with
> correct code next_type < end_type is sufficient check
> 
> But really, this can also be written cleanly as a simple for loop
> 
> for (i = 0; i < nr_types; i++) {
>     btf->type_offs[i] = next_type - new_types;
>     next_type += btf_type_size(next_type);
> }
>

Adding to what Andrii says, the whole group of assignments is
reducible:

  +       new_type_len = nt - new_types;
  +       next_type = new_types;
  +       end_type = next_type + new_type_len;

=> end_type = new_types + new_type_len; // subst next_type -> new_types
=> end_type = new_types + nt - new_types; // subst new_types -> nt - new_types
=> end_type = nt

Why recomputing it in such a convoluted way?

> > +               btf->type_offs[i++] = next_type - new_types;
> > +               next_type += btf_type_size(next_type);
> > +       }
> > +

[...]