Re: [PATCH v2 bpf-next 02/13] libbpf: add btf__distill_base() creating split BTF with distilled base BTF

[Eduard Zingerman <eddyz87@xxxxxxxxx>]

On Wed, 2024-04-24 at 16:47 +0100, Alan Maguire wrote:

Hi Alan,

Looked through the patch, noted a few minor logical inconsistencies.
Agree with Andrii's comments about memory size allocated for dist.ids.
Otherwise this patch makes sense to me.

[...]

> @@ -5217,3 +5223,301 @@ int btf_ext_visit_str_offs(struct btf_ext *btf_ext, str_off_visit_fn visit, void
>  
>  	return 0;
>  }
> +
> +struct btf_distill_id {
> +	int id;
> +	bool embedded;		/* true if id refers to a struct/union in base BTF
> +				 * that is embedded in a split BTF struct/union.
> +				 */
> +};
> +
> +struct btf_distill {
> +	struct btf_pipe pipe;
> +	struct btf_distill_id *ids;
> +	__u32 query_id;
> +	unsigned int nr_base_types;
> +	unsigned int diff_id;
> +};
> +
> +/* Check if a member of a split BTF struct/union refers to a base BTF
> + * struct/union.  Members can be const/restrict/volatile/typedef
> + * reference types, but if a pointer is encountered, type is no longer
> + * considered embedded.
> + */
> +static int btf_find_embedded_composite_type_ids(__u32 *id, void *ctx)
> +{
> +	struct btf_distill *dist = ctx;
> +	const struct btf_type *t;
> +	__u32 next_id = *id;
> +
> +	do {
> +		if (next_id == 0)
> +			return 0;
> +		t = btf_type_by_id(dist->pipe.src, next_id);
> +		switch (btf_kind(t)) {
> +		case BTF_KIND_CONST:
> +		case BTF_KIND_RESTRICT:
> +		case BTF_KIND_VOLATILE:
> +		case BTF_KIND_TYPEDEF:

I think BTF_KIND_TYPE_TAG is missing.

> +			next_id = t->type;
> +			break;
> +		case BTF_KIND_ARRAY: {
> +			struct btf_array *a = btf_array(t);
> +
> +			next_id = a->type;
> +			break;
> +		}
> +		case BTF_KIND_STRUCT:
> +		case BTF_KIND_UNION:
> +			dist->ids[next_id].embedded = next_id > 0 &&
> +						      next_id <= dist->nr_base_types;

I think next_id can't be zero, otherwise it's kind would be UNKN.
Also, should this be 'next_id < dist->nr_base_types'?

__u32 btf__type_cnt(const struct btf *btf)
{
	return btf->start_id + btf->nr_types;
}

static struct btf *btf_new(const void *data, __u32 size, struct btf *base_btf)
{
	...
	btf->nr_types = 0;
	btf->start_id = 1;
	...
	if (base_btf) {
		...
		btf->start_id = btf__type_cnt(base_btf);
		...
	}
	...
}

int btf__distill_base(const struct btf *src_btf, struct btf **new_base_btf,
		      struct btf **new_split_btf)
{
	...
	dist.nr_base_types = btf__type_cnt(btf__base_btf(src_btf));
	...
}

So, suppose there is only one base type:
- it's ID would be 1;
- nr_types would be 1;
- nr_base_types would be 2;
- meaning that split BTF ids would start from 2.

Maybe use .split_start_id instead of .nr_base_types to avoid confusion?

> +			return 0;
> +		default:
> +			return 0;
> +		}
> +
> +	} while (next_id != 0);
> +
> +	return 0;
> +}