Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Ihor Solodrai]

On 7/28/25 7:03 PM, Ihor Solodrai wrote:
> btf_encoder collects function ELF symbols into a table, which is later
> used for processing DWARF data and determining whether a function can
> be added to BTF.
> 
> So far the ELF symbol name was used as a key for search in this table,
> and a search by prefix match was attempted in cases when ELF symbol
> name has a compiler-generated suffix.
> 
> This implementation has bugs [1][2], causing some functions to be
> inappropriately excluded from (or included into) BTF.
> 
> Rework the implementation of the ELF functions table. Use a name of a
> function without any suffix - symbol name before the first occurrence
> of '.' - as a key. This way btf_encoder__find_function() always
> returns a valid elf_function object (or NULL).
> 
> Collect an array of symbol name + address pairs from GElf_Sym for each
> elf_function when building the elf_functions table.
> 
> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
> when the function is saved by examining the array of ELF symbols in
> elf_function__has_ambiguous_address(). It tests whether there is only
> one unique address for this function name, taking into account that
> some addresses associated with it are not relevant:
>    * ".cold" suffix indicates a piece of hot/cold split
>    * ".part" suffix indicates a piece of partial inline
> 
> When inspecting symbol name we have to search for any occurrence of
> the target suffix, as opposed to testing the entire suffix, or the end
> of a string. This is because suffixes may be combined by the compiler,
> for example producing ".isra0.cold", and the conclusion will be
> incorrect.
> 
> In saved_functions_combine() check ambiguous_addr when deciding
> whether a function should be included in BTF.
> 
> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
> 
> I manually spot checked some of the ~200 functions from vmlinux (BPF
> CI-like kconfig) that are now excluded: all of those that I checked
> had multiple addresses, and some where static functions from different
> files with the same name.
> 
> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-b1cb-10266c72bd45@linux.dev/
> [2] https://lore.kernel.org/dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
> 
> v1: https://lore.kernel.org/dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
> [...]

Not sure what's wrong, but it appears this message can't reach
vger.kernel.org, or maybe is spam filtered.

I sent to vger.kernel.org from @meta.com email in the past w/o issues.

Any suggestions?..

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Ihor Solodrai]

On 7/28/25 8:19 PM, Ihor Solodrai wrote:
> On 7/28/25 7:03 PM, Ihor Solodrai wrote:
>> btf_encoder collects function ELF symbols into a table, which is later
>> used for processing DWARF data and determining whether a function can
>> be added to BTF.
>>
>> So far the ELF symbol name was used as a key for search in this table,
>> and a search by prefix match was attempted in cases when ELF symbol
>> name has a compiler-generated suffix.
>>
>> This implementation has bugs [1][2], causing some functions to be
>> inappropriately excluded from (or included into) BTF.
>>
>> Rework the implementation of the ELF functions table. Use a name of a
>> function without any suffix - symbol name before the first occurrence
>> of '.' - as a key. This way btf_encoder__find_function() always
>> returns a valid elf_function object (or NULL).
>>
>> Collect an array of symbol name + address pairs from GElf_Sym for each
>> elf_function when building the elf_functions table.
>>
>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
>> when the function is saved by examining the array of ELF symbols in
>> elf_function__has_ambiguous_address(). It tests whether there is only
>> one unique address for this function name, taking into account that
>> some addresses associated with it are not relevant:
>>    * ".cold" suffix indicates a piece of hot/cold split
>>    * ".part" suffix indicates a piece of partial inline
>>
>> When inspecting symbol name we have to search for any occurrence of
>> the target suffix, as opposed to testing the entire suffix, or the end
>> of a string. This is because suffixes may be combined by the compiler,
>> for example producing ".isra0.cold", and the conclusion will be
>> incorrect.
>>
>> In saved_functions_combine() check ambiguous_addr when deciding
>> whether a function should be included in BTF.
>>
>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>
>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>> CI-like kconfig) that are now excluded: all of those that I checked
>> had multiple addresses, and some where static functions from different
>> files with the same name.
>>
>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385- 
>> b1cb-10266c72bd45@linux.dev/
>> [2] https://lore.kernel.org/ 
>> dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>
>> v1: https://lore.kernel.org/ 
>> dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>> [...]
> 
> Not sure what's wrong, but it appears this message can't reach
> vger.kernel.org, or maybe is spam filtered.
> 
> I sent to vger.kernel.org from @meta.com email in the past w/o issues.
> 
> Any suggestions?..
> 

Never mind, I think it's some security rules on Meta side.

Alan, please let me know if you received the patch, or if I should
resend again. Thanks.

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Jiri Olsa]

On Mon, Jul 28, 2025 at 07:03:08PM -0700, Ihor Solodrai wrote:
> btf_encoder collects function ELF symbols into a table, which is later
> used for processing DWARF data and determining whether a function can
> be added to BTF.
> 
> So far the ELF symbol name was used as a key for search in this table,
> and a search by prefix match was attempted in cases when ELF symbol
> name has a compiler-generated suffix.
> 
> This implementation has bugs [1][2], causing some functions to be
> inappropriately excluded from (or included into) BTF.
> 
> Rework the implementation of the ELF functions table. Use a name of a
> function without any suffix - symbol name before the first occurrence
> of '.' - as a key. This way btf_encoder__find_function() always
> returns a valid elf_function object (or NULL).
> 
> Collect an array of symbol name + address pairs from GElf_Sym for each
> elf_function when building the elf_functions table.
> 
> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
> when the function is saved by examining the array of ELF symbols in
> elf_function__has_ambiguous_address(). It tests whether there is only
> one unique address for this function name, taking into account that
> some addresses associated with it are not relevant:
>   * ".cold" suffix indicates a piece of hot/cold split
>   * ".part" suffix indicates a piece of partial inline
> 
> When inspecting symbol name we have to search for any occurrence of
> the target suffix, as opposed to testing the entire suffix, or the end
> of a string. This is because suffixes may be combined by the compiler,
> for example producing ".isra0.cold", and the conclusion will be
> incorrect.
> 
> In saved_functions_combine() check ambiguous_addr when deciding
> whether a function should be included in BTF.
> 
> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
> 
> I manually spot checked some of the ~200 functions from vmlinux (BPF
> CI-like kconfig) that are now excluded: all of those that I checked
> had multiple addresses, and some where static functions from different
> files with the same name.

in my config the change removed 464, did the same check and all of them
had more different addresses

couple nits below, but feel free to ignore

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka


> 
> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-b1cb-10266c72bd45@linux.dev/
> [2] https://lore.kernel.org/dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
> 
> v1: https://lore.kernel.org/dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
> ---
>  btf_encoder.c | 250 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 162 insertions(+), 88 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 0bc2334..0aa94ae 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -87,16 +87,22 @@ struct btf_encoder_func_state {
>  	uint8_t optimized_parms:1;
>  	uint8_t unexpected_reg:1;
>  	uint8_t inconsistent_proto:1;
> +	uint8_t ambiguous_addr:1;
>  	int ret_type_id;
>  	struct btf_encoder_func_parm *parms;
>  	struct btf_encoder_func_annot *annots;
>  };
>  
> +struct elf_function_sym {
> +	const char *name;
> +	uint64_t addr;
> +};
> +
>  struct elf_function {
> -	const char	*name;
> -	char		*alias;
> -	size_t		prefixlen;
> -	bool		kfunc;
> +	char		*name;
> +	struct elf_function_sym *syms;
> +	uint8_t 	sym_cnt;

should we make this bigger, or at least check the overflow

256 dups are probably not possible, but with all those different
suffixes we might get close soon ;-)

