Messages in this thread | | | From | Ian Rogers <> | Date | Sun, 31 Jul 2022 09:51:53 -0700 | Subject | Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols |
| |
On Sun, Jul 31, 2022 at 5:37 AM Leo Yan <leo.yan@linaro.org> wrote: > > 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!
Done. Thanks for sanity checking all of this! Please double check and add the reviewed/acked-by: https://lore.kernel.org/lkml/20220731164923.691193-1-irogers@google.com/
Thanks, Ian
> 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 > > > > >
| |