Re: [PATCH v2 dwarves 2/5] btf_encoder: Add true_signature feature support for "."-suffixed functions
Alan Maguire <[email protected]>
| Newsgroups | org.kernel.vger.dwarves,org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
On 26/01/2026 09:52, Matt Bobrowski wrote: > On Fri, Jan 23, 2026 at 05:26:47PM +0000, Alan Maguire wrote: >> Currently we collate function information by name and add functions >> provided there are no inconsistencies across various representations. >> >> For true_signature support - where we wish to add the real signature >> of a function even if it differs from source level - we need to do >> a few things: >> >> 1. For "."-suffixed functions, we need to match from DWARF->ELF; >> we can do this via the address associated with the function. >> In doing this, we can then be confident that the debug info >> for foo.isra.0 is the right info for the function at that >> address. >> >> 2. When adding saved functions we need to look for such cases >> and provided they do not violate other constraints around BTF >> representation - unexpected reg usage for function, uncertain >> parameter location or ambiguous address - we add them with >> their "."-suffixed name. The latter can be used as a signal >> that the function is transformed from the original. >> >> Doing this adds 500 functions to BTF. These are traceable with >> their "."-suffix names and because we have excluded ambiguous >> address cases we know exactly which function address they refer >> to. >> >> Signed-off-by: Alan Maguire <[email protected]> > > Some minor nits, but apart from that it looks OK to me. > > Acked-by: Matt Bobrowski <[email protected]> > >> --- >> btf_encoder.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++----- >> dwarves.h | 1 + >> pahole.c | 1 + >> 3 files changed, 71 insertions(+), 7 deletions(-) >> >> diff --git a/btf_encoder.c b/btf_encoder.c >> index 9a567e4..c1002c3 100644 >> --- a/btf_encoder.c >> +++ b/btf_encoder.c >> @@ -77,9 +77,16 @@ struct btf_encoder_func_annot { >> int16_t component_idx; >> }; >> >> +struct elf_function_sym { >> + const char *name; >> + uint64_t addr; >> +}; >> + >> /* state used to do later encoding of saved functions */ >> struct btf_encoder_func_state { >> struct elf_function *elf; >> + struct elf_function_sym *sym; >> + uint64_t addr; > ^ > This appears to have leaked into wrong commit? > This member should've been introduced within patch > 5/5. > It's used here too though; specifically in btf_encoder__save_func() below. We need to use addresses to map between a DWARF representation and the associated ELF function to ensure we're using debug info for the correct "."-suffixed function. We need to do this because the late DWARF generated after optimizations are applied uses the original name ("foo" rather than "foo.isra.0"). >> uint32_t type_id_off; >> uint16_t nr_parms; >> uint16_t nr_annots; >> @@ -94,11 +101,6 @@ struct btf_encoder_func_state { >> struct btf_encoder_func_annot *annots; >> }; >> >> -struct elf_function_sym { >> - const char *name; >> - uint64_t addr; >> -}; >> - >> struct elf_function { >> char *name; >> struct elf_function_sym *syms; >> @@ -145,7 +147,8 @@ struct btf_encoder { >> skip_encoding_decl_tag, >> tag_kfuncs, >> gen_distilled_base, >> - encode_attributes; >> + encode_attributes, >> + true_signature; >> uint32_t array_index_id; >> struct elf_secinfo *secinfo; >> size_t seccnt; >> @@ -1270,14 +1273,36 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi >> goto out; >> } >> } >> + if (encoder->true_signature && fn->lexblock.ip.addr) { >> + int i; >> + >> + for (i = 0; i < func->sym_cnt; i++) { >> + if (fn->lexblock.ip.addr != func->syms[i].addr) >> + continue; >> + /* Only need to record address for '.'-suffixed >> + * functions, since we only currently need true >> + * signatures for them. >> + */ >> + if (!strchr(func->syms[i].name, '.')) >> + continue; >> + state->sym = &func->syms[i]; >> + break; >> + } >> + } >> state->inconsistent_proto = ftype->inconsistent_proto; >> state->unexpected_reg = ftype->unexpected_reg; >> state->optimized_parms = ftype->optimized_parms; >> state->uncertain_parm_loc = ftype->uncertain_parm_loc; >> state->reordered_parm = ftype->reordered_parm; >> ftype__for_each_parameter(ftype, param) { >> - const char *name = parameter__name(param) ?: ""; >> + const char *name; >> >> + /* No location info/optimized + reordered means optimized out. */ >> + if (ftype->reordered_parm && (!param->has_loc || param->optimized)) { >> + state->nr_parms--; >> + continue; >> + } >> + name = parameter__name(param) ?: ""; >> str_off = btf__add_str(btf, name); >> if (str_off < 0) { >> err = str_off; >> @@ -1366,6 +1391,9 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, >> >> btf_fnproto_id = btf_encoder__add_func_proto_for_state(encoder, state); >> name = func->name; >> + if (encoder->true_signature && state->sym) >> + name = state->sym->name; >> + >> if (btf_fnproto_id >= 0) >> btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, >> name, false); >> @@ -1508,6 +1536,39 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e >> while (j < nr_saved_fns && saved_functions_combine(encoder, &saved_fns[i], &saved_fns[j]) == 0) >> j++; >> >> + /* Add true signatures for case where we have an exact >> + * symbol match by address from DWARF->ELF and have a >> + * "." suffixed name. >> + */ >> + if (encoder->true_signature) { >> + bool true_added = false; >> + int k; >> + >> + for (k = i; k < nr_saved_fns; k++) { >> + struct btf_encoder_func_state *true_state = &saved_fns[k]; >> + >> + if (state->elf != true_state->elf) >> + break; >> + if (!true_state->sym) >> + continue; >> + /* Unexpected reg, uncertain parm loc and >> + * ambiguous address mean we cannot trust fentry. >> + */ >> + if (true_state->unexpected_reg || >> + true_state->uncertain_parm_loc || >> + true_state->ambiguous_addr) >> + continue; >> + err = btf_encoder__add_func(encoder, true_state); >> + if (err < 0) >> + goto out; >> + true_added = true; >> + break; >> + } >> + /* If true symbol was added, skip the below. */ >> + if (true_added) >> + continue; >> + } >> + > > I think this hunk should be factored out into a > helper. btf_encoder__add_saved_funcs() is starting to get hairy IMO. > Yeah, I think that's reasonable. The combination of loops within loops, breaks and continues is getting hard to follow. I'll respin with a btf_encoder__add_true_signature() function replicating this logic. >> /* do not exclude functions with optimized-out parameters; they >> * may still be _called_ with the right parameter values, they >> * just do not _use_ them. Only exclude functions with >> @@ -2584,6 +2645,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam >> encoder->tag_kfuncs = conf_load->btf_decl_tag_kfuncs; >> encoder->gen_distilled_base = conf_load->btf_gen_distilled_base; >> encoder->encode_attributes = conf_load->btf_attributes; >> + encoder->true_signature = conf_load->true_signature; >> encoder->verbose = verbose; >> encoder->has_index_type = false; >> encoder->need_index_type = false; >> diff --git a/dwarves.h b/dwarves.h >> index 78bedf5..d7c6474 100644 >> --- a/dwarves.h >> +++ b/dwarves.h >> @@ -101,6 +101,7 @@ struct conf_load { >> bool btf_decl_tag_kfuncs; >> bool btf_gen_distilled_base; >> bool btf_attributes; >> + bool true_signature; >> uint8_t hashtable_bits; >> uint8_t max_hashtable_bits; >> uint16_t kabi_prefix_len; >> diff --git a/pahole.c b/pahole.c >> index ef01e58..02a0d19 100644 >> --- a/pahole.c >> +++ b/pahole.c >> @@ -1234,6 +1234,7 @@ struct btf_feature { >> BTF_NON_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false), >> BTF_NON_DEFAULT_FEATURE_CHECK(attributes, btf_attributes, false, >> attributes_check), >> + BTF_NON_DEFAULT_FEATURE(true_signature, true_signature, false), >> }; >> >> #define BTF_MAX_FEATURE_STR 1024 >> -- >> 2.43.5 >>