On Thu, Mar 13, 2025 at 12:23:10PM +0800, Hengqi Chen wrote:
SNIP
> > > > > tools/lib/bpf/elf.c | 32 ++++++++++++++++++++++++--------
> > > > > 1 file changed, 24 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
> > > > > index 823f83ad819c..9b561c8d1eec 100644
> > > > > --- a/tools/lib/bpf/elf.c
> > > > > +++ b/tools/lib/bpf/elf.c
> > > > > @@ -260,13 +260,29 @@ static bool symbol_match(struct elf_sym_iter *iter, int sh_type, struct elf_sym
> > > > > * for shared libs) into file offset, which is what kernel is expecting
> > > > > * for uprobe/uretprobe attachment.
> > > > > * See Documentation/trace/uprobetracer.rst for more details. This is done
> > > > > - * by looking up symbol's containing section's header and using iter's virtual
> > > > > - * address (sh_addr) and corresponding file offset (sh_offset) to transform
> > > > > + * by looking up symbol's containing program header and using its virtual
> > > > > + * address (p_vaddr) and corresponding file offset (p_offset) to transform
> > > > > * sym.st_value (virtual address) into desired final file offset.
> > > > > */
> > > > > -static unsigned long elf_sym_offset(struct elf_sym *sym)
> > > > > +static unsigned long elf_sym_offset(Elf *elf, struct elf_sym *sym)
> > > > > {
> > > > > - return sym->sym.st_value - sym->sh.sh_addr + sym->sh.sh_offset;
> > > > > + size_t nhdrs, i;
> > > > > + GElf_Phdr phdr;
> > > > > +
> > > > > + if (elf_getphdrnum(elf, &nhdrs))
> > > > > + return -1;
> > > > > +
> > > > > + for (i = 0; i < nhdrs; i++) {
> > > > > + if (!gelf_getphdr(elf, (int)i, &phdr))
> > > > > + continue;
> > > > > + if (phdr.p_type != PT_LOAD || !(phdr.p_flags & PF_X))
> > > > > + continue;
> > > > > + if (sym->sym.st_value >= phdr.p_vaddr &&
> > > > > + sym->sym.st_value < (phdr.p_vaddr + phdr.p_memsz))
> > > > > + return sym->sym.st_value - phdr.p_vaddr + phdr.p_offset;
> >
> > Hengqi,
> >
> > Can you please provide an example where existing code doesn't work? I think that
> >
> > sym->sym.st_value - sym->sh.sh_addr + sym->sh.sh_offset
> >
> > and
> >
> > sym->sym.st_value - phdr.p_vaddr + phdr.p_offset
> >
> >
> > Should result in the same, and we don't need to search for a matching
> > segment if we have an ELF symbol and its section. But maybe I'm
> > mistaken, so can you please elaborate a bit?
>
> The binary ([0]) provided in the issue shows some counterexamples.
> I could't find an authoritative documentation describing this though.
> A modified version ([1]) of this patch could pass the CI now.
yes, I tried that binary and it gives me different offsets
IIUC the symbol seems to be from .eh_frame_hdr (odd?) while the new logic
base it on offset of .text section.. I'm still not following that binary
layout completely.. will try to check on that more later today
jirka
>
> [0]: https://github.com/libbpf/libbpf-rs/issues/1110#issuecomment-2699221802
> [1]: https://github.com/kernel-patches/bpf/pull/8647
>
> >
> > > > > + }
> > > > > +
> > > > > + return -1;
> > > > > }
> > > > >
> > > > > /* Find offset of function name in the provided ELF object. "binary_path" is
> > > > > @@ -329,7 +345,7 @@ long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
> > > > >
> > > > > if (ret > 0) {
> > > > > /* handle multiple matches */
> > > > > - if (elf_sym_offset(sym) == ret) {
> > > > > + if (elf_sym_offset(elf, sym) == ret) {
> > > > > /* same offset, no problem */
> > > > > continue;
> > > > > } else if (last_bind != STB_WEAK && cur_bind != STB_WEAK) {
> > > >
> > > > [...]
> > > >
>
