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

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

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!

Alan

[1]
https://github.com/acmel/dwarves/commit/e256ffaf13cce96c4e782192b2814e1a2664fe99