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. [1] https://github.com/acmel/dwarves/compare/master...alan-maguire:dwarves:pahole-next-remove-dupaddrs [2] https://github.com/acmel/dwarves/commit/e256ffaf13cce96c4e782192b2814e1a2664fe99 [3] https://github.com/acmel/dwarves/commit/7cc2c1e21f1daeb29aa270fd9f23ef1ba99fcd4e [4] https://github.com/acmel/dwarves/commit/893f14c2a854c240a927294996f449a3ad63eaed > [1] https://lore.kernel.org/dwarves/20250801202009.3942492-1- > ihor.solodrai@xxxxxxxxx/ > > >> >> 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 >> >> >> [...] >> > >
