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@xxxxxxxxx/ >>>>>>> [2] https://lore.kernel.org/ >>>>>>> dwarves/6b4fda90fbf8f6aeeb2732bbfb6e81ba5669e2f3@xxxxxxxxx/ >>>>>>> >>>>>>> v1: https://lore.kernel.org/ >>>>>>> dwarves/98f41eaf6dd364745013650d58c5f254a592221c@xxxxxxxxx/ >>>>>>> Signed-off-by: Ihor Solodrai <isolodrai@xxxxxxxx> >>>>>> >>>>>> 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
