Re: [PATCH dwarves v2] btf_encoder: group all function ELF syms by function name

[Alan Maguire <alan.maguire@oracle.com>]

On 20/08/2025 12:04, Alan Maguire wrote:
> On 19/08/2025 20:34, Ihor Solodrai wrote:
>> On 8/5/25 4:24 AM, Alan Maguire wrote:
>>> On 01/08/2025 21:51, Ihor Solodrai wrote:
>>>> On 7/31/25 11:57 AM, Alan Maguire wrote:
>>>>> On 31/07/2025 15:16, Alan Maguire wrote:
>>>>>> On 29/07/2025 03:03, Ihor Solodrai wrote:
>>>>>>> btf_encoder collects function ELF symbols into a table, which is
>>>>>>> later
>>>>>>> used for processing DWARF data and determining whether a function can
>>>>>>> be added to BTF.
>>>>>>>
>>>>>>> So far the ELF symbol name was used as a key for search in this
>>>>>>> table,
>>>>>>> and a search by prefix match was attempted in cases when ELF symbol
>>>>>>> name has a compiler-generated suffix.
>>>>>>>
>>>>>>> This implementation has bugs [1][2], causing some functions to be
>>>>>>> inappropriately excluded from (or included into) BTF.
>>>>>>>
>>>>>>> Rework the implementation of the ELF functions table. Use a name of a
>>>>>>> function without any suffix - symbol name before the first occurrence
>>>>>>> of '.' - as a key. This way btf_encoder__find_function() always
>>>>>>> returns a valid elf_function object (or NULL).
>>>>>>>
>>>>>>> Collect an array of symbol name + address pairs from GElf_Sym for
>>>>>>> each
>>>>>>> elf_function when building the elf_functions table.
>>>>>>>
>>>>>>> Introduce ambiguous_addr flag to the btf_encoder_func_state. It is
>>>>>>> set
>>>>>>> when the function is saved by examining the array of ELF symbols in
>>>>>>> elf_function__has_ambiguous_address(). It tests whether there is only
>>>>>>> one unique address for this function name, taking into account that
>>>>>>> some addresses associated with it are not relevant:
>>>>>>>     * ".cold" suffix indicates a piece of hot/cold split
>>>>>>>     * ".part" suffix indicates a piece of partial inline
>>>>>>>
>>>>>>> When inspecting symbol name we have to search for any occurrence of
>>>>>>> the target suffix, as opposed to testing the entire suffix, or the
>>>>>>> end
>>>>>>> of a string. This is because suffixes may be combined by the
>>>>>>> compiler,
>>>>>>> for example producing ".isra0.cold", and the conclusion will be
>>>>>>> incorrect.
>>>>>>>
>>>>>>> In saved_functions_combine() check ambiguous_addr when deciding
>>>>>>> whether a function should be included in BTF.
>>>>>>>
>>>>>>> Successful CI run: https://github.com/acmel/dwarves/pull/68/checks
>>>>>>>
>>>>>>> I manually spot checked some of the ~200 functions from vmlinux (BPF
>>>>>>> CI-like kconfig) that are now excluded: all of those that I checked
>>>>>>> had multiple addresses, and some where static functions from
>>>>>>> different
>>>>>>> files with the same name.
>>>>>>>
>>>>>>> [1] https://lore.kernel.org/bpf/2f8c792e-9675-4385-
>>>>>>> b1cb-10266c72bd45@linux.dev/
>>>>>>> [2] https://lore.kernel.org/
>>>>>>> dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@linux.dev/
>>>>>>>
>>>>>>> v1: https://lore.kernel.org/
>>>>>>> dwarves/98f41eaf6dd364745013650d58c5f254a592221c@linux.dev/
>>>>>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>>>>>
>>>>>> Thanks for doing this Ihor! Apologies for just thinking of this
>>>>>> now, but
>>>>>> why don't we filter out the .cold and .part functions earlier, not
>>>>>> even
>>>>>> adding them to the ELF functions list? Something like this on top of
>>>>>> your patch:
>>>>>>
>>>>>> $ git diff
>>>>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>>>>> index 0aa94ae..f61db0f 100644
>>>>>> --- a/btf_encoder.c
>>>>>> +++ b/btf_encoder.c
>>>>>> @@ -1188,27 +1188,6 @@ static struct btf_encoder_func_state
>>>>>> *btf_encoder__alloc_func_state(struct btf_e
>>>>>>           return state;
>>>>>>    }
>>>>>>
>>>>>> -/* some "." suffixes do not correspond to real functions;
>>>>>> - * - .part for partial inline
>>>>>> - * - .cold for rarely-used codepath extracted for better code
>>>>>> locality
>>>>>> - */
>>>>>> -static bool str_contains_non_fn_suffix(const char *str) {
>>>>>> -       static const char *skip[] = {
>>>>>> -               ".cold",
>>>>>> -               ".part"
>>>>>> -       };
>>>>>> -       char *suffix = strchr(str, '.');
>>>>>> -       int i;
>>>>>> -
>>>>>> -       if (!suffix)
>>>>>> -               return false;
>>>>>> -       for (i = 0; i < ARRAY_SIZE(skip); i++) {
>>>>>> -               if (strstr(suffix, skip[i]))
>>>>>> -                       return true;
>>>>>> -       }
>>>>>> -       return false;
>>>>>> -}
>>>>>> -
>>>>>>    static bool elf_function__has_ambiguous_address(struct elf_function
>>>>>> *func) {
>>>>>>           struct elf_function_sym *sym;
>>>>>>           uint64_t addr;
>>>>>> @@ -1219,12 +1198,10 @@ static bool
>>>>>> elf_function__has_ambiguous_address(struct elf_function *func) {
>>>>>>           addr = 0;
>>>>>>           for (int i = 0; i < func->sym_cnt; i++) {
>>>>>>                   sym = &func->syms[i];
>>>>>> -               if (!str_contains_non_fn_suffix(sym->name)) {
>>>>>> -                       if (addr && addr != sym->addr)
>>>>>> -                               return true;
>>>>>> -                       else
>>>>>> +               if (addr && addr != sym->addr)
>>>>>> +                       return true;
>>>>>> +               else
>>>>>>                                   addr = sym->addr;
>>>>>> -               }
>>>>>>           }
>>>>>>
>>>>>>
>>>>>>           return false;
>>>>>> @@ -2208,9 +2185,12 @@ static int elf_functions__collect(struct
>>>>>> elf_functions *functions)
>>>>>>                   func = &functions->entries[functions->cnt];
>>>>>>
>>>>>>                   suffix = strchr(sym_name, '.');
>>>>>> -               if (suffix)
>>>>>> +               if (suffix) {
>>>>>> +                       if (strstr(suffix, ".part") ||
>>>>>> +                           strstr(suffix, ".cold"))
>>>>>> +                               continue;
>>>>>>                           func->name = strndup(sym_name, suffix -
>>>>>> sym_name);
>>>>>> -               else
>>>>>> +               } else
>>>>>>                           func->name = strdup(sym_name);
>>>>>>
>>>>>>                   if (!func->name) {
>>>>>>
>>>>>> I think that would work and saves later string comparisons, what do
>>>>>> you
>>>>>> think?
>>>>>>
>>>>>
>>>>> Apologies, this isn't sufficient. Considering cases like
>>>>> objpool_free(),
>>>>> the problem is it has two entries in ELF for objpool_free and
>>>>> objpool_free.part.0. So let's say we exclude objpool_free.part.0 from
>>>>> the ELF representation, then we could potentially end up excluding
>>>>> objpool_free as inconsistent if the DWARF for objpool_free.part.0
>>>>> doesn't match that of objpool_free. It would appear to be inconsistent
>>>>> but isn't really.
>>>>
>>>> Alan, as far as I can tell, in your example the function would be
>>>> considered inconsistent independent of whether .part is included in
>>>> elf_function->syms or not. We determine argument inconsistency based
>>>> on DWARF data (struct function) passed into btf_encoder__save_func().
>>>>
>>>> So if there is a difference in arguments between objpool_free.part.0
>>>> and objpool_free, it will be detected anyways.
>>>>
>>>
>>> I think I've solved that in the following proof-of-concept series [1] -
>>> by retaining the .part functions in the ELF list _and_ matching the
>>> DWARF and ELF via address we can distinguish between foo and foo.part.0
>>> debug information and discard the latter such that it is not included in
>>> the determination of inconsistency.
>>>
>>>> A significant difference between v2 and v3 (just sent [1]) is in that
>>>> if there is *only* "foo.part.0" symbol but no "foo", then it will not
>>>> be included in v3 (because it's not in the elf_functions table), but
>>>> would be in v2 (because there is only one address). And the correct
>>>> behavior from the BTF encoding point of view is v3.
>>>>
>>>
>>> Yep, that part sounds good; I _think_ the approach I'm proposing solves
>>> that too, along with not incorrectly marking foo/foo.part.0 as
>>> inconsistent.
>>>
>>> The series is the top 3 commits in [1]; the first commit [2] is yours
>>> modulo the small tweak of marking non-functions during ELF function
>>> creation. The second [3] uses address ranges to try harder to get
>>> address info from DWARF, while the final one [4] skips creating function
>>> state for functions that we address-match as the .part/.cold functions
>>> in debug info. This all allows us to better identify debug information
>>> that is tied to the non-function .part/.cold optimizations.
>>
>> Hi Alan. Bumping this thread.
>>
>> I haven't reviewed/tested your github changes thoroughly, but the
>> approach makes sense to me overall.
>>
>> What do you think about applying the group-by-name patch [1] first,
>> maybe including your tweak? It would fix a couple of bugs right away.
>>
>> And later you can send a more refined draft of patches to use
>> addresses for matching.
>>
> 
> Yep, sounds good. Better to separate these changes; to support that I'll
> add the tweak to your patch where we record but flag .part/.cold
> functions as non-functions in [1]
> 
> If no-one objects, I'll land that in pahole next later. Thanks!
>

Done, thank you!

Alan