Messages in this thread Patch in this message | | | Date | Sun, 31 Jul 2022 20:37:02 +0800 | From | Leo Yan <> | Subject | Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols |
| |
On Sat, Jul 30, 2022 at 08:21:10AM -0700, Ian Rogers wrote:
[...]
> > On the other hand, I can accept to simply change pr_warning() to > > pr_debug4() to avoid warning flood, the log still can help us to find > > potential symbol parsing issue, so far they are not false-positive > > reporting. > > Thanks, I suspect the ELF that the Java agent has created isn't good. > The Java agent is part of perf as and so is the ELF file generation > code: > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/genelf.c?h=perf/core#n367
I think it's no need to change the function jit_write_elf(), please see below comment.
> I took a quick look but most of the logic is in libelf - it seems less > obvious the bug would be there rather than perf. Could you take a > look? Ideally there'd be a quick fix that keeps all the benefits of > your change and the jvmti code working.
A bit more finding for java JIT symbols. Let's see below two samples:
6.72% java /home/leoy/.debug/jit/java-jit-20220731.XXKRpgmR/jitted-214330-29.so 0x5b54 B [.] Interpreter 0.90% java /home/leoy/.debug/jit/java-jit-20220731.XXKRpgmR/jitted-214330-3475.so 0x4fc B [.] jdk.internal.math.FloatingDecimal$BinaryToASCIIBuffer.dtoa(int, long, int, boolean)
I can see the DSO "jitted-214330-29.so" only contains a java JIT symbol "Interpreter", and I also observed the same behavior for other JIT symbols, e.g. jitted-214330-3475.so only contains the symbol "jdk.internal.math.FloatingDecimal$BinaryToASCIIBuffer.dtoa(int, long, int, boolean)".
We always see these JIT symbols always have the consistent start file address, but this would not introduce conflit since every JIT symbol has its own DSO rather than share the same DSO.
I think now I understand your meaning, and your below change is fine for me, I tested it and confirmed it can show up java JIT symbols properly. Just a comment, it would be better to add a comment for why we need to add symbols when failed to find program header, like:
if (elf_read_program_header(syms_ss->elf, (u64)sym.st_value, &phdr)) { pr_debug4("%s: failed to find program header for " "symbol: %s\n", __func__, elf_name); pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " " "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n", __func__, (u64)sym.st_value, (u64)shdr.sh_addr, (u64)shdr.sh_offset); /* * Fail to find program header, let's rollback to use shdr.sh_addr * and shdr.sh_offset to calibrate symbol's file address, though * this is not necessary for normal C ELF file, we still need to * handle java JIT symbols in this case. */ sym.st_value -= shdr.sh_addr - shdr.sh_offset; } else { ... }
Could you send a formal patch for this? Thanks!
Leo
> > Thanks, > > Leo > > > > > ``` > > > --- a/tools/perf/util/symbol-elf.c > > > +++ b/tools/perf/util/symbol-elf.c > > > @@ -1305,16 +1305,21 @@ dso__load_sym_internal(struct dso *dso, struct > > > map *map, struct symsrc *syms > > > _ss, > > > > > > if (elf_read_program_header(syms_ss->elf, > > > (u64)sym.st_value, &phdr)) { > > > - pr_warning("%s: failed to find program > > > header for " > > > + pr_debug4("%s: failed to find program > > > header for " > > > "symbol: %s st_value: %#" PRIx64 "\n", > > > __func__, elf_name, > > > (u64)sym.st_value); > > > - continue; > > > + pr_debug4("%s: adjusting symbol: > > > st_value: %#" PRIx64 " " > > > + "sh_addr: %#" PRIx64 " > > > sh_offset: %#" PRIx64 "\n", > > > + __func__, (u64)sym.st_value, > > > (u64)shdr.sh_addr, > > > + (u64)shdr.sh_offset); > > > + sym.st_value -= shdr.sh_addr - shdr.sh_offset; > > > + } else { > > > + pr_debug4("%s: adjusting symbol: > > > st_value: %#" PRIx64 " " > > > + "p_vaddr: %#" PRIx64 " > > > p_offset: %#" PRIx64 "\n", > > > + __func__, (u64)sym.st_value, > > > (u64)phdr.p_vaddr, > > > + (u64)phdr.p_offset); > > > + sym.st_value -= phdr.p_vaddr - phdr.p_offset; > > > } > > > - pr_debug4("%s: adjusting symbol: st_value: %#" > > > PRIx64 " " > > > - "p_vaddr: %#" PRIx64 " p_offset: %#" > > > PRIx64 "\n", > > > - __func__, (u64)sym.st_value, > > > (u64)phdr.p_vaddr, > > > - (u64)phdr.p_offset); > > > - sym.st_value -= phdr.p_vaddr - phdr.p_offset; > > > } > > > > > > demangled = demangle_sym(dso, kmodule, elf_name); > > > ``` > > > > > > Thanks, > > > Ian > > > > > > > > > > > demangled = demangle_sym(dso, kmodule, elf_name); > > > > -- > > > > 2.25.1 > > > >
| |