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

[Matt Bobrowski <mattbobrowski@google.com>]

On Thu, Jan 08, 2026 at 06:18:07PM +0000, Alan Maguire wrote:
> On 07/01/2026 18:55, Matt Bobrowski wrote:
> > On Wed, Jan 07, 2026 at 03:50:40PM +0000, Alan Maguire wrote:
> >> On 05/01/2026 20:43, Matt Bobrowski wrote:
> >>> 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.
> >>>
> >>
> >> hi Matt, I ran the change through github CI and there is some differences in
> >> the set of generated functions from vmlinux (see the "Compare functions generated"
> >> step):
> >>
> >> https://github.com/alan-maguire/dwarves/actions/runs/20786255742/job/59698755550
> >>
> >> Specifically we see changes in some function signatures like this:
> >>
> >> < int neightbl_fill_info(struct sk_buff * skb, struct neigh_table * tbl, u32 pid, u32 seq, int type, int flags);
> >> ---
> >>> int neightbl_fill_info(struct sk_buff * skb, struct neigh_table * tbl, u32 pid, u32 seq, int flags, int type);
> >>
> >> Note the reordering of the last two parameters. The "<" line matches the source code, and the
> >> ">" line is what we get from pahole with your change. We've seen this before and the reason is 
> >> that we're not paying close enough attention to cases where the actual function omits parameters
> >> due to optimization; that last "type" parameter doesn't have a DW_AT_location and that indicates 
> >> it's been optimized out. We should really get in this case is:
> >>
> >> int neightbl_fill_info.constprop.0(struct sk_buff * skb, struct neigh_table * tbl, u32 pid, u32 seq, int flags);
> >>
> >> So it's not that your change causes this exactly; it's that paradoxically because
> >> your change does a better job of selecting the real function signature in the CU
> >> (and then we go on to misrepresent it) the problem is more glaringly exposed.
> >>
> >> The good news is I think I have a workable fix for this problem; what I'd propose is - 
> >> presuming it works - we land it prior to your change. Once I've tested that out a bit
> >> I'll follow up. Thanks!
> > 
> > Ha, interesting! Curious, should the .constprop.0 based functions also
> > not be emitted within the generated BTF given that they too
> > technically would exist in the .text section and can be called?
> 
> Yep, but the original policy - that we are changing - was to avoid encoding
> anything that violated source-level expectations. So if a .constprop
> function omitted a parameter it was left out of BTF encoding. But annoyingly
> we often weren't detecting that correctly, so we would up with function signatures
> that had out-of-order parameters which should have been recognized as having
> missng parameters (and thus omitted). We're moving towards the concept of "true" 
> function signatures where we emit a possibly changed function with its "." suffix
> name; see Yonghong's presentation at Linux Plumbers for more details.

Noted, and appreciate the explanation here. I'll also watch Yonghong's
presentation [0] to get a little bit more of an idea on what we're
effectively moving towards.

> I've put together a series that weaves together better detection of
> misordered/missing parameters, optional support for emitting true function 
> signatures for optimized functions and finally your patch. With the prerequisite
> patches in place, your patch (which needed some merging but is essentially the
> same) no longer emits signatures with different ordered parameters; we better
> detect such cases. See [1]; changes are in branch in [2].
> 
> Most of the detected function signature changes are syscalls which have
> the pt_regs "regs" name rather than "__unused"; but in this case:
> 
> < int pcibios_enable_device(struct pci_dev * dev, int bars);
> < void pcibios_fixup_bus(struct pci_bus * bus);
> ---
> > int pcibios_enable_device(struct pci_dev * dev, int mask);
> > void pcibios_fixup_bus(struct pci_bus * b);
> 
> the signatures match the strong rather than weak declarations:
> 
> strong: 
> 
>  int pcibios_enable_device(struct pci_dev *dev, int mask);
>  void pcibios_fixup_bus(struct pci_bus *b);
> 
> weak:
> 
>   int __weak pcibios_enable_device(struct pci_dev *dev, int bars);
>   void __weak pcibios_fixup_bus(struct pci_bus *bus);
> 
> 
> so that's what we want.
>
> I need to do a bit more testing but it seems like this gets the behaviour
> you want without the side-effects due to existing brokenness in pahole.
> Let me know what you think. Thanks!

The prerequisite patches look OK to me, and the detected function
signature modifications bring us closer to where we want to be
IMO. So, LGTM overall.

> [1] https://github.com/alan-maguire/dwarves/actions/runs/20813525302/job/59783355126
> [2] https://github.com/acmel/dwarves/compare/master...alan-maguire:dwarves:refs/heads/pahole-next-true-sig-gcc
> 
> 
> > Apologies if I'm not understanding something correctly here, but my
> > current understanding is that non-.constprop.0 and .constprop.0
> > variants share differing addresses.
> > 
> > Anyway, please keep me updated on how you're progressing with the fix
> > that you're intending to work on.

[0] https://www.youtube.com/watch?v=u4-yWFiIZ98