Re: [PATCH bpf-next 1/3] bpf: btf: export btf types and name by offset from lib

[Okash Khawaja <osk@fb.com>]

On Wed, Jun 20, 2018 at 11:40:19PM +0100, Song Liu wrote:
> 
> 
> > On Jun 20, 2018, at 1:30 PM, Okash Khawaja <osk@fb.com> wrote:
> > 
> > This patch introduces btf__resolve_type() function and exports two
> > existing functions from libbpf. btf__resolve_type follows modifier
> > types like const and typedef until it hits a type which actually takes
> > up memory, and then returns it. This function follows similar pattern
> > to btf__resolve_size but instead of computing size, it just returns
> > the type.
> > 
> > These  functions will be used in the followig patch which parses
> > information inside array of `struct btf_type *`. btf_name_by_offset is
> > used for printing variable names.
> > 
> > Signed-off-by: Okash Khawaja <osk@fb.com>
> > Acked-by: Martin KaFai Lau <kafai@fb.com>
> > 
> > ---
> > tools/lib/bpf/btf.c |   65 ++++++++++++++++++++++++++++++++++++----------------
> > tools/lib/bpf/btf.h |    3 ++
> > 2 files changed, 48 insertions(+), 20 deletions(-)
> > 
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -17,6 +17,11 @@
> > 
> > #define BTF_MAX_NR_TYPES 65535
> > 
> > +#define IS_MODIFIER(k) (((k) == BTF_KIND_TYPEDEF) || \
> > +		((k) == BTF_KIND_VOLATILE) || \
> > +		((k) == BTF_KIND_CONST) || \
> > +		((k) == BTF_KIND_RESTRICT))
> > +
> > static struct btf_type btf_void;
> > 
> > struct btf {
> > @@ -33,14 +38,6 @@ struct btf {
> > 	int fd;
> > };
> > 
> > -static const char *btf_name_by_offset(const struct btf *btf, uint32_t offset)
> > -{
> > -	if (offset < btf->hdr->str_len)
> > -		return &btf->strings[offset];
> > -	else
> > -		return NULL;
> > -}
> > -
> > static int btf_add_type(struct btf *btf, struct btf_type *t)
> > {
> > 	if (btf->types_size - btf->nr_types < 2) {
> > @@ -190,15 +187,6 @@ static int btf_parse_type_sec(struct btf
> > 	return 0;
> > }
> > 
> > -static const struct btf_type *btf_type_by_id(const struct btf *btf,
> > -					     uint32_t type_id)
> > -{
> > -	if (type_id > btf->nr_types)
> > -		return NULL;
> > -
> > -	return btf->types[type_id];
> > -}
> 
> nit: Why do we need to move these two functions to later of the file? 
No real reason. It looked like this file was following a convention of
keeping statics together. I'll put them back in place if no one objects.

> 
> Other than this,
> 
> Acked-by: Song Liu <songliubraving@fb.com>
Thanks

> 
> > -
> > static bool btf_type_is_void(const struct btf_type *t)
> > {
> > 	return t == &btf_void || BTF_INFO_KIND(t->info) == BTF_KIND_FWD;
> > @@ -234,7 +222,7 @@ int64_t btf__resolve_size(const struct b
> > 	int64_t size = -1;
> > 	int i;
> > 
> > -	t = btf_type_by_id(btf, type_id);
> > +	t = btf__type_by_id(btf, type_id);
> > 	for (i = 0; i < MAX_RESOLVE_DEPTH && !btf_type_is_void_or_null(t);
> > 	     i++) {
> > 		size = btf_type_size(t);
> > @@ -259,7 +247,7 @@ int64_t btf__resolve_size(const struct b
> > 			return -EINVAL;
> > 		}
> > 
> > -		t = btf_type_by_id(btf, type_id);
> > +		t = btf__type_by_id(btf, type_id);
> > 	}
> > 
> > 	if (size < 0)
> > @@ -271,6 +259,26 @@ int64_t btf__resolve_size(const struct b
> > 	return nelems * size;
> > }
> > 
> > +int32_t btf__resolve_type(const struct btf *btf, uint32_t type_id)
> > +{
> > +	const struct btf_type *t;
> > +	int depth = 0;
> > +
> > +	t = btf__type_by_id(btf, type_id);
> > +	while (depth < MAX_RESOLVE_DEPTH &&
> > +			!btf_type_is_void_or_null(t) &&
> > +			IS_MODIFIER(BTF_INFO_KIND(t->info))) {
> > +		type_id = t->type;
> > +		t = btf__type_by_id(btf, type_id);
> > +		depth++;
> > +	}
> > +
> > +	if (depth == MAX_RESOLVE_DEPTH || btf_type_is_void_or_null(t))
> > +		return -EINVAL;
> > +
> > +	return type_id;
> > +}
> > +
> > int32_t btf__find_by_name(const struct btf *btf, const char *type_name)
> > {
> > 	uint32_t i;
> > @@ -280,7 +288,7 @@ int32_t btf__find_by_name(const struct b
> > 
> > 	for (i = 1; i <= btf->nr_types; i++) {
> > 		const struct btf_type *t = btf->types[i];
> > -		const char *name = btf_name_by_offset(btf, t->name_off);
> > +		const char *name = btf__name_by_offset(btf, t->name_off);
> > 
> > 		if (name && !strcmp(type_name, name))
> > 			return i;
> > @@ -371,3 +379,20 @@ int btf__fd(const struct btf *btf)
> > {
> > 	return btf->fd;
> > }
> > +
> > +const char *btf__name_by_offset(const struct btf *btf, uint32_t offset)
> > +{
> > +	if (offset < btf->hdr->str_len)
> > +		return &btf->strings[offset];
> > +	else
> > +		return NULL;
> > +}
> > +
> > +const struct btf_type *btf__type_by_id(const struct btf *btf,
> > +					     uint32_t type_id)
> > +{
> > +	if (type_id > btf->nr_types)
> > +		return NULL;
> > +
> > +	return btf->types[type_id];
> > +}
> > --- a/tools/lib/bpf/btf.h
> > +++ b/tools/lib/bpf/btf.h
> > @@ -17,6 +17,9 @@ void btf__free(struct btf *btf);
> > struct btf *btf__new(uint8_t *data, uint32_t size, btf_print_fn_t err_log);
> > int32_t btf__find_by_name(const struct btf *btf, const char *type_name);
> > int64_t btf__resolve_size(const struct btf *btf, uint32_t type_id);
> > +int32_t btf__resolve_type(const struct btf *btf, uint32_t type_id);
> > int btf__fd(const struct btf *btf);
> > +const char *btf__name_by_offset(const struct btf *btf, uint32_t offset);
> > +const struct btf_type *btf__type_by_id(const struct btf *btf, uint32_t type_id);
> > 
> > #endif
> > 
>