> +	uint8_t		kfunc:1;
>  	uint32_t	kfunc_flags;
>  };
>  
> @@ -115,7 +121,6 @@ struct elf_functions {
>  	struct elf_symtab *symtab;
>  	struct elf_function *entries;
>  	int cnt;
> -	int suffix_cnt; /* number of .isra, .part etc */
>  };
>  
>  /*
> @@ -161,10 +166,18 @@ struct btf_kfunc_set_range {
>  	uint64_t end;
>  };
>  
> +static inline void elf_function__free_content(struct elf_function *func) {

elf_function__clear ?

> +	free(func->name);
> +	if (func->sym_cnt)
> +		free(func->syms);
> +	memset(func, 0, sizeof(*func));
> +}
> +
>  static inline void elf_functions__delete(struct elf_functions *funcs)
>  {
> -	for (int i = 0; i < funcs->cnt; i++)
> -		free(funcs->entries[i].alias);
> +	for (int i = 0; i < funcs->cnt; i++) {
> +		elf_function__free_content(&funcs->entries[i]);
> +	}
>  	free(funcs->entries);
>  	elf_symtab__delete(funcs->symtab);
>  	list_del(&funcs->node);
> @@ -981,8 +994,7 @@ static void btf_encoder__log_func_skip(struct btf_encoder *encoder, struct elf_f
>  
>  	if (!encoder->verbose)
>  		return;
> -	printf("%s (%s): skipping BTF encoding of function due to ",
> -	       func->alias ?: func->name, func->name);
> +	printf("%s : skipping BTF encoding of function due to ", func->name);
>  	va_start(ap, fmt);
>  	vprintf(fmt, ap);
>  	va_end(ap);
> @@ -1176,6 +1188,48 @@ static struct btf_encoder_func_state *btf_encoder__alloc_func_state(struct btf_e
>  	return state;
>  }
>  
> +/* some "." suffixes do not correspond to real functions;
> + * - .part for partial inline
> + * - .cold for rarely-used codepath extracted for better code locality
> + */
> +static bool str_contains_non_fn_suffix(const char *str) {
> +	static const char *skip[] = {
> +		".cold",
> +		".part"
> +	};
> +	char *suffix = strchr(str, '.');
> +	int i;
> +
> +	if (!suffix)
> +		return false;
> +	for (i = 0; i < ARRAY_SIZE(skip); i++) {
> +		if (strstr(suffix, skip[i]))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static bool elf_function__has_ambiguous_address(struct elf_function *func) {
> +	struct elf_function_sym *sym;
> +	uint64_t addr;
> +
> +	if (func->sym_cnt <= 1)
> +		return false;
> +
> +	addr = 0;
> +	for (int i = 0; i < func->sym_cnt; i++) {
> +		sym = &func->syms[i];
> +		if (!str_contains_non_fn_suffix(sym->name)) {
> +			if (addr && addr != sym->addr)
> +				return true;
> +			else
> +			 	addr = sym->addr;

nit extra space ' '    ^^^

> +		}
> +	}
> +
> +	return false;
> +}
> +

SNIP

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Alan Maguire]

On 29/07/2025 03:03, Ihor Solodrai wrote:
> btf_encoder collects function ELF symbols into a table, which is later
> used for processing DWARF data and determining whether a function can
> be added to BTF.
> 
> So far the ELF symbol name was used as a key for search in this table,
> and a search by prefix match was attempted in cases when ELF symbol
> name has a compiler-generated suffix.
> 
> This implementation has bugs [1][2], causing some functions to be
> inappropriately excluded from (or included into) BTF.
> 
> Rework the implementation of the ELF functions table. Use a name of a
> function without any suffix - symbol name before the first occurrence
> of '.' - as a key. This way btf_encoder__find_function() always
> returns a valid elf_function object (or NULL).
> 
> Collect an array of symbol name + address pairs from GElf_Sym for each
> elf_function when building the elf_functions table.
> 
> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
> when the function is saved by examining the array of ELF symbols in
> elf_function__has_ambiguous_address(). It tests whether there is only
> one unique address for this function name, taking into account that
> some addresses associated with it are not relevant:
>   * ".cold" suffix indicates a piece of hot/cold split
>   * ".part" suffix indicates a piece of partial inline
> 
> When inspecting symbol name we have to search for any occurrence of
> the target suffix, as opposed to testing the entire suffix, or the end
> of a string. This is because suffixes may be combined by the compiler,
> for example producing ".isra0.cold", and the conclusion will be
> incorrect.
> 
> In saved_functions_combine() check ambiguous_addr when deciding
> whether a function should be included in BTF.
> 
> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
> 
> I manually spot checked some of the ~200 functions from vmlinux (BPF
> CI-like kconfig) that are now excluded: all of those that I checked
> had multiple addresses, and some where static functions from different
> files with the same name.
> 
> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-b1cb-10266c72bd45@linux.dev/
> [2] https://lore.kernel.org/dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
> 
> v1: https://lore.kernel.org/dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>

Thanks for doing this Ihor! Apologies for just thinking of this now, but
why don't we filter out the .cold and .part functions earlier, not even
adding them to the ELF functions list? Something like this on top of
your patch:

$ git diff
diff --git a/btf_encoder.c b/btf_encoder.c
index 0aa94ae..f61db0f 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
*btf_encoder__alloc_func_state(struct btf_e
        return state;
 }

-/* some "." suffixes do not correspond to real functions;
- * - .part for partial inline
- * - .cold for rarely-used codepath extracted for better code locality
- */
-static bool str_contains_non_fn_suffix(const char *str) {
-       static const char *skip[] = {
-               ".cold",
-               ".part"
-       };
-       char *suffix = strchr(str, '.');
-       int i;
-
-       if (!suffix)
-               return false;
-       for (i = 0; i < ARRAY_SIZE(skip); i++) {
-               if (strstr(suffix, skip[i]))
-                       return true;
-       }
-       return false;
-}
-
 static bool elf_function__has_ambiguous_address(struct elf_function
*func) {
        struct elf_function_sym *sym;
        uint64_t addr;
@@ -1219,12 +1198,10 @@ static bool
elf_function__has_ambiguous_address(struct elf_function *func) {
        addr = 0;
        for (int i = 0; i < func->sym_cnt; i++) {
                sym = &func->syms[i];
-               if (!str_contains_non_fn_suffix(sym->name)) {
-                       if (addr && addr != sym->addr)
-                               return true;
-                       else
+               if (addr && addr != sym->addr)
+                       return true;
+               else
                                addr = sym->addr;
-               }
        }


        return false;
@@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
elf_functions *functions)
                func = &functions->entries[functions->cnt];

                suffix = strchr(sym_name, '.');
-               if (suffix)
+               if (suffix) {
+                       if (strstr(suffix, ".part") ||
+                           strstr(suffix, ".cold"))
+                               continue;
                        func->name = strndup(sym_name, suffix - sym_name);
-               else
+               } else
                        func->name = strdup(sym_name);

                if (!func->name) {

I think that would work and saves later string comparisons, what do you
think?

> ---
>  btf_encoder.c | 250 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 162 insertions(+), 88 deletions(-)
> 
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 0bc2334..0aa94ae 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -87,16 +87,22 @@ struct btf_encoder_func_state {
>  	uint8_t optimized_parms:1;
>  	uint8_t unexpected_reg:1;
>  	uint8_t inconsistent_proto:1;
> +	uint8_t ambiguous_addr:1;
>  	int ret_type_id;
>  	struct btf_encoder_func_parm *parms;
>  	struct btf_encoder_func_annot *annots;
>  };
>  
> +struct elf_function_sym {
> +	const char *name;
> +	uint64_t addr;
> +};
> +
>  struct elf_function {
> -	const char	*name;
> -	char		*alias;
> -	size_t		prefixlen;
> -	bool		kfunc;
> +	char		*name;
> +	struct elf_function_sym *syms;
> +	uint8_t 	sym_cnt;
> +	uint8_t		kfunc:1;
>  	uint32_t	kfunc_flags;
>  };
>  
> @@ -115,7 +121,6 @@ struct elf_functions {
>  	struct elf_symtab *symtab;
>  	struct elf_function *entries;
>  	int cnt;
> -	int suffix_cnt; /* number of .isra, .part etc */
>  };
>  
>  /*
> @@ -161,10 +166,18 @@ struct btf_kfunc_set_range {
>  	uint64_t end;
>  };
>  
> +static inline void elf_function__free_content(struct elf_function *func) {
> +	free(func->name);
> +	if (func->sym_cnt)
> +		free(func->syms);
> +	memset(func, 0, sizeof(*func));
> +}
> +
>  static inline void elf_functions__delete(struct elf_functions *funcs)
>  {
> -	for (int i = 0; i < funcs->cnt; i++)
> -		free(funcs->entries[i].alias);
> +	for (int i = 0; i < funcs->cnt; i++) {
> +		elf_function__free_content(&funcs->entries[i]);
> +	}
>  	free(funcs->entries);
>  	elf_symtab__delete(funcs->symtab);
>  	list_del(&funcs->node);
> @@ -981,8 +994,7 @@ static void btf_encoder__log_func_skip(struct btf_encoder *encoder, struct elf_f
>  
>  	if (!encoder->verbose)
>  		return;
> -	printf("%s (%s): skipping BTF encoding of function due to ",
> -	       func->alias ?: func->name, func->name);
> +	printf("%s : skipping BTF encoding of function due to ", func->name);
>  	va_start(ap, fmt);
>  	vprintf(fmt, ap);
>  	va_end(ap);
> @@ -1176,6 +1188,48 @@ static struct btf_encoder_func_state *btf_encoder__alloc_func_state(struct btf_e
>  	return state;
>  }
>  
> +/* some "." suffixes do not correspond to real functions;
> + * - .part for partial inline
> + * - .cold for rarely-used codepath extracted for better code locality
> + */
> +static bool str_contains_non_fn_suffix(const char *str) {
> +	static const char *skip[] = {
> +		".cold",
> +		".part"
> +	};
> +	char *suffix = strchr(str, '.');
> +	int i;
> +
> +	if (!suffix)
> +		return false;
> +	for (i = 0; i < ARRAY_SIZE(skip); i++) {
> +		if (strstr(suffix, skip[i]))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static bool elf_function__has_ambiguous_address(struct elf_function *func) {
> +	struct elf_function_sym *sym;
> +	uint64_t addr;
> +
> +	if (func->sym_cnt <= 1)
> +		return false;
> +
> +	addr = 0;
> +	for (int i = 0; i < func->sym_cnt; i++) {
> +		sym = &func->syms[i];
> +		if (!str_contains_non_fn_suffix(sym->name)) {
> +			if (addr && addr != sym->addr)
> +				return true;
> +			else
> +			 	addr = sym->addr;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>  {
>  	struct btf_encoder_func_state *state = btf_encoder__alloc_func_state(encoder);
> @@ -1191,6 +1245,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
>  
>  	state->encoder = encoder;
>  	state->elf = func;
> +	state->ambiguous_addr = elf_function__has_ambiguous_address(func);
>  	state->nr_parms = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
>  	state->ret_type_id = ftype->tag.type == 0 ? 0 : encoder->type_id_off + ftype->tag.type;
>  	if (state->nr_parms > 0) {
> @@ -1294,7 +1349,7 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder,
>  	int err;
>  
>  	btf_fnproto_id = btf_encoder__add_func_proto(encoder, NULL, state);
> -	name = func->alias ?: func->name;
> +	name = func->name;
>  	if (btf_fnproto_id >= 0)
>  		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id,
>  						      name, false);
> @@ -1338,48 +1393,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);
>  	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;
>  
>  	return 0;
>  }
> @@ -1432,7 +1478,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);
> @@ -1447,32 +1493,6 @@ out:
>  	return err;
>  }
>  
> -static void elf_functions__collect_function(struct elf_functions *functions, GElf_Sym *sym)
> -{
> -	struct elf_function *func;
> -	const char *name;
> -
> -	if (elf_sym__type(sym) != STT_FUNC)
> -		return;
> -
> -	name = elf_sym__name(sym, functions->symtab);
> -	if (!name)
> -		return;
> -
> -	func = &functions->entries[functions->cnt];
> -	func->name = name;
> -	if (strchr(name, '.')) {
> -		const char *suffix = strchr(name, '.');
> -
> -		functions->suffix_cnt++;
> -		func->prefixlen = suffix - name;
> -	} else {
> -		func->prefixlen = strlen(name);
> -	}
> -
> -	functions->cnt++;
> -}
> -
>  static struct elf_functions *btf_encoder__elf_functions(struct btf_encoder *encoder)
>  {
>  	struct elf_functions *funcs = NULL;
> @@ -1490,13 +1510,12 @@ static struct elf_functions *btf_encoder__elf_functions(struct btf_encoder *enco
>  	return funcs;
>  }
>  
> -static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
> -						       const char *name, size_t prefixlen)
> +static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder, const char *name)
>  {
>  	struct elf_functions *funcs = elf_functions__find(encoder->cu->elf, &encoder->elf_functions_list);
> -	struct elf_function key = { .name = name, .prefixlen = prefixlen };
> +	struct elf_function key = { .name = (char*)name };
>  
> -	return bsearch(&key, funcs->entries, funcs->cnt, sizeof(key), functions_cmp);
> +	return bsearch(&key, funcs->entries, funcs->cnt, sizeof(key), elf_function__name_cmp);
>  }
>  
>  static bool btf_name_char_ok(char c, bool first)
> @@ -2060,7 +2079,7 @@ static int btf_encoder__collect_kfuncs(struct btf_encoder *encoder)
>  			continue;
>  		}
>  
> -		elf_fn = btf_encoder__find_function(encoder, func, 0);
> +		elf_fn = btf_encoder__find_function(encoder, func);
>  		if (elf_fn) {
>  			elf_fn->kfunc = true;
>  			elf_fn->kfunc_flags = pair->flags;
> @@ -2135,14 +2154,34 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
>  	return err;
>  }
>  
> +static inline int elf_function__push_sym(struct elf_function *func, struct elf_function_sym *sym) {
> +	struct elf_function_sym *tmp;
> +
> +	if (func->sym_cnt)
> +		tmp = realloc(func->syms, (func->sym_cnt + 1) * sizeof(func->syms[0]));
> +	else
> +		tmp = calloc(sizeof(func->syms[0]), 1);
> +
> +	if (!tmp)
> +		return -ENOMEM;
> +
> +	func->syms = tmp;
> +	func->syms[func->sym_cnt] = *sym;
> +	func->sym_cnt++;
> +
> +	return 0;
> +}
> +
>  static int elf_functions__collect(struct elf_functions *functions)
>  {
>  	uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab);
> -	struct elf_function *tmp;
> +	struct elf_function_sym func_sym;
> +	struct elf_function *func, *tmp;
> +	const char *sym_name, *suffix;
>  	Elf32_Word sym_sec_idx;
> +	int err = 0, i, j;
>  	uint32_t core_id;
>  	GElf_Sym sym;
> -	int err = 0;
>  
>  	/* We know that number of functions is less than number of symbols,
>  	 * so we can overallocate temporarily.
> @@ -2153,18 +2192,72 @@ 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];
> +
> +		suffix = strchr(sym_name, '.');
> +		if (suffix)
> +			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++;
>  	}
>  
> +	/* At this point functions->entries is an unordered array of elf_function
> +	 * each having a name (without a suffix) and a single elf_function_sym (maybe with suffix)
> +	 * Now let's sort this table by name.
> +	 */
>  	if (functions->cnt) {
> -		qsort(functions->entries, functions->cnt, sizeof(*functions->entries), functions_cmp);
> +		qsort(functions->entries, functions->cnt, sizeof(*functions->entries), elf_function__name_cmp);
>  	} else {
>  		err = 0;
>  		goto out_free;
>  	}
>  
> +	/* Finally dedup by name, transforming { name -> syms[1] } entries into { name -> syms[n] } */
> +	i = 0;
> +	j = 1;
> +	for (j = 1; j < functions->cnt; j++) {
> +		struct elf_function *a = &functions->entries[i];
> +		struct elf_function *b = &functions->entries[j];
> +
> +		if (!strcmp(a->name, b->name)) {
> +			elf_function__push_sym(a, &b->syms[0]);
> +			elf_function__free_content(b);
> +		} else {
> +			i++;
> +			if (i != j)
> +				functions->entries[i] = functions->entries[j];
> +		}
> +	}
> +
> +	functions->cnt = i + 1;
> +
>  	/* Reallocate to the exact size */
>  	tmp = realloc(functions->entries, functions->cnt * sizeof(struct elf_function));
>  	if (tmp) {
> @@ -2661,30 +2754,11 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>  			if (!name)
>  				continue;
>  
> -			/* prefer exact function name match... */
> -			func = btf_encoder__find_function(encoder, name, 0);
> -			if (!func && funcs->suffix_cnt &&
> -			    conf_load->btf_gen_optimized) {
> -				/* falling back to name.isra.0 match if no exact
> -				 * match is found; only bother if we found any
> -				 * .suffix function names.  The function
> -				 * will be saved and added once we ensure
> -				 * it does not have optimized-out parameters
> -				 * in any cu.
> -				 */
> -				func = btf_encoder__find_function(encoder, name,
> -								  strlen(name));
> -				if (func) {
> -					if (encoder->verbose)
> -						printf("matched function '%s' with '%s'%s\n",
> -						       name, func->name,
> -						       fn->proto.optimized_parms ?
> -						       ", has optimized-out parameters" :
> -						       fn->proto.unexpected_reg ? ", has unexpected register use by params" :
> -						       "");
> -					if (!func->alias)
> -						func->alias = strdup(name);
> -				}
> +			func = btf_encoder__find_function(encoder, name);
> +			if (!func) {
> +				if (encoder->verbose)
> +					printf("could not find function '%s' in the ELF functions table\n", name);
> +				continue;
>  			}
>  		} else {
>  			if (!fn->external)

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Ihor Solodrai]

On 7/31/25 7:16 AM, Alan Maguire wrote:
> On 29/07/2025 03:03, Ihor Solodrai wrote:
>> btf_encoder collects function ELF symbols into a table, which is later
>> used for processing DWARF data and determining whether a function can
>> be added to BTF.
>>
>> So far the ELF symbol name was used as a key for search in this table,
>> and a search by prefix match was attempted in cases when ELF symbol
>> name has a compiler-generated suffix.
>>
>> This implementation has bugs [1][2], causing some functions to be
>> inappropriately excluded from (or included into) BTF.
>>
>> Rework the implementation of the ELF functions table. Use a name of a
>> function without any suffix - symbol name before the first occurrence
>> of '.' - as a key. This way btf_encoder__find_function() always
>> returns a valid elf_function object (or NULL).
>>
>> Collect an array of symbol name + address pairs from GElf_Sym for each
>> elf_function when building the elf_functions table.
>>
>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
>> when the function is saved by examining the array of ELF symbols in
>> elf_function__has_ambiguous_address(). It tests whether there is only
>> one unique address for this function name, taking into account that
>> some addresses associated with it are not relevant:
>>    * ".cold" suffix indicates a piece of hot/cold split
>>    * ".part" suffix indicates a piece of partial inline
>>
>> When inspecting symbol name we have to search for any occurrence of
>> the target suffix, as opposed to testing the entire suffix, or the end
>> of a string. This is because suffixes may be combined by the compiler,
>> for example producing ".isra0.cold", and the conclusion will be
>> incorrect.
>>
>> In saved_functions_combine() check ambiguous_addr when deciding
>> whether a function should be included in BTF.
>>
>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>
>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>> CI-like kconfig) that are now excluded: all of those that I checked
>> had multiple addresses, and some where static functions from different
>> files with the same name.
>>
>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-b1cb-10266c72bd45@linux.dev/
>> [2] https://lore.kernel.org/dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>
>> v1: https://lore.kernel.org/dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
> 
> Thanks for doing this Ihor! Apologies for just thinking of this now, but
> why don't we filter out the .cold and .part functions earlier, not even
> adding them to the ELF functions list? Something like this on top of
> your patch:
> 
> $ git diff
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 0aa94ae..f61db0f 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
> *btf_encoder__alloc_func_state(struct btf_e
>          return state;
>   }
> 
> -/* some "." suffixes do not correspond to real functions;
> - * - .part for partial inline
> - * - .cold for rarely-used codepath extracted for better code locality
> - */
> -static bool str_contains_non_fn_suffix(const char *str) {
> -       static const char *skip[] = {
> -               ".cold",
> -               ".part"
> -       };
> -       char *suffix = strchr(str, '.');
> -       int i;
> -
> -       if (!suffix)
> -               return false;
> -       for (i = 0; i < ARRAY_SIZE(skip); i++) {
> -               if (strstr(suffix, skip[i]))
> -                       return true;
> -       }
> -       return false;
> -}
> -
>   static bool elf_function__has_ambiguous_address(struct elf_function
> *func) {
>          struct elf_function_sym *sym;
>          uint64_t addr;
> @@ -1219,12 +1198,10 @@ static bool
> elf_function__has_ambiguous_address(struct elf_function *func) {
>          addr = 0;
>          for (int i = 0; i < func->sym_cnt; i++) {
>                  sym = &func->syms[i];
> -               if (!str_contains_non_fn_suffix(sym->name)) {
> -                       if (addr && addr != sym->addr)
> -                               return true;
> -                       else
> +               if (addr && addr != sym->addr)
> +                       return true;
> +               else
>                                  addr = sym->addr;
> -               }
>          }
> 
> 
>          return false;
> @@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
> elf_functions *functions)
>                  func = &functions->entries[functions->cnt];
> 
>                  suffix = strchr(sym_name, '.');
> -               if (suffix)
> +               if (suffix) {
> +                       if (strstr(suffix, ".part") ||
> +                           strstr(suffix, ".cold"))
> +                               continue;
>                          func->name = strndup(sym_name, suffix - sym_name);
> -               else
> +               } else
>                          func->name = strdup(sym_name);
> 
>                  if (!func->name) {
> 
> I think that would work and saves later string comparisons, what do you
> think?

My thinking was that in the future pahole may want to use the
information about those symbols when generating BTF, such as for
(partial) inline support. But that is a stretch, as those potential
features are complicated.

And it would be easy to add the suffixed symbols back when
necessary. So will change in v3.


> 
>> ---
>>   btf_encoder.c | 250 ++++++++++++++++++++++++++++++++------------------
>>   1 file changed, 162 insertions(+), 88 deletions(-)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 0bc2334..0aa94ae 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -87,16 +87,22 @@ struct btf_encoder_func_state {
>>   	uint8_t optimized_parms:1;
>>   	uint8_t unexpected_reg:1;
>>   	uint8_t inconsistent_proto:1;
>> +	uint8_t ambiguous_addr:1;
>>   	int ret_type_id;
>>   	struct btf_encoder_func_parm *parms;
>>   	struct btf_encoder_func_annot *annots;
>>   };
>>   
>> +struct elf_function_sym {
>> +	const char *name;
>> +	uint64_t addr;
>> +};
>> +
>>   struct elf_function {
>> -	const char	*name;
>> -	char		*alias;
>> -	size_t		prefixlen;
>> -	bool		kfunc;
>> +	char		*name;
>> +	struct elf_function_sym *syms;
>> +	uint8_t 	sym_cnt;
>> +	uint8_t		kfunc:1;
>>   	uint32_t	kfunc_flags;
>>   };
>>   
>> @@ -115,7 +121,6 @@ struct elf_functions {
>>   	struct elf_symtab *symtab;
>>   	struct elf_function *entries;
>>   	int cnt;
>> -	int suffix_cnt; /* number of .isra, .part etc */
>>   };
>>   
>>   /*
>> @@ -161,10 +166,18 @@ struct btf_kfunc_set_range {
>>   	uint64_t end;
>>   };
>>   
>> +static inline void elf_function__free_content(struct elf_function *func) {
>> +	free(func->name);
>> +	if (func->sym_cnt)
>> +		free(func->syms);
>> +	memset(func, 0, sizeof(*func));
>> +}
>> +
>>   static inline void elf_functions__delete(struct elf_functions *funcs)
>>   {
>> -	for (int i = 0; i < funcs->cnt; i++)
>> -		free(funcs->entries[i].alias);
>> +	for (int i = 0; i < funcs->cnt; i++) {
>> +		elf_function__free_content(&funcs->entries[i]);
>> +	}
>>   	free(funcs->entries);
>>   	elf_symtab__delete(funcs->symtab);
>>   	list_del(&funcs->node);
>> @@ -981,8 +994,7 @@ static void btf_encoder__log_func_skip(struct btf_encoder *encoder, struct elf_f
>>   
>>   	if (!encoder->verbose)
>>   		return;
>> -	printf("%s (%s): skipping BTF encoding of function due to ",
>> -	       func->alias ?: func->name, func->name);
>> +	printf("%s : skipping BTF encoding of function due to ", func->name);
>>   	va_start(ap, fmt);
>>   	vprintf(fmt, ap);
>>   	va_end(ap);
>> @@ -1176,6 +1188,48 @@ static struct btf_encoder_func_state *btf_encoder__alloc_func_state(struct btf_e
>>   	return state;
>>   }
>>   
>> +/* some "." suffixes do not correspond to real functions;
>> + * - .part for partial inline
>> + * - .cold for rarely-used codepath extracted for better code locality
>> + */
>> +static bool str_contains_non_fn_suffix(const char *str) {
>> +	static const char *skip[] = {
>> +		".cold",
>> +		".part"
>> +	};
>> +	char *suffix = strchr(str, '.');
>> +	int i;
>> +
>> +	if (!suffix)
>> +		return false;
>> +	for (i = 0; i < ARRAY_SIZE(skip); i++) {
>> +		if (strstr(suffix, skip[i]))
>> +			return true;
>> +	}
>> +	return false;
>> +}
>> +
>> +static bool elf_function__has_ambiguous_address(struct elf_function *func) {
>> +	struct elf_function_sym *sym;
>> +	uint64_t addr;
>> +
>> +	if (func->sym_cnt <= 1)
>> +		return false;
>> +
>> +	addr = 0;
>> +	for (int i = 0; i < func->sym_cnt; i++) {
>> +		sym = &func->syms[i];
>> +		if (!str_contains_non_fn_suffix(sym->name)) {
>> +			if (addr && addr != sym->addr)
>> +				return true;
>> +			else
>> +			 	addr = sym->addr;
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>>   {
>>   	struct btf_encoder_func_state *state = btf_encoder__alloc_func_state(encoder);
>> @@ -1191,6 +1245,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
>>   
>>   	state->encoder = encoder;
>>   	state->elf = func;
>> +	state->ambiguous_addr = elf_function__has_ambiguous_address(func);
>>   	state->nr_parms = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
>>   	state->ret_type_id = ftype->tag.type == 0 ? 0 : encoder->type_id_off + ftype->tag.type;
>>   	if (state->nr_parms > 0) {
>> @@ -1294,7 +1349,7 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder,
>>   	int err;
>>   
>>   	btf_fnproto_id = btf_encoder__add_func_proto(encoder, NULL, state);
>> -	name = func->alias ?: func->name;
>> +	name = func->name;
>>   	if (btf_fnproto_id >= 0)
>>   		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id,
>>   						      name, false);
>> @@ -1338,48 +1393,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);
>>   	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;
>>   
>>   	return 0;
>>   }
>> @@ -1432,7 +1478,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);
>> @@ -1447,32 +1493,6 @@ out:
>>   	return err;
>>   }
>>   
>> -static void elf_functions__collect_function(struct elf_functions *functions, GElf_Sym *sym)
>> -{
>> -	struct elf_function *func;
>> -	const char *name;
>> -
>> -	if (elf_sym__type(sym) != STT_FUNC)
>> -		return;
>> -
>> -	name = elf_sym__name(sym, functions->symtab);
>> -	if (!name)
>> -		return;
>> -
>> -	func = &functions->entries[functions->cnt];
>> -	func->name = name;
>> -	if (strchr(name, '.')) {
>> -		const char *suffix = strchr(name, '.');
>> -
>> -		functions->suffix_cnt++;
>> -		func->prefixlen = suffix - name;
>> -	} else {
>> -		func->prefixlen = strlen(name);
>> -	}
>> -
>> -	functions->cnt++;
>> -}
>> -
>>   static struct elf_functions *btf_encoder__elf_functions(struct btf_encoder *encoder)
>>   {
>>   	struct elf_functions *funcs = NULL;
>> @@ -1490,13 +1510,12 @@ static struct elf_functions *btf_encoder__elf_functions(struct btf_encoder *enco
>>   	return funcs;
>>   }
>>   
>> -static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
>> -						       const char *name, size_t prefixlen)
>> +static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder, const char *name)
>>   {
>>   	struct elf_functions *funcs = elf_functions__find(encoder->cu->elf, &encoder->elf_functions_list);
>> -	struct elf_function key = { .name = name, .prefixlen = prefixlen };
>> +	struct elf_function key = { .name = (char*)name };
>>   
>> -	return bsearch(&key, funcs->entries, funcs->cnt, sizeof(key), functions_cmp);
>> +	return bsearch(&key, funcs->entries, funcs->cnt, sizeof(key), elf_function__name_cmp);
>>   }
>>   
>>   static bool btf_name_char_ok(char c, bool first)
>> @@ -2060,7 +2079,7 @@ static int btf_encoder__collect_kfuncs(struct btf_encoder *encoder)
>>   			continue;
>>   		}
>>   
>> -		elf_fn = btf_encoder__find_function(encoder, func, 0);
>> +		elf_fn = btf_encoder__find_function(encoder, func);
>>   		if (elf_fn) {
>>   			elf_fn->kfunc = true;
>>   			elf_fn->kfunc_flags = pair->flags;
>> @@ -2135,14 +2154,34 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
>>   	return err;
>>   }
>>   
>> +static inline int elf_function__push_sym(struct elf_function *func, struct elf_function_sym *sym) {
>> +	struct elf_function_sym *tmp;
>> +
>> +	if (func->sym_cnt)
>> +		tmp = realloc(func->syms, (func->sym_cnt + 1) * sizeof(func->syms[0]));
>> +	else
>> +		tmp = calloc(sizeof(func->syms[0]), 1);
>> +
>> +	if (!tmp)
>> +		return -ENOMEM;
>> +
>> +	func->syms = tmp;
>> +	func->syms[func->sym_cnt] = *sym;
>> +	func->sym_cnt++;
>> +
>> +	return 0;
>> +}
>> +
>>   static int elf_functions__collect(struct elf_functions *functions)
>>   {
>>   	uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab);
>> -	struct elf_function *tmp;
>> +	struct elf_function_sym func_sym;
>> +	struct elf_function *func, *tmp;
>> +	const char *sym_name, *suffix;
>>   	Elf32_Word sym_sec_idx;
>> +	int err = 0, i, j;
>>   	uint32_t core_id;
>>   	GElf_Sym sym;
>> -	int err = 0;
>>   
>>   	/* We know that number of functions is less than number of symbols,
>>   	 * so we can overallocate temporarily.
>> @@ -2153,18 +2192,72 @@ 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];
>> +
>> +		suffix = strchr(sym_name, '.');
>> +		if (suffix)
>> +			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++;
>>   	}
>>   
>> +	/* At this point functions->entries is an unordered array of elf_function
>> +	 * each having a name (without a suffix) and a single elf_function_sym (maybe with suffix)
>> +	 * Now let's sort this table by name.
>> +	 */
>>   	if (functions->cnt) {
>> -		qsort(functions->entries, functions->cnt, sizeof(*functions->entries), functions_cmp);
>> +		qsort(functions->entries, functions->cnt, sizeof(*functions->entries), elf_function__name_cmp);
>>   	} else {
>>   		err = 0;
>>   		goto out_free;
>>   	}
>>   
>> +	/* Finally dedup by name, transforming { name -> syms[1] } entries into { name -> syms[n] } */
>> +	i = 0;
>> +	j = 1;
>> +	for (j = 1; j < functions->cnt; j++) {
>> +		struct elf_function *a = &functions->entries[i];
>> +		struct elf_function *b = &functions->entries[j];
>> +
>> +		if (!strcmp(a->name, b->name)) {
>> +			elf_function__push_sym(a, &b->syms[0]);
>> +			elf_function__free_content(b);
>> +		} else {
>> +			i++;
>> +			if (i != j)
>> +				functions->entries[i] = functions->entries[j];
>> +		}
>> +	}
>> +
>> +	functions->cnt = i + 1;
>> +
>>   	/* Reallocate to the exact size */
>>   	tmp = realloc(functions->entries, functions->cnt * sizeof(struct elf_function));
>>   	if (tmp) {
>> @@ -2661,30 +2754,11 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>>   			if (!name)
>>   				continue;
>>   
>> -			/* prefer exact function name match... */
>> -			func = btf_encoder__find_function(encoder, name, 0);
>> -			if (!func && funcs->suffix_cnt &&
>> -			    conf_load->btf_gen_optimized) {
>> -				/* falling back to name.isra.0 match if no exact
>> -				 * match is found; only bother if we found any
>> -				 * .suffix function names.  The function
>> -				 * will be saved and added once we ensure
>> -				 * it does not have optimized-out parameters
>> -				 * in any cu.
>> -				 */
>> -				func = btf_encoder__find_function(encoder, name,
>> -								  strlen(name));
>> -				if (func) {
>> -					if (encoder->verbose)
>> -						printf("matched function '%s' with '%s'%s\n",
>> -						       name, func->name,
>> -						       fn->proto.optimized_parms ?
>> -						       ", has optimized-out parameters" :
>> -						       fn->proto.unexpected_reg ? ", has unexpected register use by params" :
>> -						       "");
>> -					if (!func->alias)
>> -						func->alias = strdup(name);
>> -				}
>> +			func = btf_encoder__find_function(encoder, name);
>> +			if (!func) {
>> +				if (encoder->verbose)
>> +					printf("could not find function '%s' in the ELF functions table\n", name);
>> +				continue;
>>   			}
>>   		} else {
>>   			if (!fn->external)
> 

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Alan Maguire]

On 31/07/2025 15:16, Alan Maguire wrote:
> On 29/07/2025 03:03, Ihor Solodrai wrote:
>> btf_encoder collects function ELF symbols into a table, which is later
>> used for processing DWARF data and determining whether a function can
>> be added to BTF.
>>
>> So far the ELF symbol name was used as a key for search in this table,
>> and a search by prefix match was attempted in cases when ELF symbol
>> name has a compiler-generated suffix.
>>
>> This implementation has bugs [1][2], causing some functions to be
>> inappropriately excluded from (or included into) BTF.
>>
>> Rework the implementation of the ELF functions table. Use a name of a
>> function without any suffix - symbol name before the first occurrence
>> of '.' - as a key. This way btf_encoder__find_function() always
>> returns a valid elf_function object (or NULL).
>>
>> Collect an array of symbol name + address pairs from GElf_Sym for each
>> elf_function when building the elf_functions table.
>>
>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
>> when the function is saved by examining the array of ELF symbols in
>> elf_function__has_ambiguous_address(). It tests whether there is only
>> one unique address for this function name, taking into account that
>> some addresses associated with it are not relevant:
>>   * ".cold" suffix indicates a piece of hot/cold split
>>   * ".part" suffix indicates a piece of partial inline
>>
>> When inspecting symbol name we have to search for any occurrence of
>> the target suffix, as opposed to testing the entire suffix, or the end
>> of a string. This is because suffixes may be combined by the compiler,
>> for example producing ".isra0.cold", and the conclusion will be
>> incorrect.
>>
>> In saved_functions_combine() check ambiguous_addr when deciding
>> whether a function should be included in BTF.
>>
>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>
>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>> CI-like kconfig) that are now excluded: all of those that I checked
>> had multiple addresses, and some where static functions from different
>> files with the same name.
>>
>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-b1cb-10266c72bd45@linux.dev/
>> [2] https://lore.kernel.org/dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>
>> v1: https://lore.kernel.org/dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
> 
> Thanks for doing this Ihor! Apologies for just thinking of this now, but
> why don't we filter out the .cold and .part functions earlier, not even
> adding them to the ELF functions list? Something like this on top of
> your patch:
> 
> $ git diff
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 0aa94ae..f61db0f 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
> *btf_encoder__alloc_func_state(struct btf_e
>         return state;
>  }
> 
> -/* some "." suffixes do not correspond to real functions;
> - * - .part for partial inline
> - * - .cold for rarely-used codepath extracted for better code locality
> - */
> -static bool str_contains_non_fn_suffix(const char *str) {
> -       static const char *skip[] = {
> -               ".cold",
> -               ".part"
> -       };
> -       char *suffix = strchr(str, '.');
> -       int i;
> -
> -       if (!suffix)
> -               return false;
> -       for (i = 0; i < ARRAY_SIZE(skip); i++) {
> -               if (strstr(suffix, skip[i]))
> -                       return true;
> -       }
> -       return false;
> -}
> -
>  static bool elf_function__has_ambiguous_address(struct elf_function
> *func) {
>         struct elf_function_sym *sym;
>         uint64_t addr;
> @@ -1219,12 +1198,10 @@ static bool
> elf_function__has_ambiguous_address(struct elf_function *func) {
>         addr = 0;
>         for (int i = 0; i < func->sym_cnt; i++) {
>                 sym = &func->syms[i];
> -               if (!str_contains_non_fn_suffix(sym->name)) {
> -                       if (addr && addr != sym->addr)
> -                               return true;
> -                       else
> +               if (addr && addr != sym->addr)
> +                       return true;
> +               else
>                                 addr = sym->addr;
> -               }
>         }
> 
> 
>         return false;
> @@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
> elf_functions *functions)
>                 func = &functions->entries[functions->cnt];
> 
>                 suffix = strchr(sym_name, '.');
> -               if (suffix)
> +               if (suffix) {
> +                       if (strstr(suffix, ".part") ||
> +                           strstr(suffix, ".cold"))
> +                               continue;
>                         func->name = strndup(sym_name, suffix - sym_name);
> -               else
> +               } else
>                         func->name = strdup(sym_name);
> 
>                 if (!func->name) {
> 
> I think that would work and saves later string comparisons, what do you
> think?
>

Apologies, this isn't sufficient. Considering cases like objpool_free(),
the problem is it has two entries in ELF for objpool_free and
objpool_free.part.0. So let's say we exclude objpool_free.part.0 from
the ELF representation, then we could potentially end up excluding
objpool_free as inconsistent if the DWARF for objpool_free.part.0
doesn't match that of objpool_free. It would appear to be inconsistent
but isn't really.

I think the best thing might be to retain the .part/.cold repesentations
in the ELF table but perhaps mark them with a flag (non_fn for
non-function or similar?) at creation time to avoid expensive string
comparisons later.

On the subject of improving matching, we do have address info for DWARF
functions in many cases like this, and that can help guide DWARF->ELF
mapping. I have a patch that helps collect function address info in
dwarf_loader.c; perhaps we could make use of it in doing more accurate
matching? In the above case for example, we actually have DWARF function
addresses for both objpool_free and objpool_free.part.0 so we could in
that case figure out the DWARF-ELF mapping even though there are
multiple ELF addresses and multiple DWARF representations.

Haven't thought it through fully to be honest, but I think we want to
avoid edge cases like the above where we either label a function as
inconsistent or ambiguous unnecessarily. I'll try to come up with a
rough proof-of-concept that weaves address-based matching into the
approach you have here, since what you've done is a huge improvement.
Again sorry for the noise here, I struggle to think through all the
permutations we have to consider here to be honest.

Thanks!

Alan



>> ---
>>  btf_encoder.c | 250 ++++++++++++++++++++++++++++++++------------------
>>  1 file changed, 162 insertions(+), 88 deletions(-)
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 0bc2334..0aa94ae 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -87,16 +87,22 @@ struct btf_encoder_func_state {
>>  	uint8_t optimized_parms:1;
>>  	uint8_t unexpected_reg:1;
>>  	uint8_t inconsistent_proto:1;
>> +	uint8_t ambiguous_addr:1;
>>  	int ret_type_id;
>>  	struct btf_encoder_func_parm *parms;
>>  	struct btf_encoder_func_annot *annots;
>>  };
>>  
>> +struct elf_function_sym {
>> +	const char *name;
>> +	uint64_t addr;
>> +};
>> +
>>  struct elf_function {
>> -	const char	*name;
>> -	char		*alias;
>> -	size_t		prefixlen;
>> -	bool		kfunc;
>> +	char		*name;
>> +	struct elf_function_sym *syms;
>> +	uint8_t 	sym_cnt;
>> +	uint8_t		kfunc:1;
>>  	uint32_t	kfunc_flags;
>>  };
>>  
>> @@ -115,7 +121,6 @@ struct elf_functions {
>>  	struct elf_symtab *symtab;
>>  	struct elf_function *entries;
>>  	int cnt;
>> -	int suffix_cnt; /* number of .isra, .part etc */
>>  };
>>  
>>  /*
>> @@ -161,10 +166,18 @@ struct btf_kfunc_set_range {
>>  	uint64_t end;
>>  };
>>  
>> +static inline void elf_function__free_content(struct elf_function *func) {
>> +	free(func->name);
>> +	if (func->sym_cnt)
>> +		free(func->syms);
>> +	memset(func, 0, sizeof(*func));
>> +}
>> +
>>  static inline void elf_functions__delete(struct elf_functions *funcs)
>>  {
>> -	for (int i = 0; i < funcs->cnt; i++)
>> -		free(funcs->entries[i].alias);
>> +	for (int i = 0; i < funcs->cnt; i++) {
>> +		elf_function__free_content(&funcs->entries[i]);
>> +	}
>>  	free(funcs->entries);
>>  	elf_symtab__delete(funcs->symtab);
>>  	list_del(&funcs->node);
>> @@ -981,8 +994,7 @@ static void btf_encoder__log_func_skip(struct btf_encoder *encoder, struct elf_f
>>  
>>  	if (!encoder->verbose)
>>  		return;
>> -	printf("%s (%s): skipping BTF encoding of function due to ",
>> -	       func->alias ?: func->name, func->name);
>> +	printf("%s : skipping BTF encoding of function due to ", func->name);
>>  	va_start(ap, fmt);
>>  	vprintf(fmt, ap);
>>  	va_end(ap);
>> @@ -1176,6 +1188,48 @@ static struct btf_encoder_func_state *btf_encoder__alloc_func_state(struct btf_e
>>  	return state;
>>  }
>>  
>> +/* some "." suffixes do not correspond to real functions;
>> + * - .part for partial inline
>> + * - .cold for rarely-used codepath extracted for better code locality
>> + */
>> +static bool str_contains_non_fn_suffix(const char *str) {
>> +	static const char *skip[] = {
>> +		".cold",
>> +		".part"
>> +	};
>> +	char *suffix = strchr(str, '.');
>> +	int i;
>> +
>> +	if (!suffix)
>> +		return false;
>> +	for (i = 0; i < ARRAY_SIZE(skip); i++) {
>> +		if (strstr(suffix, skip[i]))
>> +			return true;
>> +	}
>> +	return false;
>> +}
>> +
>> +static bool elf_function__has_ambiguous_address(struct elf_function *func) {
>> +	struct elf_function_sym *sym;
>> +	uint64_t addr;
>> +
>> +	if (func->sym_cnt <= 1)
>> +		return false;
>> +
>> +	addr = 0;
>> +	for (int i = 0; i < func->sym_cnt; i++) {
>> +		sym = &func->syms[i];
>> +		if (!str_contains_non_fn_suffix(sym->name)) {
>> +			if (addr && addr != sym->addr)
>> +				return true;
>> +			else
>> +			 	addr = sym->addr;
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>  static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct function *fn, struct elf_function *func)
>>  {
>>  	struct btf_encoder_func_state *state = btf_encoder__alloc_func_state(encoder);
>> @@ -1191,6 +1245,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
>>  
>>  	state->encoder = encoder;
>>  	state->elf = func;
>> +	state->ambiguous_addr = elf_function__has_ambiguous_address(func);
>>  	state->nr_parms = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0);
>>  	state->ret_type_id = ftype->tag.type == 0 ? 0 : encoder->type_id_off + ftype->tag.type;
>>  	if (state->nr_parms > 0) {
>> @@ -1294,7 +1349,7 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder,
>>  	int err;
>>  
>>  	btf_fnproto_id = btf_encoder__add_func_proto(encoder, NULL, state);
>> -	name = func->alias ?: func->name;
>> +	name = func->name;
>>  	if (btf_fnproto_id >= 0)
>>  		btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id,
>>  						      name, false);
>> @@ -1338,48 +1393,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);
>>  	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;
>>  
>>  	return 0;
>>  }
>> @@ -1432,7 +1478,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);
>> @@ -1447,32 +1493,6 @@ out:
>>  	return err;
>>  }
>>  
>> -static void elf_functions__collect_function(struct elf_functions *functions, GElf_Sym *sym)
>> -{
>> -	struct elf_function *func;
>> -	const char *name;
>> -
>> -	if (elf_sym__type(sym) != STT_FUNC)
>> -		return;
>> -
>> -	name = elf_sym__name(sym, functions->symtab);
>> -	if (!name)
>> -		return;
>> -
>> -	func = &functions->entries[functions->cnt];
>> -	func->name = name;
>> -	if (strchr(name, '.')) {
>> -		const char *suffix = strchr(name, '.');
>> -
>> -		functions->suffix_cnt++;
>> -		func->prefixlen = suffix - name;
>> -	} else {
>> -		func->prefixlen = strlen(name);
>> -	}
>> -
>> -	functions->cnt++;
>> -}
>> -
>>  static struct elf_functions *btf_encoder__elf_functions(struct btf_encoder *encoder)
>>  {
>>  	struct elf_functions *funcs = NULL;
>> @@ -1490,13 +1510,12 @@ static struct elf_functions *btf_encoder__elf_functions(struct btf_encoder *enco
>>  	return funcs;
>>  }
>>  
>> -static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder,
>> -						       const char *name, size_t prefixlen)
>> +static struct elf_function *btf_encoder__find_function(const struct btf_encoder *encoder, const char *name)
>>  {
>>  	struct elf_functions *funcs = elf_functions__find(encoder->cu->elf, &encoder->elf_functions_list);
>> -	struct elf_function key = { .name = name, .prefixlen = prefixlen };
>> +	struct elf_function key = { .name = (char*)name };
>>  
>> -	return bsearch(&key, funcs->entries, funcs->cnt, sizeof(key), functions_cmp);
>> +	return bsearch(&key, funcs->entries, funcs->cnt, sizeof(key), elf_function__name_cmp);
>>  }
>>  
>>  static bool btf_name_char_ok(char c, bool first)
>> @@ -2060,7 +2079,7 @@ static int btf_encoder__collect_kfuncs(struct btf_encoder *encoder)
>>  			continue;
>>  		}
>>  
>> -		elf_fn = btf_encoder__find_function(encoder, func, 0);
>> +		elf_fn = btf_encoder__find_function(encoder, func);
>>  		if (elf_fn) {
>>  			elf_fn->kfunc = true;
>>  			elf_fn->kfunc_flags = pair->flags;
>> @@ -2135,14 +2154,34 @@ int btf_encoder__encode(struct btf_encoder *encoder, struct conf_load *conf)
>>  	return err;
>>  }
>>  
>> +static inline int elf_function__push_sym(struct elf_function *func, struct elf_function_sym *sym) {
>> +	struct elf_function_sym *tmp;
>> +
>> +	if (func->sym_cnt)
>> +		tmp = realloc(func->syms, (func->sym_cnt + 1) * sizeof(func->syms[0]));
>> +	else
>> +		tmp = calloc(sizeof(func->syms[0]), 1);
>> +
>> +	if (!tmp)
>> +		return -ENOMEM;
>> +
>> +	func->syms = tmp;
>> +	func->syms[func->sym_cnt] = *sym;
>> +	func->sym_cnt++;
>> +
>> +	return 0;
>> +}
>> +
>>  static int elf_functions__collect(struct elf_functions *functions)
>>  {
>>  	uint32_t nr_symbols = elf_symtab__nr_symbols(functions->symtab);
>> -	struct elf_function *tmp;
>> +	struct elf_function_sym func_sym;
>> +	struct elf_function *func, *tmp;
>> +	const char *sym_name, *suffix;
>>  	Elf32_Word sym_sec_idx;
>> +	int err = 0, i, j;
>>  	uint32_t core_id;
>>  	GElf_Sym sym;
>> -	int err = 0;
>>  
>>  	/* We know that number of functions is less than number of symbols,
>>  	 * so we can overallocate temporarily.
>> @@ -2153,18 +2192,72 @@ 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];
>> +
>> +		suffix = strchr(sym_name, '.');
>> +		if (suffix)
>> +			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++;
>>  	}
>>  
>> +	/* At this point functions->entries is an unordered array of elf_function
>> +	 * each having a name (without a suffix) and a single elf_function_sym (maybe with suffix)
>> +	 * Now let's sort this table by name.
>> +	 */
>>  	if (functions->cnt) {
>> -		qsort(functions->entries, functions->cnt, sizeof(*functions->entries), functions_cmp);
>> +		qsort(functions->entries, functions->cnt, sizeof(*functions->entries), elf_function__name_cmp);
>>  	} else {
>>  		err = 0;
>>  		goto out_free;
>>  	}
>>  
>> +	/* Finally dedup by name, transforming { name -> syms[1] } entries into { name -> syms[n] } */
>> +	i = 0;
>> +	j = 1;
>> +	for (j = 1; j < functions->cnt; j++) {
>> +		struct elf_function *a = &functions->entries[i];
>> +		struct elf_function *b = &functions->entries[j];
>> +
>> +		if (!strcmp(a->name, b->name)) {
>> +			elf_function__push_sym(a, &b->syms[0]);
>> +			elf_function__free_content(b);
>> +		} else {
>> +			i++;
>> +			if (i != j)
>> +				functions->entries[i] = functions->entries[j];
>> +		}
>> +	}
>> +
>> +	functions->cnt = i + 1;
>> +
>>  	/* Reallocate to the exact size */
>>  	tmp = realloc(functions->entries, functions->cnt * sizeof(struct elf_function));
>>  	if (tmp) {
>> @@ -2661,30 +2754,11 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co
>>  			if (!name)
>>  				continue;
>>  
>> -			/* prefer exact function name match... */
>> -			func = btf_encoder__find_function(encoder, name, 0);
>> -			if (!func && funcs->suffix_cnt &&
>> -			    conf_load->btf_gen_optimized) {
>> -				/* falling back to name.isra.0 match if no exact
>> -				 * match is found; only bother if we found any
>> -				 * .suffix function names.  The function
>> -				 * will be saved and added once we ensure
>> -				 * it does not have optimized-out parameters
>> -				 * in any cu.
>> -				 */
>> -				func = btf_encoder__find_function(encoder, name,
>> -								  strlen(name));
>> -				if (func) {
>> -					if (encoder->verbose)
>> -						printf("matched function '%s' with '%s'%s\n",
>> -						       name, func->name,
>> -						       fn->proto.optimized_parms ?
>> -						       ", has optimized-out parameters" :
>> -						       fn->proto.unexpected_reg ? ", has unexpected register use by params" :
>> -						       "");
>> -					if (!func->alias)
>> -						func->alias = strdup(name);
>> -				}
>> +			func = btf_encoder__find_function(encoder, name);
>> +			if (!func) {
>> +				if (encoder->verbose)
>> +					printf("could not find function '%s' in the ELF functions table\n", name);
>> +				continue;
>>  			}
>>  		} else {
>>  			if (!fn->external)
> 
> 

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Ihor Solodrai]

On 7/31/25 11:57 AM, Alan Maguire wrote:
> On 31/07/2025 15:16, Alan Maguire wrote:
>> On 29/07/2025 03:03, Ihor Solodrai wrote:
>>> btf_encoder collects function ELF symbols into a table, which is later
>>> used for processing DWARF data and determining whether a function can
>>> be added to BTF.
>>>
>>> So far the ELF symbol name was used as a key for search in this table,
>>> and a search by prefix match was attempted in cases when ELF symbol
>>> name has a compiler-generated suffix.
>>>
>>> This implementation has bugs [1][2], causing some functions to be
>>> inappropriately excluded from (or included into) BTF.
>>>
>>> Rework the implementation of the ELF functions table. Use a name of a
>>> function without any suffix - symbol name before the first occurrence
>>> of '.' - as a key. This way btf_encoder__find_function() always
>>> returns a valid elf_function object (or NULL).
>>>
>>> Collect an array of symbol name + address pairs from GElf_Sym for each
>>> elf_function when building the elf_functions table.
>>>
>>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
>>> when the function is saved by examining the array of ELF symbols in
>>> elf_function__has_ambiguous_address(). It tests whether there is only
>>> one unique address for this function name, taking into account that
>>> some addresses associated with it are not relevant:
>>>    * ".cold" suffix indicates a piece of hot/cold split
>>>    * ".part" suffix indicates a piece of partial inline
>>>
>>> When inspecting symbol name we have to search for any occurrence of
>>> the target suffix, as opposed to testing the entire suffix, or the end
>>> of a string. This is because suffixes may be combined by the compiler,
>>> for example producing ".isra0.cold", and the conclusion will be
>>> incorrect.
>>>
>>> In saved_functions_combine() check ambiguous_addr when deciding
>>> whether a function should be included in BTF.
>>>
>>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>>
>>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>>> CI-like kconfig) that are now excluded: all of those that I checked
>>> had multiple addresses, and some where static functions from different
>>> files with the same name.
>>>
>>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-b1cb-10266c72bd45@linux.dev/
>>> [2] https://lore.kernel.org/dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>>
>>> v1: https://lore.kernel.org/dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>
>> Thanks for doing this Ihor! Apologies for just thinking of this now, but
>> why don't we filter out the .cold and .part functions earlier, not even
>> adding them to the ELF functions list? Something like this on top of
>> your patch:
>>
>> $ git diff
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 0aa94ae..f61db0f 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
>> *btf_encoder__alloc_func_state(struct btf_e
>>          return state;
>>   }
>>
>> -/* some "." suffixes do not correspond to real functions;
>> - * - .part for partial inline
>> - * - .cold for rarely-used codepath extracted for better code locality
>> - */
>> -static bool str_contains_non_fn_suffix(const char *str) {
>> -       static const char *skip[] = {
>> -               ".cold",
>> -               ".part"
>> -       };
>> -       char *suffix = strchr(str, '.');
>> -       int i;
>> -
>> -       if (!suffix)
>> -               return false;
>> -       for (i = 0; i < ARRAY_SIZE(skip); i++) {
>> -               if (strstr(suffix, skip[i]))
>> -                       return true;
>> -       }
>> -       return false;
>> -}
>> -
>>   static bool elf_function__has_ambiguous_address(struct elf_function
>> *func) {
>>          struct elf_function_sym *sym;
>>          uint64_t addr;
>> @@ -1219,12 +1198,10 @@ static bool
>> elf_function__has_ambiguous_address(struct elf_function *func) {
>>          addr = 0;
>>          for (int i = 0; i < func->sym_cnt; i++) {
>>                  sym = &func->syms[i];
>> -               if (!str_contains_non_fn_suffix(sym->name)) {
>> -                       if (addr && addr != sym->addr)
>> -                               return true;
>> -                       else
>> +               if (addr && addr != sym->addr)
>> +                       return true;
>> +               else
>>                                  addr = sym->addr;
>> -               }
>>          }
>>
>>
>>          return false;
>> @@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
>> elf_functions *functions)
>>                  func = &functions->entries[functions->cnt];
>>
>>                  suffix = strchr(sym_name, '.');
>> -               if (suffix)
>> +               if (suffix) {
>> +                       if (strstr(suffix, ".part") ||
>> +                           strstr(suffix, ".cold"))
>> +                               continue;
>>                          func->name = strndup(sym_name, suffix - sym_name);
>> -               else
>> +               } else
>>                          func->name = strdup(sym_name);
>>
>>                  if (!func->name) {
>>
>> I think that would work and saves later string comparisons, what do you
>> think?
>>
> 
> Apologies, this isn't sufficient. Considering cases like objpool_free(),
> the problem is it has two entries in ELF for objpool_free and
> objpool_free.part.0. So let's say we exclude objpool_free.part.0 from
> the ELF representation, then we could potentially end up excluding
> objpool_free as inconsistent if the DWARF for objpool_free.part.0
> doesn't match that of objpool_free. It would appear to be inconsistent
> but isn't really.

Alan, as far as I can tell, in your example the function would be
considered inconsistent independent of whether .part is included in
elf_function->syms or not. We determine argument inconsistency based
on DWARF data (struct function) passed into btf_encoder__save_func().

So if there is a difference in arguments between objpool_free.part.0
and objpool_free, it will be detected anyways.

A significant difference between v2 and v3 (just sent [1]) is in that
if there is *only* "foo.part.0" symbol but no "foo", then it will not
be included in v3 (because it's not in the elf_functions table), but
would be in v2 (because there is only one address). And the correct
behavior from the BTF encoding point of view is v3.

[1] 
https://lore.kernel.org/dwarves/20250801202009.3942492-1-ihor.solodrai@linux.dev/


> 
> I think the best thing might be to retain the .part/.cold repesentations
> in the ELF table but perhaps mark them with a flag (non_fn for
> non-function or similar?) at creation time to avoid expensive string
> comparisons later.

That's a good point. In v3 I exclude .part and .cold from the table,
and store ambiguous_addr flag in elf_function. If anything, we should
be doing less string comparisons now.

> 
> On the subject of improving matching, we do have address info for DWARF
> functions in many cases like this, and that can help guide DWARF->ELF
> mapping. I have a patch that helps collect function address info in
> dwarf_loader.c; perhaps we could make use of it in doing more accurate
> matching? In the above case for example, we actually have DWARF function
> addresses for both objpool_free and objpool_free.part.0 so we could in
> that case figure out the DWARF-ELF mapping even though there are
> multiple ELF addresses and multiple DWARF representations.
> 
> Haven't thought it through fully to be honest, but I think we want to
> avoid edge cases like the above where we either label a function as
> inconsistent or ambiguous unnecessarily. I'll try to come up with a
> rough proof-of-concept that weaves address-based matching into the
> approach you have here, since what you've done is a huge improvement.
> Again sorry for the noise here, I struggle to think through all the
> permutations we have to consider here to be honest.
> 
> Thanks!
> 
> Alan
> 
> 
> [...]
> 

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Alan Maguire]

On 01/08/2025 21:51, Ihor Solodrai wrote:
> On 7/31/25 11:57 AM, Alan Maguire wrote:
>> On 31/07/2025 15:16, Alan Maguire wrote:
>>> On 29/07/2025 03:03, Ihor Solodrai wrote:
>>>> btf_encoder collects function ELF symbols into a table, which is later
>>>> used for processing DWARF data and determining whether a function can
>>>> be added to BTF.
>>>>
>>>> So far the ELF symbol name was used as a key for search in this table,
>>>> and a search by prefix match was attempted in cases when ELF symbol
>>>> name has a compiler-generated suffix.
>>>>
>>>> This implementation has bugs [1][2], causing some functions to be
>>>> inappropriately excluded from (or included into) BTF.
>>>>
>>>> Rework the implementation of the ELF functions table. Use a name of a
>>>> function without any suffix - symbol name before the first occurrence
>>>> of '.' - as a key. This way btf_encoder__find_function() always
>>>> returns a valid elf_function object (or NULL).
>>>>
>>>> Collect an array of symbol name + address pairs from GElf_Sym for each
>>>> elf_function when building the elf_functions table.
>>>>
>>>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
>>>> when the function is saved by examining the array of ELF symbols in
>>>> elf_function__has_ambiguous_address(). It tests whether there is only
>>>> one unique address for this function name, taking into account that
>>>> some addresses associated with it are not relevant:
>>>>    * ".cold" suffix indicates a piece of hot/cold split
>>>>    * ".part" suffix indicates a piece of partial inline
>>>>
>>>> When inspecting symbol name we have to search for any occurrence of
>>>> the target suffix, as opposed to testing the entire suffix, or the end
>>>> of a string. This is because suffixes may be combined by the compiler,
>>>> for example producing ".isra0.cold", and the conclusion will be
>>>> incorrect.
>>>>
>>>> In saved_functions_combine() check ambiguous_addr when deciding
>>>> whether a function should be included in BTF.
>>>>
>>>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>>>
>>>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>>>> CI-like kconfig) that are now excluded: all of those that I checked
>>>> had multiple addresses, and some where static functions from different
>>>> files with the same name.
>>>>
>>>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-
>>>> b1cb-10266c72bd45@linux.dev/
>>>> [2] https://lore.kernel.org/
>>>> dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>>>
>>>> v1: https://lore.kernel.org/
>>>> dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>>
>>> Thanks for doing this Ihor! Apologies for just thinking of this now, but
>>> why don't we filter out the .cold and .part functions earlier, not even
>>> adding them to the ELF functions list? Something like this on top of
>>> your patch:
>>>
>>> $ git diff
>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>> index 0aa94ae..f61db0f 100644
>>> --- a/btf_encoder.c
>>> +++ b/btf_encoder.c
>>> @@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
>>> *btf_encoder__alloc_func_state(struct btf_e
>>>          return state;
>>>   }
>>>
>>> -/* some "." suffixes do not correspond to real functions;
>>> - * - .part for partial inline
>>> - * - .cold for rarely-used codepath extracted for better code locality
>>> - */
>>> -static bool str_contains_non_fn_suffix(const char *str) {
>>> -       static const char *skip[] = {
>>> -               ".cold",
>>> -               ".part"
>>> -       };
>>> -       char *suffix = strchr(str, '.');
>>> -       int i;
>>> -
>>> -       if (!suffix)
>>> -               return false;
>>> -       for (i = 0; i < ARRAY_SIZE(skip); i++) {
>>> -               if (strstr(suffix, skip[i]))
>>> -                       return true;
>>> -       }
>>> -       return false;
>>> -}
>>> -
>>>   static bool elf_function__has_ambiguous_address(struct elf_function
>>> *func) {
>>>          struct elf_function_sym *sym;
>>>          uint64_t addr;
>>> @@ -1219,12 +1198,10 @@ static bool
>>> elf_function__has_ambiguous_address(struct elf_function *func) {
>>>          addr = 0;
>>>          for (int i = 0; i < func->sym_cnt; i++) {
>>>                  sym = &func->syms[i];
>>> -               if (!str_contains_non_fn_suffix(sym->name)) {
>>> -                       if (addr && addr != sym->addr)
>>> -                               return true;
>>> -                       else
>>> +               if (addr && addr != sym->addr)
>>> +                       return true;
>>> +               else
>>>                                  addr = sym->addr;
>>> -               }
>>>          }
>>>
>>>
>>>          return false;
>>> @@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
>>> elf_functions *functions)
>>>                  func = &functions->entries[functions->cnt];
>>>
>>>                  suffix = strchr(sym_name, '.');
>>> -               if (suffix)
>>> +               if (suffix) {
>>> +                       if (strstr(suffix, ".part") ||
>>> +                           strstr(suffix, ".cold"))
>>> +                               continue;
>>>                          func->name = strndup(sym_name, suffix -
>>> sym_name);
>>> -               else
>>> +               } else
>>>                          func->name = strdup(sym_name);
>>>
>>>                  if (!func->name) {
>>>
>>> I think that would work and saves later string comparisons, what do you
>>> think?
>>>
>>
>> Apologies, this isn't sufficient. Considering cases like objpool_free(),
>> the problem is it has two entries in ELF for objpool_free and
>> objpool_free.part.0. So let's say we exclude objpool_free.part.0 from
>> the ELF representation, then we could potentially end up excluding
>> objpool_free as inconsistent if the DWARF for objpool_free.part.0
>> doesn't match that of objpool_free. It would appear to be inconsistent
>> but isn't really.
> 
> Alan, as far as I can tell, in your example the function would be
> considered inconsistent independent of whether .part is included in
> elf_function->syms or not. We determine argument inconsistency based
> on DWARF data (struct function) passed into btf_encoder__save_func().
> 
> So if there is a difference in arguments between objpool_free.part.0
> and objpool_free, it will be detected anyways.
> 

I think I've solved that in the following proof-of-concept series [1] -
by retaining the .part functions in the ELF list _and_ matching the
DWARF and ELF via address we can distinguish between foo and foo.part.0
debug information and discard the latter such that it is not included in
the determination of inconsistency.

> A significant difference between v2 and v3 (just sent [1]) is in that
> if there is *only* "foo.part.0" symbol but no "foo", then it will not
> be included in v3 (because it's not in the elf_functions table), but
> would be in v2 (because there is only one address). And the correct
> behavior from the BTF encoding point of view is v3.
>

Yep, that part sounds good; I _think_ the approach I'm proposing solves
that too, along with not incorrectly marking foo/foo.part.0 as inconsistent.

The series is the top 3 commits in [1]; the first commit [2] is yours
modulo the small tweak of marking non-functions during ELF function
creation. The second [3] uses address ranges to try harder to get
address info from DWARF, while the final one [4] skips creating function
state for functions that we address-match as the .part/.cold functions
in debug info. This all allows us to better identify debug information
that is tied to the non-function .part/.cold optimizations.


[1]
https://github.com/acmel/dwarves/compare/master...alan-maguire:dwarves:pahole-next-remove-dupaddrs
[2]
https://github.com/acmel/dwarves/commit/e256ffaf13cce96c4e782192b2814e1a2664fe99
[3]
https://github.com/acmel/dwarves/commit/7cc2c1e21f1daeb29aa270fd9f23ef1ba99fcd4e
[4]
https://github.com/acmel/dwarves/commit/893f14c2a854c240a927294996f449a3ad63eaed
> [1] https://lore.kernel.org/dwarves/20250801202009.3942492-1-
> ihor.solodrai@linux.dev/
> 
> 
>>
>> I think the best thing might be to retain the .part/.cold repesentations
>> in the ELF table but perhaps mark them with a flag (non_fn for
>> non-function or similar?) at creation time to avoid expensive string
>> comparisons later.
> 
> That's a good point. In v3 I exclude .part and .cold from the table,
> and store ambiguous_addr flag in elf_function. If anything, we should
> be doing less string comparisons now.
> 
>>
>> On the subject of improving matching, we do have address info for DWARF
>> functions in many cases like this, and that can help guide DWARF->ELF
>> mapping. I have a patch that helps collect function address info in
>> dwarf_loader.c; perhaps we could make use of it in doing more accurate
>> matching? In the above case for example, we actually have DWARF function
>> addresses for both objpool_free and objpool_free.part.0 so we could in
>> that case figure out the DWARF-ELF mapping even though there are
>> multiple ELF addresses and multiple DWARF representations.
>>
>> Haven't thought it through fully to be honest, but I think we want to
>> avoid edge cases like the above where we either label a function as
>> inconsistent or ambiguous unnecessarily. I'll try to come up with a
>> rough proof-of-concept that weaves address-based matching into the
>> approach you have here, since what you've done is a huge improvement.
>> Again sorry for the noise here, I struggle to think through all the
>> permutations we have to consider here to be honest.
>>
>> Thanks!
>>
>> Alan
>>
>>
>> [...]
>>
> 
> 

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Ihor Solodrai]

On 8/5/25 4:24 AM, Alan Maguire wrote:
> On 01/08/2025 21:51, Ihor Solodrai wrote:
>> On 7/31/25 11:57 AM, Alan Maguire wrote:
>>> On 31/07/2025 15:16, Alan Maguire wrote:
>>>> On 29/07/2025 03:03, Ihor Solodrai wrote:
>>>>> btf_encoder collects function ELF symbols into a table, which is later
>>>>> used for processing DWARF data and determining whether a function can
>>>>> be added to BTF.
>>>>>
>>>>> So far the ELF symbol name was used as a key for search in this table,
>>>>> and a search by prefix match was attempted in cases when ELF symbol
>>>>> name has a compiler-generated suffix.
>>>>>
>>>>> This implementation has bugs [1][2], causing some functions to be
>>>>> inappropriately excluded from (or included into) BTF.
>>>>>
>>>>> Rework the implementation of the ELF functions table. Use a name of a
>>>>> function without any suffix - symbol name before the first occurrence
>>>>> of '.' - as a key. This way btf_encoder__find_function() always
>>>>> returns a valid elf_function object (or NULL).
>>>>>
>>>>> Collect an array of symbol name + address pairs from GElf_Sym for each
>>>>> elf_function when building the elf_functions table.
>>>>>
>>>>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is set
>>>>> when the function is saved by examining the array of ELF symbols in
>>>>> elf_function__has_ambiguous_address(). It tests whether there is only
>>>>> one unique address for this function name, taking into account that
>>>>> some addresses associated with it are not relevant:
>>>>>     * ".cold" suffix indicates a piece of hot/cold split
>>>>>     * ".part" suffix indicates a piece of partial inline
>>>>>
>>>>> When inspecting symbol name we have to search for any occurrence of
>>>>> the target suffix, as opposed to testing the entire suffix, or the end
>>>>> of a string. This is because suffixes may be combined by the compiler,
>>>>> for example producing ".isra0.cold", and the conclusion will be
>>>>> incorrect.
>>>>>
>>>>> In saved_functions_combine() check ambiguous_addr when deciding
>>>>> whether a function should be included in BTF.
>>>>>
>>>>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>>>>
>>>>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>>>>> CI-like kconfig) that are now excluded: all of those that I checked
>>>>> had multiple addresses, and some where static functions from different
>>>>> files with the same name.
>>>>>
>>>>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-
>>>>> b1cb-10266c72bd45@linux.dev/
>>>>> [2] https://lore.kernel.org/
>>>>> dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>>>>
>>>>> v1: https://lore.kernel.org/
>>>>> dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>>>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>>>
>>>> Thanks for doing this Ihor! Apologies for just thinking of this now, but
>>>> why don't we filter out the .cold and .part functions earlier, not even
>>>> adding them to the ELF functions list? Something like this on top of
>>>> your patch:
>>>>
>>>> $ git diff
>>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>>> index 0aa94ae..f61db0f 100644
>>>> --- a/btf_encoder.c
>>>> +++ b/btf_encoder.c
>>>> @@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
>>>> *btf_encoder__alloc_func_state(struct btf_e
>>>>           return state;
>>>>    }
>>>>
>>>> -/* some "." suffixes do not correspond to real functions;
>>>> - * - .part for partial inline
>>>> - * - .cold for rarely-used codepath extracted for better code locality
>>>> - */
>>>> -static bool str_contains_non_fn_suffix(const char *str) {
>>>> -       static const char *skip[] = {
>>>> -               ".cold",
>>>> -               ".part"
>>>> -       };
>>>> -       char *suffix = strchr(str, '.');
>>>> -       int i;
>>>> -
>>>> -       if (!suffix)
>>>> -               return false;
>>>> -       for (i = 0; i < ARRAY_SIZE(skip); i++) {
>>>> -               if (strstr(suffix, skip[i]))
>>>> -                       return true;
>>>> -       }
>>>> -       return false;
>>>> -}
>>>> -
>>>>    static bool elf_function__has_ambiguous_address(struct elf_function
>>>> *func) {
>>>>           struct elf_function_sym *sym;
>>>>           uint64_t addr;
>>>> @@ -1219,12 +1198,10 @@ static bool
>>>> elf_function__has_ambiguous_address(struct elf_function *func) {
>>>>           addr = 0;
>>>>           for (int i = 0; i < func->sym_cnt; i++) {
>>>>                   sym = &func->syms[i];
>>>> -               if (!str_contains_non_fn_suffix(sym->name)) {
>>>> -                       if (addr && addr != sym->addr)
>>>> -                               return true;
>>>> -                       else
>>>> +               if (addr && addr != sym->addr)
>>>> +                       return true;
>>>> +               else
>>>>                                   addr = sym->addr;
>>>> -               }
>>>>           }
>>>>
>>>>
>>>>           return false;
>>>> @@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
>>>> elf_functions *functions)
>>>>                   func = &functions->entries[functions->cnt];
>>>>
>>>>                   suffix = strchr(sym_name, '.');
>>>> -               if (suffix)
>>>> +               if (suffix) {
>>>> +                       if (strstr(suffix, ".part") ||
>>>> +                           strstr(suffix, ".cold"))
>>>> +                               continue;
>>>>                           func->name = strndup(sym_name, suffix -
>>>> sym_name);
>>>> -               else
>>>> +               } else
>>>>                           func->name = strdup(sym_name);
>>>>
>>>>                   if (!func->name) {
>>>>
>>>> I think that would work and saves later string comparisons, what do you
>>>> think?
>>>>
>>>
>>> Apologies, this isn't sufficient. Considering cases like objpool_free(),
>>> the problem is it has two entries in ELF for objpool_free and
>>> objpool_free.part.0. So let's say we exclude objpool_free.part.0 from
>>> the ELF representation, then we could potentially end up excluding
>>> objpool_free as inconsistent if the DWARF for objpool_free.part.0
>>> doesn't match that of objpool_free. It would appear to be inconsistent
>>> but isn't really.
>>
>> Alan, as far as I can tell, in your example the function would be
>> considered inconsistent independent of whether .part is included in
>> elf_function->syms or not. We determine argument inconsistency based
>> on DWARF data (struct function) passed into btf_encoder__save_func().
>>
>> So if there is a difference in arguments between objpool_free.part.0
>> and objpool_free, it will be detected anyways.
>>
> 
> I think I've solved that in the following proof-of-concept series [1] -
> by retaining the .part functions in the ELF list _and_ matching the
> DWARF and ELF via address we can distinguish between foo and foo.part.0
> debug information and discard the latter such that it is not included in
> the determination of inconsistency.
> 
>> A significant difference between v2 and v3 (just sent [1]) is in that
>> if there is *only* "foo.part.0" symbol but no "foo", then it will not
>> be included in v3 (because it's not in the elf_functions table), but
>> would be in v2 (because there is only one address). And the correct
>> behavior from the BTF encoding point of view is v3.
>>
> 
> Yep, that part sounds good; I _think_ the approach I'm proposing solves
> that too, along with not incorrectly marking foo/foo.part.0 as inconsistent.
> 
> The series is the top 3 commits in [1]; the first commit [2] is yours
> modulo the small tweak of marking non-functions during ELF function
> creation. The second [3] uses address ranges to try harder to get
> address info from DWARF, while the final one [4] skips creating function
> state for functions that we address-match as the .part/.cold functions
> in debug info. This all allows us to better identify debug information
> that is tied to the non-function .part/.cold optimizations.

Hi Alan. Bumping this thread.

I haven't reviewed/tested your github changes thoroughly, but the
approach makes sense to me overall.

What do you think about applying the group-by-name patch [1] first,
maybe including your tweak? It would fix a couple of bugs right away.

And later you can send a more refined draft of patches to use
addresses for matching.

[1] 
https://lore.kernel.org/dwarves/20250801202009.3942492-1-ihor.solodrai@linux.dev/

> 
> 
> [1]
> https://github.com/acmel/dwarves/compare/master...alan-maguire:dwarves:pahole-next-remove-dupaddrs
> [2]
> https://github.com/acmel/dwarves/commit/e256ffaf13cce96c4e782192b2814e1a2664fe99
> [3]
> https://github.com/acmel/dwarves/commit/7cc2c1e21f1daeb29aa270fd9f23ef1ba99fcd4e
> [4]
> https://github.com/acmel/dwarves/commit/893f14c2a854c240a927294996f449a3ad63eaed
>> [1] https://lore.kernel.org/dwarves/20250801202009.3942492-1-
>> ihor.solodrai@linux.dev/
>>
>>
>>>
>>> I think the best thing might be to retain the .part/.cold repesentations
>>> in the ELF table but perhaps mark them with a flag (non_fn for
>>> non-function or similar?) at creation time to avoid expensive string
>>> comparisons later.
>>
>> That's a good point. In v3 I exclude .part and .cold from the table,
>> and store ambiguous_addr flag in elf_function. If anything, we should
>> be doing less string comparisons now.
>>
>>>
>>> On the subject of improving matching, we do have address info for DWARF
>>> functions in many cases like this, and that can help guide DWARF->ELF
>>> mapping. I have a patch that helps collect function address info in
>>> dwarf_loader.c; perhaps we could make use of it in doing more accurate
>>> matching? In the above case for example, we actually have DWARF function
>>> addresses for both objpool_free and objpool_free.part.0 so we could in
>>> that case figure out the DWARF-ELF mapping even though there are
>>> multiple ELF addresses and multiple DWARF representations.
>>>
>>> Haven't thought it through fully to be honest, but I think we want to
>>> avoid edge cases like the above where we either label a function as
>>> inconsistent or ambiguous unnecessarily. I'll try to come up with a
>>> rough proof-of-concept that weaves address-based matching into the
>>> approach you have here, since what you've done is a huge improvement.
>>> Again sorry for the noise here, I struggle to think through all the
>>> permutations we have to consider here to be honest.
>>>
>>> Thanks!
>>>
>>> Alan
>>>
>>>
>>> [...]
>>>
>>
>>
> 

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Alan Maguire]

On 19/08/2025 20:34, Ihor Solodrai wrote:
> On 8/5/25 4:24 AM, Alan Maguire wrote:
>> On 01/08/2025 21:51, Ihor Solodrai wrote:
>>> On 7/31/25 11:57 AM, Alan Maguire wrote:
>>>> On 31/07/2025 15:16, Alan Maguire wrote:
>>>>> On 29/07/2025 03:03, Ihor Solodrai wrote:
>>>>>> btf_encoder collects function ELF symbols into a table, which is
>>>>>> later
>>>>>> used for processing DWARF data and determining whether a function can
>>>>>> be added to BTF.
>>>>>>
>>>>>> So far the ELF symbol name was used as a key for search in this
>>>>>> table,
>>>>>> and a search by prefix match was attempted in cases when ELF symbol
>>>>>> name has a compiler-generated suffix.
>>>>>>
>>>>>> This implementation has bugs [1][2], causing some functions to be
>>>>>> inappropriately excluded from (or included into) BTF.
>>>>>>
>>>>>> Rework the implementation of the ELF functions table. Use a name of a
>>>>>> function without any suffix - symbol name before the first occurrence
>>>>>> of '.' - as a key. This way btf_encoder__find_function() always
>>>>>> returns a valid elf_function object (or NULL).
>>>>>>
>>>>>> Collect an array of symbol name + address pairs from GElf_Sym for
>>>>>> each
>>>>>> elf_function when building the elf_functions table.
>>>>>>
>>>>>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is
>>>>>> set
>>>>>> when the function is saved by examining the array of ELF symbols in
>>>>>> elf_function__has_ambiguous_address(). It tests whether there is only
>>>>>> one unique address for this function name, taking into account that
>>>>>> some addresses associated with it are not relevant:
>>>>>>     * ".cold" suffix indicates a piece of hot/cold split
>>>>>>     * ".part" suffix indicates a piece of partial inline
>>>>>>
>>>>>> When inspecting symbol name we have to search for any occurrence of
>>>>>> the target suffix, as opposed to testing the entire suffix, or the
>>>>>> end
>>>>>> of a string. This is because suffixes may be combined by the
>>>>>> compiler,
>>>>>> for example producing ".isra0.cold", and the conclusion will be
>>>>>> incorrect.
>>>>>>
>>>>>> In saved_functions_combine() check ambiguous_addr when deciding
>>>>>> whether a function should be included in BTF.
>>>>>>
>>>>>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>>>>>
>>>>>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>>>>>> CI-like kconfig) that are now excluded: all of those that I checked
>>>>>> had multiple addresses, and some where static functions from
>>>>>> different
>>>>>> files with the same name.
>>>>>>
>>>>>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-
>>>>>> b1cb-10266c72bd45@linux.dev/
>>>>>> [2] https://lore.kernel.org/
>>>>>> dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>>>>>
>>>>>> v1: https://lore.kernel.org/
>>>>>> dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>>>>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>>>>
>>>>> Thanks for doing this Ihor! Apologies for just thinking of this
>>>>> now, but
>>>>> why don't we filter out the .cold and .part functions earlier, not
>>>>> even
>>>>> adding them to the ELF functions list? Something like this on top of
>>>>> your patch:
>>>>>
>>>>> $ git diff
>>>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>>>> index 0aa94ae..f61db0f 100644
>>>>> --- a/btf_encoder.c
>>>>> +++ b/btf_encoder.c
>>>>> @@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
>>>>> *btf_encoder__alloc_func_state(struct btf_e
>>>>>           return state;
>>>>>    }
>>>>>
>>>>> -/* some "." suffixes do not correspond to real functions;
>>>>> - * - .part for partial inline
>>>>> - * - .cold for rarely-used codepath extracted for better code
>>>>> locality
>>>>> - */
>>>>> -static bool str_contains_non_fn_suffix(const char *str) {
>>>>> -       static const char *skip[] = {
>>>>> -               ".cold",
>>>>> -               ".part"
>>>>> -       };
>>>>> -       char *suffix = strchr(str, '.');
>>>>> -       int i;
>>>>> -
>>>>> -       if (!suffix)
>>>>> -               return false;
>>>>> -       for (i = 0; i < ARRAY_SIZE(skip); i++) {
>>>>> -               if (strstr(suffix, skip[i]))
>>>>> -                       return true;
>>>>> -       }
>>>>> -       return false;
>>>>> -}
>>>>> -
>>>>>    static bool elf_function__has_ambiguous_address(struct elf_function
>>>>> *func) {
>>>>>           struct elf_function_sym *sym;
>>>>>           uint64_t addr;
>>>>> @@ -1219,12 +1198,10 @@ static bool
>>>>> elf_function__has_ambiguous_address(struct elf_function *func) {
>>>>>           addr = 0;
>>>>>           for (int i = 0; i < func->sym_cnt; i++) {
>>>>>                   sym = &func->syms[i];
>>>>> -               if (!str_contains_non_fn_suffix(sym->name)) {
>>>>> -                       if (addr && addr != sym->addr)
>>>>> -                               return true;
>>>>> -                       else
>>>>> +               if (addr && addr != sym->addr)
>>>>> +                       return true;
>>>>> +               else
>>>>>                                   addr = sym->addr;
>>>>> -               }
>>>>>           }
>>>>>
>>>>>
>>>>>           return false;
>>>>> @@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
>>>>> elf_functions *functions)
>>>>>                   func = &functions->entries[functions->cnt];
>>>>>
>>>>>                   suffix = strchr(sym_name, '.');
>>>>> -               if (suffix)
>>>>> +               if (suffix) {
>>>>> +                       if (strstr(suffix, ".part") ||
>>>>> +                           strstr(suffix, ".cold"))
>>>>> +                               continue;
>>>>>                           func->name = strndup(sym_name, suffix -
>>>>> sym_name);
>>>>> -               else
>>>>> +               } else
>>>>>                           func->name = strdup(sym_name);
>>>>>
>>>>>                   if (!func->name) {
>>>>>
>>>>> I think that would work and saves later string comparisons, what do
>>>>> you
>>>>> think?
>>>>>
>>>>
>>>> Apologies, this isn't sufficient. Considering cases like
>>>> objpool_free(),
>>>> the problem is it has two entries in ELF for objpool_free and
>>>> objpool_free.part.0. So let's say we exclude objpool_free.part.0 from
>>>> the ELF representation, then we could potentially end up excluding
>>>> objpool_free as inconsistent if the DWARF for objpool_free.part.0
>>>> doesn't match that of objpool_free. It would appear to be inconsistent
>>>> but isn't really.
>>>
>>> Alan, as far as I can tell, in your example the function would be
>>> considered inconsistent independent of whether .part is included in
>>> elf_function->syms or not. We determine argument inconsistency based
>>> on DWARF data (struct function) passed into btf_encoder__save_func().
>>>
>>> So if there is a difference in arguments between objpool_free.part.0
>>> and objpool_free, it will be detected anyways.
>>>
>>
>> I think I've solved that in the following proof-of-concept series [1] -
>> by retaining the .part functions in the ELF list _and_ matching the
>> DWARF and ELF via address we can distinguish between foo and foo.part.0
>> debug information and discard the latter such that it is not included in
>> the determination of inconsistency.
>>
>>> A significant difference between v2 and v3 (just sent [1]) is in that
>>> if there is *only* "foo.part.0" symbol but no "foo", then it will not
>>> be included in v3 (because it's not in the elf_functions table), but
>>> would be in v2 (because there is only one address). And the correct
>>> behavior from the BTF encoding point of view is v3.
>>>
>>
>> Yep, that part sounds good; I _think_ the approach I'm proposing solves
>> that too, along with not incorrectly marking foo/foo.part.0 as
>> inconsistent.
>>
>> The series is the top 3 commits in [1]; the first commit [2] is yours
>> modulo the small tweak of marking non-functions during ELF function
>> creation. The second [3] uses address ranges to try harder to get
>> address info from DWARF, while the final one [4] skips creating function
>> state for functions that we address-match as the .part/.cold functions
>> in debug info. This all allows us to better identify debug information
>> that is tied to the non-function .part/.cold optimizations.
> 
> Hi Alan. Bumping this thread.
> 
> I haven't reviewed/tested your github changes thoroughly, but the
> approach makes sense to me overall.
> 
> What do you think about applying the group-by-name patch [1] first,
> maybe including your tweak? It would fix a couple of bugs right away.
> 
> And later you can send a more refined draft of patches to use
> addresses for matching.
> 

Yep, sounds good. Better to separate these changes; to support that I'll
add the tweak to your patch where we record but flag .part/.cold
functions as non-functions in [1]

If no-one objects, I'll land that in pahole next later. Thanks!

Alan

[1]
https://github.com/acmel/dwarves/commit/e256ffaf13cce96c4e782192b2814e1a2664fe99

Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Alan Maguire]

On 20/08/2025 12:04, Alan Maguire wrote:
> On 19/08/2025 20:34, Ihor Solodrai wrote:
>> On 8/5/25 4:24 AM, Alan Maguire wrote:
>>> On 01/08/2025 21:51, Ihor Solodrai wrote:
>>>> On 7/31/25 11:57 AM, Alan Maguire wrote:
>>>>> On 31/07/2025 15:16, Alan Maguire wrote:
>>>>>> On 29/07/2025 03:03, Ihor Solodrai wrote:
>>>>>>> btf_encoder collects function ELF symbols into a table, which is
>>>>>>> later
>>>>>>> used for processing DWARF data and determining whether a function can
>>>>>>> be added to BTF.
>>>>>>>
>>>>>>> So far the ELF symbol name was used as a key for search in this
>>>>>>> table,
>>>>>>> and a search by prefix match was attempted in cases when ELF symbol
>>>>>>> name has a compiler-generated suffix.
>>>>>>>
>>>>>>> This implementation has bugs [1][2], causing some functions to be
>>>>>>> inappropriately excluded from (or included into) BTF.
>>>>>>>
>>>>>>> Rework the implementation of the ELF functions table. Use a name of a
>>>>>>> function without any suffix - symbol name before the first occurrence
>>>>>>> of '.' - as a key. This way btf_encoder__find_function() always
>>>>>>> returns a valid elf_function object (or NULL).
>>>>>>>
>>>>>>> Collect an array of symbol name + address pairs from GElf_Sym for
>>>>>>> each
>>>>>>> elf_function when building the elf_functions table.
>>>>>>>
>>>>>>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is
>>>>>>> set
>>>>>>> when the function is saved by examining the array of ELF symbols in
>>>>>>> elf_function__has_ambiguous_address(). It tests whether there is only
>>>>>>> one unique address for this function name, taking into account that
>>>>>>> some addresses associated with it are not relevant:
>>>>>>>     * ".cold" suffix indicates a piece of hot/cold split
>>>>>>>     * ".part" suffix indicates a piece of partial inline
>>>>>>>
>>>>>>> When inspecting symbol name we have to search for any occurrence of
>>>>>>> the target suffix, as opposed to testing the entire suffix, or the
>>>>>>> end
>>>>>>> of a string. This is because suffixes may be combined by the
>>>>>>> compiler,
>>>>>>> for example producing ".isra0.cold", and the conclusion will be
>>>>>>> incorrect.
>>>>>>>
>>>>>>> In saved_functions_combine() check ambiguous_addr when deciding
>>>>>>> whether a function should be included in BTF.
>>>>>>>
>>>>>>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>>>>>>
>>>>>>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>>>>>>> CI-like kconfig) that are now excluded: all of those that I checked
>>>>>>> had multiple addresses, and some where static functions from
>>>>>>> different
>>>>>>> files with the same name.
>>>>>>>
>>>>>>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-
>>>>>>> b1cb-10266c72bd45@linux.dev/
>>>>>>> [2] https://lore.kernel.org/
>>>>>>> dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>>>>>>
>>>>>>> v1: https://lore.kernel.org/
>>>>>>> dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>>>>>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>>>>>
>>>>>> Thanks for doing this Ihor! Apologies for just thinking of this
>>>>>> now, but
>>>>>> why don't we filter out the .cold and .part functions earlier, not
>>>>>> even
>>>>>> adding them to the ELF functions list? Something like this on top of
>>>>>> your patch:
>>>>>>
>>>>>> $ git diff
>>>>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>>>>> index 0aa94ae..f61db0f 100644
>>>>>> --- a/btf_encoder.c
>>>>>> +++ b/btf_encoder.c
>>>>>> @@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
>>>>>> *btf_encoder__alloc_func_state(struct btf_e
>>>>>>           return state;
>>>>>>    }
>>>>>>
>>>>>> -/* some "." suffixes do not correspond to real functions;
>>>>>> - * - .part for partial inline
>>>>>> - * - .cold for rarely-used codepath extracted for better code
>>>>>> locality
>>>>>> - */
>>>>>> -static bool str_contains_non_fn_suffix(const char *str) {
>>>>>> -       static const char *skip[] = {
>>>>>> -               ".cold",
>>>>>> -               ".part"
>>>>>> -       };
>>>>>> -       char *suffix = strchr(str, '.');
>>>>>> -       int i;
>>>>>> -
>>>>>> -       if (!suffix)
>>>>>> -               return false;
>>>>>> -       for (i = 0; i < ARRAY_SIZE(skip); i++) {
>>>>>> -               if (strstr(suffix, skip[i]))
>>>>>> -                       return true;
>>>>>> -       }
>>>>>> -       return false;
>>>>>> -}
>>>>>> -
>>>>>>    static bool elf_function__has_ambiguous_address(struct elf_function
>>>>>> *func) {
>>>>>>           struct elf_function_sym *sym;
>>>>>>           uint64_t addr;
>>>>>> @@ -1219,12 +1198,10 @@ static bool
>>>>>> elf_function__has_ambiguous_address(struct elf_function *func) {
>>>>>>           addr = 0;
>>>>>>           for (int i = 0; i < func->sym_cnt; i++) {
>>>>>>                   sym = &func->syms[i];
>>>>>> -               if (!str_contains_non_fn_suffix(sym->name)) {
>>>>>> -                       if (addr && addr != sym->addr)
>>>>>> -                               return true;
>>>>>> -                       else
>>>>>> +               if (addr && addr != sym->addr)
>>>>>> +                       return true;
>>>>>> +               else
>>>>>>                                   addr = sym->addr;
>>>>>> -               }
>>>>>>           }
>>>>>>
>>>>>>
>>>>>>           return false;
>>>>>> @@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
>>>>>> elf_functions *functions)
>>>>>>                   func = &functions->entries[functions->cnt];
>>>>>>
>>>>>>                   suffix = strchr(sym_name, '.');
>>>>>> -               if (suffix)
>>>>>> +               if (suffix) {
>>>>>> +                       if (strstr(suffix, ".part") ||
>>>>>> +                           strstr(suffix, ".cold"))
>>>>>> +                               continue;
>>>>>>                           func->name = strndup(sym_name, suffix -
>>>>>> sym_name);
>>>>>> -               else
>>>>>> +               } else
>>>>>>                           func->name = strdup(sym_name);
>>>>>>
>>>>>>                   if (!func->name) {
>>>>>>
>>>>>> I think that would work and saves later string comparisons, what do
>>>>>> you
>>>>>> think?
>>>>>>
>>>>>
>>>>> Apologies, this isn't sufficient. Considering cases like
>>>>> objpool_free(),
>>>>> the problem is it has two entries in ELF for objpool_free and
>>>>> objpool_free.part.0. So let's say we exclude objpool_free.part.0 from
>>>>> the ELF representation, then we could potentially end up excluding
>>>>> objpool_free as inconsistent if the DWARF for objpool_free.part.0
>>>>> doesn't match that of objpool_free. It would appear to be inconsistent
>>>>> but isn't really.
>>>>
>>>> Alan, as far as I can tell, in your example the function would be
>>>> considered inconsistent independent of whether .part is included in
>>>> elf_function->syms or not. We determine argument inconsistency based
>>>> on DWARF data (struct function) passed into btf_encoder__save_func().
>>>>
>>>> So if there is a difference in arguments between objpool_free.part.0
>>>> and objpool_free, it will be detected anyways.
>>>>
>>>
>>> I think I've solved that in the following proof-of-concept series [1] -
>>> by retaining the .part functions in the ELF list _and_ matching the
>>> DWARF and ELF via address we can distinguish between foo and foo.part.0
>>> debug information and discard the latter such that it is not included in
>>> the determination of inconsistency.
>>>
>>>> A significant difference between v2 and v3 (just sent [1]) is in that
>>>> if there is *only* "foo.part.0" symbol but no "foo", then it will not
>>>> be included in v3 (because it's not in the elf_functions table), but
>>>> would be in v2 (because there is only one address). And the correct
>>>> behavior from the BTF encoding point of view is v3.
>>>>
>>>
>>> Yep, that part sounds good; I _think_ the approach I'm proposing solves
>>> that too, along with not incorrectly marking foo/foo.part.0 as
>>> inconsistent.
>>>
>>> The series is the top 3 commits in [1]; the first commit [2] is yours
>>> modulo the small tweak of marking non-functions during ELF function
>>> creation. The second [3] uses address ranges to try harder to get
>>> address info from DWARF, while the final one [4] skips creating function
>>> state for functions that we address-match as the .part/.cold functions
>>> in debug info. This all allows us to better identify debug information
>>> that is tied to the non-function .part/.cold optimizations.
>>
>> Hi Alan. Bumping this thread.
>>
>> I haven't reviewed/tested your github changes thoroughly, but the
>> approach makes sense to me overall.
>>
>> What do you think about applying the group-by-name patch [1] first,
>> maybe including your tweak? It would fix a couple of bugs right away.
>>
>> And later you can send a more refined draft of patches to use
>> addresses for matching.
>>
> 
> Yep, sounds good. Better to separate these changes; to support that I'll
> add the tweak to your patch where we record but flag .part/.cold
> functions as non-functions in [1]
> 
> If no-one objects, I'll land that in pahole next later. Thanks!
>

Done, thank you!

Alan