Re: [PATCH v2 dwarves 2/5] btf_encoder: refactor function addition into dedicated btf_encoder__add_func

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, Jan 30, 2023 at 02:29:42PM +0000, Alan Maguire escreveu:
> This will be useful for postponing local function addition later on.
> As part of this, store the type id offset and unspecified type in
> the encoder, as this will simplify late addition of local functions.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  btf_encoder.c | 101 +++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 57 insertions(+), 44 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index a5fa04a..44f1905 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -54,6 +54,8 @@ struct btf_encoder {
>  	struct gobuffer   percpu_secinfo;
>  	const char	  *filename;
>  	struct elf_symtab *symtab;
> +	uint32_t	  type_id_off;
> +	uint32_t	  unspecified_type;
>  	bool		  has_index_type,
>  			  need_index_type,
>  			  skip_encoding_vars,
> @@ -593,20 +595,20 @@ static int32_t btf_encoder__add_func_param(struct btf_encoder *encoder, const ch
>  	}
>  }
>  
> -static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t type_id_off, uint32_t tag_type)
> +static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_type)
>  {
>  	if (tag_type == 0)
>  		return 0;
>  
> -	if (encoder->cu->unspecified_type.tag && tag_type == encoder->cu->unspecified_type.type) {
> +	if (tag_type == encoder->unspecified_type) {
>  		// No provision for encoding this, turn it into void.
>  		return 0;
>  	}

Humm, are those two lines (above) really equivalent? IIRC I read that as
encoder->cu->unspecified_type.tag being zero means we still didn't set
it, not that it is void (zero), right?

So if we're passing a tag_type zero, void, we'll return 0, i.e. turn
into a void, so seems equivalent, try not to combine patches like this
in the future, i.e. I would expect, from a quick glance, to have:

-     if (encoder->cu->unspecified_type.tag && tag_type == encoder->cu->unspecified_type.type) {
+     if (encoder->unspecified_type && tag_type == encoder->unspecified_type) {

I.e. just the removal of the indirection thru encoder->cu. Or am I
missing something here?

- Arnaldo

>  
> -	return type_id_off + tag_type;
> +	return encoder->type_id_off + tag_type;
>  }
>  
> -static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype, uint32_t type_id_off)
> +static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype)
>  {
>  	struct btf *btf = encoder->btf;
>  	const struct btf_type *t;
> @@ -616,7 +618,7 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
>  
>  	/* add btf_type for func_proto */
>  	nr_params = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
> -	type_id = btf_encoder__tag_type(encoder, type_id_off, ftype->tag.type);
> +	type_id = btf_encoder__tag_type(encoder, ftype->tag.type);
>  
>  	id = btf__add_func_proto(btf, type_id);
>  	if (id > 0) {
> @@ -634,7 +636,7 @@ static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
>  	ftype__for_each_parameter(ftype, param) {
>  		const char *name = parameter__name(param);
>  
> -		type_id = param->tag.type == 0 ? 0 : type_id_off + param->tag.type;
> +		type_id = param->tag.type == 0 ? 0 : encoder->type_id_off + param->tag.type;
>  		++param_idx;
>  		if (btf_encoder__add_func_param(encoder, name, type_id, param_idx == nr_params))
>  			return -1;
> @@ -762,6 +764,31 @@ static int32_t btf_encoder__add_decl_tag(struct btf_encoder *encoder, const char
>  	return id;
>  }
>  
> +static int32_t btf_encoder__add_func(struct btf_encoder *encoder, struct function *fn)
> +{
> +	int btf_fnproto_id, btf_fn_id, tag_type_id;
> +	struct llvm_annotation *annot;
> +	const char *name;
> +
> +	btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto);
> +	name = function__name(fn);
> +	btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
> +	if (btf_fnproto_id < 0 || btf_fn_id < 0) {
> +		printf("error: failed to encode function '%s'\n", function__name(fn));
> +		return -1;
> +	}
> +	list_for_each_entry(annot, &fn->annots, node) {
> +		tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id,
> +							annot->component_idx);
> +		if (tag_type_id < 0) {
> +			fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
> +				annot->value, name, annot->component_idx);
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  /*
>   * This corresponds to the same macro defined in
>   * include/linux/kallsyms.h
> @@ -859,22 +886,21 @@ static void dump_invalid_symbol(const char *msg, const char *sym,
>  	fprintf(stderr, "PAHOLE: Error: Use '--btf_encode_force' to ignore such symbols and force emit the btf.\n");
>  }
>  
> -static int tag__check_id_drift(const struct tag *tag,
> -			       uint32_t core_id, uint32_t btf_type_id,
> -			       uint32_t type_id_off)
> +static int tag__check_id_drift(struct btf_encoder *encoder, const struct tag *tag,
> +			       uint32_t core_id, uint32_t btf_type_id)
>  {
> -	if (btf_type_id != (core_id + type_id_off)) {
> +	if (btf_type_id != (core_id + encoder->type_id_off)) {
>  		fprintf(stderr,
>  			"%s: %s id drift, core_id: %u, btf_type_id: %u, type_id_off: %u\n",
>  			__func__, dwarf_tag_name(tag->tag),
> -			core_id, btf_type_id, type_id_off);
> +			core_id, btf_type_id, encoder->type_id_off);
>  		return -1;
>  	}
>  
>  	return 0;
>  }
>  
> -static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct tag *tag, uint32_t type_id_off)
> +static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct tag *tag)
>  {
>  	struct type *type = tag__type(tag);
>  	struct class_member *pos;
> @@ -896,7 +922,8 @@ static int32_t btf_encoder__add_struct_type(struct btf_encoder *encoder, struct
>  		 * is required.
>  		 */
>  		name = class_member__name(pos);
> -		if (btf_encoder__add_field(encoder, name, type_id_off + pos->tag.type, pos->bitfield_size, pos->bit_offset))
> +		if (btf_encoder__add_field(encoder, name, encoder->type_id_off + pos->tag.type,
> +					   pos->bitfield_size, pos->bit_offset))
>  			return -1;
>  	}
>  
> @@ -936,11 +963,11 @@ static int32_t btf_encoder__add_enum_type(struct btf_encoder *encoder, struct ta
>  	return type_id;
>  }
>  
> -static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag, uint32_t type_id_off,
> +static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
>  				   struct conf_load *conf_load)
>  {
>  	/* single out type 0 as it represents special type "void" */
> -	uint32_t ref_type_id = tag->type == 0 ? 0 : type_id_off + tag->type;
> +	uint32_t ref_type_id = tag->type == 0 ? 0 : encoder->type_id_off + tag->type;
>  	struct base_type *bt;
>  	const char *name;
>  
> @@ -970,7 +997,7 @@ static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
>  		if (tag__type(tag)->declaration)
>  			return btf_encoder__add_ref_type(encoder, BTF_KIND_FWD, 0, name, tag->tag == DW_TAG_union_type);
>  		else
> -			return btf_encoder__add_struct_type(encoder, tag, type_id_off);
> +			return btf_encoder__add_struct_type(encoder, tag);
>  	case DW_TAG_array_type:
>  		/* TODO: Encode one dimension at a time. */
>  		encoder->need_index_type = true;
> @@ -978,7 +1005,7 @@ static int btf_encoder__encode_tag(struct btf_encoder *encoder, struct tag *tag,
>  	case DW_TAG_enumeration_type:
>  		return btf_encoder__add_enum_type(encoder, tag, conf_load);
>  	case DW_TAG_subroutine_type:
> -		return btf_encoder__add_func_proto(encoder, tag__ftype(tag), type_id_off);
> +		return btf_encoder__add_func_proto(encoder, tag__ftype(tag));
>          case DW_TAG_unspecified_type:
>  		/* Just don't encode this for now, converting anything with this type to void (0) instead.
>  		 *
> @@ -1281,7 +1308,7 @@ static bool ftype__has_arg_names(const struct ftype *ftype)
>  	return true;
>  }
>  
> -static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder, uint32_t type_id_off)
> +static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder)
>  {
>  	struct cu *cu = encoder->cu;
>  	uint32_t core_id;
> @@ -1366,7 +1393,7 @@ static int btf_encoder__encode_cu_variables(struct btf_encoder *encoder, uint32_
>  			continue;
>  		}
>  
> -		type = var->ip.tag.type + type_id_off;
> +		type = var->ip.tag.type + encoder->type_id_off;
>  		linkage = var->external ? BTF_VAR_GLOBAL_ALLOCATED : BTF_VAR_STATIC;
>  
>  		if (encoder->verbose) {
> @@ -1507,7 +1534,6 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>  
>  int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load)
>  {
> -	uint32_t type_id_off = btf__type_cnt(encoder->btf) - 1;
>  	struct llvm_annotation *annot;
>  	int btf_type_id, tag_type_id, skipped_types = 0;
>  	uint32_t core_id;
> @@ -1516,21 +1542,24 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>  	int err = 0;
>  
>  	encoder->cu = cu;
> +	encoder->type_id_off = btf__type_cnt(encoder->btf) - 1;
> +	if (encoder->cu->unspecified_type.tag)
> +		encoder->unspecified_type = encoder->cu->unspecified_type.type;
>  
>  	if (!encoder->has_index_type) {
>  		/* cu__find_base_type_by_name() takes "type_id_t *id" */
>  		type_id_t id;
>  		if (cu__find_base_type_by_name(cu, "int", &id)) {
>  			encoder->has_index_type = true;
> -			encoder->array_index_id = type_id_off + id;
> +			encoder->array_index_id = encoder->type_id_off + id;
>  		} else {
>  			encoder->has_index_type = false;
> -			encoder->array_index_id = type_id_off + cu->types_table.nr_entries;
> +			encoder->array_index_id = encoder->type_id_off + cu->types_table.nr_entries;
>  		}
>  	}
>  
>  	cu__for_each_type(cu, core_id, pos) {
> -		btf_type_id = btf_encoder__encode_tag(encoder, pos, type_id_off, conf_load);
> +		btf_type_id = btf_encoder__encode_tag(encoder, pos, conf_load);
>  
>  		if (btf_type_id == 0) {
>  			++skipped_types;
> @@ -1538,7 +1567,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>  		}
>  
>  		if (btf_type_id < 0 ||
> -		    tag__check_id_drift(pos, core_id, btf_type_id + skipped_types, type_id_off)) {
> +		    tag__check_id_drift(encoder, pos, core_id, btf_type_id + skipped_types)) {
>  			err = -1;
>  			goto out;
>  		}
> @@ -1572,7 +1601,7 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>  			continue;
>  		}
>  
> -		btf_type_id = type_id_off + core_id;
> +		btf_type_id = encoder->type_id_off + core_id;
>  		ns = tag__namespace(pos);
>  		list_for_each_entry(annot, &ns->annots, node) {
>  			tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_type_id, annot->component_idx);
> @@ -1585,8 +1614,6 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>  	}
>  
>  	cu__for_each_function(cu, core_id, fn) {
> -		int btf_fnproto_id, btf_fn_id;
> -		const char *name;
>  
>  		/*
>  		 * Skip functions that:
> @@ -1616,27 +1643,13 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>  				continue;
>  		}
>  
> -		btf_fnproto_id = btf_encoder__add_func_proto(encoder, &fn->proto, type_id_off);
> -		name = function__name(fn);
> -		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false);
> -		if (btf_fnproto_id < 0 || btf_fn_id < 0) {
> -			err = -1;
> -			printf("error: failed to encode function '%s'\n", function__name(fn));
> +		err = btf_encoder__add_func(encoder, fn);
> +		if (err)
>  			goto out;
> -		}
> -
> -		list_for_each_entry(annot, &fn->annots, node) {
> -			tag_type_id = btf_encoder__add_decl_tag(encoder, annot->value, btf_fn_id, annot->component_idx);
> -			if (tag_type_id < 0) {
> -				fprintf(stderr, "error: failed to encode tag '%s' to func %s with component_idx %d\n",
> -					annot->value, name, annot->component_idx);
> -				goto out;
> -			}
> -		}
>  	}
>  
>  	if (!encoder->skip_encoding_vars)
> -		err = btf_encoder__encode_cu_variables(encoder, type_id_off);
> +		err = btf_encoder__encode_cu_variables(encoder);
>  out:
>  	encoder->cu = NULL;
>  	return err;
> -- 
> 1.8.3.1
> 

-- 

- Arnaldo