[PATCH v3 0/2] perf symbol: Minor fixing

[PATCH v3 0/2] perf symbol: Minor fixing

[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

[PATCH v3 1/2] perf symbol: Correct address for bss symbols

[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

[PATCH v3 2/2] perf symbol: Skip symbols if SHF_ALLOC flag is not set

[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

Re: [PATCH v3 0/2] perf symbol: Minor fixing

[Namhyung Kim]

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
>

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Arnaldo Carvalho de Melo]

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

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Leo Yan]

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

Re: [PATCH v3 0/2] perf symbol: Minor fixing

[Leo Yan]

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

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Arnaldo Carvalho de Melo]

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

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Leo Yan]

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

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Ian Rogers]

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
>

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Leo Yan]

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
> >

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Ian Rogers]

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
> > >

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Leo Yan]

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
> > > >

Re: [PATCH v3 1/2] perf symbol: Correct address for bss symbols

[Ian Rogers]

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
> > > > >