On 7/24/25 10:54 AM, Alan Maguire wrote:
On 23/07/2025 12:22, Jiri Olsa wrote:
On Tue, Jul 22, 2025 at 10:58:52PM +0000, Ihor Solodrai wrote:
SNIP
@@ -1338,48 +1381,39 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder,
return 0;
}
-static int functions_cmp(const void *_a, const void *_b)
+static int elf_function__name_cmp(const void *_a, const void *_b)
{
const struct elf_function *a = _a;
const struct elf_function *b = _b;
- /* if search key allows prefix match, verify target has matching
- * prefix len and prefix matches.
- */
- if (a->prefixlen && a->prefixlen == b->prefixlen)
- return strncmp(a->name, b->name, b->prefixlen);
nice to see this one removed ;-)
return strcmp(a->name, b->name);
}
-#ifndef max
-#define max(x, y) ((x) < (y) ? (y) : (x))
-#endif
-
static int saved_functions_cmp(const void *_a, const void *_b)
{
const struct btf_encoder_func_state *a = _a;
const struct btf_encoder_func_state *b = _b;
- return functions_cmp(a->elf, b->elf);
+ return elf_function__name_cmp(a->elf, b->elf);
}
static int saved_functions_combine(struct btf_encoder_func_state *a, struct btf_encoder_func_state *b)
{
- uint8_t optimized, unexpected, inconsistent;
- int ret;
+ uint8_t optimized, unexpected, inconsistent, ambiguous_addr;
+
+ if (a->elf != b->elf)
+ return 1;
- ret = strncmp(a->elf->name, b->elf->name,
- max(a->elf->prefixlen, b->elf->prefixlen));
- if (ret != 0)
- return ret;
optimized = a->optimized_parms | b->optimized_parms;
unexpected = a->unexpected_reg | b->unexpected_reg;
inconsistent = a->inconsistent_proto | b->inconsistent_proto;
- if (!unexpected && !inconsistent && !funcs__match(a, b))
+ ambiguous_addr = a->ambiguous_addr | b->ambiguous_addr;
+ if (!unexpected && !inconsistent && !ambiguous_addr && !funcs__match(a, b))
inconsistent = 1;
a->optimized_parms = b->optimized_parms = optimized;
a->unexpected_reg = b->unexpected_reg = unexpected;
a->inconsistent_proto = b->inconsistent_proto = inconsistent;
+ a->ambiguous_addr = b->ambiguous_addr = ambiguous_addr;
I had to add change below to get the functions with multiple addresses out
diff --git a/btf_encoder.c b/btf_encoder.c
index fcc30aa9d97f..7b9679794790 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -1466,7 +1466,7 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e
* just do not _use_ them. Only exclude functions with
* unexpected register use or multiple inconsistent prototypes.
*/
- add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto;
+ add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->ambiguous_addr;
if (add_to_btf) {
err = btf_encoder__add_func(state->encoder, state);
other than that I like the approach
Thanks for the patch! I ran it through CI [1] with the above change plus
an added whitespace after the function name in the printf() in
btf_encoder__log_func_skip(). The btf_functions.sh test expects
whitespace after function names when examining skipped functions, so
either the test should be updated to handle no whitespace or we should
ensure the space is there after the function name like this:
printf("%s : skipping BTF encoding of function due to ",
func->name);
Otherwise we get a CI failure that is nothing to do with the changes.
With this in place we do however lose a lot of functions it seems, some
I suspect unnecessarily. For example:
Looking at
< void __tcp_send_ack(struct sock * sk, u32 rcv_nxt, u16 flags);
ffffffff83c83170 t __tcp_send_ack.part.0
ffffffff83c83310 T __tcp_send_ack
So __tcp_send_ack is partially inlined, but partial inlines should not
count as ambiguous addresses I think. We should probably ensure we skip
.part suffixes as well as .cold in calculating ambiguous addresses.
I modified the patch somewhat and we wind up losing ~400 functions
instead of over 700, see [2].
Modified patch is at [3]. If the mods look okay to you Ihor would you
mind sending it officially? Would be great to get wider testing to
ensure it doesn't break anything or leave any functions out unexpectedly.
Alan, Jiri, thank you for review and testing. I sent this draft in a bit
of a rush, sorry.
I'll incorporate your suggestions, test the patch a bit more and then
will send a clean version. I am curious what functions are lost and
why, will report if notice anything interesting.
SNIP
@@ -2153,18 +2191,75 @@ static int elf_functions__collect(struct elf_functions *functions)
goto out_free;
}
+ /* First, collect an elf_function for each GElf_Sym
+ * Where func->name is without a suffix
+ */
functions->cnt = 0;
elf_symtab__for_each_symbol_index(functions->symtab, core_id, sym, sym_sec_idx) {
- elf_functions__collect_function(functions, &sym);
+
+ if (elf_sym__type(&sym) != STT_FUNC)
+ continue;
+
+ sym_name = elf_sym__name(&sym, functions->symtab);
+ if (!sym_name)
+ continue;
+
+ func = &functions->entries[functions->cnt];
+
+ const char *suffix = strchr(sym_name, '.');
+ if (suffix) {
+ functions->suffix_cnt++;
do we need suffix_cnt now?
think it's been unused for a while now, so can be removed I think.
Thanks again for working on this!
Alan
[1] https://github.com/alan-maguire/dwarves/actions/runs/16500065295
[2]
https://github.com/alan-maguire/dwarves/actions/runs/16501897430/job/46662503155
[3]
https://github.com/acmel/dwarves/commit/30dffd7fc34e7753b3d21b4b3f1a5e17814c224f
thanks,
jirka
+ func->name = strndup(sym_name, suffix - sym_name);
+ } else {
+ func->name = strdup(sym_name);
+ }
+ if (!func->name) {
+ err = -ENOMEM;
+ goto out_free;
+ }
+
+ func_sym.name = sym_name;
+ func_sym.addr = sym.st_value;
+
+ err = elf_function__push_sym(func, &func_sym);
+ if (err)
+ goto out_free;
+
+ functions->cnt++;
}
SNIP
