Re: [PATCH v2 dwarves 2/5] btf_encoder: Add true_signature feature support for "."-suffixed functions

[Matt Bobrowski <mattbobrowski@google.com>]

On Mon, Jan 26, 2026 at 10:49:00AM +0000, Alan Maguire wrote:
> 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 <alan.maguire@oracle.com>
> > 
> > Some minor nits, but apart from that it looks OK to me.
> > 
> > Acked-by: Matt Bobrowski <mattbobrowski@google.com>
> > 
> >> ---
> >>  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").

Sorry, I thought I only saw usage of member "addr" within
btf_encoder__save_func() against fn->lenblock.ip.addr which maps to
member "addr" within struct ip_tag and func->syms[i].addr which maps
to member "addr" within struct elf_function_sym. Member "addr" within
struct btf_encoder_func_state remains unused until patch 5/5 where we
assign it the value returned from function__addr().

With that said, within btf_encoder__save_func() why don't you do the
following instead:

state->addr = function__addr(fn);

...

if (encoder->true_signature && state->addr) {
	if (state->addr != func->syms[i].addr) {
		...
	}
}

There's no need to open code fn->lexblock.ip.addr everywhere as it
deminishes readability.

> >>  	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
> >>
>