On Wed, Nov 2, 2022 at 8:35 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> Resolve forward declarations that don't take part in type graphs
> comparisons if declaration name is unambiguous. Example:
>
> CU #1:
>
> struct foo; // standalone forward declaration
> struct foo *some_global;
>
> CU #2:
>
> struct foo { int x; };
> struct foo *another_global;
>
> The `struct foo` from CU #1 is not a part of any definition that is
> compared against another definition while `btf_dedup_struct_types`
> processes structural types. The the BTF after `btf_dedup_struct_types`
> the BTF looks as follows:
>
> [1] STRUCT 'foo' size=4 vlen=1 ...
> [2] INT 'int' size=4 ...
> [3] PTR '(anon)' type_id=1
> [4] FWD 'foo' fwd_kind=struct
> [5] PTR '(anon)' type_id=4
>
> This commit adds a new pass `btf_dedup_resolve_fwds`, that maps such
> forward declarations to structs or unions with identical name in case
> if the name is not ambiguous.
>
> The pass is positioned before `btf_dedup_ref_types` so that types
> [3] and [5] could be merged as a same type after [1] and [4] are merged.
> The final result for the example above looks as follows:
>
> [1] STRUCT 'foo' size=4 vlen=1
> 'x' type_id=2 bits_offset=0
> [2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> [3] PTR '(anon)' type_id=1
>
> For defconfig kernel with BTF enabled this removes 63 forward
> declarations. Examples of removed declarations: `pt_regs`, `in6_addr`.
> The running time of `btf__dedup` function is increased by about 3%.
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> tools/lib/bpf/btf.c | 143 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 139 insertions(+), 4 deletions(-)
>
LGTM, small nit about hashmap__new initialization
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> +}
> +
> +static int btf_dedup_resolve_fwd(struct btf_dedup *d, struct hashmap *names_map, __u32 type_id)
> +{
> + struct btf_type *t = btf_type_by_id(d->btf, type_id);
> + enum btf_fwd_kind fwd_kind = btf_kflag(t);
this is a bit subtle, but probably won't ever break as enum
btf_fwd_kind is part of libbpf UAPI
> + __u16 cand_kind, kind = btf_kind(t);
> + struct btf_type *cand_t;
> + uintptr_t cand_id;
> +
> + if (kind != BTF_KIND_FWD)
> + return 0;
> +
> + /* Skip if this FWD already has a mapping */
> + if (type_id != d->map[type_id])
> + return 0;
> +
> + if (!hashmap__find(names_map, t->name_off, &cand_id))
> + return 0;
> +
> + /* Zero is a special value indicating that name is not unique */
> + if (!cand_id)
> + return 0;
> +
> + cand_t = btf_type_by_id(d->btf, cand_id);
> + cand_kind = btf_kind(cand_t);
> + if ((cand_kind == BTF_KIND_STRUCT && fwd_kind != BTF_FWD_STRUCT) ||
> + (cand_kind == BTF_KIND_UNION && fwd_kind != BTF_FWD_UNION))
> + return 0;
> +
> + d->map[type_id] = cand_id;
> +
> + return 0;
> +}
> +
> +/*
> + * Resolve unambiguous forward declarations.
> + *
> + * The lion's share of all FWD declarations is resolved during
> + * `btf_dedup_struct_types` phase when different type graphs are
> + * compared against each other. However, if in some compilation unit a
> + * FWD declaration is not a part of a type graph compared against
> + * another type graph that declaration's canonical type would not be
> + * changed. Example:
> + *
> + * CU #1:
> + *
> + * struct foo;
> + * struct foo *some_global;
> + *
> + * CU #2:
> + *
> + * struct foo { int u; };
> + * struct foo *another_global;
> + *
> + * After `btf_dedup_struct_types` the BTF looks as follows:
> + *
> + * [1] STRUCT 'foo' size=4 vlen=1 ...
> + * [2] INT 'int' size=4 ...
> + * [3] PTR '(anon)' type_id=1
> + * [4] FWD 'foo' fwd_kind=struct
> + * [5] PTR '(anon)' type_id=4
> + *
> + * This pass assumes that such FWD declarations should be mapped to
> + * structs or unions with identical name in case if the name is not
> + * ambiguous.
> + */
> +static int btf_dedup_resolve_fwds(struct btf_dedup *d)
> +{
> + int i, err;
> + struct hashmap *names_map =
> + hashmap__new(btf_dedup_identity_hash_fn, btf_dedup_equal_fn, NULL);
if variable declaration and initialization doesn't even fit in a
single line, that's a signal that they should better be split.
In general we also try to avoid doing "complex" initialization at
declaration time. So please split.
> +
> + if (!names_map)
> + return -ENOMEM;
> +
[...]
