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.
[1]
https://lore.kernel.org/dwarves/20250801202009.3942492-1-ihor.solodrai@xxxxxxxxx/
[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
[...]
