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

[Alan Maguire <alan.maguire@oracle.com>]

On 01/05/2024 00:06, Eduard Zingerman wrote:
> 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.
>
thanks for taking a look! I'm working on an updated series incorporating
the approach of limiting distilled base to named struct/union/enum
types, hope to have that ready by the end of the week. It will also OR
in flags to mark types as embedded as per Andrii and your suggestion.
A bit more below..
> [...]
> 
>> @@ -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.
>

It's implicit in the default clause; I can't see a case for having a
split BTF type tag base BTF types, but I might be missing something
there. I can make all the unexpected types explicit if that would be
clearer?


>> +			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'?
> 
yeah this needs to be fixed; also isn't worth range-checking this as
it's got to be a base type AFAICT.

> __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?
> 

good idea, will fix. thanks!

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