Re: [PATCH dwarves] btf_encoder: prefer strong function definitions for BTF generation

[Matt Bobrowski <mattbobrowski@google.com>]

On Mon, Jan 05, 2026 at 08:23:29AM -0800, Yonghong Song wrote:
> 
> 
> On 1/5/26 3:47 AM, Matt Bobrowski wrote:
> > On Mon, Jan 05, 2026 at 08:27:11AM +0000, Matt Bobrowski wrote:
> > > On Fri, Jan 02, 2026 at 10:46:00AM -0800, Yonghong Song wrote:
> > > > 
> > > > On 12/31/25 12:53 AM, Matt Bobrowski wrote:
> > > > > Currently, when a function has both a weak and a strong definition
> > > > > across different compilation units (CUs), the BTF encoder arbitrarily
> > > > > selects one to generate the BTF entry. This selection fundamentally is
> > > > > dependent on the order in which pahole processes the CUs.
> > > > > 
> > > > > This indifference often leads to a mismatch where the generated BTF
> > > > > reflects the weak definition's prototype, even though the linker
> > > > > selected the strong definition for the final vmlinux binary.
> > > > > 
> > > > > A notable example described in [0] involving function
> > > > > bpf_lsm_mmap_file(). Both weak and strong definitions exist,
> > > > > distinguished only by parameter names (e.g., file vs
> > > > > file__nullable). While the strong definition is linked into the
> > > > > vmlinux object, the generated BTF contained the prototype for the weak
> > > > > definition. This causes issues for BPF verifier (e.g., __nullable
> > > > > annotation semantics), or tools relying on accurate type information.
> > > > > 
> > > > > To fix this, ensure the BTF encoder selects the function definition
> > > > > corresponding to the actual code linked into the binary. This is
> > > > > achieved by comparing the DWARF function address (DW_AT_low_pc) with
> > > > > the ELF symbol address (st_value). Only the DWARF entry for the strong
> > > > > definition will match the final resolved ELF symbol address.
> > > > > 
> > > > > [0] https://lore.kernel.org/all/aVJY9H-e83T7ivT4@google.com/
> > > > > 
> > > > > Link: https://lore.kernel.org/all/aVJY9H-e83T7ivT4@google.com/
> > > > > Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
> > > > LGTM with some nits below.
> > > Thanks for the review.
> > > 
> > > > Acked-by: Yonghong Song <yonghong.song@linux.dev>
> > > > 
> > > > > ---
> > > > >    btf_encoder.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 36 insertions(+)
> > > > > 
> > > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > > index b37ee7f..0462094 100644
> > > > > --- a/btf_encoder.c
> > > > > +++ b/btf_encoder.c
> > > > > @@ -79,6 +79,7 @@ struct btf_encoder_func_annot {
> > > > >    /* state used to do later encoding of saved functions */
> > > > >    struct btf_encoder_func_state {
> > > > > +	uint64_t addr;
> > > > >    	struct elf_function *elf;
> > > > >    	uint32_t type_id_off;
> > > > >    	uint16_t nr_parms;
> > > > > @@ -1258,6 +1259,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi
> > > > >    	if (!state)
> > > > >    		return -ENOMEM;
> > > > > +	state->addr = function__addr(fn);
> > > > >    	state->elf = 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;
> > > > > @@ -1477,6 +1479,29 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder)
> > > > >    	encoder->func_states.cap = 0;
> > > > >    }
> > > > > +static struct btf_encoder_func_state *btf_encoder__select_canonical_state(struct btf_encoder_func_state *combined_states,
> > > > > +									  int combined_cnt)
> > > > > +{
> > > > > +	int i, j;
> > > > > +
> > > > > +	/*
> > > > > +	 * The same elf_function is shared amongst combined functions,
> > > > > +	 * as per saved_functions_combine().
> > > > > +	 */
> > > > > +	struct elf_function *elf = combined_states[0].elf;
> > > > The logic is okay. But can we limit elf->sym_cnt to be 1 here?
> > > > This will match the case where two functions (weak and strong)
> > > > co-exist in compiler and eventually only strong/global function
> > > > will survive.
> > > In fact, checking again I believe that the loop is redundant because
> > > elf_function__has_ambiguous_address() ensures that if we reach this
> > > point, all symbols for the function share the same address. Therefore,
> > > checking the first symbol (elf->syms[0]) should be sufficient and
> > > equivalent to checking all of them.
> > > 
> > > Will send through a v2 with this amendment.
> > Hm, actually, no. I don't think the addresses stored within
> > elf->syms[#].addr should all be assumed to be the same at the point
> > which the new btf_encoder__select_canonical_state() function is called
> > (due to things like skip_encoding_inconsistent_proto possibly taking
> > effect). Therefore, I think it's best that we leave things as is and
> > exhaustively iterate through all elf->syms? I don't believe there's
> > any adverse effects in doing it this way anyway?
> 
> No. Your code is correct. For elf->sym_cnt, it covers both sym_cnt
> is 1 or more than 1. My previous suggestion is to single out the
> sym_cnt = 1 case since it is what you try to fix.
> 
> I am okay with the current implementation since it is correct.
> Maybe Alan and Arnaldo have additional comments about the code.

Sure, sounds good. I think leaving it as is probably our best bet at
this point.

> > > > > +
> > > > > +	for (i = 0; i < combined_cnt; i++) {
> > > > > +		struct btf_encoder_func_state *state = &combined_states[i];
> > > > > +
> > > > > +		for (j = 0; j < elf->sym_cnt; j++) {
> > > > > +			if (state->addr == elf->syms[j].addr)
> > > > > +				return state;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return &combined_states[0];
> > > > > +}
> > > > > +
> > > > >    static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_encoding_inconsistent_proto)
> > > > >    {
> > > > >    	struct btf_encoder_func_state *saved_fns = encoder->func_states.array;
> > > > > @@ -1517,6 +1542,17 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e
> > > > >    					0, 0);
> > > > >    		if (add_to_btf) {
> > > > > +			/*
> > > > > +			 * We're to add the current function within
> > > > > +			 * BTF. Although, from all functions that have
> > > > > +			 * possibly been combined via
> > > > > +			 * saved_functions_combine(), ensure to only
> > > > > +			 * select and emit BTF for the most canonical
> > > > > +			 * function definition.
> > > > > +			 */
> > > > > +			if (j - i > 1)
> > > > > +				state = btf_encoder__select_canonical_state(state, j - i);
> > > > > +
> > > > >    			if (is_kfunc_state(state))
> > > > >    				err = btf_encoder__add_bpf_kfunc(encoder, state);
> > > > >    			else
>