* [PATCH v3 0/2] perf symbol: Minor fixing @ 2022-07-24 6:00 Leo Yan 2022-07-24 6:00 ` [PATCH v3 1/2] perf symbol: Correct address for bss symbols Leo Yan ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Leo Yan @ 2022-07-24 6:00 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel Cc: Leo Yan This patch set contains two minor fixing for parsing symbols. The first patch changes to use program header for parsing symbols of user space executable and shared objects. Since kernel's symbol parsing is more complex than userspace for support both kernel symbols and module symbols, this is why this patch set uses conservative way and doesn't change kernel symbols parsing. The second patch is to detect symbols from sections without setting attribute flag SHF_ALLOC, these symbols are used for linker warning, skip to record them to avoid spurious symbols. Changes from v2: - Changed to use more gernal way to check the attribute bit SHF_ALLOC for sections rather than check the section string name (Fangrui). Changes from v1: - Changed to use program header / PT_LOAD segments to parse symbols for userspace executable and shared object files (Fangrui). Leo Yan (2): perf symbol: Correct address for bss symbols perf symbol: Skip symbols if SHF_ALLOC flag is not set tools/perf/util/symbol-elf.c | 56 +++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 4 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-24 6:00 [PATCH v3 0/2] perf symbol: Minor fixing Leo Yan @ 2022-07-24 6:00 ` Leo Yan 2022-07-25 18:29 ` Arnaldo Carvalho de Melo 2022-07-30 5:13 ` Ian Rogers 2022-07-24 6:00 ` [PATCH v3 2/2] perf symbol: Skip symbols if SHF_ALLOC flag is not set Leo Yan 2022-07-25 16:54 ` [PATCH v3 0/2] perf symbol: Minor fixing Namhyung Kim 2 siblings, 2 replies; 14+ messages in thread From: Leo Yan @ 2022-07-24 6:00 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel Cc: Leo Yan When using 'perf mem' and 'perf c2c', an issue is observed that tool reports the wrong offset for global data symbols. This is a common issue on both x86 and Arm64 platforms. Let's see an example, for a test program, below is the disassembly for its .bss section which is dumped with objdump: ... Disassembly of section .bss: 0000000000004040 <completed.0>: ... 0000000000004080 <buf1>: ... 00000000000040c0 <buf2>: ... 0000000000004100 <thread>: ... First we used 'perf mem record' to run the test program and then used 'perf --debug verbose=4 mem report' to observe what's the symbol info for 'buf1' and 'buf2' structures. # ./perf mem record -e ldlat-loads,ldlat-stores -- false_sharing.exe 8 # ./perf --debug verbose=4 mem report ... dso__load_sym_internal: adjusting symbol: st_value: 0x40c0 sh_addr: 0x4040 sh_offset: 0x3028 symbol__new: buf2 0x30a8-0x30e8 ... dso__load_sym_internal: adjusting symbol: st_value: 0x4080 sh_addr: 0x4040 sh_offset: 0x3028 symbol__new: buf1 0x3068-0x30a8 ... Perf tool relies on libelf to parse symbols, in executable and shared object files, 'st_value' holds a virtual address; 'sh_addr' is the address at which section's first byte should reside in memory, and 'sh_offset' is the byte offset from the beginning of the file to the first byte in the section. Perf tool uses below formula to convert symbol's memory address to file address: file_address = st_value - sh_addr + sh_offset ^ ` Memory address We can see the final adjusted address ranges for buf1 and buf2 are [0x30a8-0x30e8) and [0x3068-0x30a8) respectively, apparently this is incorrect, in the code, the structure for 'buf1' and 'buf2' specifies compiler attribute with 64-byte alignment. The problem happens for 'sh_offset', libelf returns it as 0x3028 which is not 64-byte aligned, combining with disassembly, it's likely libelf doesn't respect the alignment for .bss section, therefore, it doesn't return the aligned value for 'sh_offset'. Suggested by Fangrui Song, elf file contains program header which contains PT_LOAD segments, the fields p_vaddr and p_offset in PT_LOAD segments contain the execution info. A better choice for converting memory address to file address is using the formula: file_address = st_value - p_vaddr + p_offset This patch introduces function elf_read_program_header() which returns out the program header based on the passed 'st_value', then it uses above formula to calculate the symbol file address; and the debugging log is updated respectively. After applying the change: # ./perf --debug verbose=4 mem report ... dso__load_sym_internal: adjusting symbol: st_value: 0x40c0 p_vaddr: 0x3d28 p_offset: 0x2d28 symbol__new: buf2 0x30c0-0x3100 ... dso__load_sym_internal: adjusting symbol: st_value: 0x4080 p_vaddr: 0x3d28 p_offset: 0x2d28 symbol__new: buf1 0x3080-0x30c0 ... Fixes: f17e04afaff8 ("perf report: Fix ELF symbol parsing") Reported-by: Chang Rui <changruinj@gmail.com> Suggested-by: Fangrui Song <maskray@google.com> Signed-off-by: Leo Yan <leo.yan@linaro.org> --- tools/perf/util/symbol-elf.c | 45 ++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c index ecd377938eea..ef6ced5c5746 100644 --- a/tools/perf/util/symbol-elf.c +++ b/tools/perf/util/symbol-elf.c @@ -233,6 +233,33 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep, return NULL; } +static int elf_read_program_header(Elf *elf, u64 vaddr, GElf_Phdr *phdr) +{ + size_t i, phdrnum; + u64 sz; + + if (elf_getphdrnum(elf, &phdrnum)) + return -1; + + for (i = 0; i < phdrnum; i++) { + if (gelf_getphdr(elf, i, phdr) == NULL) + return -1; + + if (phdr->p_type != PT_LOAD) + continue; + + sz = max(phdr->p_memsz, phdr->p_filesz); + if (!sz) + continue; + + if (vaddr >= phdr->p_vaddr && (vaddr < phdr->p_vaddr + sz)) + return 0; + } + + /* Not found any valid program header */ + return -1; +} + static bool want_demangle(bool is_kernel_sym) { return is_kernel_sym ? symbol_conf.demangle_kernel : symbol_conf.demangle; @@ -1209,6 +1236,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss, sym.st_value); used_opd = true; } + /* * When loading symbols in a data mapping, ABS symbols (which * has a value of SHN_ABS in its st_shndx) failed at @@ -1262,11 +1290,20 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss, goto out_elf_end; } else if ((used_opd && runtime_ss->adjust_symbols) || (!used_opd && syms_ss->adjust_symbols)) { + GElf_Phdr phdr; + + if (elf_read_program_header(syms_ss->elf, + (u64)sym.st_value, &phdr)) { + pr_warning("%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; + "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); -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-24 6:00 ` [PATCH v3 1/2] perf symbol: Correct address for bss symbols Leo Yan @ 2022-07-25 18:29 ` Arnaldo Carvalho de Melo 2022-07-26 0:53 ` Leo Yan 2022-07-30 5:13 ` Ian Rogers 1 sibling, 1 reply; 14+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-07-25 18:29 UTC (permalink / raw) To: Leo Yan Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel Em Sun, Jul 24, 2022 at 02:00:12PM +0800, Leo Yan escreveu: > When using 'perf mem' and 'perf c2c', an issue is observed that tool > reports the wrong offset for global data symbols. This is a common > issue on both x86 and Arm64 platforms. > > Let's see an example, for a test program, below is the disassembly for > its .bss section which is dumped with objdump: > > ... > > Disassembly of section .bss: > > 0000000000004040 <completed.0>: > ... > > 0000000000004080 <buf1>: > ... > > 00000000000040c0 <buf2>: > ... > > 0000000000004100 <thread>: > ... > > First we used 'perf mem record' to run the test program and then used > 'perf --debug verbose=4 mem report' to observe what's the symbol info > for 'buf1' and 'buf2' structures. > > # ./perf mem record -e ldlat-loads,ldlat-stores -- false_sharing.exe 8 Can you share the source code for your false sharing proggie? We need to have something in 'perf test' exercising these routines :-) > # ./perf --debug verbose=4 mem report > ... > dso__load_sym_internal: adjusting symbol: st_value: 0x40c0 sh_addr: 0x4040 sh_offset: 0x3028 > symbol__new: buf2 0x30a8-0x30e8 > ... > dso__load_sym_internal: adjusting symbol: st_value: 0x4080 sh_addr: 0x4040 sh_offset: 0x3028 > symbol__new: buf1 0x3068-0x30a8 > ... > > Perf tool relies on libelf to parse symbols, in executable and shared > object files, 'st_value' holds a virtual address; 'sh_addr' is the > address at which section's first byte should reside in memory, and > 'sh_offset' is the byte offset from the beginning of the file to the > first byte in the section. Perf tool uses below formula to convert > symbol's memory address to file address: > > file_address = st_value - sh_addr + sh_offset > ^ > ` Memory address > > We can see the final adjusted address ranges for buf1 and buf2 are > [0x30a8-0x30e8) and [0x3068-0x30a8) respectively, apparently this is > incorrect, in the code, the structure for 'buf1' and 'buf2' specifies > compiler attribute with 64-byte alignment. > > The problem happens for 'sh_offset', libelf returns it as 0x3028 which > is not 64-byte aligned, combining with disassembly, it's likely libelf > doesn't respect the alignment for .bss section, therefore, it doesn't > return the aligned value for 'sh_offset'. > > Suggested by Fangrui Song, elf file contains program header which > contains PT_LOAD segments, the fields p_vaddr and p_offset in PT_LOAD > segments contain the execution info. A better choice for converting > memory address to file address is using the formula: > > file_address = st_value - p_vaddr + p_offset > > This patch introduces function elf_read_program_header() which returns > out the program header based on the passed 'st_value', then it uses > above formula to calculate the symbol file address; and the debugging > log is updated respectively. > > After applying the change: > > # ./perf --debug verbose=4 mem report > ... > dso__load_sym_internal: adjusting symbol: st_value: 0x40c0 p_vaddr: 0x3d28 p_offset: 0x2d28 > symbol__new: buf2 0x30c0-0x3100 > ... > dso__load_sym_internal: adjusting symbol: st_value: 0x4080 p_vaddr: 0x3d28 p_offset: 0x2d28 > symbol__new: buf1 0x3080-0x30c0 > ... > > Fixes: f17e04afaff8 ("perf report: Fix ELF symbol parsing") > Reported-by: Chang Rui <changruinj@gmail.com> > Suggested-by: Fangrui Song <maskray@google.com> > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > tools/perf/util/symbol-elf.c | 45 ++++++++++++++++++++++++++++++++---- > 1 file changed, 41 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c > index ecd377938eea..ef6ced5c5746 100644 > --- a/tools/perf/util/symbol-elf.c > +++ b/tools/perf/util/symbol-elf.c > @@ -233,6 +233,33 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep, > return NULL; > } > > +static int elf_read_program_header(Elf *elf, u64 vaddr, GElf_Phdr *phdr) > +{ > + size_t i, phdrnum; > + u64 sz; > + > + if (elf_getphdrnum(elf, &phdrnum)) > + return -1; > + > + for (i = 0; i < phdrnum; i++) { > + if (gelf_getphdr(elf, i, phdr) == NULL) > + return -1; > + > + if (phdr->p_type != PT_LOAD) > + continue; > + > + sz = max(phdr->p_memsz, phdr->p_filesz); > + if (!sz) > + continue; > + > + if (vaddr >= phdr->p_vaddr && (vaddr < phdr->p_vaddr + sz)) > + return 0; > + } > + > + /* Not found any valid program header */ > + return -1; > +} > + > static bool want_demangle(bool is_kernel_sym) > { > return is_kernel_sym ? symbol_conf.demangle_kernel : symbol_conf.demangle; > @@ -1209,6 +1236,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss, > sym.st_value); > used_opd = true; > } > + > /* > * When loading symbols in a data mapping, ABS symbols (which > * has a value of SHN_ABS in its st_shndx) failed at > @@ -1262,11 +1290,20 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss, > goto out_elf_end; > } else if ((used_opd && runtime_ss->adjust_symbols) || > (!used_opd && syms_ss->adjust_symbols)) { > + GElf_Phdr phdr; > + > + if (elf_read_program_header(syms_ss->elf, > + (u64)sym.st_value, &phdr)) { > + pr_warning("%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; > + "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); > -- > 2.25.1 -- - Arnaldo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-25 18:29 ` Arnaldo Carvalho de Melo @ 2022-07-26 0:53 ` Leo Yan 2022-07-26 1:04 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 14+ messages in thread From: Leo Yan @ 2022-07-26 0:53 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel Hi Arnaldo, On Mon, Jul 25, 2022 at 03:29:45PM -0300, Arnaldo Carvalho de Melo wrote: [...] > > First we used 'perf mem record' to run the test program and then used > > 'perf --debug verbose=4 mem report' to observe what's the symbol info > > for 'buf1' and 'buf2' structures. > > > > # ./perf mem record -e ldlat-loads,ldlat-stores -- false_sharing.exe 8 > > Can you share the source code for your false sharing proggie? We need to > have something in 'perf test' exercising these routines :-) Sure, I am using the false sharing test case: https://github.com/joemario/perf-c2c-usage-files/blob/master/false_sharing_example.c ... with build command: $ gcc -g false_sharing_example.c -pthread -lnuma -o false_sharing.exe Do want me to proceed for adding test case? Otherwise, it's fine if you will work on the enabling test case :) Thanks, Leo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-26 0:53 ` Leo Yan @ 2022-07-26 1:04 ` Arnaldo Carvalho de Melo 2022-07-26 1:06 ` Leo Yan 0 siblings, 1 reply; 14+ messages in thread From: Arnaldo Carvalho de Melo @ 2022-07-26 1:04 UTC (permalink / raw) To: Leo Yan, Arnaldo Carvalho de Melo Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel On July 25, 2022 9:53:07 PM GMT-03:00, Leo Yan <leo.yan@linaro.org> wrote: >Hi Arnaldo, > >On Mon, Jul 25, 2022 at 03:29:45PM -0300, Arnaldo Carvalho de Melo wrote: > >[...] > >> > First we used 'perf mem record' to run the test program and then used >> > 'perf --debug verbose=4 mem report' to observe what's the symbol info >> > for 'buf1' and 'buf2' structures. >> > >> > # ./perf mem record -e ldlat-loads,ldlat-stores -- false_sharing.exe 8 >> >> Can you share the source code for your false sharing proggie? We need to >> have something in 'perf test' exercising these routines :-) > >Sure, I am using the false sharing test case: >https://github.com/joemario/perf-c2c-usage-files/blob/master/false_sharing_example.c > >... with build command: > >$ gcc -g false_sharing_example.c -pthread -lnuma -o false_sharing.exe > >Do want me to proceed for adding test case? Yes, go ahead, that will be really nice of you to add it :) - Arnaldo > Otherwise, it's fine if >you will work on the enabling test case :) > >Thanks, >Leo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-26 1:04 ` Arnaldo Carvalho de Melo @ 2022-07-26 1:06 ` Leo Yan 0 siblings, 0 replies; 14+ messages in thread From: Leo Yan @ 2022-07-26 1:06 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel On Mon, Jul 25, 2022 at 10:04:58PM -0300, Arnaldo Carvalho de Melo wrote: [...] > >Do want me to proceed for adding test case? > > Yes, go ahead, that will be really nice of you to add it :) Get it, will do it. Thanks, Leo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-24 6:00 ` [PATCH v3 1/2] perf symbol: Correct address for bss symbols Leo Yan 2022-07-25 18:29 ` Arnaldo Carvalho de Melo @ 2022-07-30 5:13 ` Ian Rogers 2022-07-30 9:38 ` Leo Yan 1 sibling, 1 reply; 14+ messages in thread From: Ian Rogers @ 2022-07-30 5:13 UTC (permalink / raw) To: Leo Yan Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel On Sat, Jul 23, 2022 at 11:00 PM Leo Yan <leo.yan@linaro.org> wrote: > > When using 'perf mem' and 'perf c2c', an issue is observed that tool > reports the wrong offset for global data symbols. This is a common > issue on both x86 and Arm64 platforms. > > Let's see an example, for a test program, below is the disassembly for > its .bss section which is dumped with objdump: > > ... > > Disassembly of section .bss: > > 0000000000004040 <completed.0>: > ... > > 0000000000004080 <buf1>: > ... > > 00000000000040c0 <buf2>: > ... > > 0000000000004100 <thread>: > ... > > First we used 'perf mem record' to run the test program and then used > 'perf --debug verbose=4 mem report' to observe what's the symbol info > for 'buf1' and 'buf2' structures. > > # ./perf mem record -e ldlat-loads,ldlat-stores -- false_sharing.exe 8 > # ./perf --debug verbose=4 mem report > ... > dso__load_sym_internal: adjusting symbol: st_value: 0x40c0 sh_addr: 0x4040 sh_offset: 0x3028 > symbol__new: buf2 0x30a8-0x30e8 > ... > dso__load_sym_internal: adjusting symbol: st_value: 0x4080 sh_addr: 0x4040 sh_offset: 0x3028 > symbol__new: buf1 0x3068-0x30a8 > ... > > Perf tool relies on libelf to parse symbols, in executable and shared > object files, 'st_value' holds a virtual address; 'sh_addr' is the > address at which section's first byte should reside in memory, and > 'sh_offset' is the byte offset from the beginning of the file to the > first byte in the section. Perf tool uses below formula to convert > symbol's memory address to file address: > > file_address = st_value - sh_addr + sh_offset > ^ > ` Memory address > > We can see the final adjusted address ranges for buf1 and buf2 are > [0x30a8-0x30e8) and [0x3068-0x30a8) respectively, apparently this is > incorrect, in the code, the structure for 'buf1' and 'buf2' specifies > compiler attribute with 64-byte alignment. > > The problem happens for 'sh_offset', libelf returns it as 0x3028 which > is not 64-byte aligned, combining with disassembly, it's likely libelf > doesn't respect the alignment for .bss section, therefore, it doesn't > return the aligned value for 'sh_offset'. > > Suggested by Fangrui Song, elf file contains program header which > contains PT_LOAD segments, the fields p_vaddr and p_offset in PT_LOAD > segments contain the execution info. A better choice for converting > memory address to file address is using the formula: > > file_address = st_value - p_vaddr + p_offset > > This patch introduces function elf_read_program_header() which returns > out the program header based on the passed 'st_value', then it uses > above formula to calculate the symbol file address; and the debugging > log is updated respectively. > > After applying the change: > > # ./perf --debug verbose=4 mem report > ... > dso__load_sym_internal: adjusting symbol: st_value: 0x40c0 p_vaddr: 0x3d28 p_offset: 0x2d28 > symbol__new: buf2 0x30c0-0x3100 > ... > dso__load_sym_internal: adjusting symbol: st_value: 0x4080 p_vaddr: 0x3d28 p_offset: 0x2d28 > symbol__new: buf1 0x3080-0x30c0 > ... > > Fixes: f17e04afaff8 ("perf report: Fix ELF symbol parsing") > Reported-by: Chang Rui <changruinj@gmail.com> > Suggested-by: Fangrui Song <maskray@google.com> > Signed-off-by: Leo Yan <leo.yan@linaro.org> I am seeing a problem with this patch with jvmti. To repro: 1) download a Java workload dacapo-9.12-MR1-bach.jar from https://sourceforge.net/projects/dacapobench/ 2) build perf such as "make -C tools/perf O=/tmp/perf NO_LIBBFD=1" it should detect Java and create /tmp/perf/libperf-jvmti.so 3) run perf with the jvmti agent: /tmp/perf/perf record -k 1 java -agentpath:/tmp/perf/libperf-jvmti.so -jar dacapo-9.12-MR1-bach.jar -n 10 fop 4) run perf inject: /tmp/perf/perf inject -i perf.data -o perf-injected.data -j 5) run perf report /tmp/perf/perf report -i perf-injected.data | grep org.apache.fop With this patch reverted I see lots of symbols like: 0.00% java jitted-388040-4656.so [.] org.apache.fop.fo.FObj.bind(org.apache.fop.fo.PropertyList) With the patch I see lots of: dso__load_sym_internal: failed to find program header for symbol: Lorg/apache/fop/fo/FObj;bind(Lorg/apache/fop/fo/PropertyList;)V st_value: 0x40 > --- > tools/perf/util/symbol-elf.c | 45 ++++++++++++++++++++++++++++++++---- > 1 file changed, 41 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c > index ecd377938eea..ef6ced5c5746 100644 > --- a/tools/perf/util/symbol-elf.c > +++ b/tools/perf/util/symbol-elf.c > @@ -233,6 +233,33 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep, > return NULL; > } > > +static int elf_read_program_header(Elf *elf, u64 vaddr, GElf_Phdr *phdr) > +{ > + size_t i, phdrnum; > + u64 sz; > + > + if (elf_getphdrnum(elf, &phdrnum)) > + return -1; > + > + for (i = 0; i < phdrnum; i++) { > + if (gelf_getphdr(elf, i, phdr) == NULL) > + return -1; > + > + if (phdr->p_type != PT_LOAD) > + continue; > + > + sz = max(phdr->p_memsz, phdr->p_filesz); > + if (!sz) > + continue; > + > + if (vaddr >= phdr->p_vaddr && (vaddr < phdr->p_vaddr + sz)) > + return 0; > + } > + > + /* Not found any valid program header */ > + return -1; > +} > + > static bool want_demangle(bool is_kernel_sym) > { > return is_kernel_sym ? symbol_conf.demangle_kernel : symbol_conf.demangle; > @@ -1209,6 +1236,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss, > sym.st_value); > used_opd = true; > } > + > /* > * When loading symbols in a data mapping, ABS symbols (which > * has a value of SHN_ABS in its st_shndx) failed at > @@ -1262,11 +1290,20 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss, > goto out_elf_end; > } else if ((used_opd && runtime_ss->adjust_symbols) || > (!used_opd && syms_ss->adjust_symbols)) { > + GElf_Phdr phdr; > + > + if (elf_read_program_header(syms_ss->elf, > + (u64)sym.st_value, &phdr)) { > + pr_warning("%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; > + "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; > } Combining the old and new behaviors fixes the issue for me, wdyt? ``` --- 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-30 5:13 ` Ian Rogers @ 2022-07-30 9:38 ` Leo Yan 2022-07-30 15:21 ` Ian Rogers 0 siblings, 1 reply; 14+ messages in thread From: Leo Yan @ 2022-07-30 9:38 UTC (permalink / raw) To: Ian Rogers Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel Hi Ian, On Fri, Jul 29, 2022 at 10:13:04PM -0700, Ian Rogers wrote: [...] > I am seeing a problem with this patch with jvmti. To repro: > > 1) download a Java workload dacapo-9.12-MR1-bach.jar from > https://sourceforge.net/projects/dacapobench/ > 2) build perf such as "make -C tools/perf O=/tmp/perf NO_LIBBFD=1" it > should detect Java and create /tmp/perf/libperf-jvmti.so > 3) run perf with the jvmti agent: > /tmp/perf/perf record -k 1 java -agentpath:/tmp/perf/libperf-jvmti.so > -jar dacapo-9.12-MR1-bach.jar -n 10 fop > 4) run perf inject: > /tmp/perf/perf inject -i perf.data -o perf-injected.data -j > 5) run perf report > /tmp/perf/perf report -i perf-injected.data | grep org.apache.fop > > With this patch reverted I see lots of symbols like: > 0.00% java jitted-388040-4656.so [.] > org.apache.fop.fo.FObj.bind(org.apache.fop.fo.PropertyList) > > With the patch I see lots of: > dso__load_sym_internal: failed to find program header for symbol: > Lorg/apache/fop/fo/FObj;bind(Lorg/apache/fop/fo/PropertyList;)V > st_value: 0x40 Thanks for sharing the steps, I can reproduce the issue. I tried to add more logs to dump and hope can find specific pattern for these symbols, one thing I observed that if a symbol fails to find program header, it has the same values for st_value, shdr.sh_addr and shdr.sh_offset: all of them are 0x40. So that means if with you proposed change in below, then we will get the file address is: file_addr = st_value - shdr.sh_addr + shdr.sh_offset = 0x40 Seems to me this is not reasonable: perf tries to add many symbols with the same file address 0x40. > Combining the old and new behaviors fixes the issue for me, wdyt? So far we don't answer a question is what's the purpose for these JAVA symbols. I checked these symbols and concluded as: - They are not label, this is because sym.st_info is 0x2, so its symbol type is STT_FUNC; - They are from ".text" section; - Symbol visibility is STV_DEFAULT; - Symbol's section index number is 0x1, which is different from some special sections (STV_DEFAULT/SHN_COMMON/SHN_UNDEF/SHN_XINDEX). This is a rough summary, these symbols are likewise the normal function symbols, but they have special st_value (0x40) and has no matched the program header for them. If we rollback to use old offsets to calculate the symbol file address, it still is incorrect. I list all relevant symbols in: https://termbin.com/s0fb, for a reliable fixing, could anyone with java experience shed some lights for handling the symbols? 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, 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-30 9:38 ` Leo Yan @ 2022-07-30 15:21 ` Ian Rogers 2022-07-31 12:37 ` Leo Yan 0 siblings, 1 reply; 14+ messages in thread From: Ian Rogers @ 2022-07-30 15:21 UTC (permalink / raw) To: Leo Yan Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel On Sat, Jul 30, 2022 at 2:38 AM Leo Yan <leo.yan@linaro.org> wrote: > > Hi Ian, > > On Fri, Jul 29, 2022 at 10:13:04PM -0700, Ian Rogers wrote: > > [...] > > > I am seeing a problem with this patch with jvmti. To repro: > > > > 1) download a Java workload dacapo-9.12-MR1-bach.jar from > > https://sourceforge.net/projects/dacapobench/ > > 2) build perf such as "make -C tools/perf O=/tmp/perf NO_LIBBFD=1" it > > should detect Java and create /tmp/perf/libperf-jvmti.so > > 3) run perf with the jvmti agent: > > /tmp/perf/perf record -k 1 java -agentpath:/tmp/perf/libperf-jvmti.so > > -jar dacapo-9.12-MR1-bach.jar -n 10 fop > > 4) run perf inject: > > /tmp/perf/perf inject -i perf.data -o perf-injected.data -j > > 5) run perf report > > /tmp/perf/perf report -i perf-injected.data | grep org.apache.fop > > > > With this patch reverted I see lots of symbols like: > > 0.00% java jitted-388040-4656.so [.] > > org.apache.fop.fo.FObj.bind(org.apache.fop.fo.PropertyList) > > > > With the patch I see lots of: > > dso__load_sym_internal: failed to find program header for symbol: > > Lorg/apache/fop/fo/FObj;bind(Lorg/apache/fop/fo/PropertyList;)V > > st_value: 0x40 > > Thanks for sharing the steps, I can reproduce the issue. > > I tried to add more logs to dump and hope can find specific pattern for > these symbols, one thing I observed that if a symbol fails to find > program header, it has the same values for st_value, shdr.sh_addr and > shdr.sh_offset: all of them are 0x40. So that means if with you > proposed change in below, then we will get the file address is: > > file_addr = st_value - shdr.sh_addr + shdr.sh_offset = 0x40 > > Seems to me this is not reasonable: perf tries to add many symbols > with the same file address 0x40. > > > Combining the old and new behaviors fixes the issue for me, wdyt? > > So far we don't answer a question is what's the purpose for these JAVA > symbols. I checked these symbols and concluded as: > > - They are not label, this is because sym.st_info is 0x2, so its > symbol type is STT_FUNC; > - They are from ".text" section; > - Symbol visibility is STV_DEFAULT; > - Symbol's section index number is 0x1, which is different from some > special sections (STV_DEFAULT/SHN_COMMON/SHN_UNDEF/SHN_XINDEX). > > This is a rough summary, these symbols are likewise the normal function > symbols, but they have special st_value (0x40) and has no matched the > program header for them. > > If we rollback to use old offsets to calculate the symbol file address, > it still is incorrect. > > I list all relevant symbols in: https://termbin.com/s0fb, for a reliable > fixing, could anyone with java experience shed some lights for handling > the symbols? > > 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 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. Thanks, Ian > 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 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-30 15:21 ` Ian Rogers @ 2022-07-31 12:37 ` Leo Yan 2022-07-31 16:51 ` Ian Rogers 0 siblings, 1 reply; 14+ messages in thread From: Leo Yan @ 2022-07-31 12:37 UTC (permalink / raw) To: Ian Rogers Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel 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 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols 2022-07-31 12:37 ` Leo Yan @ 2022-07-31 16:51 ` Ian Rogers 0 siblings, 0 replies; 14+ messages in thread From: Ian Rogers @ 2022-07-31 16:51 UTC (permalink / raw) To: Leo Yan Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel 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 > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/2] perf symbol: Skip symbols if SHF_ALLOC flag is not set 2022-07-24 6:00 [PATCH v3 0/2] perf symbol: Minor fixing Leo Yan 2022-07-24 6:00 ` [PATCH v3 1/2] perf symbol: Correct address for bss symbols Leo Yan @ 2022-07-24 6:00 ` Leo Yan 2022-07-25 16:54 ` [PATCH v3 0/2] perf symbol: Minor fixing Namhyung Kim 2 siblings, 0 replies; 14+ messages in thread From: Leo Yan @ 2022-07-24 6:00 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Namhyung Kim, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel Cc: Leo Yan Some symbols are observed the 'st_value' field are zeros. E.g. libc.so.6 in Ubuntu contains a symbol '__evoke_link_warning_getwd' which resides in the '.gnu.warning.getwd' section. Unlike normal sections, such kind of sections are used for linker warning when a file calls deprecated functions, but they are not part of memory images, the symbols in these sections should be dropped. This patch checks the section attribute SHF_ALLOC bit, if the bit is not set, it skips symbols to avoid spurious ones. Suggested-by: Fangrui Song <maskray@google.com> Signed-off-by: Leo Yan <leo.yan@linaro.org> --- tools/perf/util/symbol-elf.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c index ef6ced5c5746..b3be5b1d9dbb 100644 --- a/tools/perf/util/symbol-elf.c +++ b/tools/perf/util/symbol-elf.c @@ -1255,6 +1255,17 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss, gelf_getshdr(sec, &shdr); + /* + * If the attribute bit SHF_ALLOC is not set, the section + * doesn't occupy memory during process execution. + * E.g. ".gnu.warning.*" section is used by linker to generate + * warnings when calling deprecated functions, the symbols in + * the section aren't loaded to memory during process execution, + * so skip them. + */ + if (!(shdr.sh_flags & SHF_ALLOC)) + continue; + secstrs = secstrs_sym; /* -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/2] perf symbol: Minor fixing 2022-07-24 6:00 [PATCH v3 0/2] perf symbol: Minor fixing Leo Yan 2022-07-24 6:00 ` [PATCH v3 1/2] perf symbol: Correct address for bss symbols Leo Yan 2022-07-24 6:00 ` [PATCH v3 2/2] perf symbol: Skip symbols if SHF_ALLOC flag is not set Leo Yan @ 2022-07-25 16:54 ` Namhyung Kim 2022-07-26 0:55 ` Leo Yan 2 siblings, 1 reply; 14+ messages in thread From: Namhyung Kim @ 2022-07-25 16:54 UTC (permalink / raw) To: Leo Yan Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel Hi Leo, On Sat, Jul 23, 2022 at 11:00 PM Leo Yan <leo.yan@linaro.org> wrote: > > This patch set contains two minor fixing for parsing symbols. > > The first patch changes to use program header for parsing symbols of > user space executable and shared objects. Since kernel's symbol parsing > is more complex than userspace for support both kernel symbols and module > symbols, this is why this patch set uses conservative way and doesn't > change kernel symbols parsing. > > The second patch is to detect symbols from sections without setting > attribute flag SHF_ALLOC, these symbols are used for linker warning, > skip to record them to avoid spurious symbols. > > Changes from v2: > - Changed to use more gernal way to check the attribute bit SHF_ALLOC > for sections rather than check the section string name (Fangrui). > > Changes from v1: > - Changed to use program header / PT_LOAD segments to parse symbols for > userspace executable and shared object files (Fangrui). > > > Leo Yan (2): > perf symbol: Correct address for bss symbols > perf symbol: Skip symbols if SHF_ALLOC flag is not set Acked-by: Namhyung Kim <namhyung@kernel.org> Thanks, Namhyung > > tools/perf/util/symbol-elf.c | 56 +++++++++++++++++++++++++++++++++--- > 1 file changed, 52 insertions(+), 4 deletions(-) > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/2] perf symbol: Minor fixing 2022-07-25 16:54 ` [PATCH v3 0/2] perf symbol: Minor fixing Namhyung Kim @ 2022-07-26 0:55 ` Leo Yan 0 siblings, 0 replies; 14+ messages in thread From: Leo Yan @ 2022-07-26 0:55 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Jiri Olsa, Alexander Shishkin, Ian Rogers, Fangrui Song, Chang Rui, linux-perf-users, linux-kernel On Mon, Jul 25, 2022 at 09:54:21AM -0700, Namhyung Kim wrote: [...] > > This patch set contains two minor fixing for parsing symbols. > Acked-by: Namhyung Kim <namhyung@kernel.org> Thanks for reviewing, Namhyung. Leo ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-07-31 16:52 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-24 6:00 [PATCH v3 0/2] perf symbol: Minor fixing Leo Yan 2022-07-24 6:00 ` [PATCH v3 1/2] perf symbol: Correct address for bss symbols Leo Yan 2022-07-25 18:29 ` Arnaldo Carvalho de Melo 2022-07-26 0:53 ` Leo Yan 2022-07-26 1:04 ` Arnaldo Carvalho de Melo 2022-07-26 1:06 ` Leo Yan 2022-07-30 5:13 ` Ian Rogers 2022-07-30 9:38 ` Leo Yan 2022-07-30 15:21 ` Ian Rogers 2022-07-31 12:37 ` Leo Yan 2022-07-31 16:51 ` Ian Rogers 2022-07-24 6:00 ` [PATCH v3 2/2] perf symbol: Skip symbols if SHF_ALLOC flag is not set Leo Yan 2022-07-25 16:54 ` [PATCH v3 0/2] perf symbol: Minor fixing Namhyung Kim 2022-07-26 0:55 ` Leo Yan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.