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! Alan [1] https://github.com/acmel/dwarves/commit/e256ffaf13cce96c4e782192b2814e1a2664fe99
