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

[Ihor Solodrai <ihor.solodrai@linux.dev>]

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.

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.

[1] 
https://lore.kernel.org/dwarves/20250801202009.3942492-1-ihor.solodrai@linux.dev/


> 
> I think the best thing might be to retain the .part/.cold repesentations
> in the ELF table but perhaps mark them with a flag (non_fn for
> non-function or similar?) at creation time to avoid expensive string
> comparisons later.

That's a good point. In v3 I exclude .part and .cold from the table,
and store ambiguous_addr flag in elf_function. If anything, we should
be doing less string comparisons now.

> 
> On the subject of improving matching, we do have address info for DWARF
> functions in many cases like this, and that can help guide DWARF->ELF
> mapping. I have a patch that helps collect function address info in
> dwarf_loader.c; perhaps we could make use of it in doing more accurate
> matching? In the above case for example, we actually have DWARF function
> addresses for both objpool_free and objpool_free.part.0 so we could in
> that case figure out the DWARF-ELF mapping even though there are
> multiple ELF addresses and multiple DWARF representations.
> 
> Haven't thought it through fully to be honest, but I think we want to
> avoid edge cases like the above where we either label a function as
> inconsistent or ambiguous unnecessarily. I'll try to come up with a
> rough proof-of-concept that weaves address-based matching into the
> approach you have here, since what you've done is a huge improvement.
> Again sorry for the noise here, I struggle to think through all the
> permutations we have to consider here to be honest.
> 
> Thanks!
> 
> Alan
> 
> 
> [...]
>