Re: [PATCH dwarves v3 2/8] btf_encoder: separate elf function, saved function representations

[Jiri Olsa <olsajiri@gmail.com>]

On Sat, Dec 21, 2024 at 01:23:01AM +0000, Ihor Solodrai wrote:

SNIP

> +static int saved_functions_combine(void *_a, void *_b)
> +{
> +	uint8_t optimized, unexpected, inconsistent;
> +	struct btf_encoder_func_state *a = _a;
> +	struct btf_encoder_func_state *b = _b;
> +	int ret;
> +
> +	ret = strncmp(a->elf->name, b->elf->name,
> +		      max(a->elf->prefixlen, b->elf->prefixlen));
> +	if (ret != 0)
> +		return ret;
> +	optimized = a->optimized_parms | b->optimized_parms;
> +	unexpected = a->unexpected_reg | b->unexpected_reg;
> +	inconsistent = a->inconsistent_proto | b->inconsistent_proto;
> +	if (!unexpected && !inconsistent && !funcs__match(a, b))
> +		inconsistent = 1;
> +	a->optimized_parms = b->optimized_parms = optimized;
> +	a->unexpected_reg = b->unexpected_reg = unexpected;
> +	a->inconsistent_proto = b->inconsistent_proto = inconsistent;

do we still need to update the 'b' state object?

> +
> +	return 0;
> +}
> +
> +static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
> +{
> +	struct btf_encoder_func_state *pos, *s;
> +
> +	list_for_each_entry_safe(pos, s, &encoder->func_states, node) {
> +		list_del(&pos->node);
> +		free(pos->parms);
> +		free(pos->annots);
> +		free(pos);
> +	}
> +
> +	for (int i = 0; i < encoder->functions.cnt; i++)
> +		free(encoder->functions.entries[i].alias);
> +}
> +
> +int btf_encoder__add_saved_funcs(struct btf_encoder *encoder)
> +{
> +	struct btf_encoder_func_state **saved_fns, *s;
> +	struct btf_encoder *e = NULL;
> +	int i = 0, j, nr_saved_fns = 0;
> +
> +	/* Retrieve function states from each encoder, combine them
> +	 * and sort by name, addr.
> +	 */
> +	btf_encoders__for_each_encoder(e) {
> +		list_for_each_entry(s, &e->func_states, node)
> +			nr_saved_fns++;
> +	}

the encoder loop goes eventualy away, but still would it make to store
func_states count instead of the loop?

now when there's just single place that stores 'state' it seems like it
should be straighforward

SNIP

>  void btf_encoder__delete(struct btf_encoder *encoder)
>  {
> -	int i;
>  	size_t shndx;
>  
>  	if (encoder == NULL)
> @@ -2447,18 +2469,19 @@ void btf_encoder__delete(struct btf_encoder *encoder)
>  	btf_encoders__delete(encoder);
>  	for (shndx = 0; shndx < encoder->seccnt; shndx++)
>  		__gobuffer__delete(&encoder->secinfo[shndx].secinfo);
> +	free(encoder->secinfo);

nit, this seems unrelated to this change, should be in separate fix?

thanks,
jirka


>  	zfree(&encoder->filename);
>  	zfree(&encoder->source_filename);
>  	btf__free(encoder->btf);
>  	encoder->btf = NULL;
>  	elf_symtab__delete(encoder->symtab);
>  
> -	for (i = 0; i < encoder->functions.cnt; i++)
> -		btf_encoder__delete_func(&encoder->functions.entries[i]);
>  	encoder->functions.allocated = encoder->functions.cnt = 0;
>  	free(encoder->functions.entries);
>  	encoder->functions.entries = NULL;
>  
> +	btf_encoder__delete_saved_funcs(encoder);
> +
>  	free(encoder);
>  }
>  

SNIP