Re: [RFC bpf-next 01/12] libbpf: Deduplicate unambigous standalone forward declarations

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2022-10-27 at 15:07 -0700, Andrii Nakryiko wrote:
> On Tue, Oct 25, 2022 at 3:28 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
[...] 
> > +
> > +/*
> > + * Collect a `name_off_map` that maps type names to type ids for all
> > + * canonical structs and unions. If the same name is shared by several
> > + * canonical types use a special value 0 to indicate this fact.
> > + */
> > +static int btf_dedup_fill_unique_names_map(struct btf_dedup *d, struct hashmap *names_map)
> > +{
> > +       int i, err = 0;
> > +       __u32 type_id, collision_id;
> > +       __u16 kind;
> > +       struct btf_type *t;
> > +
> > +       for (i = 0; i < d->btf->nr_types; i++) {
> > +               type_id = d->btf->start_id + i;
> > +               t = btf_type_by_id(d->btf, type_id);
> > +               kind = btf_kind(t);
> > +
> > +               if (kind != BTF_KIND_STRUCT && kind != BTF_KIND_UNION)
> > +                       continue;
> 
> let's also do ENUM FWD resolution. ENUM FWD is just ENUM with vlen=0

Interestingly this is necessary only for mixed enum / enum64 case.
Forward enum declarations are resolved by bpf/btf.c:btf_dedup_prim_type:

	case BTF_KIND_ENUM:
		h = btf_hash_enum(t);
		for_each_dedup_cand(d, hash_entry, h) {
			cand_id = (__u32)(long)hash_entry->value;
			cand = btf_type_by_id(d->btf, cand_id);
			if (btf_equal_enum(t, cand)) {
				new_id = cand_id;
				break;
			}
			if (btf_compat_enum(t, cand)) {
				if (btf_is_enum_fwd(t)) {
					/* resolve fwd to full enum */
					new_id = cand_id;
					break;
				}
				/* resolve canonical enum fwd to full enum */
				d->map[cand_id] = type_id;
			}
		}
		break;
    // ... similar logic for ENUM64 ...

- btf_hash_enum ignores vlen when hashing;
- btf_compat_enum compares only names and sizes.

So, if forward and main declaration kinds match (either BTF_KIND_ENUM
or BTF_KIND_ENUM64) the forward declaration would be removed. But if
the kinds are different the forward declaration would remain. E.g.:

CU #1:
enum foo;
enum foo *a;

CU #2:
enum foo { x = 0xfffffffff };
enum foo *b;

BTF:
[1] ENUM64 'foo' encoding=UNSIGNED size=8 vlen=1
	'x' val=68719476735ULL
[2] INT 'long unsigned int' size=8 bits_offset=0 nr_bits=64 encoding=(none)
[3] PTR '(anon)' type_id=1
[4] ENUM 'foo' encoding=UNSIGNED size=4 vlen=0
[5] PTR '(anon)' type_id=4

BTF_KIND_FWDs are unified during btf_dedup_struct_types but enum
forward declarations are not. So it would be incorrect to add enum
forward declaration unification logic to btf_dedup_resolve_fwds,
because the following case would not be covered:

CU #1:
enum foo;
struct s { enum foo *a; } *a;

CU #2:
enum foo { x = 0xfffffffff };
struct s { enum foo *a; } *b;

Currently STRUCTs 's' are not de-duplicated.

I think that btf_dedup_prim_type should be adjusted to handle this case.

[...]