Re: [RFC dwarves] btf_encoder: Remove duplicates from functions entries

[Ihor Solodrai <ihor.solodrai@linux.dev>]

On 7/24/25 10:54 AM, Alan Maguire wrote:
> On 23/07/2025 12:22, Jiri Olsa wrote:
>> On Tue, Jul 22, 2025 at 10:58:52PM +0000, Ihor Solodrai wrote:
>>
>> SNIP
>>
>>> @@ -1338,48 +1381,39 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder,
>>>   	return 0;
>>>   }
>>>   
>>> -static int functions_cmp(const void *_a, const void *_b)
>>> +static int elf_function__name_cmp(const void *_a, const void *_b)
>>>   {
>>>   	const struct elf_function *a = _a;
>>>   	const struct elf_function *b = _b;
>>>   
>>> -	/* if search key allows prefix match, verify target has matching
>>> -	 * prefix len and prefix matches.
>>> -	 */
>>> -	if (a->prefixlen && a->prefixlen == b->prefixlen)
>>> -		return strncmp(a->name, b->name, b->prefixlen);
>>
>> nice to see this one removed ;-)
>>
>>>   	return strcmp(a->name, b->name);
>>>   }
>>>   
>>> -#ifndef max
>>> -#define max(x, y) ((x) < (y) ? (y) : (x))
>>> -#endif
>>> -
>>>   static int saved_functions_cmp(const void *_a, const void *_b)
>>>   {
>>>   	const struct btf_encoder_func_state *a = _a;
>>>   	const struct btf_encoder_func_state *b = _b;
>>>   
>>> -	return functions_cmp(a->elf, b->elf);
>>> +	return elf_function__name_cmp(a->elf, b->elf);
>>>   }
>>>   
>>>   static int saved_functions_combine(struct btf_encoder_func_state *a, struct btf_encoder_func_state *b)
>>>   {
>>> -	uint8_t optimized, unexpected, inconsistent;
>>> -	int ret;
>>> +	uint8_t optimized, unexpected, inconsistent, ambiguous_addr;
>>> +
>>> +	if (a->elf != b->elf)
>>> +		return 1;
>>>   
>>> -	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))
>>> +	ambiguous_addr = a->ambiguous_addr | b->ambiguous_addr;
>>> +	if (!unexpected && !inconsistent && !ambiguous_addr && !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;
>>> +	a->ambiguous_addr = b->ambiguous_addr = ambiguous_addr;
>>
>>
>> I had to add change below to get the functions with multiple addresses out
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index fcc30aa9d97f..7b9679794790 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -1466,7 +1466,7 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e
>>   		 * just do not _use_ them.  Only exclude functions with
>>   		 * unexpected register use or multiple inconsistent prototypes.
>>   		 */
>> -		add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto;
>> +		add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->ambiguous_addr;
>>   
>>   		if (add_to_btf) {
>>   			err = btf_encoder__add_func(state->encoder, state);
>>
>>
>> other than that I like the approach
>>
> 
> Thanks for the patch! I ran it through CI [1] with the above change plus
> an added whitespace after the function name in the printf() in
> btf_encoder__log_func_skip(). The btf_functions.sh test expects
> whitespace after function names when examining skipped functions, so
> either the test should be updated to handle no whitespace or we should
> ensure the space is there after the function name like this:
> 
>          printf("%s : skipping BTF encoding of function due to ",
> func->name);
> 
> Otherwise we get a CI failure that is nothing to do with the changes.
> 
> With this in place we do however lose a lot of functions it seems, some
> I suspect unnecessarily. For example:
> 
> 
> Looking at
> 
>   < void __tcp_send_ack(struct sock * sk, u32 rcv_nxt, u16 flags);
> 
> ffffffff83c83170 t __tcp_send_ack.part.0
> ffffffff83c83310 T __tcp_send_ack
> 
> So __tcp_send_ack is partially inlined, but partial inlines should not
> count as ambiguous addresses I think. We should probably ensure we skip
> .part suffixes as well as .cold in calculating ambiguous addresses.
> 
> I modified the patch somewhat and we wind up losing ~400 functions
> instead of over 700, see [2].
> 
> Modified patch is at [3]. If the mods look okay to you Ihor would you
> mind sending it officially? Would be great to get wider testing to
> ensure it doesn't break anything or leave any functions out unexpectedly.

Alan, Jiri, thank you for review and testing. I sent this draft in a bit 
of a rush, sorry.

I'll incorporate your suggestions, test the patch a bit more and then
will send a clean version. I am curious what functions are lost and
why, will report if notice anything interesting.

> 
>> SNIP
>>
>>> @@ -2153,18 +2191,75 @@ static int elf_functions__collect(struct elf_functions *functions)
>>>   		goto out_free;
>>>   	}
>>>   
>>> +	/* First, collect an elf_function for each GElf_Sym
>>> +	 * Where func->name is without a suffix
>>> +	 */
>>>   	functions->cnt = 0;
>>>   	elf_symtab__for_each_symbol_index(functions->symtab, core_id, sym, sym_sec_idx) {
>>> -		elf_functions__collect_function(functions, &sym);
>>> +
>>> +		if (elf_sym__type(&sym) != STT_FUNC)
>>> +			continue;
>>> +
>>> +		sym_name = elf_sym__name(&sym, functions->symtab);
>>> +		if (!sym_name)
>>> +			continue;
>>> +
>>> +		func = &functions->entries[functions->cnt];
>>> +
>>> +		const char *suffix = strchr(sym_name, '.');
>>> +		if (suffix) {
>>> +			functions->suffix_cnt++;
>>
>> do we need suffix_cnt now?
>>
> 
> think it's been unused for a while now, so can be removed I think.
> 
> Thanks again for working on this!
> 
> Alan
> 
> [1] https://github.com/alan-maguire/dwarves/actions/runs/16500065295
> [2]
> https://github.com/alan-maguire/dwarves/actions/runs/16501897430/job/46662503155
> [3]
> https://github.com/acmel/dwarves/commit/30dffd7fc34e7753b3d21b4b3f1a5e17814c224f
> 
>> thanks,
>> jirka
>>
>>
>>> +			func->name = strndup(sym_name, suffix - sym_name);
>>> +		} else {
>>> +			func->name = strdup(sym_name);
>>> +		}
>>> +		if (!func->name) {
>>> +			err = -ENOMEM;
>>> +			goto out_free;
>>> +		}
>>> +
>>> +		func_sym.name = sym_name;
>>> +		func_sym.addr = sym.st_value;
>>> +
>>> +		err = elf_function__push_sym(func, &func_sym);
>>> +		if (err)
>>> +			goto out_free;
>>> +
>>> +		functions->cnt++;
>>>   	}
>>
>> SNIP
>